Repository navigation
bun test --isolate: deactivate leaked fake timers between files - #36385
Conversation
WalkthroughChangesThe change deactivates leaked fake timers before isolation handles close, adds cleanup for abort-signal timeout references, and introduces serial and parallel regression tests covering real timers and server activity across isolated test files. Fake timer isolation cleanup
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
vi.useFakeTimers() stores its active flag in the per-thread timer::All, not on the JS global, so a test file that activates fake timers and never restores them (an it.failing that throws before useRealTimers(), for instance) left fake_timers.active = true across the isolation global swap. Every setTimeout in subsequent files was then routed into the never-driven fake heap. net.Server.listen() emits 'listening' via setTimeout, so any later file that listens on a port hung until its per-test timeout. In the CI parallel bucket this showed up as test/regression/issue/29684.test.ts timing out on every test whenever it landed in the same worker after test/js/bun/test/fake-timers/sinonjs/issue-2449.test.ts (or issue-276). close_isolation_handles now deactivates fake timers (and releases the heap pins) before the global swap, so each file starts with real timers regardless of what the previous file left behind.
597a9bb to
2fafc35
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/runtime/jsc_hooks.rs:1656-1670— Draining the fake heap here viadeactivate()→clear()unlinksAbortSignalTimeoutnodes without releasing the+1they hold on the C++AbortSignal, so the cycle-breaking pass incancel_all_timeout_objects(which the subsequentswap_global_for_test_isolationstill calls) now walks an empty fake heap and never sees them — leaking both theAbortSignaland its boxedTimeoutfor the process lifetime. Before this PR the isolation swap freed these correctly. ExtendFakeTimers::clear()to null(*t).signalandunref()forAbortSignalTimeoutentries the same waycancel_all_timeout_objectsdoes at mod.rs:1252-1265 (which also fixes the pre-existinguse_real_timers/clearAllTimershole).Extended reasoning...
What leaks
AbortSignal.timeout(ms)boxes a RustTimeoutand forms a two-way refcount cycle with the C++AbortSignal: the C++ side holdsm_timeout(raw*mut Timeout) and takessignal->ref()on behalf of the Rust box (AbortSignal.cpp:71), while the RustTimeoutstores that+1'd pointer in.signal. The only free paths for the box are (a) the timer fires and the signal aborts, or (b) something breaks the cycle by nulling.signalandunref()'ing so~AbortSignal→cancelTimer()→AbortSignal__Timeout__deinitcan run.Code path
EventLoopTimerTag::allow_fake_timers()falls through to_ => trueforAbortSignalTimeout(EventLoopTimer.rs:212-222), so undervi.useFakeTimers()anAbortSignal.timeout(...)is routed intofake_timers.timersbyAll::insert(mod.rs:679-685).Pre-PR:
close_isolation_handlesdid not touch fake timers.swap_global_for_test_isolation→hooks.cancel_all_timers→All::cancel_all_timeout_objectswalked both heap roots —[(*this).timers.0.root, (*this).fake_timers.timers.0.root]at mod.rs:1193 — collected eachAbortSignalTimeoutnode, and at mod.rs:1252-1265 removed it from the heap, nulled(*t).signal, and calledunref()on the C++ signal. That drops the cycle's+1; when the JS wrapper is collected,~AbortSignal→cancelTimer()frees the box.Post-PR:
close_isolation_handlesruns before the swap at both call sites (test_command.rs:3125-3126, parallel/runner.rs:550-551) and now callsfake_timers.deactivate()→clear().clear()delete_min()s every node, setsin_heap = None/state = CANCELLED, but only branches ontag == TimeoutObject—AbortSignalTimeoutnodes are silently unlinked with.signalstill holding the+1. By the timecancel_all_timeout_objectsruns,fake_timers.timers.0.rootis null and those boxes are unreachable from either heap root, sosignal_timeoutsstays empty and the cycle-break pass never runs.Why nothing else catches it
The only other release of the Timeout's
+1isAbortSignal::eventListenersDidChange(), which requires an observer add/remove to fire — a bareAbortSignal.timeout(n)(or one used only asfetch(url, { signal: AbortSignal.timeout(n) })) never triggers it. GC of the JS wrapper drops the wrapper's ref (2→1), but the Timeout's+1keeps theAbortSignalrefcount at ≥1 forever, so~AbortSignalnever runs andcancelTimer()never frees the box.Step-by-step
Isolated file A under leaked fake timers:
vi.useFakeTimers(); const s = AbortSignal.timeout(100); // box in fake heap; AbortSignal refcount = 2 (wrapper +1, Timeout +1) // file ends without vi.useRealTimers()
close_isolation_handles→deactivate()→clear(): node popped,in_heap=None,state=CANCELLED.tag == AbortSignalTimeout≠TimeoutObject→ not pushed topinned..signalstill non-null,+1still held.swap_global_for_test_isolation→cancel_all_timeout_objects:fake_timers.timers.0.root == null, real-heap root has no such node →signal_timeoutsempty → cycle-break loop skipped.- Global swap + GC: wrapper collected →
AbortSignalrefcount 2→1. Stuck at 1 forever. ~AbortSignalnever runs →cancelTimer()never runs → boxedTimeoutand C++AbortSignalboth leak for the process lifetime.
Repeat once per isolated file that hits this pattern across a
--isolate/--parallelrun.Impact & fix
This is a regression in exactly the scenario the PR targets (leaked fake timers under
--isolate): the pre-PR path freed these; the post-PR path does not. Per REVIEW.md, refcounts must be provably balanced on every terminal path, and re-ordering a release before a fallible/consuming call requires re-auditing everything it pre-empts.Fix in
FakeTimers::clear(): forEventLoopTimerTag::AbortSignalTimeout, recover the parent viaAbortSignalTimeout::from_timer_ptr(timer), null.signal, andAbortSignal::opaque_ref(signal).unref()— mirroringcancel_all_timeout_objects. That also closes the identical pre-existing hole invi.useRealTimers()/vi.clearAllTimers(), which shareclear()("fix the whole class in the same PR").
clear() previously only released the heap pin for TimeoutObject nodes. AbortSignalTimeout nodes were unlinked with .signal still holding its +1 on the C++ AbortSignal, leaving the AbortSignal <-> Timeout refcount cycle intact so neither side ever freed. This was a pre-existing leak in vi.useRealTimers()/vi.clearAllTimers() and became a regression for the --isolate path once close_isolation_handles started draining the fake heap before cancel_all_timeout_objects (which would otherwise have walked the fake heap and broken the cycle). Mirror the cycle-break from All::cancel_all_timeout_objects: null .signal and unref() so ~AbortSignal -> cancelTimer() can free the box.
|
Good catch on the AbortSignalTimeout cycle. Fixed in 44c0e26: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/test_runner/timers/FakeTimers.rs`:
- Around line 157-166: Add a regression test covering the
EventLoopTimerTag::AbortSignalTimeout cleanup path that observes the AbortSignal
allocation/refcount being released after fake timers are deactivated. Ensure the
test fails if AbortSignalTimeout::signal is not cleared or
AbortSignal::opaque_ref(signal).unref() is omitted, while preserving the
existing isolation behavior.
In `@test/cli/test/isolation.test.ts`:
- Around line 330-334: Update the promise in the server setup around
server.listen so it rejects on the server's error event by attaching
server.once("error", reject), while retaining callback resolution for successful
binding. Ensure bind failures surface immediately instead of leaving the promise
pending.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: da21b9ad-5c41-46fd-8c40-cbebd994779b
📒 Files selected for processing (3)
src/runtime/jsc_hooks.rssrc/runtime/test_runner/timers/FakeTimers.rstest/cli/test/isolation.test.ts
…rain Reverts the AbortSignalTimeout unref in FakeTimers::clear() from 44c0e26. That unref is unsafe on the mid-test useRealTimers()/clearAllTimers() path: it leaves C++ m_timeout non-null, so a later removeEventListener reaches eventListenersDidChange() and deref()s the same +1 again, freeing the AbortSignal under its live JS wrapper (ASAN heap-use-after-free in JSAbortSignal visitChildren, reproduced with Malloc=1). Instead, the --isolate file boundary now calls a new reset_for_isolation() that only flips active=false and clears CURRENT_TIME. The fake heap is left populated so swap_global_for_test_isolation -> cancel_all_timeout_objects (which already walks both heaps and breaks the AbortSignal cycle, and runs at a point where no user JS can touch the outgoing signals) drains it. clear()/deactivate() are restored to their pre-PR form.
There was a problem hiding this comment.
LGTM — the final reset_for_isolation approach is the minimal safe fix.
Checked that both call sites (test_command.rs:3125, parallel/runner.rs:550) run close_isolation_handles immediately before swap_global_for_test_isolation, whose cancel_all_timers hook (jsc_hooks.rs:1601 → All::cancel_all_timeout_objects) still walks the un-drained fake heap and releases both TimeoutObject pins and AbortSignalTimeout cycle refs. CURRENT_TIME.clear also resets vm.overridden_performance_now; the setTimeout.clock marker doesn't need clearing since the global is swapped. The two earlier findings (AbortSignal cycle leak, mid-test clear() UAF) are both resolved by ed45e77 reverting to flag-only reset.
Extended reasoning...
Overview
Fixes a CI flake where vi.useFakeTimers() state (stored on the per-thread timer::All, not the JS global) leaks across the --isolate / --parallel file boundary, causing the next file's setTimeout-driven callbacks (e.g. net.Server 'listening') to route into the never-driven fake heap and hang. Net change: a 2-line FakeTimers::reset_for_isolation() that flips active = false and clears CURRENT_TIME, called from close_isolation_handles before the global swap; plus a 60-line isolation test covering both --isolate and --parallel.
Security risks
None. Test-runner isolation boundary only; no user-facing API, parsing, or network surface.
Level of scrutiny
Medium — touches per-thread timer state and the isolation teardown path, both of which have refcount/GC-lifetime implications. This PR went through two revision cycles on exactly those concerns: my first review caught that draining the fake heap in close_isolation_handles would bypass cancel_all_timeout_objects' AbortSignal cycle-break; the second caught that adding the cycle-break to FakeTimers::clear() (reachable mid-test via vi.useRealTimers()) would double-deref() the C++ AbortSignal when eventListenersDidChange later fires. The author confirmed the UAF under ASAN and landed the minimal fix in ed45e77: reset mode only, leave the heap intact for the existing swap-time drain. The final diff touches no refcounts and no heap nodes.
Other factors
- Verified the ordering contract: both callers run
close_isolation_handles→swap_global_for_test_isolationback-to-back, and the swap'scancel_all_timershook walksfake_timers.timers.0.root(timer/mod.rs:1193), so leaving the heap populated is correct. - The redundant
!all.is_null()guard is harmless (stateis already checked non-null at function entry). - Test follows the neighboring
chdirisolation test's exact structure (serial--isolate+ gated--parallel=2), wiresserver.once('error', reject), and bounds failure with--timeout=5000. Evidence block shows fail-before (ASAN) / pass-after on both debug and release. - All CodeRabbit and comment-cop threads are resolved.
|
Opened #36398 which resets at the end of |
|
Build 85401 failures are all pre-existing flakes unrelated to this diff (parallel.test.ts dots ordering, security-scanner TTY, fetch-tls-abortsignal timing, 08965 on Windows, quic RTT, fastutf8stream-reopen, bun-install-registry, spawn-stdin-destroy, spawn-streaming-stdout). |
2 participants
Symptom
test/regression/issue/29684.test.tstimes out on every test (10 × 90s) in the CI parallel bucket on ~4% of builds, then passes in 66ms on solo retry:Repro
BUN_TEST_PARALLEL_SCALE_MS=999999999 bun test --parallel=2 --timeout=5000 \ test/js/bun/test/fake-timers/sinonjs/issue-2449.test.ts \ test/regression/issue/29684.test.ts(Scale-up gated so one worker runs both files sequentially.)
Cause
vi.useFakeTimers()stores itsactiveflag in the per-threadtimer::All, not on the JS global. Under--isolate/--parallel,swap_global_for_test_isolationreplaces the global and cancels pending timers but never touchesfake_timers.active, so a file that activates fake timers and never restores them leaks the flag into every subsequent file in the same worker.test/js/bun/test/fake-timers/sinonjs/issue-2449.test.ts(andissue-276.test.ts) haveit.failingcases that callFakeTimers.install()and then throw on anassert.exceptionbefore reachingclock.uninstall(), sovi.useRealTimers()never runs.With
fake_timers.active == true,All::insertroutes newsetTimeoutcalls into the fake heap, which only advances onadvanceTimersByTime.net.Server.listen()emits'listening'viasetTimeout(emitListeningNextTick, 1, this)(src/js/node/net.ts), so 29684'sserver.listen(0, "127.0.0.1", cb)callback never fires and the WebSocket client is never created.Fix
close_isolation_handlesnow deactivates fake timers and releases their heap pins before the global swap, matching how it already closes leaked watchers and servers. Each file starts with real timers regardless of what the previous file left behind.Verification
Also re-ran the exact 71-file CI batch from build 85266 with
--parallel=3three times; 29684 passes all three.[review] gate passed · iteration 0 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 0
evidence per changed file