Repository navigation
Conversation
Multiplying epoch milliseconds by 1e6 as a Number exceeds Number.MAX_SAFE_INTEGER, so the product is rounded to a multiple of 256 before it is converted to BigInt. Span start times and durations could therefore be off by up to a few hundred nanoseconds. Fixes triggerdotdev#3292
|
|
Hi @oziass, thanks for your interest in contributing! This project requires that pull request authors are vouched, and you are not in the list of vouched users. This PR will be closed automatically. See https://github.com/triggerdotdev/trigger.dev/blob/main/CONTRIBUTING.md for more details. |
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configuration
📒 Files selected for processing: 4
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| vi.useFakeTimers(); | ||
| vi.setSystemTime(unsafeMs); |
There was a problem hiding this comment.
🔍 Clock fakes in conversion test
The getNowInNanoseconds test replaces the clock with fake timers. The repository's testing rules prohibit fakes; use a real-clock assertion or a testable clock input.
Was this helpful? React with 👍 or 👎 to provide feedback.
| import { afterEach, describe, expect, it, vi } from "vitest"; | ||
| import { | ||
| calculateDurationFromStart, | ||
| convertDateToNanoseconds, | ||
| getNowInNanoseconds, | ||
| } from "~/v3/eventRepository/common.server"; |
There was a problem hiding this comment.
🔍 Conversion tests are not colocated
The new conversion tests live outside the event repository. The repository's testing guide requires new test files beside the source they exercise.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Updated in 33f68e7 to resolve both review findings:
|
1 participant
Multiplying epoch milliseconds by
1_000_000as aNumberexceedsNumber.MAX_SAFE_INTEGER, so the product is rounded to a multiple of 256 before it is converted toBigInt. Span start times and durations could therefore be off by up to a few hundred nanoseconds.This updates
getNowInNanosecondsandcalculateDurationFromStartincommon.server.tsto useconvertDateToNanoseconds, and converts milliseconds toBigIntbefore multiplying byBigInt(1_000_000)inindex.server.tsandrunEngineHandlers.server.ts.Fixes #3292
/claim #3292
✅ Checklist
Testing
Added colocated unit tests in
apps/webapp/app/v3/eventRepository/common.server.test.tscovering exact epoch conversion, a real-clock nanosecond assertion, duration precision, and minimum duration. No fake timers are used. All four tests pass withNODE_ENV=test node ../../node_modules/vitest/vitest.mjs run app/v3/eventRepository/common.server.test.tsfromapps/webapp.The test directory is included in Vitest discovery. The updated fork branch is
oziass:job_42at33f68e7456275e66cee691c943e964f74f3ddfed.Changelog
Fixed floating-point precision loss when converting millisecond timestamps to nanoseconds in the webapp event repository and run engine handlers.
Screenshots
N/A