Repository navigation
Conversation
The test signs the same payload once asynchronously and once synchronously and compares the tokens. Each sign() reads Date.now() for iat. When a second boundary falls between the two calls the tokens differ and expect() throws inside the jws callback. jws catches the throw and emits 'error', but jsonwebtoken's once() wrapped callback already ran, so the error is lost and the test times out after 90s with no message. Pin iat in the payload so both tokens are deterministic. Wrap the assertions in try/catch so a failure reaches done(err).
|
Status: closed. #45873 landed a fix for this test on main, so this PR is not needed. Reproduction: patch |
|
Updated 5:27 PM PT - Sep 3rd, 2026
❌ @robobun, your commit bc61e7b has 2 failures in
🧪 To try this PR locally: bunx bun-pr 41304That installs a local version of the PR into your bun-41304 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 4 remain after this review. WalkthroughThe async JWT signing test freezes and restores system time so asynchronous and synchronous tokens receive matching ChangesJWT signing test
Merge Risk: ⚪ Minimal · up to The JWT signing test now uses a stable timestamp and reports callback failures directly, removing the intermittent timeout without introducing an identified merge risk. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Full details: Description checkExplanation The description explains the race, the failure mode, the implementation, and verification results. It does not use the exact template headings, but it provides the required information and is sufficiently complete. Comment |
|
Review noted, nothing to change. Buildkite build 109876 is in progress. The claude-find-issues action failed inside the action itself before it ran anything, which is not related to this diff. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — a clean de-flake that keeps asserting the original property (async and sync sign produce identical tokens) while removing the wall-clock race.
What was reviewed:
- Pinning
iatstill exercises the same HS256 sign path; jsonwebtoken keeps a caller-providediat, so both tokens are now deterministic. - Sharing one
payloadobject between the twosign()calls is safe —mutatePayloaddefaults to false (the sibling test at L123-135 asserts exactly this), so the async call cannot alter it before the sync call. - The try/catch →
done(e)wrapper matches the "wire failures to reject, never throw inside event callbacks" convention and turns the silent 90s timeout into an immediate diff.
Extended reasoning...
Overview
This PR touches a single file, test/js/third_party/jsonwebtoken/async_sign.test.js, and only one test case within it. It pins the iat claim in the JWT payload to a fixed timestamp so the async and sync jwt.sign() calls produce byte-identical tokens regardless of when a wall-clock second boundary falls, and it wraps the callback's assertions in try/catch → done(e) so a failed expect() surfaces as a test failure instead of being swallowed by jws's internal try/catch and hanging until the 90s timeout.
Security risks
None. This is a test-only change to a vendored third-party compatibility test. No production code, no crypto logic, no user-facing surface is touched. The pinned timestamp is an arbitrary constant used only to make two HMAC inputs identical.
Level of scrutiny
Low. The diff is ~15 lines in a single test case. The fix addresses a well-understood race (two Date.now() reads straddling a second boundary) and the PR description documents a reproduction that confirms both the mechanism and the fix. The de-flake preserves the original assertion's intent — it still compares the async token against the sync token for equality, just with deterministic inputs — which satisfies the REVIEW.md rule that de-flakes must "keep asserting the property the original assertion protected."
Other factors
I checked whether reusing the same payload object across both jwt.sign() calls could be a problem (if the async call mutated it before the sync call ran). It isn't: mutatePayload defaults to false and jsonwebtoken shallow-copies the payload before adding claims — the same file's test at L123-135 asserts this contract directly. The two added comments are concise and explain genuinely non-obvious behavior (the iat race and the jws error-swallowing path), passing the CLAUDE.md comment-hygiene bar. No CODEOWNERS entry covers this path, there are no prior reviews or outstanding objections on the PR, and the bug hunt exited on dry_streak with no findings.
|
Thanks. The shared payload object is safe for the reason you give: mutatePayload defaults to false, so the async sign works on a copy. No change needed. Build 109876 is still running. |
|
pretty sure upstream tests solve this with fake timers. can we do the same? |
The sibling claim tests in this directory already use
sinon.useFakeTimers({ now: 60000 }). Do the same here so the async
and the sync sign read the same Date.now() and the payload stays
the upstream { foo: "bar" }.
|
Done in b35dd70. The test now uses sinon.useFakeTimers({ now: 60000 }) in beforeEach and uninstalls it in afterEach, the same pattern as claim-iat and the other claim tests in this directory. The payload is back to the upstream { foo: "bar" }. sinon leaves process.nextTick alone by default, so the jws async path still runs. Checked that Date.now() reads 60000 on both sides of the nextTick and the signed iat is 60. The try/catch around the assertions stays, since jws swallows a throw from that callback. |
|
we can't use bun:test fake timers? |
|
Yes. 86ba1c3 uses setSystemTime(60000) from bun:test in beforeEach and setSystemTime() in afterEach, and drops the sinon import. setSystemTime only fixes Date.now() and new Date(), so process.nextTick and the jws async path run as before. Checked that Date.now() reads 60000 on both sides of a nextTick, the signed iat is 60, and the clock is real again after the reset. Passes on the debug and the release build. |
There was a problem hiding this comment.
LGTM — the setSystemTime approach is the right fix and the done(e) forwarding turns the silent timeout into an actual failure report.
What was reviewed:
- Confirmed
setSystemTimeonly touchesDate, so the jwsnextTick-driven async path still runs andafterEachrestores the real clock even on failure. - Checked all sibling tests in the
when signing a tokenblock — none depend on wall-clock time, so the frozen epoch of 60 s is harmless to them. - Traced the swallowed-throw claim: jws
SignStream.signwraps the callback in try/catch and re-emits aserror, and jsonwebtoken's listener isonce()-wrapped, sotry { ... } catch (e) { done(e) }is necessary here.
Extended reasoning...
Overview
This PR touches a single file, test/js/third_party/jsonwebtoken/async_sign.test.js, to deflake a comparison between an async-signed and sync-signed JWT. The root cause is well-established: each jwt.sign() reads Date.now() independently for the iat claim, so a second boundary between the two calls yields different tokens, and jws's internal try/catch swallows the failed assertion so the test times out at 90 s instead of reporting the diff. The fix freezes the clock with setSystemTime(60000) in beforeEach, restores it in afterEach, and forwards assertion failures to done(e) so future mismatches surface immediately.
Security risks
None. This is a test-only change to a third-party compatibility test. No runtime, native, or user-facing code is touched. setSystemTime is scoped to the test process and reset in afterEach.
Level of scrutiny
Low. The diff is 22 lines added / 5 removed in one test file, with no CODEOWNER coverage. The mechanism is documented in the PR body with a working repro, and the approach matches REVIEW.md guidance verbatim: freeze the clock rather than sleep or retry, and wire every failure path to the done callback rather than throwing inside an event callback. I read the full file to confirm the beforeEach/afterEach hooks apply to the whole when signing a token block and verified none of the sibling tests (empty options, RS/PS error paths, mutatePayload, falsy-secret loop) depend on real time — they only assert on error presence, token shape, or property mutation, all unaffected by an iat of 60.
Other factors
The PR went through two revisions after maintainer comments (pinned iat → sinon fake timers → setSystemTime from bun:test), and the current diff reflects the final requested approach. The two comments added survive the "would the next reader spend tool calls discovering this" test — the jws swallowing behavior in particular is genuinely non-obvious. Bug hunt exited on dry_streak with no findings.
|
Thanks for the review of 86ba1c3. It needs no change. |
|
Closing. #45873 landed a fix for this test. On main, both One difference remains. This PR also wrapped the assertions in |
Problem
test/js/third_party/jsonwebtoken/async_sign.test.jsfails on main withthis test timed out after 90000ms, before its done callback was calledin "should return the same result as singing synchronously". Seen in builds 108836, 109097, 109276, 109807 and 109861, on five different lanes.{ foo: "bar" }once asynchronously and once synchronously and compares the tokens. Eachjwt.sign()readsDate.now()for theiatclaim (jsonwebtoken/sign.js:185). When a second boundary falls between the two calls, the tokens differ andexpect()throws.SignStream.prototype.sign(jws/lib/sign-stream.js), whosetry/catchemits'error'. jsonwebtoken attached the sameonce()-wrapped callback as the error listener, and it already ran. Sodone()never runs and the test waits out its ceiling.Fix
setSystemTime(60000)frombun:testinbeforeEachand reset it withsetSystemTime()inafterEach. It only fixesDate.now()andnew Date(), soprocess.nextTickand the jws async path run as before. The payload stays the upstream{ foo: "bar" }.try/catchand pass a failure todone(err). A future mismatch reports the diff at once instead of a silent 90 s timeout.Date.nowpatched to cross a second boundary, the old test reproduces the exact timeout with the release binary. WithsetSystemTime(60000),Date.now()reads 60000 before and after anextTick, and the signediatis 60. A forcedexpectfailure now reportsExpected/Receivedin 3 ms.bun bd test test/js/third_party/jsonwebtoken/async_sign.test.jspasses, as does all oftest/js/third_party/jsonwebtoken/.Background
Date.now()reads is about 1.5 ms on a release build and about 107 ms on a debug build (most of it is the firstutil.formatcall). That gives roughly a 0.15% to 10% chance per run, depending on the build and the load on the runner.Notes
The first version of this PR pinned
iat: 1700000000in the payload instead. Review asked for fake timers like the upstream tests, then for thebun:testones. The later commits switch tosetSystemTimeand restore the upstream payload.Repro used to confirm the mechanism (release bun,
USE_SYSTEM_BUN=1):With the original test body this prints two tokens that differ only in
iatand then times out.Window measurement (time between the two
Date.now()calls, first test in a fresh process): debug build 106 to 107 ms over 3 runs, release 1.56 to 1.57 ms. Of the debug cost,util.formatfirst call was 149 ms in a separate probe,process.nextTick32 ms, first HMAC 6 ms.CI on the first revision (build 109876): one red lane,
pnpm-migration.test.tson debian 13 x64-asan, where Verdaccio exited with code 2 before the test ran. Reported separately. Two other lanes passed on retry.[auto-merge] gate passed · iteration 2 · 1 files touched
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 0 rejected · iteration 2
evidence per changed file