shell: fold write_no_io ENOSPC into builtin exit codes - #34699
Conversation
When a shell builtin redirects stdout to an ArrayBuffer (`> ${buf}`)
that is too small to hold the output, `write_no_io` returns
Err(ENOSPC) but every builtin except `yes` discarded the error and
exited 0, hiding the data loss.
`write_no_io_to` now returns ENOSPC whenever the write is truncated
(not only when the buffer was already full on entry), and a new
`write_no_io_exit` helper folds that into the exit code. Applied to
echo, basename, dirname, export, seq, pwd, cat (stdin-sync path), and
which (sync-loop path).
Also in this change:
- usockets: zero-initialize the monotonic timespec read by
`us_internal_monotonic_ns` so a failed clock_gettime cannot feed
stack garbage into the sweep-timer deadline.
- SigintWatcher: null-guard `m_thread` in `uninstall()`; the field
is a RefPtr and `install()` sets `m_installed` before assigning
it.
|
Updated 5:20 AM PT - Jul 19th, 2026
@autofix-ci[bot], your commit 9f083a1 is building: |
|
Gate check (local): Full |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 2 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe change updates shell ArrayBuffer writes to support partial output and propagate ChangesShell output status propagation
SIGINT watcher lifecycle
Monotonic timestamp initialization
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/runtime/shell/builtin/cat.rs`:
- Around line 150-151: Update Cat::on_io_reader_chunk to capture errors from
write_no_io, including ENOSPC, instead of ignoring them; record the failure,
stop or cancel further reads, and ensure the reader-completion path returns a
nonzero exit status while preserving successful streaming behavior.
In `@test/js/bun/shell/bunshell.test.ts`:
- Around line 479-505: Expand the cases matrix and related assertions around the
concurrent shell tests to include cat with both file and file-descriptor input,
export, and which. Add buffer-view cases covering exact-fit capacity and nonzero
byte offsets, ensuring the tests verify the streaming cat success-on-ENOSPC
behavior and ArrayBufferView bounds beyond zero-offset Uint8Array views.
🪄 Autofix (Beta)
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: 76ae3d70-a11c-4ae1-b6be-c5c8d15b1868
📒 Files selected for processing (12)
packages/bun-usockets/src/loop.csrc/jsc/bindings/vm/SigintWatcher.cppsrc/runtime/shell/Builtin.rssrc/runtime/shell/builtin/basename.rssrc/runtime/shell/builtin/cat.rssrc/runtime/shell/builtin/dirname.rssrc/runtime/shell/builtin/echo.rssrc/runtime/shell/builtin/export.rssrc/runtime/shell/builtin/pwd.rssrc/runtime/shell/builtin/seq.rssrc/runtime/shell/builtin/which.rstest/js/bun/shell/bunshell.test.ts
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Superseded by #34698, which takes a cleaner approach (latch the write error on The two unrelated defense-in-depth changes here (timespec zero-init, SigintWatcher null-guard) are posted on #34698 as a diff. The |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| let code = Builtin::write_no_io_exit(interp, cmd, IoKind::Stdout, &buf, 0); | ||
| return Builtin::done(interp, cmd, code); |
There was a problem hiding this comment.
🔴 The stdin-sync path here is fixed, but on_io_reader_chunk (the streaming path — hit by cat file.txt > ${arraybuf}, or cat > ${arraybuf} with a real-fd stdin) still ends with let _ = Builtin::write_no_io(...); Yield::done(). When the ArrayBuffer fills, ENOSPC is dropped, on_io_reader_done fires with err=None, and cat exits 0 with truncated output — the same bug this PR fixes elsewhere. cat isn't covered by the stated exclusion rationale (its primary output is stdout) and this path is reachable on Windows without a feature flag; it should either be fixed (set *remove = true, stash a nonzero errno on the state — ExecStdin already has an errno field — and exit nonzero from on_io_reader_done) or explicitly listed as deferred.
Extended reasoning...
What the bug is
This PR folds write_no_io ENOSPC into builtin exit codes and, in cat.rs, fixes the stdin-sync path (line ~150: cat < ${blob} > ${buf}). But the streaming path in the same file — Cat::on_io_reader_chunk — still discards the error:
let _ = Builtin::write_no_io(interp, cmd, IoKind::Stdout, chunk);
Yield::done()This is the async twin of the path the PR did fix.
Code path that triggers it
on_io_reader_chunk is invoked whenever cat reads from an IOReader (real fd) and stdout has needs_io() == None — i.e. stdout is BuiltinIO::ArrayBuf/Blob/Buf/Ignore. Two user-reachable shapes:
cat file.txt > ${arraybuf}—ExecFilepathArgsstate:next()opens the file, creates anIOReader, and each read chunk lands inon_io_reader_chunk.cat > ${arraybuf}with a real-fd stdin (e.g. inside a pipeline:somecmd | cat > ${buf}) —ExecStdinwithstdin.needs_io() == true.
In both, stdout.needs_io() is None (ArrayBuffer sink), so the function falls through to the write_no_io call and returns Yield::done().
Why nothing catches it
With this PR's own tightening of write_no_io_to (any truncated write to an ArrayBuffer now returns Err(ENOSPC)), the error is produced but discarded by let _ =. Then on_io_reader_done fires with err = None (the read succeeded), so:
ExecStdin:errnostays0,!stdout_needs_io→Step::Done(0).ExecFilepathArgs:errno = 0,!stdout_needs_io→Step::Next→next()seesidx >= n_files→Builtin::done(interp, cmd, 0).
Exit code 0, output silently truncated.
Step-by-step proof
On Windows (cat builtin always enabled — Kind::DISABLED_ON_POSIX is only consulted when !cfg!(windows)), or on POSIX with BUN_ENABLE_EXPERIMENTAL_SHELL_BUILTINS=1:
import { $ } from "bun";
await Bun.write("big.txt", "a".repeat(100));
const buf = new Uint8Array(2);
const r = await $`cat big.txt > ${buf}`.nothrow().quiet();
console.log(r.exitCode, new TextDecoder().decode(buf));
// → 0 "aa" (should be 1 "aa", like echo/seq/pwd now do)Builtin::initsetsstdout = BuiltinIO::ArrayBuf { buf, i: 0 }(jsbuf redirect).Cat::start→ExecFilepathArgs { args_start: 0, idx: 0, ... }→next().next()opensbig.txt, creates anIOReader, callsreader.start().- Reader delivers a 100-byte chunk to
on_io_reader_chunk.stdout_needs_ioisNone, so it falls through towrite_no_io, which writes 2 bytes into the ArrayBuffer and returnsErr(ENOSPC). Thelet _ =drops it. ReturnsYield::done(). - Reader hits EOF →
on_io_reader_done(err = None).errno = 0,in_done = true,!stdout_needs_io→Step::Next. next():idx (1) >= n_files (1)→Builtin::done(interp, cmd, 0).
Why this belongs in scope
Per REVIEW.md: "Fix the whole class in the same PR … sync/async twins … If a site is intentionally excluded, say so in the PR." The PR description lists cat under fixed builtins as "cat (stdin-sync path)", and its exclusion list (touch -v/mkdir -v/ls/rm -v/cp -v) is justified by "the primary output is the filesystem side effect, not stdout". That rationale doesn't apply to cat — its primary output is stdout. So the streaming path is neither fixed nor covered by the stated exclusion.
How to fix
The plumbing already exists. In on_io_reader_chunk, when write_no_io returns Err:
- Set
*remove = trueso the IOReader stops delivering chunks. - Stash a nonzero errno on the state —
ExecStdinalready has anerrno: ExitCodefield;ExecFilepathArgswould need one (or setin_done/out_doneand reuse a shared field). - Have
on_io_reader_done(which will fire after the reader is removed) exit with that errno instead of 0 / advancing to the next file.
Alternatively, if threading it through the multi-chunk state machine is out of scope for this PR, add cat's streaming path to the explicit exclusion list in the PR description alongside the -v builtins.
| const cases: Array<[string, Run, string | undefined]> = [ | ||
| ["echo", b => $`echo hello > ${b}`, "hello\n"], | ||
| ["seq", b => $`seq 1 3 > ${b}`, "1\n2\n3\n"], | ||
| ["basename", b => $`basename /a/b/cde > ${b}`, "cde\n"], | ||
| ["dirname", b => $`dirname /a/b/cde > ${b}`, "/a/b\n"], | ||
| ["pwd", b => $`pwd > ${b}`, undefined], | ||
| ]; |
There was a problem hiding this comment.
🟡 The test.each table covers echo/seq/basename/dirname/pwd but not the other three sites this PR modifies: export (print-all path), which (sync loop), and cat (stdin-sync). which in particular uses bespoke had_write_err flag logic rather than the shared write_no_io_exit helper, so reverting that hunk would break no test. export and which are cheap to add as extra rows (export needs a big-buffer sized from the actual env; which needs a resolvable command like bunExe() so had_write_err is isolated from had_not_found); cat is in DISABLED_ON_POSIX so is reasonable to defer with a comment.
Extended reasoning...
What's missing
The PR folds ENOSPC into the exit code at 8 call sites — echo, seq, basename, dirname, pwd, export (print-all), which (sync loop), and cat (stdin-sync) — but the new describe("redirect to ArrayBuffer that is too small") block at test/js/bun/shell/bunshell.test.ts:479-485 only exercises the first 5. The changes at export.rs:73, which.rs:52-78, and cat.rs:150 have no assertion of the new nonzero exit.
REVIEW.md is direct on this: "Cover the variant matrix, not just the repro. Every sibling entry point receiving the same fix" and "Confirm deleting each load-bearing clause of your fix breaks at least one test."
Why which is the important one
Seven of the eight sites route through the new shared Builtin::write_no_io_exit helper, so the 5 tested builtins give reasonable transitive confidence for those. But which does not use the helper — it has a bespoke had_write_err boolean threaded through a per-arg loop and OR'd into the final exit code:
if Builtin::write_no_io(interp, cmd, IoKind::Stdout, &buf).is_err() {
had_write_err = true;
}
...
return Builtin::done(interp, cmd, if had_not_found || had_write_err { 1 } else { 0 });Reverting this hunk (dropping had_write_err and going back to let _ = ...) would not break a single test in the PR.
Step-by-step: adding the missing rows
which — needs a command that resolves so the exit code is driven by had_write_err, not had_not_found. bunExe() is guaranteed on PATH in the harness. The resolved path is dynamic, so treat it like pwd:
["which", b => $`which ${bunExe()} > ${b}`, undefined],With a zero-length or 2-byte buffer, the resolved path (>2 bytes) truncates → had_write_err = true → exit 1. With a 1KB buffer it fits → exit 0. That isolates the new flag.
export — the print-all path is the same shape as pwd (dynamic output, single write_no_io_exit call), so:
["export", b => $`FOO=bar export > ${b}`, undefined],One caveat: the process env is unbounded in CI, so the fixed 1 << 10 "big" buffer may itself overflow. Either size big per-case (e.g. run once into a large buffer to measure, then allocate that + slack), or for the dynamic-output rows only assert the zero/2-byte cases and skip the big check.
cat — the stdin-sync path at cat.rs:150 is only reachable when cat runs as a builtin. Kind::DISABLED_ON_POSIX includes Cat, so on Linux/macOS cat falls through to a subprocess and cat.rs never runs. Adding it to the same table would need BUN_ENABLE_EXPERIMENTAL_SHELL_BUILTINS=1 in a subprocess or a Windows-only gate. That's enough friction that deferring it with a comment is reasonable — but export and which have no such excuse.
Impact and severity
This is a coverage gap, not a correctness bug — the code as written is correct, and the shared helper is well-exercised by the 5 covered builtins. But per the repo's own review rules, untested behavioral changes are a review concern, and the which change in particular is standalone logic with zero coverage. Nit-level: worth adding two rows before merge, but not blocking.
1 participant
From the static pattern audit of ignored fallible returns.
Repro
Same for
seq,pwd,basename,dirname,export,which, andcat < ${blob} > ${buf}. Onlyyeshandled it.Cause
BuiltinIO::write_no_io_toreturnsErr(ENOSPC)when an ArrayBuffer sink can't accept more bytes, but every single-write builtin didlet _ = write_no_io(...); done(0). The fd path (on_io_writer_chunk) already turns write errors into a nonzero exit, so this only bit the no-io path (ArrayBuffer/Blob/captured-pipe sinks).Separately, the ArrayBuf arm only returned
Errwhen the cursor was already at the end; a single write that didn't fit returnedOk(partial), so a too-small-but-nonzero buffer truncated silently with no way for the caller to notice.Fix
write_no_io_toArrayBuf arm: write whatever fits, then returnErr(ENOSPC)if the write was truncated.Builtin::write_no_io_exithelper that folds the result into anExitCode.echo,basename,dirname,export,seq,pwd,cat(stdin-sync path),which(sync-loop path): use the helper instead of discarding.Stderr writes on error paths still discard (exit is already nonzero). The verbose-output sites in
touch -v/mkdir -v/ls/rm -v/cp -vare left alone; threading a write error through their multi-task state machines is a larger change and the primary output is the filesystem side effect, not stdout.Also in this PR (same audit, both cheap)
packages/bun-usockets/src/loop.c: zero-initialize thetimespecread byus_internal_monotonic_nsso a failedclock_gettimecannot feed stack garbage into the sweep-timer deadline.CLOCK_MONOTONICcan't realistically fail on supported platforms; this is defense-in-depth at zero cost.src/jsc/bindings/vm/SigintWatcher.cpp: null-guardm_threadinuninstall().WTF::Thread::createreturnsRef<>(RELEASE_ASSERTon failure) so the assignment itself is never null, butinstall()setsm_installed = truebefore assigningm_thread, andm_threadis aRefPtr.The "fallible
us_create_loop" item from the same audit is already covered by #33850, which does the full graceful-recovery version; not duplicated here.Verification
test/js/bun/shell/bunshell.test.ts"redirect to ArrayBuffer that is too small": for each of echo/seq/basename/dirname/pwd, redirects to zero-length, 2-byte, and 1KB buffers and asserts exit 1/1/0 respectively, plus that the 2-byte buffer holds the first two bytes of output.Fails on main with
exitCode: 0for the zero-length and 2-byte cases; passes with this change. Existingyes > ${buffer}and ArrayBuffer leak tests continue to pass.This is orthogonal to #32278, which fixes the exit code on the fd path's
on_io_writer_chunkcallback (negated-errno wrapping); this PR is the no-io path.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/bunshell.test.ts