shell(cat): stop reading stdin while no cat is listening by robobun · Pull Request #37901 · oven-sh/bun

robobun · 2026-08-12T17:37:27Z

Problem

Builtin cat (the default on Windows, BUN_ENABLE_EXPERIMENTAL_SHELL_BUILTINS=1 on POSIX) reading a script's stdin. When the cat that is currently reading goes away before EOF (its first write fails: cat | true, cat > /dev/full), the shell keeps reading stdin and throws the bytes away until the next cat registers. A later cat in the same script, or a later subprocess, sees only what arrives after it started, often nothing. A real shell leaves everything the dead cat had not read in the fd for the next command.
Cause, src/runtime/shell/IOReader.rs: remove_reader() only removes the list entry, and on_read_chunk_cb returns has_more != Eof whether or not anyone is left in readers, so the bun_io read stays armed (POSIX: the read loop re-registers the poll after every chunk; Windows: on_file_read issues the next uv_fs_read unless the reader is paused). Every chunk that arrives while the list is empty is dropped by the dispatch loop. Same logic as the Zig version, so inherited rather than a porting bug.

Repro shape (what the new test automates)

Child, with the builtin enabled and stdin piped from the parent:

await Bun.$`cat | true; ${process.execPath} -e ${"await (await fetch(process.env.RELEASE_URL)).text()"}; echo ready; cat`;

The parent writes one line (the first cat reads it, fails to write it, dies), waits for the fetch to arrive (so the first cat is gone), writes 300 KB, lets the fetch return, waits for ready, closes stdin. A shell prints the 300 KB from the second cat; bun prints nothing from it (the 300 KB were read and dropped while the script was parked on the fetch). Same with /bin/cat as the second reader, and with cat > /dev/full as the first.

Fix

remove_reader() pauses the bun_io reader when it removes the last listener. On POSIX that unregisters the poll, so bytes not read yet stay in the kernel for whoever reads the fd next (a later builtin cat, or a subprocess inheriting the fd); on Windows it sets the paused flag, so the completion of the read that is in flight does not issue another one (and tries to cancel it).
start() unpauses before its existing (re)arm logic. On Windows, when the read issued for the previous listener is still in flight (is_reading), it now clears the pause so that read's completion re-arms for the new listener instead of stopping after it.
Chunks that still arrive with nobody listening are kept in the (previously unused) State::buf and handed to the next listener ahead of the next chunk or the EOF of the read start() issued for it (deliver / flush_buffered). Two sources: on Windows the uv_fs_read that was already blocked in the threadpool when the cat died completes with data; on POSIX, after a hang-up, bun_io drains the pipe to EOF regardless of what the parent returns. They are delivered from the read callbacks rather than from start() because start() is called from inside the trampoline (same reason IOWriter::write returns a synchronous error instead of dispatching it).
on_read_chunk_cb returns false as well when nobody is listening, and a listener asking to be removed via the remove flag goes through remove_reader, so that path pauses too.
Why this is the right shape: which bytes belong to whom follows what a real shell does. Bytes handed to a cat that then dies were consumed by it (a real cat had read() them too); bytes the kernel handed us with nobody to give them to are kept for the next reader; bytes nobody asked for stay in the fd. pause()/unpause() is the contract bun_io already provides for a parent that wants to stop consuming (FileReader::set_flowing uses it from inside the data callback for process.stdin.pause(); the read loops, try_register_poll, on_poll and on_file_read all honor it), and the shell's IOReader is the layer that knows whether anyone is listening, so nothing in bun_io changes.
Not changed: bun_io's drain after HUP ignores the parent's return value on purpose (the shell's subprocess PipeReader relies on that to reach EOF), so after a hang-up the remaining bytes end up in State::buf rather than in the fd. That differs from a real shell only for a subprocess reading after a builtin cat died during such a drain, and needs more than one read's worth (256 KiB) of input plus the hang-up in the same wakeup, which a default Linux pipe or socketpair cannot even hold; that is also why no test covers the POSIX side of the buffer.
Verified with test/js/bun/shell/commands/cat.test.ts (new file; shell(cat): restart the shared stdin reader for a later cat, notify each listener once #37752, shell(cat): finish after a read error instead of cancelling the queued output and hanging #37743 and shell(cat): read regular files synchronously in the builtin on POSIX #35337 create the same file for their own cat fixes, the helpers are independent, whichever lands later appends its cases). Matrix: first cat dies from the event loop (EPIPE from cat | true, all platforms) or synchronously inside the chunk dispatch (cat > /dev/full, Linux) x {a later builtin cat gets the input written while nobody was listening; a later subprocess gets it (POSIX, since on Windows the in-flight read holds one chunk for builtins only); input written only after the later cat started, which exercises the resume paths including the Windows in-flight branch and passes before the fix too}. On the released bun the four "written in between" cases fail deterministically (10/10 runs; the second reader prints nothing: Expected: 300013, Received: 13), the two resume guards pass; all six pass with the debug build.
Windows (x64, where the builtin is the default): with the released bun the cat | true "written in between" case fails the same way (Received: 13); with a debug build of this branch the two cases that apply there pass, 6 runs out of 6. That case goes through the in-flight read: the 300 KB start arriving in the uv_fs_read that was blocked when the first cat died, so its completion takes the deliver-with-no-listener path and the second cat gets those bytes from flush_buffered; the "written after it started" case goes through the new is_reading branch of start().
Also ran test/js/bun/shell/bunshell.test.ts: 418 pass. With BUN_ENABLE_EXPERIMENTAL_SHELL_BUILTINS=1 exported, 5 of its tests fail; all of them read a regular file through the builtin and fail identically on the released bun (shell(cat): read regular files synchronously in the builtin on POSIX #35337's bug).
Overlaps textually with shell(cat): restart the shared stdin reader for a later cat, notify each listener once #37752 (the start() condition and the done/error callbacks) and shell(cat): finish after a read error instead of cancelling the queued output and hanging #37743 (start() returning its error) but is independent of both: they change when a finished read is restarted and how a start failure is reported, this changes what happens while nobody is listening. shell(cat): restart the shared stdin reader for a later cat, notify each listener once #37752's description lists this bug as out of its scope.

Background

IOReader: one per fd a script reads (the script's stdin, a pipeline stage's stdin, a < file), shared by every builtin cat that reads that fd during the script. A cat registers with add_reader + start() and receives chunks / EOF through the BufferedReaderParent vtable; Cat::on_io_writer_chunk unregisters it with remove_reader when its output fails. The stdin reader outlives the individual cats.
bun_io BufferedReader, POSIX: a one-shot readable poll. After each chunk the read loop re-registers the poll unless the parent returned false; once the poll reported HUP it instead loops reading until EOF, ignoring the return value. pause() unregisters the poll and turns register_poll / on_poll into no-ops; unpause() only clears the flag, the parent re-arms (here: start()).
bun_io BufferedReader, Windows: one uv_fs_read at a time; its completion callback issues the next one unless the reader is paused. pause() sets the flag and attempts uv_cancel, which fails for a read the threadpool is already blocked in, so that read still completes with data later; unpause() clears the flag and issues a read only if none is in flight.
Yield trampoline: shell state functions return a continuation that Yield::run drives; event-loop callbacks (like the read callbacks here) start a run with run_yield, while code that is itself running inside a continuation (like start(), called from Cat::next) must not start a nested run. That is why the kept bytes are delivered from the callbacks.

When the cat reading a shared stdin IOReader went away before EOF, the reader kept reading the fd and dropped every chunk it got, so a later cat (or a subprocess) in the same script saw less of the input than a real shell would leave for it. Pause the bun_io reader when the last listener unregisters and resume it in start(). Chunks that still arrive with nobody listening (a uv read that was already in flight, the posix drain of a hung-up pipe) are kept and handed to the next listener ahead of its own read.

coderabbitai · 2026-08-12T17:37:35Z

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 12 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: ff75da96-6676-4d23-9691-84724c31d6d3

📥 Commits

Reviewing files that changed from the base of the PR and between 9a543cc and d6c5eaf.

📒 Files selected for processing (2)

src/runtime/shell/IOReader.rs
test/js/bun/shell/commands/cat.test.ts

_{Comment @coderabbitai help to get the list of available commands.}

robobun · 2026-08-12T17:37:55Z

Status: reproduced on the released bun with test/js/bun/shell/commands/cat.test.ts (the four "written in between" cases fail deterministically, the second reader prints nothing; on Windows x64 the applicable case fails the same way). All cases pass with a debug build of this branch on Linux (5/5 runs) and Windows x64 (6/6 runs). Fix is in #37901 (this PR).

github-actions · 2026-08-12T17:46:36Z

This PR may be a duplicate of:

shell(cat): restart the shared stdin reader for a later cat, notify each listener once #37752 - Rewrites the same IOReader::start() restart condition and the same readers-list dispatch/lifecycle for the next-cat-on-shared-stdin-reader bug, and adds the same new test/js/bun/shell/commands/cat.test.ts.
shell(cat): finish after a read error instead of cancelling the queued output and hanging #37743 - Restructures the same IOReader::start() (signature change from Yield to sys::Result<()>) plus the read-error/completion path of builtin cat, and adds the same new test/js/bun/shell/commands/cat.test.ts.
shell(IOReader): drain readers instead of iterating a snapshot of the list #29986 - Rewrites the same IOReader listener-notification loops (drain_readers replacing the snapshot iteration this PR moves into deliver) and the Windows start() path, for the same listener-lifecycle class of bug.

🤖 Generated with Claude Code

robobun · 2026-08-12T17:50:07Z

^{Updated 10:57 AM PT - Aug 12th, 2026}

❌ @robobun, your commit d6c5eaf has 2 failures in Build #93442 (All Failures):

test/js/node/worker_threads/worker-transfer-terminate-stress.test.ts - unchecked exception at JSC__JSModuleLoader__loadAndEvaluateModule @ src/jsc/bindings/bindings.cpp:3788 (thrown from vendor/WebKit/Source/JavaScriptCore/runtime/VMTraps.cpp:443) on 🐧 13 x64-asan
scripts/build/ci.ts - build failed on :freebsd: aarch64 - build-bun
scripts/build/ci.ts - build failed on 🐧 aarch64-musl - build-bun
scripts/build/ci.ts - build failed on 🍎 aarch64 - build-bun
scripts/build/ci.ts - build failed on 🍎 x64 - build-bun
scripts/build/ci.ts - build failed on 🐧 x64-musl - build-bun

🧪 To try this PR locally:

bunx bun-pr 37901

That installs a local version of the PR into your bun-37901 executable, so you can run:

bun-37901 --bun

robobun · 2026-09-25T03:32:03Z

A note for this PR from #43900, which is open and changes src/io/PipeReader.rs.

#43900 adds PosixFlags::READ_FAILED. read_once sets it when a read fails with an errno that is not a retry, and begin_read does not read while it is set. Nothing clears the flag: PosixBufferedReader::start() keeps it.

This PR makes IOReader::start() restart the shared stdin reader after a finished read. If both PRs merge as they are, a cat that follows a stdin read error gets no data, no EOF and no error: start() returns Ok and arms the poll, and begin_read then ignores the wakeup. The two PRs share no file, so git does not show the conflict.

What this PR needs when #43900 is on main:

IOReader::start() must clear PosixFlags::READ_FAILED before its has_pending_read() check.
cat.test.ts needs a case where a read fails. test/js/bun/shell/shell-pipe-read-fault.test.ts has a shim for that.

On main today IOReader::start() does not restart a reader after a failed read (the fired one-shot poll still counts as registered), so #43900 changes nothing for the shell until this PR lands.

### 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>  --- **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

github-actions Bot added the claude label Aug 12, 2026

robobun mentioned this pull request Sep 25, 2026

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

Merged

Navigation Menu

shell(cat): stop reading stdin while no cat is listening - #37901

shell(cat): stop reading stdin while no cat is listening#37901
robobun wants to merge 1 commit into
mainfrom
farm/de1deb4a/shell-ioreader-pause-without-listeners

robobun commented Aug 12, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Aug 12, 2026

Review limit reached

Uh oh!

robobun commented Aug 12, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Aug 12, 2026

Uh oh!

robobun commented Aug 12, 2026 •

edited

Loading

Uh oh!

robobun commented Sep 25, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

robobun commented Aug 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Fix

Background

Uh oh!

coderabbitai Bot commented Aug 12, 2026

Review limit reached

Uh oh!

robobun commented Aug 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Aug 12, 2026

Uh oh!

robobun commented Aug 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

robobun commented Sep 25, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

robobun commented Aug 12, 2026 •

edited

Loading

robobun commented Aug 12, 2026 •

edited

Loading

robobun commented Aug 12, 2026 •

edited

Loading