Repository navigation
Conversation
|
Status
|
|
Warning Review limit reached
On-demand reviews are free for the next 14 days. After that, they cost $0.25 per reviewed file. Or wait 7 minutes for your next included review. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (6)
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 (6)
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review. WalkthroughChangesThe runtime now detects stopped realms, resets fake timers during isolation, and ignores timer-control calls from retired globals. A regression test covers asynchronous continuations in serial and parallel isolation modes. Timer isolation for stopped realms
Suggested reviewers: Merge Risk: ⚪ Minimal · up to No actionable merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 11:01 PM PT - Sep 6th, 2026
❌ @robobun, your commit 7655be4 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 41790That installs a local version of the PR into your bun-41790 --bun |
Under --isolate a file's jest/vi object outlives the file. A continuation the file leaked (a threadpool job's .then) runs in the retired global while a later file owns the thread, and the fake clock is per thread, so jest.useFakeTimers() from there switched the running file to fake timers (its real setTimeout never fired), and useRealTimers()/runAllTimers() drained the running file's fake heap. Every fake-timer control now checks whether its realm's ScriptExecutionContext was stopped (the first step of the isolation swap) and does nothing if so. The boundary reset moves from stop_active_handles into the cancel_all_timers hook, which the swap runs after the last point where the outgoing realm's JS runs as the live realm, so an activation in that window is undone too.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it reorders where the fake-timer reset runs in the --isolate swap sequence (from stop_active_handles into the cancel_all_timers hook), a human familiar with that lifecycle should confirm the "after the last point the outgoing realm's JS runs as live" claim.
What was reviewed:
- FFI signature match:
Zig__GlobalObject__isContextStoppedC++ definition vs Rustsafe fnextern — parameter and return types line up;dynamicDowncasthandles non-Zig globals. - All nine
#[bun_jsc::host_fn]entries in FakeTimers.rs plusBun__FakeTimers__setSystemTimeare guarded; inert return shapes (frame.this()/0/false) match each function's normal return. - Test follows harness conventions —
tempDir, concurrent pipe drain, marker-file handshake withsetImmediatepolling (no sleeps), stderr asserted before exitCode, added to the existingdescribe.concurrentblock.
Extended reasoning...
Overview
The PR fixes cross-file fake-timer leakage under bun test --isolate. It adds a C++ FFI export Zig__GlobalObject__isContextStopped (src/jsc/bindings/ZigGlobalObject.cpp) with a Rust wrapper on JSGlobalObject (src/jsc/JSGlobalObject.rs), guards every fake-timer host function in src/runtime/test_runner/timers/FakeTimers.rs to become inert when called from a retired realm, and relocates the reset_for_isolation call from stop_active_handles into the cancel_all_timers hook in src/runtime/jsc_hooks.rs. A comment-only update in src/jsc/VirtualMachine.rs documents the new hook behavior, and a new concurrent test case is added to test/cli/test/isolation.test.ts covering both --isolate and --parallel=2 paths.
Security risks
None identified. This is test-runner infrastructure with no auth, crypto, network, or user-input parsing surface. The new FFI getter is a read-only predicate on an already-managed ScriptExecutionContext state; the dynamicDowncast safely handles non-Zig globals by returning false. No untrusted data flows into the guarded paths.
Level of scrutiny
Medium-high. The mechanical parts (guarding nine host functions with the same predicate, the FFI plumbing) are straightforward and I verified each entry point is covered with a return shape matching its contract. The subtle part is the lifecycle reordering: the PR moves the fake-timer reset from stop_active_handles (pre-swap) to cancel_all_timers (post-drain), and the correctness argument hinges on cancel_all_timers running after the last microtask drain where the outgoing realm's JS executes as the live realm. That ordering claim is stated in comments and the PR body but depends on the exact call sequence inside swap_global_for_test_isolation, which someone who owns that code should confirm. The stop_active_handles signature change (dropping &mut VirtualMachine) is a mechanical consequence and the _vm witness pattern on the teardown wrapper is preserved.
Other factors
The test is well-constructed per REVIEW.md: it awaits observable conditions via marker-file handshakes and setImmediate polling rather than sleeps, drains subprocess pipes concurrently, asserts output before exit code, uses tempDir/bunExe/bunEnv, and lives in the existing describe.concurrent block alongside sibling isolation cases. No CODEOWNERS entries cover the changed paths. The bug hunt exited on dry_streak with no candidate findings. I don't have high enough confidence in the swap-sequence ordering to approve without a maintainer glance, but nothing I checked raised a concrete concern.
|
On the ordering question (why
So an activation before |
8645049 to
7655be4
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| /// The realm's `ScriptExecutionContext` went through `prepareForDestruction()`: | ||
| /// a `bun test --isolate` global whose file finished (from the start of | ||
| /// [`VirtualMachine::swap_global_for_test_isolation`] on), or any global once | ||
| /// VM teardown began. JS the realm leaked (a threadpool job's `.then`, an | ||
| /// event handler) still runs and still reaches host functions; the ones that | ||
| /// drive state outside the realm check this to stay inert. `false` for a | ||
| /// global that is not a `Zig::GlobalObject`. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // `global_exit()`. Runs no user JS. So does a `jest.useFakeTimers()` | ||
| // the file left on: the hook puts the thread back on the real clock | ||
| // here, after the last drain above ran the outgoing realm's JS. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // The realm's context went through prepareForDestruction(): a `bun test --isolate` global retired by | ||
| // the function above, or any global once VM teardown began. Script the realm leaked can still run. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// in-heap `+1` ref and the JS pin drop before the GC sweep / `~VM`, and put | ||
| /// the thread back on the real clock. | ||
| /// `timer::All` lives in `bun_runtime`; callers (VM teardown, the `--isolate` | ||
| /// swap) are in `bun_jsc`, hence the hook. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// `cancel_all_timers` hook (the `--isolate` swap, VM teardown) right | ||
| /// before `cancel_all_timeout_objects`, which walks the still-populated | ||
| /// fake heap itself to release `TimeoutObject` pins and discard | ||
| /// `AbortSignalTimeout` timers once the outgoing global's JS has stopped. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Under `--isolate` a file's `jest`/`vi` object outlives the file: JS it | ||
| /// leaked (a threadpool job's `.then`) still runs in the retired realm while a | ||
| /// later file owns the thread. The fake clock is per thread, not per realm, so | ||
| /// from a retired realm every control below is inert: it neither installs, | ||
| /// drives, reads, nor removes the clock of the file that is running now. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
…tions (#41831) ### Problem - The `--isolate` / `--parallel` swap only sweeps what exists at the file boundary. Work a finished file left in flight lands later (a thread-pool job settles an old-realm promise, a killed child reports its exit). Its continuation ran during the next file, which adopted the timers, servers, subprocesses and `chdir` it made. - Cause: nothing marks the outgoing realm finished. The swap (`src/jsc/VirtualMachine.rs`) bumps `test_isolation_generation`, but JSC keeps running the old global's microtasks and `Job` completions ignore the generation. ### Fix - The swap first sets the old global's `microtaskRunnability` to `Discard`. JSC's drain checks it per task and drops the old realm's reactions, as WebCore does for a stopped document. - `Job` records its generation and `complete_erased` releases a stale completion unrun. This covers `then` bodies that call back directly (node:crypto). `EventLoop::run_callback*` does not enter a function of a retired realm (a killed child's late `onExit`), checked only under `--isolate`. - Verified: `test/cli/test/isolation.test.ts` (new cases fail on 1.4.3-canary, pass with the fix), `parallel.test.ts`, and the password, pbkdf2 and randomBytes suites. - Self-reviewed: 3 concerns, all documented limits (see Notes). ### Background - `--isolate` gives each file a fresh `Zig::GlobalObject` on one `JSC::VM`. The old global stays alive, so native code can still settle its promises and call its functions. - JSC attributes each microtask to a global. A promise reaction goes to the promise's own realm (`JSPromise::fulfillPromise` uses `realm()`). `MicrotaskQueue::drainImpl` reads that global's `microtaskRunnability()` before each task. - `Job` (`src/jsc/job.rs`) carries thread-pool work back to the JS thread. Timers already record the generation and skip a stale fire. <details><summary>Notes</summary> - Reported internally (fuzz ledger #31292) against 1.4.2 and canary d316760. Repro: `a.test.ts` leaks `Bun.password.hash(argon2id).then(() => { process.chdir("/"); setInterval(...); Bun.serve(...) })`. `b.test.ts` sleeps 2 s and sees the interval tick three times, the kernel cwd at `/`, and A's server answering its fetch. With this change B sees none of it, in `--isolate` and in `--parallel`. - The three checks sit at gates that already existed for VM teardown (`script_allowed()` in `complete_erased`, the exception gate in `run_callback`) or inside JSC. No API entry point (`Bun.serve`, `setTimeout`, `process.chdir`, ...) needed its own guard: the finished file's script does not run, so it creates nothing. - The swap is the finished file's process exit. A leaked pipeline that needs that file's JS to make progress (an un-awaited `Bun.build` with async plugins, say) now stalls instead of completing under the next file. - What can still reach a retired realm: calls into JS that use neither a microtask nor `run_callback` (an FFI `JSCallback`, a napi threadsafe function, a `FinalizationRegistry` cleanup), and functions of a `node:vm` context or `ShadowRealm` that the finished file created (those are separate globals and are not flipped). With a debugger attached (`--inspect`), JSC wraps microtasks in `DebuggableMicrotaskDispatcher`, which does not read runnability, so the microtask fence is off there. The `Job` and `run_callback` checks still apply in all of these cases, which covers the reported thread-pool path. - The new test does not sleep. A's leaked chain hops through the thread pool until B writes a marker, then acts. B turns the loop through the thread pool up to 40 times and asserts that A never acted, that A's killed child's `onExit` never ran, and that the kernel cwd is still the fixture dir. - #41790 makes the jest fake-timer controls inert when called from a stopped context, one symptom of this root cause. With this change the leaked `.then` that called them no longer runs. The two changes do not conflict. - Self-reviewed before opening. </details> <!-- robobun:evidence:begin --> --- **[human-review]** gate passed · iteration 0 · 6 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 2 FAILED $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/cli/test/isolation.test.ts bun test v1.4.3 (f42e980) test/cli/test/isolation.test.ts: (pass) bun test --isolate > without --isolate, leaked global is visible to next file [374.71ms] (pass) bun test --isolate > with --isolate, each file gets a fresh global [346.99ms] (pass) bun test --isolate > with --isolate, --preload re-runs in each file's fresh global [336.22ms] (pass) bun test --isolate > without --isolate, --preload still runs once (regression) [331.36ms] (pass) bun test --isolate > with --isolate, module state is not shared between files [389.39ms] (pass) bun test --isolate > with --isolate, a file's process.chdir() is undone before the next file [1350.35ms] (pass) bun test --isolate > with --isolate, a file's process.env writes with native side effects are undone before the next file [1480.66ms] (pass) bun test --isolate > with --isolate, cached module records keep short, Latin-1 and UTF-16 names [1276.02ms] (pass) bun test --isolate > with --isolate, leaked outbound socket is closed before next file [1447.26ms] (pass) bu ... (truncated) release without fix: 2 FAILED bun test v1.4.2 (744846f) test/cli/test/isolation.test.ts: (pass) bun test --isolate > without --isolate, leaked global is visible to next file [30.70ms] (pass) bun test --isolate > without --isolate, --preload still runs once (regression) [27.42ms] (pass) bun test --isolate > with --isolate, --preload re-runs in each file's fresh global [28.91ms] (pass) bun test --isolate > with --isolate, module state is not shared between files [29.02ms] (pass) bun test --isolate > with --isolate, each file gets a fresh global [34.52ms] (pass) --isolate: JSC options survive a bunfig.toml with an install hoist pattern [24.89ms] (pass) bun test --isolate > with --isolate, cached module records keep short, Latin-1 and UTF-16 names [36.08ms] (pass) bun test --isolate > leaked subprocesses are killed for every isolated file, not just the first [28.53ms] (pass) --isolate: SourceProvider cache covers node_modules .mjs and type:commonjs packages [23.80ms] (pass) --isolate: delete require.cache evicts the SourceProvider cache [27.58ms] (pass) --isolate: SourceProvider cache covers CommonJS modules [26.81ms] (pass) bun test --isolate > module-scope subprocesses are killed for every isol ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/cli/test/isolation.test.ts bun test v1.4.3 (f42e980) test/cli/test/isolation.test.ts: (pass) bun test --isolate > without --isolate, leaked global is visible to next file [355.46ms] (pass) bun test --isolate > with --isolate, each file gets a fresh global [335.90ms] (pass) bun test --isolate > with --isolate, --preload re-runs in each file's fresh global [323.85ms] (pass) bun test --isolate > without --isolate, --preload still runs once (regression) [296.10ms] (pass) bun test --isolate > with --isolate, module state is not shared between files [362.81ms] (pass) bun test --isolate > with --isolate, a file's process.chdir() is undone before the next file [1334.47ms] (pass) bun test --isolate > with --isolate, a file's process.env writes with native side effects are undone before the next file [1380.33ms] (pass) bun test --isolate > with --isolate, cached module records keep short, Latin-1 and UTF-16 names [1572.32ms] (pass) bun test --isolate > with --isolate, leaked outbound socket is closed before next file [1378.23ms] (pass) bu ... (truncated) release with fix: all passed $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) target linux-x64-gnu build type Release build dir ./build/release revision 03e9b9a features baseline 23 deps, 131 codegen, 1172 objects in 693ms ninja: Entering directory `/workspace/bun/build/release' [1/1248] mkdir codegen [2/1248] mkdir stamps [3/1248] mkdir obj [4/1248] mkdir pch [5/1248] gen ErrorCode+*.h [6/1248] install /workspace/bun bun install v1.4.2 (744846f) Checked 22 installs across 61 packages (no changes) [14.00ms] [7/1248] install /workspace/bun/packages/bun-error bun install v1.4.2 (744846f) Checked 1 install across 2 packages (no changes) [2.00ms] [8/1248] install /workspace/bun/src/node-fallbacks bun install v1.4.2 (744846f) Checked 111 installs across 104 packages (no changes) [8.00ms] [9/1248] fetch picohttpparser [picohttpparser] up to date [10/1248] gen .bind.ts → GeneratedBindings.cpp [11/1248] fetch tinycc [tinycc] up to date [12/1248] fetch zlib [zlib] up to date [13/1248] gen node-fallbacks/react-refresh.js Bundled 1 module in 8ms react-refresh.js 4.81 KB (entry point) [14/1248] gen bindgenv2 [15/1248] fe ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` src/jsc/JSValue.rs | 6 +++ src/jsc/VirtualMachine.rs | 4 ++ src/jsc/bindings/ZigGlobalObject.cpp | 14 ++++++ src/jsc/event_loop.rs | 17 +++++-- src/jsc/job.rs | 11 ++++- test/cli/test/isolation.test.ts | 92 ++++++++++++++++++++++++++++++++++++ 6 files changed, 138 insertions(+), 6 deletions(-) ``` </details> **gate history** · 1 passed · 0 rejected · iteration 0 <details><summary>evidence per changed file</summary> ``` file reads edits tests src/jsc/JSValue.rs 2 3 14 src/jsc/VirtualMachine.rs 2 2 14 src/jsc/bindings/ZigGlobalObject.cpp 2 2 14 src/jsc/event_loop.rs 5 7 14 src/jsc/job.rs 3 6 13 test/cli/test/isolation.test.ts 3 5 13 ``` </details> <!-- robobun:evidence:end --> --------- Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>
|
Closing: #41831 fixed the root cause. Under Checked on main at 85e9ddc (debug build):
The move of the boundary reset into |
2 participants
Problem
--isolate/--parallel, a continuation that a finished file leaked (a threadpool job's.then) runs in its retired global while the next file runs.jest.useFakeTimers()from there put the thread on fake timers: the running file's realsetTimeoutnever fired and ittimed out after 5000ms.FakeTimers.activeand the fake clock live in the per-threadtimer::All(src/runtime/test_runner/timers/FakeTimers.rs), and the host functions never checked which realm called them. bun test --isolate: deactivate leaked fake timers between files #36385 resets the flag at the file boundary only.Fix
setSystemTime, does nothing when its realm'sScriptExecutionContextis stopped (JSGlobalObject::is_context_stopped). The swap stops the outgoing context first, so live, ShadowRealm and worker globals are not affected.stop_active_handlesinto thecancel_all_timershook, which the swap runs after the last point where the outgoing realm's JS runs as the live realm. No window is left.test/cli/test/isolation.test.ts(new case fails on 1.4.3-canary, all 34 pass with the fix), plus the fake-timers, cron and jsonwebtoken suites.Background
--isolategives each file a freshZig::GlobalObjecton the sameJSC::VMand thread. The old global is not destroyed, so its functions stay callable.ScriptExecutionContext::prepareForDestruction()is WebCore's "this realm is going away" step.ActiveDOMCallback::canInvokeCallback()reads the same flag.cancel_all_timersis the hook that drops the outgoing timers at the swap and at VM teardown.Notes
a.test.tsleaksBun.password.hash(argon2id, timeCost 12).then(() => jest.useFakeTimers()),b.test.tsawaitsBun.sleep(1500)then a real 300 mssetTimeout. B times out on 1.4.2 and 1.4.3-canary in--isolate,--paralleland plain mode.useRealTimers()/runAllTimers()/clearAllTimers()from the retired realm had the mirror effect: they drained or removed the running file's fake heap. The same check covers them, andgetTimerCount()/isFakeTimers()read0/falsethere.Zig__GlobalObject__isContextStoppedis the new C++ getter behindis_context_stopped.setImmediate(never routed through the fake clock) until A writes that it acted. Phase 1 checks that A'suseFakeTimers()+setSystemTime(0)leave B on real timers (isFakeTimers() === false,performance.now() > 0, a 1 ms timer fires). Phase 2 has B turn fake timers on itself and checks that A'sadvanceTimersByTime/runAllTimers/clearAllTimers/useRealTimersleave B's fake heap alone and that A's reads returnfalse/0. The--parallel=2run pins both files to one worker withBUN_TEST_PARALLEL_SCALE_MS, like the neighbouring cases.stop_active_handlesruns before the swap. After it, the outgoing realm's JS still runs as the live realm in socket close handlers and in the swap's two microtask drains, so an activation there used to survive into the next file.cancel_all_timersruns after those and before the new global exists. At VM teardown the same hook runs and the reset is harmless there.bun test(no--isolate) shares one global and onejestobject across files, so a late call there cannot be told apart from the running file's own. That mode is what bun test: reset fake timers and setSystemTime between test files #36398 (boundary reset inTestCommand::run) is about. This PR does not change it.stop_active_handleslost itsvmparameter because the reset was its only use.setTimeoutstill lands in the live heaps (and, when the live file has fake timers on, in its fake heap). That is the un-cancelled-continuation problem in general, not specific to fake timers, and is left for a separate change.