Conversation
…each listener once
A second builtin `cat` reading the same stdin in one script hung forever on
POSIX: IOReader::start() decided whether to (re)start the read with
FilePoll::is_registered(), which stays true after the one-shot poll has
fired without being re-armed, i.e. exactly the state a finished read (EOF
or error) leaves behind. Use BufferedReader::has_pending_read() instead,
the same predicate IOWriter::write() uses for the writable poll, so a
listener added after a finished read starts a new one.
The done/error callbacks also left the notified listeners in `readers`, so
once a restart did happen (today on the error path, after this change on
the EOF path too) the previous cat's entry was dispatched again with a
NodeId that had been freed or reused, e.g.
panic: expected Node::Cmd at Node#2, got Subshell
for `cat; (echo ---; cat)`. Take the list before notifying, so every
listener is notified exactly once, with the outcome of the read it
registered for; a cat started from inside the notification registers into
the emptied list and is served by the read it restarts. This makes the
stored `raw_err` dead (no path reports both an error and done for the same
read), so it is removed.
…eader callbacks The comments on reader() and in on_read_chunk_cb claimed every callback runs under a live &mut ReaderImpl. That holds on Windows and for a registration failure reported synchronously from PosixBufferedReader::start(), but the poll-driven POSIX dispatches hold no borrow, which is what the restart from inside on_reader_done_cb / on_reader_error relies on.
… callback and take_readers
…nd the read-error path on Windows - A tty case: ^D is consumed by the read that sees it, so the second cat only finishes if it really reads stdin again, unlike a pipe, where reading again and completing on the spot both print the same thing. - Two more /dev/full cases: a third cat served by the wakeup of the poll the second cat re-registered mid-read, and that wakeup arriving with nobody left to notify. - Only the EOF block is skipped on Windows (the source is closed at EOF there, #29986); the read-error block runs everywhere. On Windows the subshell case panics without take_readers and passes with it.
…timing out Use the inline terminal form: its exit callback fires once the exited child's output has been delivered, so it can reject whichever marker is still awaited with the output collected so far.
…in reader reached EOF The Windows reader closes its libuv source when it reaches EOF, so a second cat on the same stdin cannot start a new read there: start_with_current_pipe() panicked on the missing source. Remember the EOF on the IOReader and complete the listeners that register after it with that EOF. The last completion is returned as the start() Yield, so a script of consecutive cats does not nest one Yield::run per cat. The EOF cases in cat.test.ts now run on Windows too, plus a six-cat case that the debug build fails if the completions nest.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 5 included reviews per hour; 0 remain after this review. Walkthrough
ChangesShared stdin reader lifecycle
Possibly related PRs
Suggested reviewers: Merge Risk: ⚪ Minimal · up to This change is merge-ready after normal checks and review; no actionable merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
The review above lists no findings. On the related PRs it names: this PR replaces #37752 (same five commits, rebased) and covers the Windows case of #29986, see the notes in the description. #38886 is already in the base. Its read loop dispatches the EOF and error callbacks in tail position, which the POSIX restart from inside a notification relies on, and the test file passes on a debug build of this branch on Linux and Windows. CI build 101215 is running. |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The fix is well-reasoned and thoroughly tested, but it touches re-entrancy and UnsafeCell aliasing invariants in the shell IOReader across divergent POSIX/Windows paths, so a human look would still be worthwhile.
What was reviewed:
has_pending_read()on POSIX checksis_watching()(registered && not needs-rearm), so a fired one-shot poll correctly triggers a restart.- POSIX
done()/on_error()in PipeReader.rs dispatch via raw pointer with the vtable copied out first — the updatedreader()comment's claim that a restart from inside a done/error notification holds no live&mut ReaderImplmatches the implementation. take_readers()drains before notifying, so a listener that registers during a notification lands in a fresh list and isn't re-notified under a recycled NodeId.- Removal of
raw_err:on_reader_done_cbnow only ever passesNone, and afteron_reader_errorthe listener list is empty — no path replayed the stored error.
Extended reasoning...
Overview
This PR fixes the shell builtin cat so that a second cat reading the same stdin (in one script) is served correctly instead of hanging (POSIX) or panicking (Windows, and the recycled-NodeId subshell case everywhere). It touches src/runtime/shell/IOReader.rs and adds a 22-case test file. Core changes: (1) POSIX start() gates on has_pending_read() instead of is_registered(), so a fired one-shot poll restarts; (2) EOF/error callbacks take_readers() before notifying so entries aren't re-dispatched under recycled node ids; (3) Windows gains a reached_eof flag and finish_listeners_at_eof() that returns the last completion as the start() Yield to avoid nesting Yield::run per cat; (4) the unused raw_err replay is removed.
Security risks
None identified. This is internal shell interpreter plumbing; no untrusted-input parsing, auth, or crypto is involved.
Level of scrutiny
High. The change is not mechanical: it relaxes the documented aliasing constraint on reader() (POSIX poll dispatches now permit a nested start()), relies on PipeReader.rs dispatching done()/on_error() in tail position via a copied-out vtable with no live &mut Self, and handles re-entrancy where a listener's completion synchronously registers and starts the next listener. It also introduces platform-divergent semantics (POSIX re-reads the fd; Windows completes on the spot after EOF) and a Yield-trampoline depth fix whose correctness depends on how Cat::start threads the returned Yield. I verified the PipeReader.rs claims and the has_pending_read/is_watching semantics, and traced that on_reader_error/on_reader_done_cb no longer call self.reader() — but a maintainer familiar with the shell's Yield trampoline and the Windows libuv reader lifecycle should confirm the design choices (particularly returning Yield::suspended() from finish_listeners_at_eof when the list is empty, and the interaction with on_read_chunk_cb's in-place readers iteration which was left unchanged).
Other factors
The test coverage is strong: it exercises captured vs inherited stdout, subshell/if node-id recycling, the six-cat trampoline-depth case, a tty case that distinguishes re-read from complete-on-the-spot, /dev/full mid-read unregistration, and read-error paths. The PR description documents fail-before on both platforms and lists related suites re-run. This takes over an earlier PR (#37752) at a maintainer's request and diverges from #29986's approach with stated reasoning — that design decision is another reason a human should sign off.
|
On the two points the review above asks a human to confirm:
CI build 101215: 175 of 179 jobs passed so far, 4 running, nothing failed. |
|
Updated 3:49 AM PT - Aug 19th, 2026
❌ @robobun, your commit 95c65c2 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 39638That installs a local version of the PR into your bun-39638 --bun |
|
CI build 101215 finished: 178 of 179 jobs passed. cat.test.ts passed on every lane, including both Windows lanes (where the EOF block now runs), the ASAN lane and darwin. The one failed job is a darwin aarch64 shard on test/js/node/tls/node-tls-server.test.ts, which also fails on main and is unrelated to this change (reported separately). The other annotations are flaky tests that passed on retry. Ready for review. |
|
Another report of this panic arrived with a different trigger. It is the failed-read case that this PR already fixes. import { $ } from "bun";
const r = await $`cat; (cat); echo done`.quiet().nothrow();
console.log(r.exitCode, JSON.stringify(r.stdout.toString()));Set The kernel does not poll these fds. This branch still merges into main (7e56b40) without conflicts. On a Linux debug build of that merge the script above prints A note for anyone who tries more variants on a debug build. Consecutive cats on this synchronous error path nest one |
|
A note for this PR from #43900, which is open and changes #43900 adds This PR makes What this PR needs when #43900 is on main:
On main today |
### 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. <details><summary>Notes</summary> **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 8d36bff 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=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. </details> <!-- robobun:evidence:begin --> --- **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 <!-- robobun:evidence:end -->
Problem
cat(default on Windows,BUN_ENABLE_EXPERIMENTAL_SHELL_BUILTINS=1on POSIX) cannot read stdin twice in one script.cat; echo ---; cathangs on POSIX and panics on Windows (Option::unwrap()onNone). After a failed read,cat; (echo ---; cat)panics everywhere:expected Node::Cmd at Node#3, got Subshell(Sentry BUN-4AVS).IOReader(src/runtime/shell/IOReader.rs) keeps notified listeners, so the next read notifies the first cat again under a freed or reused node id. POSIXstart()checksis_registered(), which stays true after the one-shot poll fired. Windows closes the source at EOF.Fix
start()checkshas_pending_read(), so a later cat reads the fd again, as a systemcatwould: EOF again on a pipe, new input on a tty, the same error on a bad fd.start()after EOF completes the new listeners with that EOF. It returns the last completion as itsYield, so consecutive cats do not nest oneYield::runeach.Background
Bun.$runscatinside the interpreter. All cats in a script share the oneIOReaderof the script's stdin (or of a pipeline stage) and register on it by node id. It sends chunks, then one EOF or error, to each listener.Yieldis the trampoline value: a step returns what runs next andYield::runloops over it.Notes
Takes over #37752 at a maintainer's request. Its five commits are cherry-picked onto the current main unchanged (the resulting
IOReader.rsis identical to a clean merge). The last commit adds the Windows EOF case, which #37752 had left to #29986. #29986 drains the list under a re-entrancy flag and completes late registrants from inside the drain. That does not fit the POSIX restart (a listener that registers during a notification now has a read of its own), so the Windows case is handled instart()instead, keyed on areached_eofflag that the done callback sets before it notifies anyone.The stored error that a later EOF used to replay is removed: a read ends in exactly one of EOF or error, and after an error the listeners are gone.
Why the last completion is returned: with captured output, cat N+1 starts from inside cat N's completion. Running its completion with
run_yieldthere nests oneYield::runper cat. With the six-cat case a Windows debug build of that variant panics withassertion failed: n <= Self::MAX_DEPTH(captured and inherited stdout). Returned as thestart()Yield, the completions follow each other on the trampoline.After a read error on Windows the source stays open, so a later cat restarts the read and gets the error again. That path already worked and the last two cases cover it on every platform.
The POSIX restart from inside an EOF notification is safe with respect to
PosixBufferedReader:read_loop(src/io/PipeReader.rs) dispatchesdone()andon_error()in tail position and copies the vtable out first, so nothing touches the reader after the callback re-armed it. The/dev/fullcases cover a second cat that registers while the read that served the first cat is still running: the poll is re-registered once more and the extra wakeup either serves a third cat or reads EOF with nobody to notify.The tty case distinguishes "read the fd again" from "complete on the spot" on POSIX: the second cat only prints
moreif it reads again. On Windows the fd is closed at the first EOF, so that case stays POSIX only, as do thesh -cpipe case and the/dev/fullcases.Suites run on a Linux debug build: cat.test.ts, bunshell.test.ts, pipeline_stack.test.ts, file-io.test.ts, yield.test.ts, shell-pipe-read-fault.test.ts, shell-blocking-pipe.test.ts. On a Windows debug build: cat.test.ts, bunshell.test.ts, pipeline_stack.test.ts, file-io.test.ts (the five tilde expansion cases fail on that machine with and without this change, it has no HOME set).
Fail-before on Linux was measured with the unfixed release build: the 18 hanging cases time out,
cat || echo first-failed; (echo ---; cat) || echo second-failedpanics with the BUN-4AVS message, and the other three cases pass and act as guards. On the Windows canary (32e8703) the EOF cases panic as described and the subshell read-error case panics with the BUN-4AVS message.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/shell/commands/cat.test.ts