fix: convert epoch-ms to BigInt before nanosecond multiplication#3362
fix: convert epoch-ms to BigInt before nanosecond multiplication#3362Yanhu007 wants to merge 1 commit intotriggerdotdev:mainfrom
Conversation
Several OTLP timestamp conversions multiply epoch milliseconds by 1,000,000 before converting to BigInt. Since epoch-ms * 1e6 is ~1.7e18 (exceeding Number.MAX_SAFE_INTEGER ~9e15), this causes IEEE 754 precision loss of ~256ns in ~0.2% of cases. The correct pattern (already used in convertDateToNanoseconds) is: BigInt(date.getTime()) * BigInt(1_000_000) Fixed in: - getNowInNanoseconds() - calculateDurationFromStart() - recordEvent() call in runEngineHandlers Fixes triggerdotdev#3292
|
|
Hi @Yanhu007, 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. |
|
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)
WalkthroughUpdated nanosecond conversion math across two files to ensure multiplication operations are performed using 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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
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 precision fix in recordRunEvent (apps/webapp/app/v3/eventRepository/index.server.ts:218)
The PR fixes Number-space overflow before BigInt conversion in common.server.ts and runEngineHandlers.server.ts, but misses the identical pattern at apps/webapp/app/v3/eventRepository/index.server.ts:218:
startTime: BigInt((startTime?.getTime() ?? Date.now()) * 1_000_000),Date.now() returns ~1.7×10¹² and multiplied by 1,000,000 gives ~1.7×10¹⁸, which exceeds Number.MAX_SAFE_INTEGER (~9×10¹⁵). At that magnitude, floating-point steps are 256, so nanosecond timestamps silently lose up to ~128ns of precision before BigInt conversion. This is the exact same bug the PR fixes in the other three locations. This function is called from recordRunDebugLog (apps/webapp/app/v3/eventRepository/index.server.ts:168), which is in turn called from runEngineHandlers.server.ts — the same file being modified in this PR.
View 2 additional findings in Devin Review.
1 participant
Summary
Fixes #3292
Several OTLP timestamp conversions multiply epoch milliseconds by 1,000,000 before converting to
BigInt. Since epoch-ms × 1e6 is ~1.7e18 (exceedingNumber.MAX_SAFE_INTEGER~9e15), this causes IEEE 754 precision loss of ±256ns in ~0.2% of cases.The correct pattern (already used in
convertDateToNanoseconds()in the same file) is:Changes
getNowInNanoseconds()BigInt(new Date().getTime() * 1_000_000)BigInt(new Date().getTime()) * BigInt(1_000_000)calculateDurationFromStart()BigInt($endtime.getTime() * 1_000_000)BigInt($endtime.getTime()) * BigInt(1_000_000)runEngineHandlers.server.tsBigInt(time.getTime() * 1000000)BigInt(time.getTime()) * BigInt(1_000_000)