Skip to content

shell: fold write_no_io ENOSPC into builtin exit codes #34699

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
robobun wants to merge 2 commits into from
Closed

shell: fold write_no_io ENOSPC into builtin exit codes #34699

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a9ee41a
shell: fold write_no_io ENOSPC into builtin exit codes
robobun Jul 19, 2026
9f083a1
[autofix.ci] apply automated fixes
autofix-ci[bot] Jul 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/bun-usockets/src/loop.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ void us_internal_disable_sweep_timer(struct us_loop_t *loop) {
#define LIBUS_TIMEOUT_GRANULARITY_NS ((long long) LIBUS_TIMEOUT_GRANULARITY * 1000000000LL)

uint64_t us_internal_monotonic_ns(void) {
struct timespec ts;
struct timespec ts = {0, 0};
clock_gettime(CLOCK_MONOTONIC, &ts);
return (uint64_t) ts.tv_sec * 1000000000ULL + (uint64_t) ts.tv_nsec;
}
Expand Down
6 changes: 4 additions & 2 deletions src/jsc/bindings/vm/SigintWatcher.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ void SigintWatcher::uninstall()
{
if (m_installed.exchange(false)) {
WTF::Thread* currentThread = WTF::Thread::currentMayBeNull();
ASSERT(!currentThread || m_thread->uid() != currentThread->uid());
ASSERT(!currentThread || !m_thread || m_thread->uid() != currentThread->uid());

#if OS(WINDOWS)
SetConsoleCtrlHandler(WindowsCtrlHandler, false);
Expand All @@ -100,7 +100,9 @@ void SigintWatcher::uninstall()
#endif

m_semaphore.signal();
m_thread->waitForCompletion();
if (m_thread) {
m_thread->waitForCompletion();
}
}
}

Expand Down
34 changes: 28 additions & 6 deletions src/runtime/shell/Builtin.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -393,16 +393,21 @@ impl BuiltinIO {
// stored cursor is u32.
let idx = *i as usize;
let total = arraybuf.array_buffer.byte_len as usize;
if idx >= total {
let remaining = total.saturating_sub(idx);
let write_len = remaining.min(buf.len());
if write_len > 0 {
let dst = &mut arraybuf.slice_mut()[idx..idx + write_len];
dst.copy_from_slice(&buf[..write_len]);
*i = i.saturating_add(write_len as u32);
}
// A truncated write is ENOSPC: the caller's output did not
// fit, same as `echo foo > /dev/full`.
if write_len < buf.len() {
return Err(bun_sys::Error::from_code(
bun_sys::E::ENOSPC,
bun_sys::Tag::write,
));
}
let write_len = (total - idx).min(buf.len());
let dst = &mut arraybuf.slice_mut()[idx..idx + write_len];
dst.copy_from_slice(&buf[..write_len]);
*i = i.saturating_add(write_len as u32);
Ok(write_len)
}
BuiltinIO::Blob(_) | BuiltinIO::Ignore => Ok(buf.len()),
Expand Down Expand Up @@ -907,7 +912,9 @@ impl Builtin {
/// Write `buf` to stdout/stderr without going through IOWriter (the
/// stream is a captured buffer / arraybuffer / blob / /dev/null).
///
/// Returns `Err(ENOSPC)` when an ArrayBuffer target is already full.
/// Returns `Err(ENOSPC)` when an ArrayBuffer target cannot hold all of
/// `buf` (whatever fits is written first). Callers should fold that into
/// the builtin's exit code; use [`write_no_io_exit`] for the common case.
/// **WARNING**: caller must have checked `needs_io() == None` first.
pub fn write_no_io(
interp: &Interpreter,
Expand All @@ -934,6 +941,21 @@ impl Builtin {
unsafe { out.write_no_io_to(shell, buf) }
}

/// [`write_no_io`] folded to an exit code: `exit_code` on success, `1`
/// (or `exit_code` if already nonzero) on ENOSPC.
pub fn write_no_io_exit(
interp: &Interpreter,
cmd: NodeId,
io_kind: IoKind,
buf: &[u8],
exit_code: ExitCode,
) -> ExitCode {
match Self::write_no_io(interp, cmd, io_kind, buf) {
Ok(_) => exit_code,
Err(_) => exit_code.max(1),
}
}

/// Shell exec env of the owning Cmd.
#[inline]
pub fn shell<'a>(
Expand Down
4 changes: 2 additions & 2 deletions src/runtime/shell/builtin/basename.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,8 @@ impl Basename {
.stdout
.enqueue(child, &owned, safeguard);
}
let _ = Builtin::write_no_io(interp, cmd, IoKind::Stdout, &buf);
Builtin::done(interp, cmd, 0)
let code = Builtin::write_no_io_exit(interp, cmd, IoKind::Stdout, &buf, 0);
Builtin::done(interp, cmd, code)
}

fn fail(interp: &Interpreter, cmd: NodeId, msg: &[u8]) -> Yield {
Expand Down
4 changes: 2 additions & 2 deletions src/runtime/shell/builtin/cat.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,8 +147,8 @@
.stdout
.enqueue(child, &buf, safeguard);
}
let _ = Builtin::write_no_io(interp, cmd, IoKind::Stdout, &buf);
return Builtin::done(interp, cmd, 0);
let code = Builtin::write_no_io_exit(interp, cmd, IoKind::Stdout, &buf, 0);
return Builtin::done(interp, cmd, code);

Check failure on line 151 in src/runtime/shell/builtin/cat.rs

View check run for this annotation

Claude / Claude Code Review

cat streaming path still discards ENOSPC on ArrayBuffer stdout 

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 i
Comment thread
robobun marked this conversation as resolved.
Comment on lines +150 to +151

@claude claude Bot Jul 19, 2026

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.

🔴 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} — ExecFilepathArgs state: next() opens the file, creates an IOReader, and each read chunk lands in on_io_reader_chunk.
  • cat > ${arraybuf} with a real-fd stdin (e.g. inside a pipeline: somecmd | cat > ${buf}) — ExecStdin with stdin.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: errno stays 0, !stdout_needs_io → Step::Done(0).
  • ExecFilepathArgs: errno = 0, !stdout_needs_io → Step::Next → next() sees idx >= 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)
  1. Builtin::init sets stdout = BuiltinIO::ArrayBuf { buf, i: 0 } (jsbuf redirect).
  2. Cat::start → ExecFilepathArgs { args_start: 0, idx: 0, ... } → next().
  3. next() opens big.txt, creates an IOReader, calls reader.start().
  4. Reader delivers a 100-byte chunk to on_io_reader_chunk. stdout_needs_io is None, so it falls through to write_no_io, which writes 2 bytes into the ArrayBuffer and returns Err(ENOSPC). The let _ = drops it. Returns Yield::done().
  5. Reader hits EOF → on_io_reader_done(err = None). errno = 0, in_done = true, !stdout_needs_io → Step::Next.
  6. 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 = true so the IOReader stops delivering chunks.
  • Stash a nonzero errno on the state — ExecStdin already has an errno: ExitCode field; ExecFilepathArgs would need one (or set in_done/out_done and 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.

}
// Clone the `Arc<IOReader>`
// out of `stdin` so we hold no borrow of `interp` across
Expand Down
4 changes: 2 additions & 2 deletions src/runtime/shell/builtin/dirname.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,8 @@ impl Dirname {
.stdout
.enqueue(child, &owned, safeguard);
}
let _ = Builtin::write_no_io(interp, cmd, IoKind::Stdout, &buf);
Builtin::done(interp, cmd, 0)
let code = Builtin::write_no_io_exit(interp, cmd, IoKind::Stdout, &buf, 0);
Builtin::done(interp, cmd, code)
}

fn fail(interp: &Interpreter, cmd: NodeId, msg: &[u8]) -> Yield {
Expand Down
4 changes: 2 additions & 2 deletions src/runtime/shell/builtin/echo.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,9 +98,9 @@ impl Echo {
.enqueue(child, &buf, safeguard);
}
let buf = Self::state_mut(interp, cmd).output.clone();
let _ = Builtin::write_no_io(interp, cmd, IoKind::Stdout, &buf);
let code = Builtin::write_no_io_exit(interp, cmd, IoKind::Stdout, &buf, 0);
Self::state_mut(interp, cmd).state = State::Done;
Builtin::done(interp, cmd, 0)
Builtin::done(interp, cmd, code)
}

pub(crate) fn on_io_writer_chunk(
Expand Down
4 changes: 2 additions & 2 deletions src/runtime/shell/builtin/export.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,8 @@ impl Export {
.stdout
.enqueue(child, &buf, safeguard);
}
let _ = Builtin::write_no_io(interp, cmd, IoKind::Stdout, &buf);
Builtin::done(interp, cmd, 0)
let code = Builtin::write_no_io_exit(interp, cmd, IoKind::Stdout, &buf, 0);
Builtin::done(interp, cmd, code)
}

pub(crate) fn on_io_writer_chunk(
Expand Down
4 changes: 2 additions & 2 deletions src/runtime/shell/builtin/pwd.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,9 @@ impl Pwd {
.stdout
.enqueue(child, &cwd, safeguard);
}
let _ = Builtin::write_no_io(interp, cmd, IoKind::Stdout, &cwd);
let code = Builtin::write_no_io_exit(interp, cmd, IoKind::Stdout, &cwd, 0);
Self::state_mut(interp, cmd).state = State::Done;
Builtin::done(interp, cmd, 0)
Builtin::done(interp, cmd, code)
}

pub(crate) fn on_io_writer_chunk(
Expand Down
4 changes: 2 additions & 2 deletions src/runtime/shell/builtin/seq.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,8 +203,8 @@ impl Seq {
.stdout
.enqueue(child, &buf, safeguard);
}
let _ = Builtin::write_no_io(interp, cmd, IoKind::Stdout, &out);
Builtin::done(interp, cmd, 0)
let code = Builtin::write_no_io_exit(interp, cmd, IoKind::Stdout, &out, 0);
Builtin::done(interp, cmd, code)
}

pub(crate) fn on_io_writer_chunk(
Expand Down
30 changes: 15 additions & 15 deletions src/runtime/shell/builtin/which.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,33 +49,33 @@ impl Which {
// captured buffer, then finish.
let (path_env, cwd) = Self::path_and_cwd(interp, cmd);
let mut had_not_found = false;
let mut had_write_err = false;
for i in 0..argc {
let arg = Self::arg(interp, cmd, i);
match Self::resolve(&path_env, &cwd, &arg) {
Some(resolved) => {
let buf = Builtin::fmt_error_arena(
interp,
cmd,
None,
format_args!("{}\n", bstr::BStr::new(&resolved)),
)
.to_vec();
let _ = Builtin::write_no_io(interp, cmd, IoKind::Stdout, &buf);
}
let buf = match Self::resolve(&path_env, &cwd, &arg) {
Some(resolved) => Builtin::fmt_error_arena(
interp,
cmd,
None,
format_args!("{}\n", bstr::BStr::new(&resolved)),
)
.to_vec(),
None => {
had_not_found = true;
let buf = Builtin::fmt_error_arena(
Builtin::fmt_error_arena(
interp,
cmd,
Some(Kind::Which),
format_args!("{} not found\n", bstr::BStr::new(&arg)),
)
.to_vec();
let _ = Builtin::write_no_io(interp, cmd, IoKind::Stdout, &buf);
.to_vec()
}
};
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 { 1 } else { 0 });
return Builtin::done(interp, cmd, if had_not_found || had_write_err { 1 } else { 0 });
}

Self::state_mut(interp, cmd).state = State::MultiArgs {
Expand Down
33 changes: 33 additions & 0 deletions test/js/bun/shell/bunshell.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -472,6 +472,39 @@
expect(new TextDecoder().decode(buffer.slice(0, sentinelByte(buffer)))).toEqual(await thisFile.text());
});

describe("redirect to ArrayBuffer that is too small", () => {
// A builtin whose output does not fit in the ArrayBuffer sink must exit
// nonzero, like `echo > /dev/full` does. Whatever fits is still written.
type Run = (b: ArrayBufferView) => ReturnType<typeof $>;
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],
];

Check warning on line 485 in test/js/bun/shell/bunshell.test.ts

View check run for this annotation

Claude / Claude Code Review

Test does not cover which/export/cat — 3 of 8 fixed sites untested 

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_
Comment on lines +479 to +485

@claude claude Bot Jul 19, 2026

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.

🟡 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.

test.concurrent.each(cases)("%s", async (_name, run, full) => {
const zero = new Uint8Array(0);
const small = new Uint8Array(2);
const big = new Uint8Array(1 << 10);
const [r0, rSmall, rBig] = await Promise.all([
run(zero).nothrow().quiet(),
run(small).nothrow().quiet(),
run(big).nothrow().quiet(),
]);
const bigText = new TextDecoder().decode(big.slice(0, sentinelByte(big)));
expect({
zero: { exitCode: r0.exitCode },
small: { exitCode: rSmall.exitCode, buf: new TextDecoder().decode(small) },
big: { exitCode: rBig.exitCode, buf: full === undefined ? "<dynamic>" : bigText },
}).toEqual({
zero: { exitCode: 1 },
small: { exitCode: 1, buf: (full ?? bigText).slice(0, 2) },
big: { exitCode: 0, buf: full ?? "<dynamic>" },
});
});
Comment thread
robobun marked this conversation as resolved.
});

test("redirect Bun.File", async () => {
const filepath = join(temp_dir, "lmao.txt");
const file = Bun.file(filepath);
Expand Down
Loading