spawn(windows): tear down stdio PipeReader when start_with_current_pipe fails

Merged
RO
4mo ago
+125-53

Problem

On Windows, when the initial uv_read_start on a Bun.spawn stdout/stderr pipe fails (observed from libuv as UV_EINVAL after a bad FileAccessInformation query on the pipe handle), the process hangs idle: the event loop is parked in GetQueuedCompletionStatusEx, no error surfaces, and the code awaiting the child's output/exit has nothing to wake it.

Cause

SubprocessPipeReader::start() on Windows returned the Err straight from start_with_current_pipe(). The spawn bindings then threw and returned without tearing down either pipe:

  • stdout still carried the extra ref from the top of start() (self.r#ref() with no matching deref on the Windows Err path);
  • stderr had never been start()ed at all: refcount 1, process backref set, live uv.Pipe source.

The process had already been watch()ed and is try_killed before the throw. When the killed child's exit callback fires, on_process_exit resumes reads on both still-Readable::Pipe slots via reader.unpause(); reader.read();. The EOF that arrives on the never-started stderr reaches on_reader_done, whose trailing PipeReader::deref(self) assumes the matching start() ref exists, so it dereferences a PipeReader that on_close_io's own deref just freed.

Debug builds hit:

panic: assertion failed: self.magic == MAGIC_VALID

Release builds write through the freed refcount, which in practice manifested as a process stuck idle with no error and no exit.

POSIX already handles this correctly: PosixBufferedReader::start() never returns Err; a register_poll failure is routed through on_reader_error synchronously, which sets state = Err, detaches from the Subprocess via on_close_io, and releases the start() ref. The Windows path diverged.

Fix

Route the Windows start_with_current_pipe() error through on_reader_error (same as POSIX's register_poll failure path), under a ScopedRef keepalive so self survives the teardown long enough to return. start() now returns () on both platforms; the caller proceeds to start the sibling pipe instead of throwing mid-setup, and the failed slot is Readable::Ignore by the time on_process_exit runs.

The two reader .start() calls are also moved ahead of the Writable::Buffer stdin writer's .start(), so the remaining POSIX-only throw path there fires after both readers have taken their start() ref.

Dead code removed alongside the fix: SubprocessPipeReader::start()'s return type is now () (it was already infallible on POSIX), the if let Err { try_kill; throw } arms at its two call sites in js_bun_spawn_bindings.rs are gone, the internal Err match arm in the POSIX branch is gone, and the comment listing "the stdout/stderr Readable::Pipe .start() error paths" among the throwing paths is updated.

Verification

test/js/bun/spawn/spawn-pipe-start-error.test.ts exercises the error path via a debug-only fault-injection env var in start_with_current_pipe (a real uv_read_start failure on a freshly-spawned stdio pipe cannot be triggered from JS). On a Windows debug build:

without the fix
threw: EINVAL
script tail
panic: assertion failed: self.magic == MAGIC_VALID

(exit = debug crash handler)

with the fix
OK

(exit 0)

test/js/bun/spawn/spawn.test.ts passes 118/118 on Windows and the child_process suite passes 38/38 on Windows.

Note on the fail-before gate: the fault-injection hook lives in src/io/PipeReader.rs, so the gate's git stash -- src/ removes it along with the fix; without the hook the test spawns a healthy child and passes. The fail-before above was captured by reverting only SubprocessPipeReader.rs to origin/main with the hook in place.


no test proof · iteration 4 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/spawn/spawn-pipe-start-error.test.ts