subprocess: surface stdio read and write errors instead of dropping them

Merged
RO
2mo ago
+458-30

Problem

  • A recv() or send() on a subprocess stdio socket that fails with an errno other than EAGAIN/EINTR (ENOBUFS, ENOMEM, EIO, ECONNRESET) is lost in four places. Bun.spawn({stdin: ReadableStream}) truncates the child's input and never calls the stream's cancel(). child.stdout from node:child_process emits 'close' with no 'error'. Bun.spawn rejects the read with EIO but keeps the stdout fd open, so a still-writing child blocks and proc.exited never settles. Bun.spawnSync returns {stdout: undefined, success: true}.
  • The common cause is FileReader::on_reader_error (src/runtime/webcore/FileReader.rs): it only rejects a parked read. With none parked it drops the error, leaves the fd and the poll in place, and the next pull reads on. #41420 (merged) fixes that function for Bun.file().stream(). The buffered SubprocessPipeReader turns an error into Readable::Ignore (subprocess.rs:on_close_io), native-readable.ts _destroy calls its callback without the error, and FileSink::on_close hands the piped stream undefined as its cancel reason.

Fix

  • On top of #41420's read_error slot, now on main: on_pull returns a stored error before it consults the reader state, so a reader that never started can end with one.
  • Readable::Errored(bytes, err) keeps a buffered pipe's partial output and error, and the reader releases its fd at once, the way EOF does, so the child gets EPIPE. .stdout becomes a stream that delivers the bytes and then errors (ReadableStream::from_bytes_then_error). Bun.spawnSync throws the error, so child_process.spawnSync reports result.error and execFileSync throws.
  • native-readable.ts passes the error to the _destroy callback, so the node Readable emits 'error' before 'close'. FileSink::on_close passes the recorded write error to the source, and the pump's abrupt path (rsisAbrupt) cancels the stream with it even though its own orphaned reader holds the lock.
  • Verified: test/js/bun/spawn/spawn-stdio-syscall-error.test.ts (12 tests, all fail on 1.4.1; an LD_PRELOAD shim fails the Nth recv/send on the stdio socketpair). Also test/js/bun/spawn/, test/js/node/child_process/, test/js/node/stream/, test/js/web/streams/, test/js/bun/io/bun-write.test.js, the shell fault tests.

Background

  • Bun.spawn reads stdout into a SubprocessPipeReader until JS touches .stdout. Then the fd and its buffer move into a FileReader, the native source behind the JS ReadableStream. A JS pull first tries a synchronous read; with no data it registers a poll and parks a Pending promise.
  • The BufferedReader reports on_read_chunk, on_reader_done (EOF: it closes itself first) or on_reader_error (it did not close).
  • node:child_process wraps the native stream with native-readable.ts, a Readable whose _read calls the native pull. A sync throw from pull reaches errorOrDestroy, which calls _destroy(err, cb). Node's contract is cb(err).
  • A ReadableStream given as stdin is pumped into a FileSink by readStreamIntoSink (BunStreamSource.cpp). A sink write that fails synchronously throws into the pump (rsisAbrupt). A write that fails later closes the writer, which calls the pump's onClose(reason).
Notes

The four faces come from a fault-injection fuzz round (internal ledger entries 20002 to 20005, not GitHub issues): strace injected errnos on the parent's stdio socketpairs. All four pre-date 1.4.

Open question for review: Bun.spawnSync has no error channel other than a throw, so the partial output, the exit code and the pid are lost when a stdio read fails. Node's spawnSync returns {error, status, pid, stdout: <partial>}. A result field on Bun.spawnSync (next to exitedDueToMaxBuffer) would let child_process.spawnSync match Node. That is new API surface, so this PR keeps the throw.

Follow-up candidates, kept out of this PR: the fd release on a read error belongs once in PosixBufferedReader::on_error (src/io/PipeReader.rs) for every parent, not per parent. The native-readable.ts _destroy fix also makes a user's child.stdout.destroy(err) emit 'error' before 'close'.

The strace-based repros from the ledger reproduce with the test's shim (strace is not available in the build container). On 1.4.1:

  • stdin stream, send #3 fails with ENOBUFS: cancelReason "not-called", the child reads 131072 of 8388608 bytes, exit code 0, nothing thrown.
  • child_process, recv #3 fails with EIO: events ["stdout.close", "exit:1:null"], 360448 of 8000000 bytes.
  • Bun.spawn for await, recv #3 fails: rerr "EIO", then exited does not settle, the child sits in do_wait/sock_alloc_send_pskb. With recv #4 failing instead the error is swallowed and reading continues.
  • Bun.spawnSync: {stdout: undefined, exitCode: 0, success: true}; cp.spawnSync {stdout: null, status: 0, error: undefined}; execFileSync returns null.

rsisAbrupt cleared op->m_reader and then tested isReadableStreamLocked(stream). The orphaned reader still locks the stream, so that branch never ran and the source's cancel() was never called. The old JS pump had the same gap (stream.cancel(e) on a locked stream rejects).

The shim interposes syscall() for SYS_close as well as close(): bun closes fds through the raw syscall, so a per-fd counter that only reset in close() carried over to a recycled fd number.

Pre-existing on this machine with or without the change (debug ASAN build): test/js/bun/spawn/spawn-pipe-leak.test.ts exceeds its 30 s budget (450 ms per iteration), test/js/web/streams/streams-leak.test.ts "Absolute memory usage" sits at the 700 MB ASAN bound (692 to 713 MB across runs on both builds), and child_process.test.ts "spawn in the default shell" fails on the released bun too.

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/spawn-stdio-syscall-error.test.ts