Conversation
While Bun.spawnSync waits for its child, vm.event_loop_handle points at spawnSync's private uws loop. FilePoll, KeepAlive and the JS timer / immediate ref counts all adjusted whichever loop that handle named at the moment they ran, so a ref taken on the main loop and released during the wait (a timer or reader swept by GC, or tests run by the test runner's timeout path) was subtracted from the private loop instead. Once the private loop's num_polls read 0 with the child's pidfd still registered, us_loop_run_bun_tick returned without polling and spawnSync spun forever without reading stdout or observing the exit. FilePoll now records the loop it was counted on (and registered with) and uses it for unregister/deactivate and the active-count adjustments; KeepAlive records the loop it ref'd; timer::All records the loop each of its two ref counts ref'd. The handle saved by SpawnSyncEventLoop::prepare moves to the caller's frame so a spawnSync nested inside another one's wait restores the correct handle at each level instead of leaving the VM on the private loop.
|
Warning Review limit reached
Next review available in: 4 minutes 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: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (13)
Comment |
|
Updated 3:19 AM PT - Aug 12th, 2026
✅ @robobun, your commit 1bb1627f21b5e7e19e589d206f7b26c923212a99 passed in 🧪 To try this PR locally: bunx bun-pr 37754That installs a local version of the PR into your bun-37754 --bun |
|
Status: ready for review Reproduced locally by looping CI-shaped batches ( The fix is in three commits: the FilePoll / KeepAlive / timer counters plus the nested-handle restore (c1e1ed4); the CI passed on all three commits (builds 92859, 93047, 93058). The only items in those builds are retry-passed flakes unrelated to this change ( Review: the provenance thread is addressed in d94f890 and the re-review found nothing further; the comment-length bot's threads are resolved (see the comment below). A separate self-review pass over the final diff turned up nothing either. |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
Checked #34069 against this: it is the same state. That report (macOS, so the kqueue side of the same FilePoll accounting) has the worker spinning inside |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…p; FilePoll takes raw loop pointers EventLoop::apply_concurrent_ref_delta() applied MessagePort / BroadcastChannel / ScriptExecutionContext keep-alive refs to vm.event_loop_handle, which Bun.spawnSync repoints at its private loop while it waits, so a port or channel released during that wait (GC sweep, or tests run by the test runner's timeout path) was subtracted from the private loop. Every EventLoop now records the uws loop it drives (uws_loop, previously Windows-only) and folds its refs into that. FilePoll::register/unregister and friends now take *mut Loop instead of &mut Loop: the poll keeps the pointer in counted_loop until it is unregistered, and a pointer derived from a caller's reborrow would not stay valid for that long. Callers pass the loop handle they already hold, which also removes the &'static mut accessor EventLoopCtx::platform_event_loop. The test is now one fixture per kind of ref (JS timer, KeepAlive, FilePoll, EventLoop keep-alive ref), each released on its own inside a spawnSync wait, plus one for a spawnSync nested inside the wait restoring the main loop.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
…ack in unref_refd_loop
|
Pushed two more commits on top of the original fix:
The coderabbit comments are rate-limit notices. The comment-length bot flags every doc comment in the diff, including the one- and two-line ones left after the trim; those are the docs for the new fields and functions rather than workaround justifications, so I have resolved those threads. The PR description is updated to match. |
There was a problem hiding this comment.
I re-reviewed after d94f890 and didn't find any bugs. The earlier Stacked Borrows note is addressed — activate/register/unregister now take *mut Loop and counted_loop is stored from the raw handle, matching the KeepAlive::loop_ and timer_refd_loop paths. Given this reworks ref-count bookkeeping across FilePoll, KeepAlive, timer::All, and EventLoop::uws_loop on every POSIX target, a human look is still worthwhile.
Checked: counted_or routing for re-register/unregister and the keep-alive toggles; unref_on_next_tick's Loop::get() compare falling back to a direct unref; own_uws_loop vs. usockets_loop so apply_concurrent_ref_delta folds into the isolated loop; the prepare→frame-local→cleanup handle for nested spawnSync; that removed helpers (platform_event_loop, loop_unref) have no remaining callers.
Extended reasoning...
Overview
The PR fixes cross-loop ref-count leakage between the main uws loop and spawnSync's private loop by recording, at increment time, which loop each of three counters (FilePoll::counted_loop, KeepAlive::loop_, timer::All::{timer,immediate}_refd_loop) was taken on, and decrementing that same loop later. It also makes jsc::EventLoop::uws_loop a real field on POSIX so apply_concurrent_ref_delta folds into the loop this EventLoop actually drives, and moves SpawnSyncEventLoop's saved handle onto the caller's stack frame so nested spawnSync restores correctly. Signature changes ripple *mut Loop (instead of &mut Loop) through FilePoll::register*/unregister* and their call sites in process.rs, dns.rs, dns_sd.rs, memory_pressure.rs, ParentDeathWatchdog.rs, and io/lib.rs.
Security risks
None identified. No untrusted input parsing, no auth/crypto, no path handling. The new raw-pointer fields point at per-thread uws loops that outlive everything counted on them; SAFETY comments state the lifetime and single-thread invariants.
Level of scrutiny
High. This is core event-loop accounting on the hot path for every fd poll, timer, and keep-alive on POSIX, with new raw-pointer state whose lifetime is enforced only by convention. A miscount here is exactly the kind of bug the PR is fixing (spins/hangs, process-won't-exit). The change is coherent and well-tested, but it is not mechanical.
Other factors
- My earlier inline comment about
activate()storing a pointer derived from a transient&mut Loopreborrow was addressed in d94f890:FilePollnow takes and stores*mut Loopend-to-end, andcounted_or/{enable,disable}_keeping_process_alivederef throughloop_mut(*mut Loop)with provenance from the long-lived handle. - The new test exercises each ref kind (timer,
KeepAliveviaBun.serve,FilePollvia a piped child,concurrent_refviaBroadcastChannel) plus the nested-spawnSync restore, and the PR description confirms it fails on the unfixed build. unref_refd_looppanics if the recorded loop is null when the count crosses back to ≤0 — that's a hard invariant check on a state that should be unreachable, which seems appropriate.- The comment-cop bot has open style notes about comment length; those are not correctness issues and 1bb1627 already shortened several.
- Windows is explicitly untouched (all new fields are
cfg(not(windows));EventLoopCtx::loop_add_active/loop_sub_activeare now Windows-only, which matches their only remaining caller inwindows_event_loop).
|
Thanks for the re-review. One correction to its summary so nobody is misled: after d94f890 Windows is not entirely untouched. The A separate self-review pass over the diff did not turn up anything further either. Waiting on CI (build 93058). |
|
Superseded by #40078, which applies the same invariant (every poll, keep-alive and timer ref is released against the loop it was counted on) to current main. This branch conflicts with main in |
1 participant
Problem
bun test --parallelworkers intermittently hang insideBun.spawnSyncof a local bun child: CI printsInterrupted while still running: <file>after 4 minutes, or a run of tests in one worker fails withkilled 1 dangling processandthis test timed out after 90000mswith empty child stdout. The files pass alone. Also reported on macOS (worker at 100% CPU, child a zombie) and reproduced locally in 3 of about 73 CI-shaped batches.Fix
bun testspins until killed) and pass with the fix; four release one kind of ref each during a wait, and the nesting case alone still fails with only the counter fix applied. Related suites were also run and the remaining failures judged unrelated. The new tests are skipped on Windows, where three of the counters do not go through the swapped handle.Background
Bun.spawnSyncwaits on a private uws loop with its own epoll/kqueue fd, not the main loop, so main-loop work such as JS timers does not run during the wait. While it waits,vm.event_loop_handleis repointed at that private loop so the child's pidfd and pipe polls register there.us_loop_run_bun_tickreturns at once without polling when that count is 0. That is why an off-by-one shows up as a busy spin, not a blocked wait.FilePoll(one per fd registered on a loop: pipes, pidfds, sockets),KeepAlive(the on/off ref used by servers, sockets, fetch, fs, dns and similar),timer::All(refs the loop while any JS timer or immediate is pending), andjsc::EventLoop'sconcurrent_ref(refs from MessagePort, BroadcastChannel and similar, folded into a uws loop).no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/spawn/spawnsync-isolated-event-loop.test.ts
Original description
Problem
bun test --parallelworkers intermittently stop insideBun.spawnSyncof a local bun child. In CI this is theflakyannotation "failed in the parallel batch ... passed alone": the batch goes quiet, the runner kills it after 4 minutes and printsInterrupted while still running: <file> (24xs), and the file passes in ~100ms on its own. The victims are whatever spawn-heavy files land in the batch (napido.test.tsfiles,resolve.test.ts,child_process.test.ts,ctrl-c.test.ts,text-loader.test.ts,malformed-integrity-base64.test.ts, ...), across ubuntu, debian and alpine lanes (for example builds 92307, 92340, 92426, 92160). A second shape of the same failure is a run of consecutive tests in one worker each failing withkilled 1 dangling process+this test timed out after 90000ms, with the spawned child's stdout coming back empty (build 92426,resolve.test.ts, six in a row).#34069 describes the same state outside our CI, on macOS (kqueue): a
bun test --parallel(or plain--isolate) worker pegged at 100% CPU insidespawnMaybeSync, the child a zombie, the pipes already closed,tickWithTimeoutreturning immediately, and the same nested-spawnSync escalation through the bun:test timeout path once the per-test timeout fires inside the spin. The accounting below is shared by the epoll and kqueue backends, and the new tests run on both.Reproduced locally by running CI-shaped batches (
bun test --parallel=3 --timeout=90000, CI'sBUN_GARBAGE_COLLECTOR_LEVEL=1environment, pinned to 4 CPUs): 3 of ~73 batches stalled or timed out the same way, and the stalled worker's main thread was busy-spinning insideBunObject_callback_spawnSyncwith its child already a zombie.Cause
spawn_maybe_syncpointsvm.event_loop_handleat spawnSync's private uws loop for the duration of the call, so that the child's pidfd and pipe polls register on that loop. Everything that maintains a loop's keep-alive bookkeeping resolved that handle at the moment it ran, so it acted on whichever loop happened to be current:FilePoll::activate/deactivate(num_pollsandactive, plus the epoll/kqueue fd used to register and unregister),KeepAlive::ref_/unref(Loop::ref_/unref), used by servers, sockets, fetch, node:fs, dns, zlib, napi async work, ...timer::All::increment_timer_ref / increment_immediate_ref, which ref the loop when the first JS timer or immediate appears and unref it when the last one goes away,jsc::EventLoop::apply_concurrent_ref_delta, which folds therefKeepAlivecounter used byMessagePort,BroadcastChannelandScriptExecutionContextintovm.platform_loop_opt(), immediately on every JS-thread ref/unref.So a ref taken on the main loop and released while a spawnSync is waiting was subtracted from the private loop instead. That happens whenever teardown runs during the wait: JSC sweeping a dead
Timeout, port or pipe reader while spawnSync allocates its result (very frequent under--isolate+BUN_GARBAGE_COLLECTOR_LEVEL=1, where the previous file's leftovers are swept during the next file), or the test runner carrying on with the following tests after a test times out inside spawnSync. The private loop is reused for every spawnSync in the process, so the error persists for the rest of the worker's life.us_loop_run_bun_tickreturns immediately whenloop->num_polls == 0. With the private loop's count off by one, a pipeless spawnSync (the napi harness) starts at 0 and spins from the first tick; a piped one reaches 0 as soon as its pipes hit EOF, before the pidfd event is dispatched. Either way the wait loop spins without polling and never observes the exit. The per-test timeout still fires inside the spin: it printskilled 1 dangling processinto the worker's captured output, and if nothing is left to close the spin continues forever (the silent 240s variant); if pipe readers are still open, closing them makes the count non-zero, the loop polls once, and spawnSync returns the long-exited child with empty stdout (the timed-out variant), leaving the count wrong for the next call, hence the consecutive failures. The main loop is left with counts that are too high, which in a plain script also keeps the process from exiting.A second problem in the same code:
SpawnSyncEventLoopkept the saved handle in the struct that both levels of a nested spawnSync share (the timeout path above nests), so after the inner call returned, the outer call restored the private loop's handle and the VM stayed on the private loop for good.Fix
Each counter records the loop it counted on and uncounts that one:
FilePollgets acounted_loop, used for unregister/deactivate, re-registration and the active-count adjustments (this also makes the unregister hit the epoll/kqueue instance the fd is actually registered with). Because the poll keeps the pointer,FilePoll::register/unregisterand friends now take*mut Loopinstead of&mut Loopand the callers pass the loop handle they already hold; the&'static mutaccessorEventLoopCtx::platform_event_loopgoes away with that.KeepAliverecords the loop it ref'd.unref_on_next_tickkeeps deferring through the pending counter when that is the thread's loop and unrefs directly otherwise, since only the thread's loop drains that counter.timer::Allrecords the loop for each of its two counts.jsc::EventLooprecords the uws loop it drives (uws_loop, previously a Windows-only field; the VM's loops get the thread's loop inensure_waker, a spawnSync loop gets the private one) and folds its keep-alive counter into that.SpawnSyncEventLoop::preparelives on the calling frame and is passed back tocleanup, so nesting restores correctly.Why this is the right shape: the invariant that broke is "an increment and its matching decrement hit the same loop", and the handle swap itself is what spawnSync's own polls rely on to find the private loop, so the swap stays and each counter remembers its loop instead. That is independent of when or why the release happens (GC, the runner, user code), and the recorded pointers are always live when used: the thread's loop is freed by
VirtualMachine::teardownonly after timers are cancelled and the heap is destroyed (which is also when the last fold happens), and the private loop, owned byRareData, is freed after that. On Windows the first three counters go through the thread's loop regardless of the handle, so their new fields arecfg(not(windows)); theEventLoopcounter did follow the swapped handle there too and is fixed on every platform.Tests
test/js/bun/spawn/spawnsync-isolated-event-loop.test.tsgets a block of five cases. Each runs a small file underbun testin which testaenters a spawnSync after leaving its synchronous section and times out, so the runner executes the following tests while the private loop is current.a's spawnSync holds exactly one poll on the private loop, so one misdirected release is enough to make it spin forever; on the fixed build the release comes off the main loop,a's (killed) child is observed, and the file finishes with everything butapassing.clearTimeout), aKeepAlive(Bun.serve().unref()), a registeredFilePoll(cancelling a subprocess's stdout), and an event-loop keep-alive ref (BroadcastChannel.unref());On the unfixed build all five fail (the inner
bun testspins, the outer test times out and bun test kills it). On a build with the counter fixes but without the nested-handle fix, the four release cases pass and only the nesting case fails, so the cases are independent. Each case takes ~1.4s on a debug build; the block is deliberately sequential because bun test only kills a timed-out test's dangling processes when the test is not in a concurrent group, and that is what cleans up a spinning inner process on a regressed build.Also ran the spawn, spawnSync, maxBuffer, pidfd, child_process, BroadcastChannel, MessagePort/worker, dns, macro, fs.watch, timers and setImmediate suites on the debug build, plus
bun test --parallel=3over the CI victim files withBUN_GARBAGE_COLLECTOR_LEVEL=1. The failures left in those runs are unrelated to this change: network-dependent dns lookups, RSS and throughput tests over their budget under ASAN, a pre-existing LSan report inworker-terminate-lifetimethat reproduces with a plainrequire("fs"), and container-specific process/uid cases; the ones that were checked against an unfixed debug build fail identically there.Fixes #34069