Skip to content

shell(cat): finish after a read error instead of cancelling the queued output and hanging - #37743

Open
robobun wants to merge 8 commits into
mainfrom
farm/bb60e710/shell-cat-read-error-hang
Open

shell(cat): finish after a read error instead of cancelling the queued output and hanging#37743
robobun wants to merge 8 commits into
mainfrom
farm/bb60e710/shell-cat-read-error-hang

Conversation

@robobun

@robobun robobun commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

Problem

  • The shell's builtin cat hangs forever when its input fails to read while it still has output queued on stdout: the $ promise never settles and the bytes already read are dropped. On POSIX the builtin only runs with BUN_ENABLE_EXPERIMENTAL_SHELL_BUILTINS=1; on Windows it is the cat every shell script gets.
  • Cause: on a read error cat cancelled its queued chunks and waited, but a cancelled chunk completes without calling cat back, so nothing ever finished the command. With stdout on a pipe or tty, a chunk read in the same wake as the error is always still queued.
  • The file-argument form only finished on error once some chunk had completed, so a read error before any output (cat some_directory with stdout on an fd) hung too. The Zig version had the same logic; this is inherited, not a porting bug.
  • When the reader cannot start at all (a directory; on Linux today any regular file), the failure was delivered as a completion from inside the start call. true | cat some_dir dies with panic: expected Node::Pipeline at Node#2, got Free; cat some_dir; cat some_dir; ... nests one interpreter frame per statement. true | cat < regular_file panics the same way on main today.

Fix

  • A read error now ends the input exactly like EOF, plus an errno: the queued output drains, and whichever of "reader finished" or "last chunk written" happens second finishes the command with that errno. cat no longer cancels chunks.
  • "Drained" is a count comparison (chunks completed >= chunks queued) in both forms. It replaces a flag that stayed set after the first completion, which let the command move on with a later chunk still in flight.
  • Waiting for the drain cannot add a new hang: after EOF cat already waits for its output the same way, and if stdout goes away the existing write-error path finishes the command. The exit code stays the errno; making it 1 is shell: exit 1 on builtin failure instead of raw errno; fix cd ENOENT message #32278.
  • A reader that fails to start is now reported back to cat, which finishes the input from its own frame instead of from inside the start call, the same shape the write side already uses for synchronous write errors.
  • Verification: tests that fail without the change (every error case times out without the first commit; with only the first, the pipeline cases hit the panic above and the sequences trip a re-entrancy assertion). The read errors are real: a pty whose other end closed, directories as arguments, and an LD_PRELOAD shim that fails a poll re-arm with a chunk queued. Run on Linux only; for Windows the crate was compiled, not run.

Background

  • Builtin cat: bun's shell ($) can run cat in-process instead of spawning the system binary. It has two states, one reading stdin and one reading each file argument in turn; a non-zero errno in the file state ends the command instead of moving to the next file.
  • IOReader: the shell's async input reader. It hands cat data, then either EOF or an error. Some failures (epoll refusing a directory or, on Linux, a regular file) surface synchronously from inside its start call rather than from a later poll wake.
  • IOWriter: the shell's shared writer for a builtin's stdout. cat queues each chunk it read and is called back once per chunk when written, so it tracks output by counting queued against completed chunks. A cancelled chunk completes without that callback.
  • Yield trampoline: a shell step does not complete a command by calling into it; it returns a Yield and a driver loop runs the next step. Completing a command from inside another step, as the start-failure path did, frees a node the driver is still holding, which is the panic above.
Original description

What

The shell's builtin cat never finishes when its input fails to read while it still has output queued. The command, and the $ promise awaiting it, hang forever and the data that was already read is dropped.

On POSIX the builtin is only used with BUN_ENABLE_EXPERIMENTAL_SHELL_BUILTINS=1 (Kind::DISABLED_ON_POSIX); on Windows it is the cat every shell script gets.

Repro (Linux; the child's stdin is a pty master whose slave wrote some bytes and closed, so read() returns the bytes and then EIO):

// parent: openpty(); write(slave, "read before the error"); close(slave);
//         Bun.spawn([bun, "-e", script], { stdin: master, env: { BUN_ENABLE_EXPERIMENTAL_SHELL_BUILTINS: "1" } })
// script:
const r = await $`cat`.nothrow();
console.log(r.exitCode, r.stdout.toString());

Before: nothing is printed and the process stays alive until it is killed. With .quiet() (no IOWriter involved) the same script prints 5 read before the error, which is also what the non-quiet form prints after this change.

Cause

Cat::on_io_reader_done (src/runtime/shell/builtin/cat.rs), in both the stdin and the file-argument state: when the reader reports an error and chunks_done < chunks_queued, it called cancel_chunks on stdout and returned Suspend. IOWriter::bump completes a cancelled chunk with Yield::done() and never calls the child's on_io_writer_chunk, so chunks_done never advanced and Builtin::done was never reached. The only read-error path that finished was the one where nothing happened to be queued at that instant; with stdout on a pipe or tty, a chunk read in the same poll wake as the error is always still queued. The file-argument state was worse: its error path checked out_done, which only a completed chunk ever sets, so a read error before the first chunk (for example cat some_directory with stdout on an fd) hung as well. The same logic was in the Zig version, so this is inherited rather than a porting bug.

Fix

A read error ends the input exactly like EOF, plus an errno. on_io_reader_done records the errno and marks the input done, on_io_writer_chunk counts the chunk, and both then run the same CatState::input_step: once the reader is done and the queue has drained, whichever callback got there last finishes the command with the recorded errno (ExecStdin already had an errno field that nothing read; ExecFilepathArgs gets one, and a non-zero value ends the command instead of moving on to the next file, as before). cancel_chunks is no longer used here.

Both states now use chunks_done >= chunks_queued for "drained". The out_done flag is removed: besides the zero-chunk hang above, once set it stayed set, so a chunk queued after it could still be in flight when the reader finished and cat moved on to the next file or completed. The exit code stays the errno, which is what this builtin (and shell-pipe-read-fault.test.ts) already use for read failures; whether that should become 1 is a separate question (#32278).

Why this is the right shape rather than "cancel and finish immediately": the bytes were read successfully, and cat writes what it read before reporting the failure; the drain path also already existed for EOF, so the error path now shares it instead of having its own. Waiting for the drain cannot introduce a new way to get stuck: if stdout stalls, cat waits exactly as it does after EOF today (and as a blocked write(2) would), and if stdout goes away, the IOWriter fails the chunk and the existing write-error arm finishes the command. The old path never finished at all in this situation.

Start-time failures (second commit, found while reviewing the first). When the reader cannot be started at all, bun_io reports the error synchronously from inside IOReader::start() (epoll refuses directories, and on Linux today any regular file), and IOReader dispatched cat's completion right there, nested inside the trampoline frame that was starting the command. Once a read error finishes cat even with nothing queued, that nested completion tears down the pipeline node the outer trampoline is still holding: true | cat some_dir died with panic: expected Node::Pipeline at Node#2, got Free, and cat some_dir; cat some_dir; cat some_dir nested one trampoline per statement (the debug re-entrancy assertion fires at three). The stdin state already had this on main: true | cat < regular_file panics the same way with the released binary. IOReader::start() now returns a start failure instead of dispatching it (an error bun_io reports through on_reader_error while start() is on the stack is parked and returned too), and Cat::start_reader finishes the input in tail position, so the completion flows back through cat's own frame. This is the read-side version of what IOWriter::on_sync_error does for synchronous write failures; IOReader::start() has no other callers.

Related open PRs: #37719 changes the write-error side of the same builtin (IOWriter failing every queued chunk) and leaves this read-side path as is; #35337 makes the builtin read regular files synchronously and, in passing, aligns the file-argument zero-chunk condition, but keeps the cancel-and-suspend branch for queued chunks. Neither fixes this hang.

Tests

test/js/bun/shell/commands/cat.test.ts (new): EOF through a pipe; the pty read error with nothing queued (.quiet(), exit 5 plus the data, unchanged behavior), with the data still queued (data written out, then exit 5), and with an 8 KiB payload, which the master hands back as three reads in the wake that also delivers the error, so three chunks are queued and each completion has to be counted; cat <directory> with stdout on an fd (file-argument state, zero chunks) on its own, inside pipelines (cat sub | cat sub, and true | cat sub with captured output), and four times in a row followed by echo.

test/js/bun/shell/yield.test.ts: the read-side counterpart of the existing "synchronous write errors in sequential statements" case (cat / || echo f1; ...).

test/js/bun/shell/shell-pipe-read-fault.test.ts: the existing LD_PRELOAD shim gains SHELL_FAIL_EPOLL_REARM_INO (and ..._REARM_FROM), which fails the poll re-arm of the one FIFO the fixture hands to cat, so the read error lands right after a chunk was queued. cat fifo (file-argument state) and cat < fifo (redirect reader) must print the chunk and exit with ENOMEM. A third case fails the re-arm one wake later: the test feeds the second chunk only after the first has shown up on the fixture's stdout, so when the error lands one chunk has completed and another is queued. A sticky "drained" flag (I checked by temporarily putting one back) ends the command there and reports only the first chunk; the counter waits for the second.

Without the first commit, every error case above times out (the child never exits). With only the first commit, the pipeline cases crash on the panic quoted above and the sequence cases trip the re-entrancy assertion. With both, the three files pass (bun bd test, Linux; also looped without flakes), cargo clippy -p bun_runtime is clean and the crate checks for x86_64-pc-windows-msvc. Running bunshell.test.ts with BUN_ENABLE_EXPERIMENTAL_SHELL_BUILTINS=1 gives the same 5 failures before and after (all regular-file reads through the builtin, i.e. #35337), with redirect Bun.File turning from a 5 s timeout into an immediate failure.

…d output and suspending

When the reader failed while cat still had stdout chunks queued,
on_io_reader_done cancelled those chunks and suspended. A cancelled chunk
completes without calling back into cat, so chunks_done never caught up,
Builtin::done was never called and the command (and the awaiting `$`
promise) hung forever. The file-argument state additionally gated its
error path on out_done, which only a completed chunk ever set, so a read
error before the first chunk hung as well.

A read error now ends the input the same way EOF does: the errno is
recorded, the queued chunks drain normally, and whichever callback sees
both the reader done and the queue drained finishes the command with that
errno. Both states use chunks_done >= chunks_queued for "drained"; the
out_done flag is gone (it also stayed true once set, so a later chunk
could still be queued when it let cat move on).
@coderabbitai

coderabbitai Bot commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

Changes

The cat builtin now preserves read errors, drains queued stdout before completion, and stops after file read failures. Tests cover pty errors, unreadable files, and injected FIFO ENOMEM failures.

cat read error handling

Layer / File(s) Summary
Preserve errors and drain output
src/runtime/shell/builtin/cat.rs
The builtin stores reader errno values, drains queued stdout chunks, clears reader state on errors, and stops processing files after a read failure.
Validate pty and file read failures
test/js/bun/shell/commands/cat.test.ts
Tests cover successful stdin copying, pty read errors with and without queued output, errno reporting, and unreadable file arguments.
Inject FIFO re-arm failures
test/js/bun/shell/shell-pipe-read-fault.test.ts
The fault shim targets a FIFO by inode and injects ENOMEM during epoll re-arm operations. Tests verify output drainage for file and stdin modes.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly and concisely describes the primary fix: preventing queued output cancellation and hangs after a cat read error.
Description check ✅ Passed The description explains the problem, fix, implementation details, regression tests, and verification results, covering the template requirements.

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

@robobun

robobun commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 2:05 PM PT - Aug 12th, 2026

@robobun, your commit bdd49fc has 3 failures in Build #93377 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 37743

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

bun-37743 --bun

@robobun

robobun commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator Author

Status

Reproduced on the released bun 1.4.0 (Linux, BUN_ENABLE_EXPERIMENTAL_SHELL_BUILTINS=1): builtin cat reading a pty master whose slave wrote data and closed never exits (.quiet() control: exit 5 plus the data). Same hang for cat <directory> with stdout on an fd, and for a FIFO whose reader fails to re-arm after the first chunk (LD_PRELOAD shim in shell-pipe-read-fault.test.ts).

Two fixes: cat.rs lets the queued output drain and exits with the errno (first commit), and IOReader::start() returns a start failure to cat instead of completing it from inside the call, which with the first fix alone made true | cat some_dir panic (expected Node::Pipeline at Node#2, got Free; true | cat < regular_file already did on main). Tests for both in commands/cat.test.ts, yield.test.ts and shell-pipe-read-fault.test.ts; all time out or panic without the matching fix and pass with it. Waiting on CI for 44cbc0b.

Comment thread src/runtime/shell/builtin/cat.rs Outdated
Comment thread src/runtime/shell/builtin/cat.rs Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/js/bun/shell/shell-pipe-read-fault.test.ts`:
- Around line 454-459: Update the environment setup in the FIFO fault test to
obtain the inode through BigInt-based stats and preserve its full value when
assigning SHELL_FAIL_EPOLL_REARM_INO, so the shim’s comparison with st.st_ino
remains exact.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 0b9b02a7-ab27-4520-b20e-12320017d699

📥 Commits

Reviewing files that changed from the base of the PR and between f59f705 and 7ede4cf.

📒 Files selected for processing (3)
  • src/runtime/shell/builtin/cat.rs
  • test/js/bun/shell/commands/cat.test.ts
  • test/js/bun/shell/shell-pipe-read-fault.test.ts

Comment thread test/js/bun/shell/shell-pipe-read-fault.test.ts
…riter callbacks

Both callbacks now update their counter and run the same CatState::input_step,
so the completion rule lives in one place instead of in comments. The FIFO
fault test passes the inode as a bigint so large inode numbers survive.
Comment thread src/runtime/shell/builtin/cat.rs Outdated
Comment thread src/runtime/shell/builtin/cat.rs Outdated
Comment thread src/runtime/shell/builtin/cat.rs Outdated
Comment thread test/js/bun/shell/commands/cat.test.ts Outdated

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this and found no issues — the state-machine refactor traces cleanly and every prior review thread (comment-cop, CodeRabbit's BigInt inode, the libutil openpty note) is resolved. Deferring to a human because it reworks the reader/writer callback ordering in the shell's IOReader/IOWriter contract and changes visible behavior on Windows (where the builtin is default), which is worth a maintainer's eye.

What was reviewed:

  • Traced input_step against both callback entry points; the removed !stdout_needs_io short-circuit was redundant (chunks_queued only increments when stdout needs IO), and the unreachable! arm is guarded by both callers.
  • Checked that Step::Next still lands on a reset state for the next file (next() zeroes chunks_done/chunks_queued/in_done/errno), and that the stdin-no-IO enqueue path in next() still completes (0 queued, 1 done → Done(0)).
  • Confirmed cancel_chunks still has other callers, so removing it here doesn't leave dead code.
  • Tests follow harness conventions (concurrent drain, tempDir, subprocess isolation, per-platform skips).
Extended reasoning...

Overview

The PR fixes a hang in the shell's builtin cat when the input reader fails while output chunks are still queued on stdout. The fix in src/runtime/shell/builtin/cat.rs (~60 net lines) refactors on_io_reader_done and on_io_writer_chunk to share a single completion check (CatState::input_step), records the reader's errno instead of calling cancel_chunks, and lets queued chunks drain before finishing with that errno. It also removes the out_done flag, whose two bugs (never set on zero chunks; sticky once set) the description spells out. Two test files add six regression cases: pty-master EIO with and without queued data, cat <directory> with stdout on an fd, and LD_PRELOAD-injected epoll re-arm failures on a FIFO for both the file-argument and stdin-redirect states.

Security risks

None. Pure Rust state-machine logic with no unsafe, no FFI, no JS heap interaction, no untrusted input parsing beyond what cat already did. The LD_PRELOAD shim in the test is test-only C compiled at test time.

Level of scrutiny

Moderate. The builtin is gated behind BUN_ENABLE_EXPERIMENTAL_SHELL_BUILTINS on POSIX but is the default cat on Windows, so the drain-then-exit behavior change is user-visible there (strictly better than the previous hang, but still a behavior change). The code is a state machine driven by two async callbacks (on_io_reader_done, on_io_writer_chunk) whose ordering the fix relies on; I traced every arm and it holds, but this is exactly the class of change where a maintainer who knows the IOWriter/IOReader contract should confirm — e.g., that letting the writer drain (rather than cancelling) can't itself stall on a slow/blocked stdout in a way the old cancel path avoided.

Other factors

All prior review threads are resolved: the comment-cop paragraph-comment complaints were cut to one-liners (e7725c9), CodeRabbit's BigInt inode suggestion was applied (4729f24), and my earlier libutil.so.1 note for openpty on glibc was applied (1ed85c0). The bug hunting system found nothing this run. Test coverage is strong — the description reports the four hang-case tests time out on the unfixed build and pass with it, and bunshell.test.ts under the experimental flag has the same failure set before and after. CI build #92865 was still building at the last timeline update. The PR description also notes overlap with two open PRs (#37719, #35337) that touch adjacent paths; a maintainer should confirm the interaction is as described.

@robobun

robobun commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator Author

Removing out_done also fixes a crash that showed up in CI, so noting it here rather than opening a second PR on the same lines.

test/js/bun/shell/bunshell.test.ts ("redirect Bun.File", i.e. the builtin cat <123 KB file> > <path>) crashed on the Windows x64 lane of build 93147 (passed on retry) with:

panic: expected Node::Cmd at Node#2, got Free

Node#2 is the Cmd. On Windows both the file reads and the redirect writes are async uv requests, and with a 64 KB read buffer the file is two data reads plus an EOF read. When the first chunk's write completes before the second read is delivered, on_io_writer_chunk sets out_done and nothing ever clears it; if the EOF read then completes before the second chunk's write, on_io_reader_done takes the *out_done branch and finishes the command while that write is still in flight. The in-flight uv_fs_write holds a ref on the IOWriter, so when it completes bump dispatches OnIoWriterChunk to the Cmd node the script already freed, which is the panic above. input_step in this PR (chunks_done >= chunks_queued) keeps the command suspended until the second chunk completes, so that ordering is fine after this change.

The same path is reachable deterministically on Linux with the experimental builtins. There the IOWriter has nothing keeping it alive, so instead of panicking it is dropped with the chunks still queued and the output is truncated. With in and out as FIFOs: write one byte, wait for it to come out the other side (first chunk done, out_done set), feed 256 KB without reading out, close in:

  • released 1.4.0 and a debug build of main: $ resolves with exit 0 right after the EOF, out receives only the first 8193 (sometimes 16385) of 262145 bytes
  • main plus this PR's cat.rs: cat stays suspended until out is drained, 262145 bytes, exit 0

None of the tests here cover the EOF-with-chunks-still-queued case in the file-argument state (the FIFO ones take the error path), so it may be worth adding the scenario to cat.test.ts. This version fails on main and passes with this branch's cat.rs (5/5 runs under bun bd test):

test
import { closeSync, openSync, promises as fsp, writeSync } from "node:fs";
import { mkfifo } from "mkfifo";

// File-argument state, EOF while chunks are still queued on stdout. Both ends
// are FIFOs: the first chunk is written out and completes, then a payload much
// larger than the pipe buffer is fed in and `in` is closed while nothing is
// reading `out`. cat used to finish on that EOF because `out_done` was still
// set from the first chunk, dropping the queued chunks with the Cmd (on
// Windows, where the write is still in flight at that point, its completion
// then dispatched to the freed Cmd node: "expected Node::Cmd at Node#2, got Free").
test.skipIf(!isLinux)("cat FILE > TARGET waits for every queued chunk before finishing on EOF", async () => {
  using dir = tempDir("shell-cat-eof-queued", {});
  const inPath = join(String(dir), "in");
  const outPath = join(String(dir), "out");
  mkfifo(inPath, 0o666);
  mkfifo(outPath, 0o666);

  // O_RDWR: cat's open(O_RDONLY) returns immediately; closing this fd is the EOF.
  const inFd = openSync(inPath, "r+");
  await using proc = Bun.spawn({
    cmd: [
      bunExe(),
      "-e",
      /* js */ `
        import { $ } from "bun";
        const r = await $\`cat in > out\`.nothrow();
        console.log("resolved " + r.exitCode);
      `,
    ],
    env: builtinEnv,
    cwd: String(dir),
    stdout: "pipe",
    stderr: "pipe",
  });

  // Blocks until the shell opens `out` for writing.
  const out = await fsp.open(outPath, "r");
  try {
    const buf = Buffer.alloc(64 * 1024);
    writeSync(inFd, "a");
    // "a" arriving here means cat's first chunk completed on the IOWriter.
    let received = (await out.read(buf, 0, buf.length)).bytesRead;
    expect(received).toBe(1);

    const payload = Buffer.alloc(256 * 1024, "b");
    for (let off = 0; off < payload.length; ) off += writeSync(inFd, payload, off);
    closeSync(inFd);

    for (;;) {
      const { bytesRead } = await out.read(buf, 0, buf.length);
      if (bytesRead === 0) break;
      received += bytesRead;
    }
    expect(received).toBe(1 + payload.length);
  } finally {
    await out.close();
  }

  const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
  expect({ stdout, stderr }).toEqual({ stdout: "resolved 0\n", stderr: "" });
  expect(exitCode).toBe(0);
});

…g it from inside start()

When the reader cannot be started (epoll refuses the fd: a directory, or on
Linux any regular file), bun_io reports the error synchronously from inside
IOReader::start(), which dispatched cat's completion right there, nested in
the trampoline frame that was starting the command. With a read error now
finishing cat even when nothing is queued, that nested completion tore down
the pipeline node the outer trampoline still held (`true | cat dir` panicked
with "expected Node::Pipeline, got Free"; `true | cat < file` already did on
the stdin side) and nested one trampoline per statement in a sequence.

IOReader::start() now parks an error reported while it is on the stack and
returns it; cat finishes the input in tail position, so the completion flows
back through its own frame, the same shape as IOWriter::on_sync_error.

Tests: pipeline and sequence forms of the unreadable-argument case, the
read-side counterpart of the sequential-statements yield test, an 8 KiB pty
payload (three chunks queued when the error lands), and a FIFO case where the
fault hits one wake after an earlier chunk already completed, which a sticky
"drained" flag would end early.
Comment thread src/runtime/shell/IOReader.rs Outdated
Comment thread src/runtime/shell/builtin/cat.rs Outdated
Comment thread src/runtime/shell/builtin/cat.rs Outdated
Comment thread src/runtime/shell/IOReader.rs Outdated

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this PR and didn't find any bugs (the earlier openpty/libutil note has been addressed). Because it reworks completion ordering in the shell's async I/O paths — IOReader::start() now returns instead of dispatching, and cat's drain/finish state machine is restructured — a human look at the state-machine invariants and the Windows path would still be worthwhile.

What was reviewed:

  • CatState::input_step unifies the reader-done and chunk-done paths; chunks_done >= chunks_queued replaces the sticky out_done flag in both states.
  • IOReader::start()'s starting/start_err parking mirrors IOWriter::on_sync_error; ReaderTag::Cat is the only reader tag, so no other callers.
  • cancel_chunks is no longer called from cat.rs but remains live in IOWriter and subproc.rs.
  • Tests: pty EIO cases, LD_PRELOAD FIFO re-arm fault (both cat states, plus the second-wake case for the removed sticky flag), and the yield.test.ts read-side re-entrancy case.
Extended reasoning...

Overview

The PR fixes a hang in the shell's builtin cat when a read error arrives while output chunks are still queued on stdout. Two source changes: src/runtime/shell/builtin/cat.rs restructures the state machine so a read error records the errno, marks input done, and lets queued chunks drain before finishing (instead of cancel_chunks + suspend, which never called back); and src/runtime/shell/IOReader.rs changes start() to return a start-time failure rather than dispatching it, so cat completes in tail position through its own trampoline frame instead of tearing down a pipeline node the outer trampoline still holds. Three test files add pty-based EIO cases, LD_PRELOAD-injected epoll re-arm failures on a FIFO, and a read-side re-entrancy case in yield.test.ts.

Security risks

None identified. No user-facing input parsing, no auth/crypto, no network. The change is internal to the shell interpreter's I/O completion ordering.

Level of scrutiny

High. This is async I/O completion logic in the shell interpreter with Arc lifetimes, trampoline re-entrancy, and node-arena free ordering — the PR description itself notes that the first commit alone made true | cat some_dir panic with "expected Node::Pipeline at Node#2, got Free", and that removing out_done incidentally fixes a Windows CI crash from a chunk completion dispatched to a freed Cmd node. These are exactly the memory-safety-adjacent invariants REVIEW.md calls out as most-blocked. The starting/start_err mechanism in IOReader is new state that interacts with on_reader_error, and the Windows start_impl path is untested by the Linux-only test suite here.

Other factors

The PR description is unusually thorough, the test coverage is good (both cat states, zero/one/several queued chunks, pipeline and sequence forms, the sticky-flag regression), and all prior review feedback (comment-cop, coderabbit BigInt inode, my libutil note) has been addressed. cancel_chunks stays live elsewhere so no dead code. Still, the change is not simple or mechanical — it restructures a state machine with multiple interacting completion callbacks — so it does not meet the bar for auto-approval.

@robobun

robobun commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator Author

The out_done crash described above hit CI again, so adding the data point here rather than opening a second PR for the same lines.

Build 96000, :windows: 11 aarch64 - test-bun, test/js/bun/shell/bunshell.test.ts (passed on retry). The job log places it in "redirect Bun.File" again (the three dots after the latin-1 output are the raw café case and the two buffer redirects; the banner follows), and the trace symbolizes against that build's bun-profile.pdb to the same path:

panic: expected Node::Cmd at Node#2, got Free
symbolized frames
Interpreter::as_cmd                                        src/runtime/shell/interpreter.rs:189
  (inlined into) shell::io_writer::on_io_writer_chunk       src/runtime/shell/IOWriter.rs:1231
  (inlined into) Yield::run                                 src/runtime/shell/Yield.rs:133
IOWriter::run_yield                                        src/runtime/shell/IOWriter.rs:941
IOWriter::on_write_pollable                                src/runtime/shell/IOWriter.rs:757
WindowsBufferedWriter<IOWriter>::on_write_complete         src/io/PipeWriter.rs:1569
WindowsBufferedWriter<IOWriter>::on_fs_write_complete      src/io/PipeWriter.rs:1640
uv__work_done / uv__process_reqs / uv_run
VirtualMachine::on_after_event_loop -> TestCommand::run

A uv_fs_write completion (the > file redirect writer) dispatching a chunk callback to the Cmd slot after the script freed it. The accessor panic for the Cmd and Builtin tags is one merged block inside Yield::run, so the line attribution cannot tell the two tags apart; the writer here is cat's stdout.

It also reproduces deterministically on Windows with the released binary (1.4.0-canary.1+7cf62962b, x64), no CI timing needed: feed cat one full 64 KiB read from a named pipe, pause so that chunk's write completes and sets out_done, then send 256 KiB more and close the input while nothing reads the output pipe. $ resolves with exit 0 while those writes are still pending; draining the output afterwards completes the next write and the process dies with the panic above. With a 64 KiB second payload the writes still fit in the pipe buffers and nothing happens, so the size matters. Expected with this PR's cat.rs: $ does not resolve until the output has been drained, and 320 KiB arrive.

repro (Windows)
import { $ } from "bun";
import net from "node:net";

const id = `${process.pid}-${Date.now()}`;
const inPath = `\\\\.\\pipe\\cat-race-in-${id}`;
const outPath = `\\\\.\\pipe\\cat-race-out-${id}`;

let received = 0;
let outSocket: net.Socket;
const { promise: outConnected, resolve: onOutConnected } = Promise.withResolvers<void>();
const { promise: outClosed, resolve: onOutClosed } = Promise.withResolvers<void>();
const outServer = net.createServer(socket => {
  outSocket = socket;
  socket.pause();
  socket.on("data", d => (received += d.length));
  socket.on("close", onOutClosed);
  onOutConnected();
});
await new Promise<void>(r => outServer.listen(outPath, r));

const inServer = net.createServer(async socket => {
  await outConnected;
  socket.write(Buffer.alloc(64 * 1024, 0x61)); // exactly one read; its write completes -> out_done
  await Bun.sleep(200);
  socket.end(Buffer.alloc(256 * 1024, 0x62)); // more chunks (their writes pend) + EOF
  await Bun.sleep(200);
  console.log("draining output, received so far =", received);
  outSocket.resume();
});
await new Promise<void>(r => inServer.listen(inPath, r));

const result = await $`cat ${inPath} > ${outPath}`.quiet().nothrow();
console.log("cat finished: exit =", result.exitCode, "received =", received);
await outClosed;
console.log("output complete: received =", received, "expected =", 320 * 1024);
inServer.close();
outServer.close();

Output on the released binary:

cat finished: exit = 0 received = 0
draining output, received so far = 0
panic: expected Node::Cmd at Node#2, got Free

alii added a commit that referenced this pull request Aug 15, 2026
### Problem
- `Cat::on_io_writer_chunk` and `Cat::on_io_reader_done`
(src/runtime/shell/builtin/cat.rs:303 and :397 before this change) both
end with the same three-arm `match` turning a `Step` into a `Yield`. A
change to one copy would miss the other.
- This is the `same_match_twice:src/runtime/shell/builtin/cat.rs` entry
in `mordant-baseline.toml`.

### Fix
- Add `Step::run(self, interp, cmd) -> Yield` holding the one mapping;
both callbacks now end with `step.run(interp, cmd)`.
- No behavior change: the arms are the ones that were there (`Suspend`
-> `Yield::suspended()`, `Done(code)` -> `Builtin::done`, `Next` ->
`Cat::next`), only the location moved. `Step` is private to cat.rs, so
nothing else is affected.
- Remove the now-fixed entry from `mordant-baseline.toml`.
- Add a test to test/js/bun/shell/bunshell.test.ts ("builtin cat
finishes from its reader and writer completions") that runs the builtin
with captured output, with stdout on a file, and on a missing file with
stderr captured and on a file. Instrumenting `Step::run` locally showed
these go through `on_io_reader_done` -> `Done(0)`, `on_io_writer_chunk`
-> `Suspend` then `on_io_reader_done` -> `Done(0)`, and
`on_io_writer_chunk` -> `Done(1)`. It is a characterization test for the
refactor: it passes before and after this change (`USE_SYSTEM_BUN=1` and
`bun bd`), it does not fail without it.
- The missing-file assertions accept the message with or without a
directory prefix: on Windows the file branch of `shell_openat` leaves
the resolved absolute path in the error, so the builtin prints `cat:
C:\...\missing.txt: ...` where POSIX prints `cat: missing.txt: ...`
(seen on both Windows lanes of the first run of this test). That
inconsistency is pre-existing and filed separately rather than fixed
here.
- Verified:
- `bun bd test test/js/bun/shell/bunshell.test.ts`: 424 pass, 0 fail.
Note that on POSIX the builtin is only used when
`BUN_ENABLE_EXPERIMENTAL_SHELL_BUILTINS` is set
(`Kind::DISABLED_ON_POSIX`), so on Linux only the new test and the
flagged case in yield.test.ts run cat.rs; the rest of the shell suite
exercises it on the Windows lanes, where the builtin is always on.
- `bun bd test` on file-io, yield, shell-write-fault,
shell-pipe-read-fault, exec, assignments-in-pipeline, bunshell-instance:
all pass.
- `bun run rust:mordant` with this diff: no findings,
`target/mordant/over-baseline.txt` not written. As a control, the same
command with cat.rs reverted and the baseline entry still removed
reports the cat.rs:397 finding as 1 over baseline in bun_runtime.
- The `Next` arm is not reachable from the new test on POSIX: reading a
regular file through the builtin currently fails there (epoll rejects
regular files), which is what #35337 fixes. #35337 and #37743 also edit
the tail of these two callbacks, so whichever of those and this lands
later needs a one-line rebase in cat.rs.

### Background
- The cat builtin is a small state machine driven by IOReader/IOWriter
callbacks. Each callback mutates `CatState` under a `&mut` borrow and
produces a `Step` (suspend, finish with an exit code, or move to the
next file), then acts on it after the borrow is released; `Step::run` is
that second half.
- mordant is the advisory Rust lint pack run by `bun run rust:mordant`;
`mordant-baseline.toml` holds per-(lint, file) counts of pre-existing
findings, and a fixed finding's entry is deleted so the ratchet
tightens.

<details>
<summary>Local leak.test.ts note</summary>

`bun bd test test/js/bun/shell/leak.test.ts` in this (debug + ASAN)
environment times out the 500-iteration `memleak_*` cases at their 100s
limit, including ones that never run cat (`memleak_change_cwd`,
`memleak_redirect_file`, `memleak_ls`, `memleak_Blob_*`); the
100-iteration cases and the 1000-iteration `fdleak_*` cases pass. The
timeouts are machine speed, not this change (and on Linux those tests
run the system cat anyway, see above).
</details>

<!-- robobun:evidence:begin -->

---

**no test proof** · iteration 2 · Platform-specific test(s) that do not
run on this machine. Deferring to CI, which covers all platforms:
test/js/bun/shell/bunshell.test.ts

<!-- robobun:evidence:end -->

---------

Co-authored-by: Alistair Smith <hi@alistair.sh>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant