shell: exit 1 and report a failed output write in every builtin

RO
1mo ago
+208-56

Problem

  • A builtin whose stdout write fails reports an errno derived status and prints nothing. With bun x.mjs | head -c1 (EPIPE), echo hello exits 65504 (ShellError: Failed with exit code 65504) and ls, mkdir -v, rm -v exit 0. bash and coreutils exit 1 and print name: write error: Broken pipe.
  • echo.rs:116 and which.rs:182 cast the negated SystemError.errno to the u16 exit code. ls, mkdir, cp and rm drop the error. Cmd.rs uses the errno as the process status.

Fix

  • Latch the failed write once, in the Builtin::on_io_writer_chunk dispatcher. Builtin::done consumes it: exit 1, after name: write error: strerror on stderr. EPIPE stays silent, as SIGPIPE is.
  • The report goes out under a new WriterTag::BuiltinReport. Its completion finishes the Cmd with exit 1, also when stderr rejects it.
  • A failed output relay of an external command and a failed [[ ]] message exit 1 too.
  • Verified: test/js/bun/shell/epipe.test.ts (8 new tests, stock bun fails all 8), shell-pipe-read-fault, yield, bunshell.

Background

  • A builtin writes through IOWriter, the shell's per-fd write queue. The result arrives later in on_io_writer_chunk as an optional SystemError.
  • A WriterTag tells IOWriter which handler a completion belongs to. The report has its own tag, so its completion does not re-enter a finished builtin.

Downsides

  • ls, mkdir -v, rm -v and cp -v now fail when stdout is a closed pipe. await $\ls`in a script piped toheadthrows unless it uses.nothrow()`.
  • Code that matched 28, 32 or 65504 gets 1.
  • Cost: one branch per completed write, one Option::take per builtin. size_of::<Builtin>() stays 328 bytes. Binary size not measured (needs two release builds).
Notes

Values from the fixture, with the process stdout closed (EPIPE) and set to /dev/full (ENOSPC). "Report" is name: write error: No space left on device on stderr, printed for ENOSPC only.

commandstock EPIPEstock ENOSPCnow
echo, which65504655081, report
pwd, seq, basename, dirname, export, yes111, report
ls, mkdir -v, rm -v, cp -v001, report
cat (builtin, stdin mode)32281, report
external command, output relayed by the shell32281, no report
true000

Every builtin ends through Builtin::done, also the ones that run one thread pool task per operand (ls, mkdir, cp, rm) and keep writing after a failure, so the report happens once, after the last task. No builtin needs its own state for it: echo, which, cat and the rm parse path stop using the errno as the status, the rest are unchanged.

History of the shape: the first version gave each builtin a WaitingWriteErr state and a closure, and ls, mkdir, cp and rm a write_err field each. A review pass pointed at the two single funnels (the dispatcher and Builtin::done). The current version latches there. #43196 (the successor of #34698) handles the no-IO half of the same invariant, a > ${buf} redirect that overflows.

Rebased onto main (29d9638) on 09-25. Two things changed with it:

  • #42288 pins that a failed write of the command not found message rejects the shell promise (yield.test.ts, "a failed write of the command-not-found message"). An earlier revision of this PR made that case exit 1; that hunk is dropped, main's behaviour stays.
  • #41436 added two tests to shell-pipe-read-fault.test.ts that expect the errno as the status of a command whose reader failed to register. They now expect 1, like the older tests in that file.

Cmd.rs: the relay status is applied when the pipe closes. A non-zero status the process already exited with is kept. If the relay fails before the exit status is known, the command gets 1 and on_exit keeps it, as it kept the errno before.

Related open PRs:

  • #32278 maps the errno to 1 at six of these sites and also changes the mv rename status, the cat read error status and the cd message. This PR covers the write failures only; those three remain for #32278.
  • #43183 (the successor of #37719) fixes a hang in ls, mkdir -v and rm -v when several chunks are queued at the moment the writer dies (IOWriter reports a failure once per child). This PR does not change IOWriter's failure path, and the tests here use shapes where the first chunk fails synchronously, so they do not depend on it. Checked together: a local merge of this branch with #43183 passes the tests of both (23 pass), and ls over six large directories with a stdout reader that exits without reading settles with exit 1, where it hangs today.
  • #43196 (no-IO write failures): see above.
  • #40035 stops reading a subprocess's output once the relay fails ($\/usr/bin/yes`` with a dead stdout never settles today).

Out of scope: cat with a file operand does not work on POSIX independent of this change (#35337), so the test feeds it through a pipe. After a failed stdout write the captured .stdout of a builtin lacks the bytes IOWriter had not written yet, while an external command's capture is complete; unchanged here.

Suites run on the debug build after the rebase: epipe, shell-pipe-read-fault, yield, bunshell, commands/rm, commands/ls (the two permission-denied tests fail as root on stock bun too), shell-leak-args. shell-hang (700 ms per fixture) and shell-load (30000 spawns in 90 s) exceed their time limits on the debug build in this container: a hang fixture takes about 0.5 s alone, and the load fixture completes 7 of 30 batches in 40 s. cargo clippy -p bun_runtime is clean; cargo check passes for linux and windows.

no test proof · iteration 6 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/shell/shell-pipe-read-fault.test.ts, test/js/bun/shell/epipe.test.ts