fix: prevent IEEE 754 precision loss in OTLP nanosecond timestamps#3378
fix: prevent IEEE 754 precision loss in OTLP nanosecond timestamps#3378aayushbaluni wants to merge 1 commit intotriggerdotdev:mainfrom
Conversation
Multiplying epoch milliseconds by 1,000,000 before BigInt conversion produces values exceeding Number.MAX_SAFE_INTEGER (~9e15), causing ~256ns precision errors in ~0.2% of timestamps. Convert to BigInt first, then multiply: - getNowInNanoseconds(): BigInt(ms) * BigInt(1_000_000) - calculateDurationFromStart(): same pattern - calculateDurationFromStartJsDate(): same pattern - recordRunDebugLog(): same pattern The correct pattern already existed in convertDateToNanoseconds() in the same file. Fixes triggerdotdev#3292 Made-with: Cursor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe changes refactor nanosecond conversion logic in two event repository files to perform BigInt arithmetic before multiplication rather than after. Functions Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes ✨ 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 |
|
Hi @aayushbaluni, 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. |
There was a problem hiding this comment.
Devin Review found 1 potential issue.
⚠️ 1 issue in files not directly in the diff
⚠️ Missed instance of the same BigInt overflow pattern at runEngineHandlers.server.ts:432 (apps/webapp/app/v3/runEngineHandlers.server.ts:432)
This PR fixes the BigInt(timestamp * 1_000_000) overflow pattern in 4 locations, but missed the same bug at apps/webapp/app/v3/runEngineHandlers.server.ts:432. The expression BigInt(time.getTime() * 1000000) suffers from the same Number.MAX_SAFE_INTEGER overflow: Date.getTime() returns ~1.78e12 and multiplying by 1,000,000 gives ~1.78e18 which exceeds Number.MAX_SAFE_INTEGER (9.007e15), causing precision loss before the BigInt conversion. At the current date, this causes a ~64-nanosecond error in the recorded startTime of retry-scheduled events.
View 3 additional findings in Devin Review.
Fixes triggerdotdev#3292 Multiple functions compute nanosecond timestamps as BigInt(milliseconds * 1_000_000). When milliseconds exceed Number.MAX_SAFE_INTEGER / 1_000_000 (≈ 9.007e9, i.e. any date after ~1970-04-15), the multiplication overflows IEEE 754 precision before the BigInt conversion captures it. Fix all instances (including runEngineHandlers.server.ts:432 which was missed in the previous PR triggerdotdev#3378) by converting to BigInt first: BigInt(milliseconds) * BigInt(1_000_000) Made-with: Cursor
Summary
Several places in the webapp multiply epoch milliseconds by 1,000,000 before converting to
BigInt, which causes IEEE 754 precision loss (~256ns errors in ~0.2% of cases). The result exceedsNumber.MAX_SAFE_INTEGER(~9e15) since epoch-ms × 1e6 is ~1.7e18.Root Cause
The correct pattern already existed in
convertDateToNanoseconds()in the same file — the other functions just didn't use it.Changes
common.server.ts: FixgetNowInNanoseconds(),calculateDurationFromStart(), andcalculateDurationFromStartJsDate()index.server.ts: FixrecordRunDebugLog()startTime calculationAll four locations now convert to
BigIntbefore multiplication, consistent with the existingconvertDateToNanoseconds()implementation.Fixes #3292
Made with Cursor