Fix crash on millisecond-scale started_at in hermes agent traces#8313
Open
DaoyuanLi2816 wants to merge 1 commit into
Open
Fix crash on millisecond-scale started_at in hermes agent traces#8313DaoyuanLi2816 wants to merge 1 commit into
DaoyuanLi2816 wants to merge 1 commit into
Conversation
When parsing hermes-format agent traces, `started_at` is meant to be
normalized to seconds before being passed to `datetime.fromtimestamp`
(which expects seconds), using a magnitude check to detect whether the
input is already in milliseconds:
milliseconds = timestamp if timestamp <= 10_000_000_000 else timestamp * 1_000
The variable name and the `* 1_000` say "convert to milliseconds", but
`datetime.fromtimestamp` needs *seconds*, and the large-magnitude branch is
exactly the "value is already in milliseconds" case -- it should divide by
1000 to convert down to seconds, not multiply further. For a `started_at`
expressed in milliseconds (a common convention, e.g. `Date.now()`-style
timestamps), multiplying by 1000 again produces a ~56-million-years-future
timestamp, and `datetime.fromtimestamp` raises `OSError: [Errno 22] Invalid
argument` (or `OverflowError`/`ValueError` on other platforms), aborting the
whole trace-loading path.
Renames the variable to `seconds` (what it actually holds and what
`datetime.fromtimestamp` expects) and fixes the arithmetic to divide.
Adds `HERMES_SESSION_MS` (the existing `HERMES_SESSION` fixture with
`started_at` re-expressed in milliseconds for the same real-world instant)
and a new `hermes-ms` parametrized case in
`test_json_generate_tables_with_agent_trace_metadata`, asserting the same
`sent_at` output as the existing seconds-form hermes case.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Parsing a hermes-format agent trace whose
started_atis expressed in milliseconds crashes withOSError: [Errno 22] Invalid argument.The bug
src/datasets/packaged_modules/json/json.pynormalizesstarted_atbefore feeding it todatetime.fromtimestamp(which expects seconds), using a magnitude check to detect whether the input is already in milliseconds:The large-magnitude branch (
timestamp > 10_000_000_000, i.e. already milliseconds) should convert down to seconds by dividing by 1000 — instead it multiplies by 1000 again. For a real millisecond-scalestarted_at(a common convention, e.g.Date.now()-style timestamps), this produces a value representing a date roughly 56 million years in the future, anddatetime.fromtimestampraisesOSError: [Errno 22] Invalid argumenton Windows (OverflowError/ValueErroron other platforms for the same reason), aborting the whole trace-loading path.The fix
Divide instead of multiply, and rename the variable to
seconds(what it actually holds, and whatdatetime.fromtimestampexpects):Tests
Adds
HERMES_SESSION_MS— the existingHERMES_SESSIONfixture withstarted_atre-expressed in milliseconds for the same real-world instant — and a newhermes-msparametrized case intest_json_generate_tables_with_agent_trace_metadata, asserting the exact samesent_atoutput as the existing seconds-formhermescase.Verified fail-before / pass-after against the real library (via
PYTHONPATHpointed at this checkout, withteichinstalled so the@require_teich-gated tests actually run): the newhermes-mscase raisesOSErrorbefore the fix and passes after; the two pre-existing hermes cases are unaffected either way. Full file:36 passed.ruff format --checkandruff checkare clean.