Preserve nanosecond precision in TestClock wall time - #6950
Conversation
fubhy
added
the
audit
🦋 Changeset detectedLatest commit: 7a13dfd The changes in this PR will be included in the next version bump. This PR includes changesets to release 30 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
This PR currently adds only the regression test; the implementation fix in packages/effect/src/testing/TestClock.ts is still outstanding.
Reviewed changes
- Added a focused regression test in
packages/effect/test/TestClock.test.tsthat asserts nanosecond precision for a large wall-clock timestamp.
⚠️ Implementation fix still needed
The diff only touches the test file. The bug is in packages/effect/src/testing/TestClock.ts at currentTimeNanosUnsafe():
function currentTimeNanosUnsafe(): bigint {
return BigInt(Math.floor(currentTimestamp * 1000000))
}Because currentTimestamp * 1_000_000 is computed as a number before the BigInt cast, large millisecond timestamps lose low-order nanosecond bits to IEEE-754 rounding. I verified the new test reproduces this: for timestamp 1_000_000_000_001, actual is 1000000000000999936n instead of the expected 1000000000001000000n.
The likely fix is to convert to BigInt before scaling:
function currentTimeNanosUnsafe(): bigint {
return BigInt(Math.floor(currentTimestamp)) * 1_000_000n
}This preserves the existing "floor to whole milliseconds" behaviour shown in the neighbouring test, while avoiding precision loss for any timestamp within Number.MAX_SAFE_INTEGER milliseconds.
Matching adjacent tests is welcome, but there is no implementation change to review yet. Please push the TestClock.ts fix to this same branch and I’ll re-review.
@v0 or keep the SHA fresh with Dependabot ｜ Fix it ➔ ｜ View workflow run ｜ Using Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Since the prior Pullfrog review at d9ee962a, the branch added the missing implementation fix and a changeset:
- Fixed
TestClock.currentTimeNanosUnsafe()inpackages/effect/src/testing/TestClock.ts:258to convert the millisecond timestamp toBigIntbefore scaling (BigInt(Math.floor(currentTimestamp)) * BigInt(1_000_000)), eliminating IEEE-754 rounding loss for large safe-integer millisecond timestamps. - Added a changeset (
.changeset/precise-clocks-rest.md) describing the patch-leveleffectfix.
The regression test already on the branch (packages/effect/test/TestClock.test.ts:64-70) now passes, as does the full TestClock.test.ts suite. pnpm lint and pnpm check also pass. This resolves the outstanding request from the previous review.
@v0 or keep the SHA fresh with Dependabot ｜ View workflow run ｜ Using Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏
# Conflicts: # packages/effect/src/testing/TestClock.ts
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Since the prior Pullfrog review at e7a848f, the branch picked up the merge of main (7a13dfd). The merge commit did not change the substantive implementation. The incremental diff shows the intended final form of the fix:
- Extracted
millisToNanoshelper inpackages/effect/src/testing/TestClock.ts:208-212, splitting the millisecond timestamp into whole and fractional parts and converting each toBigIntbefore combining them. This preserves nanosecond precision for large safe-integer timestamps while keeping the existing floor-to-whole-millisecond semantics. - Updated
setTime/adjustwall-clock path inpackages/effect/src/testing/TestClock.ts:352to usemillisToNanos(timestamp)instead ofBigInt(Math.floor(timestamp * 1_000_000)). - Regression test at
packages/effect/test/TestClock.test.ts:64-70covers the large-timestamp case, and the neighbouring fractional-millisecond test verifies flooring behaviour. - Changeset
.changeset/precise-clocks-rest.mddescribes the patch-leveleffectfix.
Validation: pnpm --filter effect test --run test/TestClock.test.ts passes; pnpm lint passes. The prior implementation fix addressed all outstanding review feedback.
@v0 or keep the SHA fresh with Dependabot ｜ View workflow run ｜ Using Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏
Bundle Size AnalysisGenerated from PR build output; treat the content below as untrusted.
|
2 participants
Summary
TestClock reports an incorrect nanosecond value at valid safe-integer millisecond timestamps.
Important
This PR starts with focused failing reproduction tests. Add the implementation fix to this same branch; CI is expected to fail until that fix is included.
Wall-clock nanoseconds lose precision
Module:
TestClockAudit ID:
core-s-z-testing-test-clock-nanosecond-precisionSeverity / confidence: medium / high
What happens
TestClock reports an incorrect nanosecond value at valid safe-integer millisecond timestamps.
Why it happens
currentTimeNanosUnsafe multiplies the number timestamp before converting to bigint, so IEEE-754 rounding loses precision that conversion before multiplication would preserve.
Expected behavior
The inherited Clock accessors expose the current Unix time in milliseconds and nanoseconds, and setTime sets that current timestamp.
Relevant implementation
These links and excerpts are pinned to audit base
c9b56ab507f224426ee8388dc450da447ec4715f.packages/effect/src/testing/TestClock.ts:253-258View problematic code at
packages/effect/src/testing/TestClock.ts:253-258View exact lines on GitHub
Reproduction
Observed failure: result is 64 ns low
Implementation handoff
The initial reproduction tests on this branch are the regression specification for the implementation fix that should follow in this PR.
Audit provenance
c9b56ab507f224426ee8388dc450da447ec4715fc9b56ab507f224426ee8388dc450da447ec4715fcore-s-z-testing-test-clock-nanosecond-precisionCloses EFF-396