[robobun]

Problem

• A Bun Shell builtin that prints several items (ls -d a b c, rm -v, mkdir -v) never finishes once the process's stdout has lost its reader, e.g. bun script.ts | head -2. The awaited $ promise never settles, so the script hangs (bun 1.4.0 too); one item, or echo, finishes.
• When a write to the shell's stdout queue failed with EPIPE, the queue reported the failure once per child command and silently dropped that child's other queued chunks.
• rm and the OutputTask builtins queue one chunk per argument under one child and finish only after counting a completion per chunk, so after the first EPIPE they waited for the other N-1 forever, and those tasks leaked.
• The Zig shell gave each chunk its own child, so one error per child was one per chunk; the Rust port put them under one child without changing the error path.

Fix

• When the queue fails, every chunk still queued gets its own error completion, oldest first. All three failure paths (async error, synchronous error during enqueue, end of file on the poll) share one function.
• Property to check: every queued chunk either receives exactly one completion or was cancelled by its owner first. Single-chunk children see exactly what they saw before.
• The two children that stop at their first error, cat and the subprocess output relay, now cancel their remaining chunks inside that first callback. For the relay this is required: the first completion can free the relay, so a second one is a use-after-free (ASAN report in the original). The cat line is covered by review only.
• Verification: new tests close the stdout reader, then run ls -d with 16 entries, mkdir -v, rm -v and a relayed subprocess. Without the fix the three builtins time out in 6/6 runs; with it all pass on a debug ASAN build. Exit codes on EPIPE are deliberately not asserted (shell: exit 1 on builtin failure instead of raw errno; fix cd ENOENT message #32278 is changing them).

Background

• Bun Shell ($) runs commands such as ls, rm, mkdir and cat in-process as builtins; their output reaches the process's stdout through the shell, not directly.
• IOWriter is the shell's per-file-descriptor write queue. A child enqueues chunks tagged with its ChildPtr and normally gets one completion callback per chunk; cancel_chunks marks a child's queued chunks dead, so they are skipped with no callback.
• rm and the OutputTask builtins produce output on a thread pool, one chunk per argument, all under the builtin's single ChildPtr, and finish when completions counted equals chunks queued.
• CapturedWriter relays a spawned subprocess's piped output into the same queue, one chunk per read. It is embedded in the subprocess's pipe reader, so finishing the command from its callback can free it.
• EPIPE is the error for writing to a pipe whose reader has closed. The shell learns of it either as a write error or as end of file from the poll, hence more than one failure path.

Original description

What does this PR do?

A Bun Shell builtin whose output consists of several chunks never finishes when the process's stdout has lost its reader, so the awaited $ promise never settles and the script hangs:

// hang.ts (dst_1, dst_2, dst_3 exist)
import { $ } from "bun";
$.nothrow();
for (let i = 0; i < 30; i++) {
  console.log("iteration", i);
  await $`ls -d dst_*`; // same with `rm -v a b c` or `mkdir -v a b c`
}
console.error("finished");

$ timeout 15 bash -c 'bun hang.ts | head -2 >/dev/null'; echo rc=$?
rc=124      # head exits after two lines; the next ls never finishes, "finished" is never printed (bun 1.4.0 too)

With a single entry (ls -d dst_1), or with echo, the command finishes. .quiet() avoids it because nothing is written to stdout at all.

Cause

IOWriter is the per-fd write queue the shell uses to echo command output. When a write fails, on_error (and the EndOfFile arm of on_write_pollable, via broken_pipe_for_writers) delivered one error completion per distinct child and dropped the rest of that child's queued chunks.

rm and the OutputTask builtins (ls, mkdir, touch, cp) queue one chunk per argument, all under the same ChildPtr ((cmd, WriterTag::Builtin)), and finish once they have counted a completion for every chunk (output_done >= output_waiting, output_queue FIFO). After the first EPIPE they received exactly one completion and waited for the other N-1 forever. The N-1 OutputTask boxes leaked as well.

In the Zig implementation each OutputTask was its own writer child, so "one error per child" was one error per chunk for these builtins; the Rust port collapsed them onto the builtin's ChildPtr (the output_queue FIFO is the stopgap for that) without changing the error path.

Fix

fail_pending_writers now gives every chunk that is still queued its own error completion, oldest first, and all three error paths (on_error, on_sync_error, EndOfFile) go through it; broken_pipe_for_writers is gone.

Two children stop at their first error while more of their chunks may still be queued: cat and the subprocess output relay (CapturedWriter). Both now cancel_chunks() their remaining chunks from inside that first error callback, and the queue is walked in place (liveness re-checked before each callback, cleared only at the end) so that the cancellation takes effect. This is the same cancel_chunks both already use on their other teardown paths. Nothing can be appended to the queue during the walk: err/broken_pipe are set before the first callback, so a re-entrant enqueue is answered by handle_dead_writer as before (#32946).

The CapturedWriter cancellation is required, not just tidy: its first error completion can go try_signal_done_to_cmd -> buffered_output_close -> close_io -> Readable::finalize -> detach, which drops the last Arc<PipeReader> and frees the CapturedWriter itself, so a second completion for it would be a use-after-free. ASAN confirmed that with the per-chunk delivery but without the cancellation (happens when the subprocess's output was fully read before the writer failed):

ASAN report (per-chunk delivery without the CapturedWriter cancellation)

ERROR: AddressSanitizer: heap-use-after-free
    #0 <bun_runtime::shell::subproc::CapturedWriter>::on_iowriter_chunk src/runtime/shell/subproc.rs
    #1 bun_runtime::shell::io_writer::on_io_writer_chunk src/runtime/shell/IOWriter.rs
    #2 <bun_runtime::shell::yield_::Yield>::run src/runtime/shell/Yield.rs
    #3 <IOWriter>::run_yield
    #4 <IOWriter>::fail_pending_writers
    #5 <IOWriter>::on_error
    ...
freed by thread T0 here:
    ...
    #12 <alloc::sync::Arc<bun_runtime::shell::subproc::PipeReader>>::drop_slow
    #15 <bun_runtime::shell::subproc::PipeReader>::detach
    #16 <bun_runtime::shell::subproc::Readable>::finalize
    #17 <bun_runtime::shell::subproc::ShellSubprocess>::close_io
    #18 <Cmd>::buffered_output_close_stdout
    #19 <Cmd>::buffered_output_close
    #20 <PipeReader>::try_signal_done_to_cmd
    #21 <CapturedWriter>::on_iowriter_chunk          <- first completion of the batch
    #25 <IOWriter>::fail_pending_writers

cat's case is the same shape (Builtin::done from the first completion retires the Cmd node, so a second completion would dispatch into a freed or reused node); it is the ExecStdin/ExecFilepathArgs write-error arm, one line away from the cancellation that its read-error arm already did. The builtin is currently behind BUN_ENABLE_EXPERIMENTAL_SHELL_BUILTINS on POSIX and I did not find a way to get more than one of its chunks queued at once there, so that line is covered by review rather than by a test.

The alternative of keeping "one error per child" and having the builtins drain their own queues does not work: their output_queue FIFO does not know which writer (stdout or stderr) a given chunk went to, so a single notification cannot tell them how many of their chunks just failed. Giving every OutputTask its own ChildPtr again (what the output_queue stopgap stands in for) would fix the OutputTask builtins but not rm, which counts chunks under the builtin's pointer too, so the contract is fixed at the queue instead and the two early-exit children opt out explicitly.

Other details:

• on_sync_error keeps its contract: the enqueuing child's own chunk is withheld and returned so the trampoline that is already on the stack delivers it (shell: don't re-enter the Yield trampoline on synchronous write errors #33008); other queued chunks, if any, are dispatched inline. It now comes back as Yield::done() only if one of those inline completions made the child cancel its chunks, i.e. the child has already finished.
• The EndOfFile path previously built its EPIPE SystemError with to_system_error(); it now uses to_shell_system_error() like the on_error path (which is the one a real EPIPE takes, since the pipe writer reports it as an error). The handle_dead_writer rejections are unchanged.
• The keepalive that on_error/on_sync_error held moved into the shared function, which is where the post-callback state access happens; the EndOfFile path gets it too.
• Single-chunk children see exactly what they saw before (one chunk, one completion), and the exit codes of the affected builtins on EPIPE are unchanged from the single-chunk behavior (ls/mkdir/rm report 0, as ls -d one_entry already did; a relayed subprocess reports the errno, as before). What the exit code should be on a write error is a separate question (shell: exit 1 on builtin failure instead of raw errno; fix cd ENOENT message #32278 is moving these around), so the tests only assert that the command settles.

How did you verify your code works?

New tests in test/js/bun/shell/epipe.test.ts. A fixture waits until writes to its own stdout fail with EPIPE (the test closes the read end right after spawning), starts the command, blocks the main thread briefly so every argument's output is queued in one batch, and awaits it. ls -d with 16 entries (OutputTask path), mkdir -v (OutputTask path, and the directories must exist afterwards), rm -v (the counting path in rm, and the files must be gone), and a relayed head -c 1048576 /dev/zero subprocess (CapturedWriter; 4 to 6 of its chunks were queued when the writer failed in every local run).

Against the release build without the fix, ls, mkdir and rm time out in 6/6 runs and the subprocess test passes (it guards the cancellation described above, which ASAN catches when the ordering hits); with the fix, all pass on a debug ASAN build (5/5 runs). The rest of test/js/bun/shell (bunshell, commands/, yield, shell-write-fault, shell-pipe-read-fault, shelloutput, shell-blocking-pipe, pipeline_stack, throw, exec, shell-hang, file-io, leak) shows no new failures on the debug build; the only failures here are pre-existing in this environment (the ls permission-denied tests, which fail as root on the release build too, and leak tests that exceed their 100s budget under ASAN).

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 6 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: 0fb03a71-8110-4710-b2f2-456c03c97ec8

📥 Commits

Reviewing files that changed from the base of the PR and between 8c7ed92 and 328b9a0.

📒 Files selected for processing (4)

• src/runtime/shell/IOWriter.rs
• src/runtime/shell/builtin/cat.rs
• src/runtime/shell/subproc.rs
• test/js/bun/shell/epipe.test.ts

---

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

[robobun]

Status

Reproduced on bun 1.4.0 (release) and on a current debug build: with the reader of the process's stdout gone, await $\ls -d a b c`(alsorm -v a b c, mkdir -v a b c) never settles; one entry or echo` settles. Repro script and the mechanism are in the PR description above.

Fix is in this PR. The new tests in test/js/bun/shell/epipe.test.ts time out (hang) for ls, mkdir and rm on the unfixed release build (6/6 runs) and pass on the fixed debug ASAN build (5/5 runs). CI is green on all lanes (build #92762). Ready for review.

[claude]

I reviewed this PR and didn't find any bugs. Because it reworks the shell IOWriter error path with re-entrant callback dispatch and Arc<PipeReader> lifetime management (the PR itself notes an ASAN-caught UAF that the cancel_chunks ordering guards against), a human look would still be worthwhile.

What was reviewed:

• fail_pending_writers walk-in-place: liveness re-checked per iteration, err/broken_pipe set before first callback so re-entrant enqueue hits handle_dead_writer; keepalive moved into the shared function.
• on_sync_error withhold semantics: only the first chunk of the enqueuing child is withheld; later chunks of the same child dispatch inline — verified those callers (Builtin/Cmd/CondExpr) count completions and don't finish on the first one.
• CapturedWriter/cat cancellation: both call cancel_chunks before their first error completion can free them, and fail_pending_writers re-derives state() per iteration so the cancellation is observed.

Extended reasoning...

Overview

This PR fixes a hang in Bun Shell where builtins that queue multiple output chunks under one ChildPtr (ls, mkdir, rm, touch, cp) never finish when the process's stdout reader goes away. The old fail_pending_writers/broken_pipe_for_writers delivered one error per distinct child and dropped the rest; the fix delivers one error completion per queued chunk. Two children (cat, CapturedWriter) that finish on their first error now cancel_chunks() from that callback so the walk skips their remaining chunks. Four files: IOWriter.rs (core change), cat.rs and subproc.rs (cancellation calls), and epipe.test.ts (four new spawned-fixture tests).

Security risks

None identified. This is internal shell I/O error-path plumbing with no untrusted-input parsing, auth, or crypto involvement.

Level of scrutiny

High. This is native memory-safety-critical code in the shell subsystem — the most-blocked category in Bun reviews. The change involves:

• Re-entrant callback dispatch while iterating a queue whose entries the callbacks may mark dead (cancel_chunks).
• Arc<PipeReader> lifetime: the first error completion for a CapturedWriter can synchronously free the PipeReader (and the embedded CapturedWriter) via try_signal_done_to_cmd → close_io → detach; the PR's ASAN trace shows this was a real UAF without the cancellation.
• UnsafeCell<State> aliasing: state() must be re-derived across every re-entrant call, and the walk correctly does so per iteration.
• The on_sync_error withhold contract (avoiding trampoline re-entry) is preserved but subtly changed: only the first of the enqueuing child's chunks is withheld, later ones dispatch inline. The finder/verifier examined this and concluded it is correct because the affected callers count completions rather than finishing on the first one.

Other factors

The PR description is unusually thorough — it names the mechanism, the ASAN trace, the withhold contract, and why the queue is walked in place rather than cleared first. Test coverage is solid (four spawned fixtures covering OutputTask builtins, the rm counting path, and the CapturedWriter relay), verified to fail on the release build without the fix. The cat change is acknowledged as review-only (behind an experimental flag on POSIX). Given the re-entrancy and lifetime reasoning here is exactly the shape that has historically caught UAFs in this subsystem, a human reviewer familiar with the shell's Yield trampoline and Cmd node lifecycle should confirm the ordering invariants.

[robobun]

Heads-up: #40033 makes the builtins in your tests (ls, mkdir -v, rm -v) and the relayed subprocess exit 1 on EPIPE instead of 0 or the errno. Your tests only check that the command settles, so they are unaffected. It also adds a cancel_chunks call in the write-error arm of cat.rs, the same line this PR adds, so expect a trivial conflict there, and it does not change IOWriter. The tests in #40033 use shapes where the first chunk fails synchronously, so they do not depend on this fix.

[robobun]

This change also fixes a hang that needs no closed process stdout. A pipeline inside the shell is enough:

import { $ } from "bun";
// dir has a few hundred subdirectories
await $`ls -R ${dir} | true`; // never settles
await $`ls -R ${dir} | head -n 1`; // never settles
await $`ls -R ${dir} | sleep 1`; // never settles

ls -R queues one chunk per directory on the IOWriter of the pipe. The reader exits, the writer fails with EPIPE, and fail_pending_writers reports the error once for the ls child. ls counts one completion and waits for the others.

Measured on bun 1.4.3-canary.1+09bb54630 (Linux x64): | true hangs in 20 of 20 runs with 300 subdirectories and in 20 of 20 runs with 1000. | head -n 1 hangs in 20 of 20 runs with 1000. With 100 subdirectories | head -n 1 hangs in 3 of 10 runs. The size only changes how often several chunks are queued when the writer fails.

This branch conflicts with main now. The conflict is in src/runtime/shell/subproc.rs: main restructured CapturedWriter::on_iowriter_chunk and moved is_done to PipeReader::captured_writer_done. The commit is rebased onto main (7e56b40) here: main...robobun/d6595e50/shell-iowriter-fail-every-chunk

• IOWriter.rs and cat.rs apply as they are.
• subproc.rs keeps the structure from main, adds child_ptr(), and calls cancel_chunks in the error arm before it falls through to try_signal_done_to_cmd.
• A second commit adds one test to the IOWriter epipe block in test/js/bun/shell/epipe.test.ts: ls -R . | true in a directory with 1000 subdirectories. It times out on 1.4.3 canary and passes on a debug ASAN build of the rebased branch.

With the rebased change the three pipelines above settle in every run (13 runs with 600 subdirectories, 0 hangs). All of epipe.test.ts passes. bunshell, commands/*, yield, pipeline_stack, shell-write-fault, shell-pipe-read-fault, throw, exec, shell-hang, file-io, shelloutput and shell-blocking-pipe pass too (the two ls permission-denied tests fail as root on stock bun as well). Every enqueue call site on main was checked again: no child other than cat and CapturedWriter can finish while it still has chunks queued.

One difference from bash remains. After the EPIPE, ls -R still walks the rest of the tree before it finishes. It settles, but a large tree takes as long as a full listing.

[robobun]

Superseded by #43183. It carries this change rebased onto main (this branch conflicts in src/runtime/shell/subproc.rs) and adds tests for the pipeline case ($`ls -R dir | true` and related pipelines, on Linux and Windows).