fix: prevent nanosecond timestamp overflow in event repository - #4499
fix: prevent nanosecond timestamp overflow in event repository#4499AkhilTrivediX wants to merge 1 commit into
Conversation
|
|
Hi @AkhilTrivediX, 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. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
|
|
||
| export function getNowInNanoseconds(): bigint { | ||
| return BigInt(new Date().getTime() * 1_000_000); | ||
| return BigInt(new Date().getTime()) * BigInt(1_000_000); |
There was a problem hiding this comment.
🟡 Server-only change ships without a release-notes entry
This change only touches server code under apps/webapp/ but no .server-changes/ note was added (see CONTRIBUTING.md and AGENTS.md), so the fix will be missing from the user-visible release notes.
Impact: Users reading the release notes won't see that the timestamp accuracy issue was fixed.
Repository rule: server-only PRs require a .server-changes entry
CONTRIBUTING.md states: "If your PR only changes server components (apps/webapp/, apps/supervisor/, etc.) with no package changes, add a .server-changes/ file so the change appears in release notes." The diff modifies only apps/webapp/app/v3/eventRepository/common.server.ts, apps/webapp/app/v3/eventRepository/index.server.ts and apps/webapp/app/v3/runEngineHandlers.server.ts, and the branch adds no file under .server-changes/. The file needs frontmatter area: webapp and type: fix plus a one-line user-facing description.
Prompt for agents
The PR changes only server code under apps/webapp, so per CONTRIBUTING.md and AGENTS.md it needs a markdown file in .server-changes/ (e.g. .server-changes/fix-event-timestamp-precision.md) with frontmatter 'area: webapp' and 'type: fix', and a one-line, user-facing body describing the behavior change (e.g. run timeline timestamps are now recorded exactly instead of being off by a fraction of a microsecond). Follow the writing guidance in .server-changes/README.md.
Was this helpful? React with 👍 or 👎 to provide feedback.
| ...attributes, | ||
| }, | ||
| startTime: BigInt((startTime?.getTime() ?? Date.now()) * 1_000_000), | ||
| startTime: BigInt(startTime?.getTime() ?? Date.now()) * BigInt(1_000_000), |
There was a problem hiding this comment.
🔍 Fix changes stored timestamp values; verify no downstream equality/dedup depends on old rounded values
Timestamps produced by getNowInNanoseconds/recordRunEvent/the retry event now carry exact nanosecond values instead of values rounded to the nearest 256ns. Any persisted data written before this change (ClickHouse/Postgres task events) keeps the old rounded values, so exact-equality comparisons or joins between newly written start times and previously derived values (e.g. span start/end matching, dedup keys built from start time) could behave differently across the deploy boundary. Callers I checked compute durations or ranges rather than exact matches, so this appears safe, but it's worth a second look at anything keying on startTime exactly.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
WalkthroughTimestamp conversion logic now converts millisecond values to ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
Just realized someone else had already submitted a fix for this (#3381) and is just waiting for a vouch as well. Apologies for the duplicate! |
2 participants
Resolves #3292
Fixes IEEE 754 precision loss that was occurring when multiplying epoch milliseconds by 1,000,000 in float-land before casting to
BigInt.The fix properly casts the epoch milliseconds to
BigIntbefore multiplying byBigInt(1_000_000), completely preventing the ~256ns errors in timestamp values.This affects:
getNowInNanoseconds()calculateDurationFromStart()recordRunDebugLog()runEngineHandlers.server.ts