fix(test): let the signal-less follower join the owner's flight instead of racing it - #3634
Conversation
…ad of racing it
`./src/transforms/esm/http-cache.test.ts => HTTP Bundle Cache ... returns a
signal-less cache follower after its bounded wait ... FAILED (527ms)`
error: Error: The signal-less follower's bounded wait did not settle
within 200 fake timer steps
That message and its budget come from #3631, which replaced a fixed
`time.tickAsync` advance with a helper that steps fake timers until the wait
settles. The helper did its job: it turned a hang that silently dropped a whole
test file into a named failure. The cause it named — a bounded wait armed past a
single fixed advance — is not what was happening.
The test starts a cancellable owner and, one `time.runMicrotasks()` later, a
signal-less follower, and assumes the owner registered the shared flight in
between. Nothing enforces that. Both calls reach `createInFlightHttpFetch`
through real filesystem work, so I/O completion order decides which of them owns
the flight. Probing `inFlightHttpFetches.size` at the moment the follower is
created reads 1 in 8 of 8 unloaded runs of the file alone, and 0 under a loaded
coverage shard: the race is open there, and the follower can win it.
When it does, the two callers swap roles. The signal-less call becomes the flight
owner and parks on the distributed read this test never resolves. The
signal-bearing call becomes the bounded waiter, and `waitForSharedInFlightHttpFetch`
deliberately keeps a cancellable caller's waiter lease across timeouts: it re-arms
a fresh `HTTP_MODULE_FETCH_MAX_WAIT_MS` wait after every one. Every step of
`runFakeTimersUntil` then fires that timer and the next step finds a new one, so
the budget is consumed rather than reached, and the follower the test is watching
never settles because it is now the owner. `assertEquals(waiterCount, 2)` passes
throughout — two waiters is true of both role assignments.
Evidence. A reproduced shard failure traces 200 identical steps, each advancing
the fake clock by exactly 35300ms with the waiter count pinned at 2 and one flight
registered. Instrumenting the wait loop records 200 iterations, every one
`hasSignal=true waitTimeoutMs=35300 same=true`. Starting the two callers in the
reverse order — which is exactly what losing the race amounts to — reproduces the
identical failure 3 times out of 3.
The fix waits for the owner's flight to be registered before creating the
follower, so the follower joins that flight instead of racing it, and asserts the
shared generation is still the owner's once both callers are on it. The two
ad-hoc "stat and drain microtasks" loops collapse into one `runRealTurnsUntil`
helper that names what it is waiting for when it gives up. `runFakeTimersUntil`
is unchanged; its comment no longer attributes the incident to a late-armed timer
and says plainly that its budget is a stuck detector, not a patience dial.
Verified: 50 consecutive runs of the file and 16 runs of coverage shard 4/8 under
`DENO_JOBS=64` on a saturated machine — the conditions under which the failure
reproduces on this branch's parent — all clean.
|
Warning Review limit reached
Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Comment |
The failure
Merge-queue run 31578405141, coverage shard 4/8 (job 94055658155), base
0ee5098b4:That message and its budget come from #3631, which replaced a fixed
time.tickAsync(HTTP_MODULE_FETCH_MAX_WAIT_MS)advance withrunFakeTimersUntil— step fake timers until the wait settles, with a 200-step budget. #3631 did
something valuable: it turned a hang that silently dropped a whole test file into
a named failure. The cause it named is not the cause.
Root cause
The test starts a cancellable owner and, one
time.runMicrotasks()later, asignal-less follower, and assumes the owner registered the shared flight in
between. Nothing enforces that. Both calls reach
createInFlightHttpFetchthrough real filesystem work, so I/O completion order — not call order — decides
which one owns the flight.
Probing
inFlightHttpFetches.sizeat the moment the follower is created:1in 8 of 8 runs — owner already registered0— race wide openWhen the follower wins that race the two callers swap roles:
read this test never resolves;
waitForSharedInFlightHttpFetchdeliberately keeps a cancellable caller'swaiter lease across timeouts, re-arming a fresh
HTTP_MODULE_FETCH_MAX_WAIT_MSwait after every one.
So every step of
runFakeTimersUntilfires a timer and the next step finds a newone. The budget is consumed, never reached, and the promise the test is
watching never settles because it is now the owner.
assertEquals(waiterCount, 2)passes throughout — two waiters is true of bothrole assignments, which is why the test could not tell it was exercising the
wrong caller.
Evidence
fake clock by exactly
35300ms (HTTP_MODULE_FETCH_MAX_WAIT_MS) with thewaiter count pinned at 2 and one flight registered.
hasSignal=true waitTimeoutMs=35300 same=true waiters=2— the looping waiteris the signal-bearing caller, not the follower the test names.
the race amounts to — reproduces the identical failure 3 times out of 3.
DENO_JOBS=64on a saturated machinereproduces it on the unpatched parent commit.
Why #3631's 100 clean runs missed it
They exercised the path; they just always won the race. Run the file alone and
the owner has registered before the follower starts, every time (8/8 above). The
inversion needs the loaded, parallel, coverage-instrumented shard. #3631's own
symptom —
Promise resolution is still pending but the event loop has already resolved— is this same inversion seen through the old fixed advance: one tickfired one re-armed wait, and
await followerOutcomethen blocked forever.The fix
Wait for the owner's flight to be registered before creating the follower, so the
follower joins that flight instead of racing it, and assert the shared
generation is still the owner's once both callers are on it. The two ad-hoc
"stat and drain microtasks" loops collapse into one
runRealTurnsUntilhelperthat names what it was waiting for when it gives up.
runFakeTimersUntilis unchanged and its budget stays at 200. Its comment nolonger attributes this incident to a late-armed timer, and now says plainly that
the budget is a stuck detector, not a patience dial — a caller that re-arms after
every timeout exhausts any budget, and raising it only restores the hang.
#3631 is not reverted. Without its step budget this would still be a hang
that drops a whole test file with no name attached to it.
Only
src/transforms/esm/http-cache.test.tschanges. No production code changes:the role inversion is legitimate production behaviour (either concurrent render
may win), so the defect was entirely in the test's assumption.
Verification
src/transforms/esm/http-cache.test.ts, 50 consecutive runsDENO_JOBS=64, saturated machine, 16 runs377 passed (3403 steps) | 0 faileddeno fmt --check/deno lint/deno check(Deno 2.7.7)Sibling two-caller tests in the same file (
fetchStarted,cacheLookupStarted,renameStarted,oldWriteStarted) already gate on an explicit promise, so thiswas the only test in the file relying on
runMicrotasks()for ordering.