PipeReader: do not read an fd again after a read on it fails

Merged
RO
1mo ago
+206-10

Problem

  • test/js/bun/spawn/spawn-stdio-syscall-error.test.ts is red on alpine: "lost": -82000, not 0 (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_once sets PosixFlags::READ_FAILED on a fatal error, and begin_read does not read while it is set. The request parks, then on_reader_error rejects it. Nothing clears the flag.
  • Correct: libuv uv__read clears UV_HANDLE_READABLE before 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

  • PosixBufferedReader reads the fd behind subprocess stdio, file streams and the shell. Its parent gets on_read_chunk, then on_reader_done or on_reader_error.
  • FileReader is the parent behind a ReadableStream. on_read_chunk resolves the parked pull(), and the reaction runs before it returns.
  • Considered: a repeating shim failure hides the lost error. An error stored in FileReader first 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 --filter no longer drains that pipe at exit.
  • Reads that do not fail pay nothing: begin_read tests 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.

Runtimestdin 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:

CaseEntryRelease, no fixDebug, no fixDebug, guard in read_into onlyDebug, this branch
child_process, 'data' listenerpull, read_intofails 6 of 6fails 5 of 5passespasses
child_process, 'readable' and read()set_flowing(true), readfails 6 of 6fails 5 of 5fails 3 of 3passes
Bun.spawn, lazypull, read_intofails 6 of 6fails 5 of 5passespasses
Bun.spawn, reader started at spawnpull, read_intofails 6 of 6fails 5 of 5passespasses
  • The first three use counts: SPAWN_FAULT_RECV_CAP=4096 makes every recv() short, so fill_scratch calls recv() again in the same wakeup. SPAWN_FAULT_RECV_EAGAIN_AT=2 ends the first read loop, so the consumer's read parks and the next read is poll-driven. SPAWN_FAULT_RECV_AT then 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 and read() 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=1 fails the recv() that follows one that returned bytes, and SPAWN_FAULT_READS_AFTER counts the recv() 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 .stdout took 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, shell subproc.rs.
  • 8 only do accounting: filter_run.rs, multi_run.rs, lifecycle_script_runner.rs, security_scanner.rs, git_runner.rs, both cron jobs, test Worker.rs. lifecycle_script_runner.rs and cron.rs build a new reader with init() 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 clear READ_FAILED there, or a cat that 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::Eof when 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