PR #44581 - oven-sh/bun

What does this PR do?

Fixes #34069. Fixes #37968. Supersedes #40078, which found the cause.

Bun.spawnSync stops pointing the VM at the loop it waits on.
A child that nobody is going to wait for is ended and reaped, wherever that happens.
What a direct stream's pull() throws is a failure whether or not it is an Error.
The scopeguards of spawn_maybe_sync are RAII types.

2 to 4 came out of the debug assert that 1 adds and out of review.

1. spawnSync and the VM's loop

Bun.spawnSync waits on an event loop of its own. For the length of the call it also pointed vm.event_loop_handle at that loop, so everything that finds its loop through the VM found the wrong one. The only other code that runs in there is a garbage collection's finalizers:

A finalizer that	did it on	and then
releases a `FilePoll` or `KeepAlive` (an earlier test file's `process.stderr`, `napi_unref_threadsafe_function`)	spawnSync's loop	that loop's `num_polls` drifts down, and it is reused for the life of the process. Once the drift cancels the polls of a later call, the tick returns without polling: that call spins at 100% CPU until its `timeout`, or forever without one, and loses the child's output (#34069). The thread's loop keeps the ref, so the process never exits
unregisters a `FilePoll`	spawnSync's epoll	the thread's epoll keeps the entry. For a dup of stderr the open file never goes away, so the next dup to get that number fails with `EEXIST: file already exists, epoll_ctl` (#37968)
takes a `KeepAlive` (`napi_queue_async_work` from a `NAPI_EXPERIMENTAL` finalizer)	spawnSync's loop	its completion releases it on the thread's loop, which ends up a ref short: the process exits with work pending

In CI this is test/internal/build-toolchain-identity.test.ts failing with Cannot tell which …/current/clang this is after exactly 30,001 ms (its spawnSync timeout), in the --parallel batch only: 39 of the 85 main builds that completed since the test was added on September 20.

Cause. What the call makes for its child (the process poll, the stdout/stderr readers, the stdin writer) is already handed the jsc::EventLoop of spawnSync's loop, and EventLoopHandle::loop_() already answers with that loop. EventLoopHandle::as_event_loop_ctx() did not: it erased the EventLoop to its VM, and the ctx asked the VM for the loop. The swap existed to make that answer come out right.

Fix. prepare() no longer swaps vm.event_loop_handle on POSIX, so js_vm_ctx() and vm.uws_loop() always mean the thread's loop, for a release and for an acquire alike. What makes that possible is that a ctx made from spawnSync's EventLoop names its loop:

EventLoopCtx and AllocatorType get the third kind of loop a thread has, SpawnSync. AllocatorType is what a FilePoll already keeps in order to find its ctx again in deinit(). No new field anywhere.
EventLoop::event_loop_ctx() picks the kind from isolated_poster, the way js_poster() already does. Every other EventLoop gets the ctx it got before.
SubprocessPipeReader::loop_() was the one thing on the sync path that asked the VM. It uses the EventLoop it was given.
Process::close and PollerPosix::deinit released the waiter thread's KeepAlive through js_vm_ctx(). Not reachable for a sync call today, but it is the same mistake.
A call asserts in debug builds that it leaves nothing on the loop.

Windows keeps the swap. A libuv handle knows the loop it was made on and the counters there are always the thread's, so it does not have this bug.

Compared with #40078: it keeps the swap and has every owner record which loop was current when it took a counter, to resolve the loop from that at release. That cannot help a finalizer that takes a ref during the call (third row).

2. A child nobody is going to wait for

All of these are what happens when epoll_ctl or kevent fails (ENOSPC once fs.epoll.max_user_watches is used up, ENOMEM), or when setting up stdin throws. In each, the caller is told the child failed or is handed nothing at all, while the child ran on with nothing left to reap it.

Where	Before	Now
`Bun.spawnSync`, stdin writer cannot register	threw at once: 3 fds, 2 registered polls and a running child left behind	remembers the error, ends the child, goes through the same wait and `finalize` as every call, throws the same error
`Bun.spawnSync`, pidfd cannot register	blocking `wait4()`: nothing serves the child's stdio and `timeout` does not apply, so a child that reads stdin or fills a pipe never exits	child ended and reaped, error thrown
`Bun.spawn`, pidfd cannot register	`exited` rejected for a child that was still running	child ended and reaped first
`Bun.spawn`, `stdin: "pipe"` or a stream cannot be set up	threw `Out of memory`, child left running	throws the real error, child ended and reaped
`Bun.spawn`, buffer stdin writer cannot register	sent `killSignal`, which the child may ignore, and its poll kept the parent alive	`SIGKILL`
`Bun.spawn` with an `onExit`, neither the pidfd nor the buffer stdin writer can register	`onExit` ran with the error already thrown and took the exception with it: `Bun.spawn` returned an empty value and the caller segfaulted (also in 1.4.3)	the exit is reported first, then the error is thrown
`bun run --parallel`, `bun run --filter`, `bun install` scripts (tested), and the other callers of `watch_or_reap`: `bun test --parallel` workers, cron, git, the security scanner	reported the script as failed and went on or exited, the script ran on as an orphan	ended and reaped
Shell, pidfd cannot register	await $`cmd` never settled	exit code 1
Shell, stdio cannot start	`SIGTERM`, then a zombie for the life of the parent	ended and reaped

It is one function, Process::kill_and_reap(), called from on_wait_pid and from watch_or_reap's error branch, so no caller of watch_or_reap had to change.

A child that turns out to have ended by itself is reported with its own status and rusage, not with the error.
A child that was reaped already is left alone: its pid may be somebody else's.
A child that cannot be signalled (EPERM: it runs as another user by now) is not waited for, since that would block until it exits by itself. It is given up, which is what happened to every child before.
watch_or_reap closes the Process before it returns the error, so a caller's later kill() is a no-op instead of a signal to a recycled pid.

It is what already happens when pidfd_open itself fails with EMFILE (spawn-pidfd-emfile.test.ts): fail instead of blocking in wait4().

One change for the worse: a spawnSync child whose pidfd cannot register and that would have exited by itself without needing its stdio served used to be waited for, and the call returned normally. Unless it has already exited, it is ended and the call throws, like Bun.spawn.

3. What `pull()` throws

A direct stream is pulled from while a sink is attached to it. FileSink::assign_to_js_stream unwrapped the Exception cell it got back to the thrown value, and both of its callers then asked that value whether it was an error:

`pull() { throw 42 }`	Before	Now
`Bun.write(file, stream)`	the promise stayed pending forever	rejects with `42`
`Bun.spawn({ stdin: stream })`	returned a `Subprocess` whose stdin never ended, so `exited` hung	throws `42`, child ended
`pull()` ends the worker (`process.exit()`)	taken for a pump that ran to the end. A later catch-all threw, leaving the child, two sockets and the pidfd	the same path as any other failure

It returns JsResult now. That made the has_exception() catch-all in spawn_maybe_sync unreachable (nothing between there and Writable::init runs JS), and its teardown was wrong anyway (kill is a no-op before the process is watched, and nothing can release a reader that was never started), so it is gone, with crate::Error::UnexpectedCreatingStdin.

4. RAII

SpawnSyncEventLoop::prepare() returns a guard whose Drop is what cleanup() was, and tick_with_timeout is a method of the guard. That removes two rare_data().spawn_sync_event_loop().expect("cached by …") lookups and their unsafe.
SpawnSyncEventLoop's methods take &self, with what changes in a Cell. A tick's callbacks reach the struct again (through the SpawnSync ctx, and on_uv_timer on Windows) while tick_with_timeout is on the stack, which &mut self did not allow for. The comments that explained how to stay clear of that are gone with it.
SpawnedTerminal and ReportUnwatchedExit replace the other two scopeguards.

Looked at and left

The waiter thread as the fallback when a pidfd cannot register, instead of ending the child. It would keep the child, its status and its rusage. It is not available for one process today: init() and reload_handlers() are gated on a process-wide sticky flag that also makes every later spawn skip pidfd_open, exits reach it through a process-wide SIGCHLD handler, rewatch_posix overwrites a live Poller::Fd without releasing it once the flag is on, and outside of Linux its loop is a sigwait on an empty mask. Ending the child would have to stay for macOS and FreeBSD, where no test could reach it. Worth its own PR.
The wait in kill_and_reap is a blocking wait4() on the JS thread. After a SIGKILL it is over at once, except for a child in uninterruptible sleep.
The Windows path where windows_configure_server fails after the child was started still leaves it running.
scopeguard in functions this does not touch (PipeReader::to_readable_stream, spawn_posix of bun run).

How did you verify your code works?

Every test below fails on main (c7b06d94, release) and passes here.

Test	On `main`
`spawnsync-isolated-event-loop.test.ts`: what is finalized during spawnSync is released on the thread's loop	`exitedDueToTimeout: true`, empty stdout
`spawnSync.test.ts`: a writer finalized during spawnSync leaves its fd number usable	`EEXIST: file already exists, epoll_ctl`
`napi.test.ts`: async work queued by a finalizer during Bun.spawnSync keeps the event loop alive	exits early, prints nothing
`napi.test.ts`: a threadsafe function unref'd by a finalizer during Bun.spawnSync lets go of the event loop	never exits
`spawn-pipe-start-error.test.ts`: 16 cases, one or more for each row of the table in 2	children left, fds left, `Out of memory`, never returns, empty return value
`spawn-stdin-readable-stream-edge-cases.test.ts`: 6 cases of a direct stream that fails inside `Bun.spawn`	child left, 3 fds left, or a `Subprocess` returned
`bun-write.test.js`: a direct stream whose `pull()` throws `42`, `"boom"`, `{ code: "E" }`, `undefined`	never settles

The GC tests are deterministic. They have to get a collection to run inside of the call and nowhere before it:

BUN_JSC_slowPathAllocsBetweenGCs=3 collects every third slow-path allocation.
The objects are dropped from the getter of maxBuffer, the last option spawnSync reads, so nothing is allocated between the drop and the part of the call that matters.
The getter calls Bun.gc(true) first, which empties every free list, so how many slow-path allocations follow does not depend on what the script did before. About 6 do (the tests start to fail at an interval of 7).
Each test asserts that the collection did run during the call, so none can pass by missing it.

Stress. Every changed test, held to 4 cores. Release, 6 at a time: 120 runs of each file (30 of napi.test.ts). Debug+ASAN, 3 at a time, which is what CI's --parallel=3 on 4 vCPUs does: 24 of each (6). Clean but for the flake described further down. These ran on the version before that flake's two-line fix, which itself was only measured as described there.

Also:

The CI failure itself: three garbage Bun.stderr.writer()s collected during a spawnSync, then the real toolIdentity() from scripts/build/tools.ts on a one-line /bin/sh script. main: Cannot tell which …/clang this is after 30,035 ms, 29,164 ms of it CPU. Here: 0.3 s.
The script from #34069 (spawnsync-spin.test.mjs, unchanged): 8 copies, bun test --isolate --timeout 0, 3 such processes at once held to 4 cores, release builds, alternating between the two. main: wedged in 5 of 60. Here: 0 of 60.
21 spawnSync variants (timeout with a grandchild holding the pipe, maxBuffer, large stdin with an early exit, ENOENT, bad cwd, aborted signals, extra fds, node:child_process) with pidfd, BUN_FEATURE_FLAG_FORCE_WAITER_THREAD, BUN_FEATURE_FLAG_DISABLE_MEMFD and BUN_FEATURE_FLAG_DISABLE_SPAWNSYNC_FAST_PATH, also in Workers, including terminate() during the call: the new assert holds.
No cost: 3,000 spawns of /bin/true with piped stdout and stderr, six interleaved runs each on the same 8 cores, Bun.spawnSync 387–405 µs on main and 388–402 µs here, Bun.spawn 395–410 µs and 391–410 µs.
bun run rust:check-all: 12 of 12 targets. Clippy is clean on the files touched.
176 test files on a debug+ASAN build (spawn/, shell/, child_process/, io/, streams, workers, timers, process, terminal, cron, bun run, bun test --parallel): 156 clean. The other 20 were run again on this and on the previous head of this PR: they fail the same way on both (they need a non-root user, a network or FUSE, or a wall-clock bound does not hold on a loaded debug build), but for 4 timeouts in bun-write.test.js that pass 3 of 3 on both when run alone.

A flake this PR introduced on the way, and what it was. An intermediate version made Bun.spawn with a buffer stdin … releases the Subprocess, a test that is on main, report leakedWrappers: 1 about once in 100 runs under load. With main's copy of the file on release builds, 8 at a time on 4 cores: main 0 of 800, that version 5 of 800.

generateHeapSnapshotForDebugging() while the wrapper is retained: no incoming edge and no roots entry. As a control, a wrapper kept by its own pending activity shows an edge from StrongRootBlock.
The fixture then reads its own stack from /proc/self/mem. In all 28 captures (of 3,000 runs of bun-profile), two adjacent words hold the cell's address. The return addresses above them are the live chain that a top-level await's continuation runs under, and with it every Bun.gc() the fixture makes: TimerObjectInternals::fire, EventLoop::exit, VM::drainMicrotasks, MicrotaskQueue::drainImpl<true>, runInternalMicrotask. The two words are in the frame of runInternalMicrotask, which never writes them. Next to them are stale return addresses into Subprocess::on_process_exit (subprocess.rs:1216) and EventLoop::exit: they are callee-saved registers that EventLoop::exit pushed while on_process_exit held this_jsvalue, in an earlier turn of the loop that got to the same depth. Every later turn builds the same frames at the same addresses, so no number of collections from JS gets rid of them.
On a debug build, gdb says the same of another word: a hardware watchpoint on the cell's cellState shows it marked by SlotVisitor::appendJSCellOrAuxiliary from the conservative scan (Heap.cpp:3719), a breakpoint on ConservativeRoots::add finds one word of the 52,456 bytes scanned that points into the cell (cell+72, and genericAddPointer takes a pointer into the middle of a cell for one to the cell), and that word is in the frame of MicrotaskQueue::drainImpl<true>, which does not write it during the collection either.
What moved on_process_exit onto those slots: it is reached through Process::on_wait_pid, whose frame that version had grown by a (Status, Rusage) temporary for the error path.

kill_and_reap is #[cold] #[inline(never)] now and writes to the Rusage its caller already has. The fixture alone, 40 at a time on 4 cores, the three builds taking turns: that version 64 of 4,000, this PR 0 of 4,000, main 0 of 4,000. No test was changed for this.

Not verified: macOS, FreeBSD and Windows at run time, other than by CI. #37968's reproduction needs the reporter's repository and was not run; it is reproduced by its mechanism in the second test above.