bun test --isolate: kill module-scope subprocesses of every file at the isolation swap

[robobun]

Problem

• bun test --isolate (and every --parallel worker) kills the subprocesses a file left running when it swaps globals between files, but a subprocess spawned at module scope (top level of the file, outside any test or hook) is only killed for the first file of the run. The same spawn in the second and later files outlives the swap and leaks out of the run.
• The killer only registers spawns while auto_killer.enabled is set. --isolate sets it once at startup (src/runtime/cli/test_command.rs:2316; --parallel workers do the same in src/runtime/cli/test/parallel/runner.rs:637), which is why the first file's module scope is tracked. Execution::on_group_completed (src/runtime/test_runner/Execution.rs:571) then calls disable() after every group of tests, and nothing turns tracking back on until the next group starts, so it is off while the next file's module scope runs. swap_global_for_test_isolation (src/jsc/VirtualMachine.rs:5057) kills and clears the tracked set, but those spawns were never in it.
• The comment next to the per-file cleanup in test_command.rs ("under --isolate ... we need tracking to remain enabled and populated until then") describes the intended behavior; the per-group disable() has defeated it since --isolate was added in bun test: add --isolate and --parallel #29354.

Fix

• Execution::on_group_completed leaves the killer enabled when vm.test_isolation_enabled is set. Under --isolate tracking is then on for the whole run and the swap's kill + clear covers every spawn the file made, at module scope or inside a test. Without --isolate nothing changes: tracking is still per group and the set is still cleared per file.
• Correct because it is the documented contract of --isolate (docs/test/parallel.mdx: between files Bun closes "subprocesses the file left open") and it is already how the first file of every run behaves; the change makes the later files match it. The swap is the only place that kills the set between files, so keeping the set populated until then has no other consumer.
• Side effect, stated explicitly: a test timeout under --isolate kills everything tracked (Execution::handle_timeout), so in files 2+ it now also kills that file's module-scope spawns, as it already did in file 1. bun test: only kill the timed-out test's own subprocesses on timeout #38774 makes the timeout kill per group; once both land a timeout no longer kills module-scope spawns in any file and the swap still kills everything. The two diffs touch adjacent functions and do not conflict.
• Module-scope spawnSync calls under --isolate now wait on the event loop in every file instead of only the first (the blocking fast path in js_bun_spawn_bindings.rs:1049 is gated on the killer being off); that is the path every spawnSync inside a test already takes.
• Considered and not done here: registering subprocesses with the ActiveHandle sweep that runs before the swap (src/runtime/jsc_hooks.rs), which would let the killer go back to being timeout-only. The swap has used the killer for subprocesses since bun test: add --isolate and --parallel #29354 and bun test: only kill the timed-out test's own subprocesses on timeout #38774 is reworking the killer's internals, so this PR keeps the one-line policy change and leaves that reshape as a follow-up.
• Test: test/cli/test/isolation.test.ts, "module-scope subprocesses are killed for every isolated file, not just the first", run once for serial --isolate and once for a single --parallel worker running all three files. Each fixture file spawns a sleeper at module scope and logs its pid; its test asserts the sleepers of the files that ran before it are dead, so the third file to run observes the second file's leak. Both variants fail on the unfixed build (one surviving pid, the second file's) and pass with the fix; the existing in-test spawn case and test-timeout-behavior.test.ts still pass.

Background

• ProcessAutoKiller (src/jsc/ProcessAutoKiller.rs) is a per-VM set of live Bun.spawn processes. Bun.spawn adds a process to it only while enabled is true, exits remove it, and kill() SIGTERMs and empties it. bun test uses it in two places: a test timeout kills whatever is in the set, and the --isolate swap kills the set to clean up after a file.
• An execution group is the unit bun:test runs at a time (a test with its hooks, or a batch of concurrent tests). Without --isolate, tracking is turned on and off around each group so a timeout only kills processes spawned while tests were running, and the set is cleared at the end of each file.
• The isolation swap (swap_global_for_test_isolation) runs after every file under --isolate, and after every file inside a --parallel worker (workers always isolate). A file's module scope is evaluated before any of its groups start, which is the window in which tracking was off.

Earlier revision

The first push also switched the two startup sites from auto_killer.enabled = true to auto_killer.enable() so that ever_enabled was set before the first group. The only effect was pruning a first-file module-scope process that exits before any test runs slightly earlier (the swap releases it anyway), so those hunks were dropped after self-review and the diff is now the on_group_completed change plus the test.

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 39 seconds

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 5cf5e75f-0deb-49cd-be1b-e66aee44ceb8

📥 Commits

Reviewing files that changed from the base of the PR and between 6324a58 and eaca035.

📒 Files selected for processing (2)

• src/runtime/test_runner/Execution.rs
• test/cli/test/isolation.test.ts

---

_{Comment @coderabbitai help to get the list of available commands.}

[robobun]

^{Updated 1:41 AM PT - Aug 15th, 2026}

@robobun, your commit eaca035 is building: #97739

[robobun]

Status: reproduced and fixed; narrowed after self-review. Ready for review.

Reproduced on the released build (serial bun test --isolate a b c and a single --parallel worker running three files): a sleeper spawned at module scope of the second file to run is still alive when the third file runs, while the first file's is killed. Both variants of the new test in test/cli/test/isolation.test.ts fail on the unfixed build with exactly that one surviving pid and pass with this branch; test-timeout-behavior.test.ts and the rest of isolation.test.ts still pass.

The diff is the Execution::on_group_completed change plus the test (the enable() startup hunks from the first push were dropped, see the description). Composes with #38774 as noted above.

CI: the first build (96883) passed. The second build's only red job was the android verify-baseline step failing to download a rustup manifest; on the re-run (97739) every lane that has run is green (177/179), with only the two darwin 14 aarch64 lanes still waiting for an agent. The remaining annotations are retry-passed flakes; the 07261 / text-loader "failed in the parallel batch, passed alone" entries are the pre-existing spawnSync hang that #38883 fixes and that time-adjacent main builds (97206, 97480) show as well. Inside a test the subprocess tracking this PR touches was already enabled, so the in-test spawn path is unchanged here.

[claude]

LGTM — small, targeted fix that makes files 2+ match the already-correct behavior of file 1 under --isolate.

What was reviewed:

• on_group_completed now skips disable() under isolation; verified on_group_started is idempotent and the swap's kill+clear (VirtualMachine.rs:5057-5058) drains the set between files, so it can't grow unbounded.
• enable() vs enabled = true at startup: confirmed the difference is ever_enabled, which gates on_subprocess_exit removal — so first-file module-scope spawns that exit on their own now get pruned immediately instead of lingering until the first group runs.
• spawnSync fast-path gate at js_bun_spawn_bindings.rs:1049: module-scope spawnSync in files 2+ now takes the event-loop path, matching file 1 — no behavior change, just consistency.
• Test follows the established scale-up-gated --parallel pattern already used twice in this file; finally cleanup covers both the last file's sleeper and anything an unfixed build leaks.

Extended reasoning...

Overview

The PR fixes a gap in bun test --isolate's subprocess cleanup: subprocesses spawned at module scope (top-level, outside any test/hook) were only tracked for the first file of the run, because Execution::on_group_completed unconditionally called auto_killer.disable() after every test group, and nothing re-enabled it before the next file's module scope evaluated. The fix is a 3-line guard on vm.test_isolation_enabled in on_group_completed, plus swapping two enabled = true field writes for enable() calls (which also sets ever_enabled). A new order-independent test in isolation.test.ts covers both the serial --isolate and single-worker --parallel paths.

Security risks

None. This is test-runner lifecycle bookkeeping — no auth, crypto, parsing of untrusted input, or user-facing API surface. The only side effect is that more leaked subprocesses get SIGTERM'd at file boundaries, which is the documented contract.

Level of scrutiny

Low-to-moderate. The Rust change is ~10 lines and mechanical: gate an existing call on an existing flag, and use the existing enable() helper instead of a raw field write. I traced the state machine (startup → module scope → group start/end → swap) and verified the fix produces the intended enabled-for-the-whole-run behavior under isolation while leaving the non-isolate per-group toggling unchanged. The per-file swap (VirtualMachine.rs:5057-5058) still kills and clears the set, so there's no unbounded growth. I also checked the one other reader of auto_killer.enabled (the spawnSync blocking fast-path gate) — under isolation it now consistently routes module-scope spawnSync through the event-loop path for every file instead of just the first, which the PR description calls out as a no-behavior-change alignment.

Other factors

• The ever_enabled change is a subtle correctness improvement: on_subprocess_exit is gated on ever_enabled, so before this PR a module-scope process that exited on its own during file 1 (before any group had called enable()) would sit in the set with a held ref until the swap. Harmless (kill() checks has_exited()), but now it's removed promptly.
• The new test reuses the scale-up-gated --parallel fixture pattern already established in two other tests in this file. It uses bounded polling (2s deadline) rather than fixed sleeps, asserts on the specific surviving PIDs, and has a finally block that kills any leaked sleepers so a regression doesn't poison later tests on persistent runners.
• The existing sibling test "leaked subprocesses are killed for every isolated file, not just the first" (spawns from inside tests) is unchanged and still covered; the new test specifically targets the module-scope window.
• No prior human or bot review comments to address; CI is building.

[robobun]

Heads up from a neighbouring change: #38774 makes the timeout kill in Execution::handle_timeout per group (each tracked process is tagged with the group it was spawned in, and a timeout only kills the current group's processes), so once both land a test timeout no longer kills module-scope spawns in any file; the isolation swap's kill() still kills everything tracked, which is what this PR relies on. The diffs touch adjacent functions (on_group_started there, on_group_completed here) and do not conflict; only the sentence in this description about timeouts killing the first file's module-scope spawns becomes stale.

[claude]

LGTM. The PR description now folds the dropped enable() hunks into an "Earlier revision" note, so my prior nit about the description/diff mismatch is resolved.

What was reviewed:

• on_group_completed gating disable() on !vm.test_isolation_enabled — verified the flag is set at both startup sites (test_command.rs:2317, parallel/runner.rs:637) and the swap at VirtualMachine.rs:5057 does the kill+clear the comment claims.
• Non-isolate path unchanged: per-group disable() still runs, and the per-file clear()+disable() at test_command.rs:3396 is already gated on !test_isolation_enabled.
• New test follows the file's existing --parallel=2 + BUN_TEST_PARALLEL_SCALE_MS one-worker pattern, polls with a deadline, and cleans up leaked pids (including the last file's un-swapped sleeper) in finally.

Extended reasoning...

Overview

Four-line change in src/runtime/test_runner/Execution.rs: on_group_completed now skips auto_killer.disable() when vm.test_isolation_enabled is set, so subprocess tracking stays on between execution groups under --isolate and module-scope spawns of files 2+ are registered for the isolation swap's kill. A new test.each case in test/cli/test/isolation.test.ts covers serial --isolate and a single --parallel worker.

Security risks

None. This is test-runner subprocess lifecycle bookkeeping — it changes when an existing SIGTERM sweep applies, not what it targets or how.

Level of scrutiny

Low-to-medium. The Rust change is a one-condition policy adjustment in a per-group callback; I traced test_isolation_enabled to confirm it is set exactly at the two startup sites the description names and read by swap_global_for_test_isolation, which does auto_killer.kill(); auto_killer.clear() — matching the new one-line comment. The non---isolate path is byte-identical to before. The stated side effects (timeout kill scope in files 2+, spawnSync fast path at js_bun_spawn_bindings.rs:1049) are real but make later files match file 1's existing behaviour, and #38774 is separately narrowing the timeout kill.

Other factors

• My earlier inline comment (description mentioned enable() calls that weren't in the diff) was addressed: the description was rewritten with an "Earlier revision" details block explaining those hunks were dropped after self-review. The comment-cop note about the paragraph-long comment was also addressed (97a895c shortened it to one line).
• The new test reuses the file's established one-worker-takes-all-files trick (--parallel=2 + BUN_TEST_PARALLEL_SCALE_MS: 60000), polls with a 2s deadline instead of sleeping, asserts three pids were logged so a partial run can't vacuously pass, and its finally reaps every logged pid so an unfixed runner (or the last file's un-swapped sleeper) doesn't leak into CI.
• robobun confirmed both test variants fail on the released build with exactly the second file's pid surviving and pass on this branch, and that test-timeout-behavior.test.ts and the rest of isolation.test.ts still pass.

[claude]

LGTM — one-condition gate matching the already-documented intent in test_command.rs ("need tracking to remain enabled and populated until then").

What was reviewed:

• Confirmed vm.test_isolation_enabled is set at both startup sites (serial --isolate and parallel worker) and read by the swap at VirtualMachine.rs:5057 for kill+clear.
• Non-isolate path unchanged: disable() still runs per group, and per-file clear()/disable() at test_command.rs:3396 is already gated on !test_isolation_enabled.
• on_group_started's enable() is idempotent, so leaving the killer enabled between groups introduces no double-ref or state mismatch.
• New test follows the file's existing --parallel=2 + BUN_TEST_PARALLEL_SCALE_MS pattern, polls with a deadline, and finally-kills the last file's sleeper.

Extended reasoning...

Overview

The runtime change is four lines in Execution::on_group_completed: skip auto_killer.disable() when vm.test_isolation_enabled is set. The rest is a new test.each case in test/cli/test/isolation.test.ts covering serial --isolate and a single --parallel worker.

I verified against the surrounding code that the fix implements exactly what the existing comment at test_command.rs:3393-3395 says the code should do: under --isolate, the swap (VirtualMachine.rs:5057-5058) is the sole consumer of the tracked set between files, and it calls kill() + clear(). The per-group disable() was defeating that by turning tracking off before the next file's module scope evaluated. ProcessAutoKiller::disable() only clears the enabled flag (no side effects), and enable() in on_group_started is idempotent, so the change has no effect on refcounts or the set contents beyond keeping tracking on.

Security risks

None. This is test-runner subprocess bookkeeping; no untrusted input, no auth/crypto/permissions.

Level of scrutiny

Low-to-medium. The Rust change is a single conditional on a boolean field that already gates the same behavior at the per-file cleanup site in test_command.rs. The non---isolate path is byte-identical to before. The stated side effects (timeout kills module-scope spawns in files 2+ to match file 1; spawnSync fast path now bypassed in every file's module scope) are both convergences toward the first file's existing behavior, not new behavior.

Other factors

The prior comment-cop feedback (paragraph-long comment) was addressed in 97a895c — the comment is now one line. My earlier note about the description mentioning dropped .enable() hunks was answered: the description now carries an "Earlier revision" note and the paragraph is gone. Both threads are resolved.

The test is well-constructed: three identical fixture files each spawn a sleeper at module scope and log its pid; each file's test polls (2s deadline) for earlier pids to be dead, so the third file catches the second file's leak regardless of file order. It reuses the file's existing pattern for forcing one worker to take all files (BUN_TEST_PARALLEL_SCALE_MS: "60000"), drains stdout/stderr concurrently, asserts before exitCode, and cleans up leaked pids in finally. The author confirmed both variants fail on the unfixed build with exactly the second file's pid surviving.