PipeReader: do not read an fd again after a read on it fails
Problem
test/js/bun/spawn/spawn-stdio-syscall-error.test.tsis red on alpine:"lost": -82000, not0(build 120303). #43739 added the case, not the bug.read_loop(src/io/PipeReader.rs:766) delivers the bytes read before a failed read, then the error. The consumer asks for more inside that delivery, and the reader reads the fd past the error. The error comes late, or never.
Fix
read_oncesetsPosixFlags::READ_FAILEDon a fatal error, andbegin_readdoes not read while it is set. The request parks, thenon_reader_errorrejects it. Nothing clears the flag.- Correct: libuv
uv__readclearsUV_HANDLE_READABLEbefore it reports a read error. - Verified:
test/js/bun/spawn/spawn-stdio-syscall-error.test.ts, 17 pass. The four new cases fail without the fix. Suites: Notes. - Self-reviewed: 15 concerns raised, 14 addressed. Not taken: a flag reset in
start(), which nothing on main needs.
Background
PosixBufferedReaderreads the fd behind subprocess stdio, file streams and the shell. Its parent getson_read_chunk, thenon_reader_doneoron_reader_error.FileReaderis the parent behind aReadableStream.on_read_chunkresolves the parkedpull(), and the reaction runs before it returns.- Considered: a repeating shim failure hides the lost error. An error stored in
FileReaderfirst needs a callback in every parent. - #43920 is a newer PR with the same change. Its test trigger is here.
Downsides
- After a read error, a stream that ended with
'end'now ends with'error', as in Node. With no listener the process stops. - After a read error,
bun run --filterno longer drains that pipe at exit. - Reads that do not fail pay nothing:
begin_readtests one more bit.
Notes
Trace without the fix (bun 1.4.3-canary.1+367d939d9, shim logs each recv(), writer dd bs=1025). The debug build of main at 8d36bff512 does the same: 3 of 8 runs, two with no 'error' and lost -7714150:
recv #5 len=262144 -> 95325
recv #6 len=166819 -> EIO (injected) same fill_scratch call as #5
JS data 95325
recv #7 len=65536 -> 65536 pull from inside the delivery, read_into
recv #8 .. #116 to EOF
{"received":452025,"got":8000125,"lost":-7548100,"events":["stdout.close","close"]}
No 'error' event: the reads reached EOF before on_reader_error ran, and a stored error is only returned by a later pull. When the reads park first, on_reader_error rejects that pull and 'error' comes late. That is the CI signature: the events match and lost is a negative multiple of 1025.
Why alpine. In CI the failure is injected: the shim fails only the Nth recv(), so a later recv() succeeds and the extra bytes show. The failing recv() must follow bytes in the same wakeup. BusyBox head writes 1025 bytes at a time, so a wakeup often holds a short recv() and then the failing one. coreutils head fills the buffer in one recv(). The same run fails on debian when the writer is slow. With a writer that copies BusyBox head (stdio, 1024-byte buffer), the RECV_AT=6 case fails 7 of 40 runs on bun 1.4.3-canary (release), and 36 of 40 when the writer also spins between chunks. This branch (debug): 0 of 120, and 0 of 60 with dd bs=1025.
With no shim. A child with one AF_UNIX socket as fd 0 and fd 1 writes a line to stdout and reads stdin. The peer leaves that line unread, sends 8192 bytes and closes. The kernel gives the child 8192 bytes, then ECONNRESET, then EOF.
| Runtime | stdin events |
|---|---|
| Node v26.3.0 | 'error' ECONNRESET after 8192 bytes |
| bun 1.4.3-canary.1+367d939d9 | 'end' after 8192 bytes, no error |
| this branch | 'error' ECONNRESET after 8192 bytes |
The new cases. Each one reaches the reader from inside the delivery in a different way. Whole-file runs of the describe block:
| Case | Entry | Release, no fix | Debug, no fix | Debug, guard in read_into only | Debug, this branch |
|---|---|---|---|---|---|
child_process, 'data' listener | pull, read_into | fails 6 of 6 | fails 5 of 5 | passes | passes |
child_process, 'readable' and read() | set_flowing(true), read | fails 6 of 6 | fails 5 of 5 | fails 3 of 3 | passes |
Bun.spawn, lazy | pull, read_into | fails 6 of 6 | fails 5 of 5 | passes | passes |
Bun.spawn, reader started at spawn | pull, read_into | fails 6 of 6 | fails 5 of 5 | passes | passes |
- The first three use counts:
SPAWN_FAULT_RECV_CAP=4096makes everyrecv()short, sofill_scratchcallsrecv()again in the same wakeup.SPAWN_FAULT_RECV_EAGAIN_AT=2ends the first read loop, so the consumer's read parks and the next read is poll-driven.SPAWN_FAULT_RECV_ATthen fails after bytes in that wakeup. For the'readable'case it is 19: #3 to #18 return 64 KiB, the highWaterMark, so the reader is stopped andread()starts it again. - The fourth uses state, and comes from #43920: the writer waits for a line on stdin, so the first read is parked when the bytes arrive.
SPAWN_FAULT_RECV_MID_FILL=1fails therecv()that follows one that returned bytes, andSPAWN_FAULT_READS_AFTERcounts therecv()calls after it. Without the fix it is 1. - With a count-based trigger and the reader started at spawn, the case passed without the fix on a debug build: the buffered reader that runs before JS reads
.stdouttook the bytes and the error. That is why this case uses state. - With the BusyBox-like writer at four speeds, the whole file passes 20 of 20 on this branch.
With the fix, CAP=4096 EAGAIN_AT=2 RECV_AT=5:
recv #3 -> 4096, recv #4 -> 4096, recv #5 -> EIO
JS data 8192
close(fd)
JS error EIO
Node. libuv uv__read (src/unix/stream.c): on a read error other than EAGAIN it clears UV_HANDLE_READABLE | UV_HANDLE_WRITABLE, calls read_cb with the error, then stops the watcher. It calls read_cb once for each read(), so it never holds bytes and an error from one batch.
Placement. EOF and the maxBuffer stop have the same guard at this site: close_if_final closes the reader before the final chunk is delivered. An error cannot use it, because a closed reader with no stored error reads as a clean end. For the same reason READ_FAILED is not part of is_done().
Parents (14 BufferedReaderParent implementations, what each does in on_reader_error):
- 3 release the fd:
FileReader,SubprocessPipeReader,Terminal. - 2 drop the reader:
FileResponseStream, shellsubproc.rs. - 8 only do accounting:
filter_run.rs,multi_run.rs,lifecycle_script_runner.rs,security_scanner.rs,git_runner.rs, both cron jobs, testWorker.rs.lifecycle_script_runner.rsandcron.rsbuild a new reader withinit()for each spawn. - 1 is shared and can restart: the shell
IOReader.
Only two read the same reader after an error. filter_run.rs drain_and_close_pipes reads once more at exit. That read is now a no-op, and deinit() follows. Before, it could reach a second terminal callback and decrement remaining_fds twice. The shell IOReader::start() does not restart a reader after a failed read on main, because the fired one-shot poll still counts as registered. The Windows reader gets one libuv callback for each read, so it has no bytes-then-error batch.
Left open.
- The flag is permanent. #39638 and #37901 change
IOReader::start()to restart the shell's stdin reader. After this PR they must clearREAD_FAILEDthere, or acatthat follows a stdin read error gets no data, no EOF and no error. - Not in this PR: a batch that stops because the buffer is full is labelled
ReadState::Eofwhen the poll event carries the hangup (read_state,None if received_hup).Bun.write(file, proc.stdout)then writes 262144 of 400000 pending bytes and resolves. It is on main and in 1.4.3-canary, and this PR does not change it. It needs its own change. - Not in this PR: release the fd on a read error once, in
PosixBufferedReader::on_error. #41456 names it as a follow-up. #41420, #41456 and #42150 did it for one parent each.
Suites run on the debug build: test/js/web/streams/streams.test.js (624 pass), test/js/node/stream/node-stream.test.js (112 pass), test/js/bun/spawn/spawn-streaming-stdout.test.ts (pass), test/js/node/child_process/child_process.test.ts (81 pass, 2 fail in my container for reasons outside this change: "spawn in the default shell" reads an empty $SHELL, and "extra stdio pipes are not double-closed on GC" needs 5.0 s in a debug build against the 5 s timeout, its script prints OK). With this branch's build, the test file of #43920 passes 17 of 17 in 3 runs.
#43790 is open and edits the comment above the failing case. It changes the event order in native-readable.ts, not the reader.
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