shell(cat): read regular files synchronously in the builtin on POSIX - #35337
shell(cat): read regular files synchronously in the builtin on POSIX#35337robobun wants to merge 1 commit into
Conversation
WalkthroughChangesCat builtin I/O flow
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 2:11 PM PT - Aug 12th, 2026
❌ @robobun, your commit fa328bc has 5 failures in
🧪 To try this PR locally: bunx bun-pr 35337That installs a local version of the PR into your bun-35337 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
robobun
changed the title
![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 471-516: Replace the whole-file buffering in slurp_regular_file
with bounded chunk reads and incremental writes through the existing
caller/writer path, mirroring IOReader’s streaming behavior. Ensure regular
files do not create a full-file Vec before write_buf_to_stdout enqueues data,
while preserving EOF handling and catchable ENOMEM propagation.
In `@test/js/bun/shell/commands/cat.test.ts`:
- Around line 27-125: Extend the cat (builtin) suite with tests for an empty
regular file and a non-regular file argument such as /dev/null. Use runShell and
assert the command completes successfully with empty stdout and stderr, covering
both the zero-length regular-file path and the IOReader fallback.
🪄 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: 7be27ae8-4a1a-4558-8d7b-7816ec9a82e7
📒 Files selected for processing (2)
src/runtime/shell/builtin/cat.rstest/js/bun/shell/commands/cat.test.ts
|
CI on 9b10466: the new |
There was a problem hiding this comment.
LGTM — the redesign addresses the earlier recursion/char-device concerns by keeping the slurp inside Cat::next and gating on S_IFREG only.
What was reviewed:
- fd ownership in both arms: stdin fd stays owned by
Builtin::stdin; the file-arg fd is closed after slurp or handed toIOReader::init. - Empty-buffer path:
IOWriter::enqueueshort-circuits toYield::OnIoWriterChunkonbuf.is_empty(), so the empty-file → fd-stdout case completes. - The
loopinCat::nextonly iterates for file-arg + captured stdout (write_buf_to_stdout→None→continue); every other branch returns aYieldto the enclosing trampoline, so the 100-arg and 4-sequential-command tests stay at depth 1. slurp_regular_fileusestry_reserve(ENOMEM as a catchable error, not a panic),is_retry()for EINTR, and thebun_core::vecspare-capacity helpers with matching SAFETY contracts.
Extended reasoning...
Overview
This PR fixes the experimental builtin cat on POSIX, which previously either exited 1 (cat < file) or hung forever (cat file) because IOReader::start() unconditionally registers the fd with epoll and Linux returns EPERM for regular files. The fix adds slurp_regular_file() in src/runtime/shell/builtin/cat.rs: on POSIX, if fstat reports S_IFREG, the file is read to EOF with bun_sys::read and handed to the existing whole-buffer output path (write_buf_to_stdout, extracted from the pre-existing !stdin_needs_io shape). Non-regular fds and Windows fall through to the unchanged IOReader::start() path. A secondary fix aligns the ExecFilepathArgs error arm of on_io_reader_done with the ExecStdin arm so a read error before any chunk is queued finishes rather than suspending. Cat::next's file-arg branch is wrapped in a loop so many file args with captured stdout iterate rather than recurse.
The only files touched are src/runtime/shell/builtin/cat.rs and a new test/js/bun/shell/commands/cat.test.ts (11 subprocess tests, all under describe.concurrent).
Prior review round
My earlier review was on a different design that modified IOReader::start() to call r.read() synchronously for non-pollable fds. That had two real problems: (1) completion callbacks fired inside the caller's stack frame, causing per-file native recursion / nested Yield::run trampolines; (2) is_pollable() returns false for char devices, so cat /dev/zero would spin forever. Both were addressed in 65869f4 by reverting the IOReader change and moving the slurp into Cat::next, gated strictly on S_IFREG. The added "sequential cat commands" (4 cmds), "sequential cat redirected to files" (3 cmds), and "many file arguments" (100 files) tests directly cover the recursion shapes I flagged. The directory-arg test (393d59b) covers the chunks_done >= chunks_queued error-arm clause: on Linux a directory opens O_RDONLY, ISREG is false so it falls through to IOReader::start, epoll registration fails, and on_io_reader_done fires with errno != 0, chunks_queued == 0, fd-backed stdout — exactly the previously-hanging path.
Security risks
None. This is read-side I/O in a shell builtin gated behind BUN_ENABLE_EXPERIMENTAL_SHELL_BUILTINS=1 (Kind::DISABLED_ON_POSIX). No parsing of untrusted headers, no size arithmetic on adversarial input beyond usize::try_from(st.st_size).unwrap_or(0) with saturating_add and try_reserve (OOM surfaces as catchable ENOMEM, not a panic). The two unsafe blocks wrap bun_core::vec::spare_bytes_mut/commit_spare around bun_sys::read, whose contracts I checked in bun_core/lib.rs:468/:514 — the SAFETY comments match.
Level of scrutiny
Low-to-medium. The builtin is disabled on POSIX by default (delegates to system cat), so this only affects users who set the experimental env flag, and Windows (where slurp_regular_file returns None). The change reuses existing machinery: write_buf_to_stdout is the pre-existing in-memory-stdin path extracted into a helper, and the IOWriter::enqueue empty-buffer short-circuit at IOWriter.rs:1033 means the empty-file → fd-stdout case returns Yield::OnIoWriterChunk immediately and completes via on_io_writer_chunk. The known limitation (whole-file buffering, no streaming/backpressure) was raised by CodeRabbit and correctly rebutted as not a regression — this path never worked before; bounded streaming is a follow-up before the DISABLED_ON_POSIX gate can be lifted.
Other factors
fd ownership traced through both arms: the stdin arm reads via reader.fd() and leaves ownership with Builtin::stdin (comment at cat.rs states this; no close is issued). The file-arg arm owns the fd from shell_openat, closes it after slurp_regular_file returns Some, and hands it to IOReader::init on None. The per-file bookkeeping reset (chunks_done/queued, in_done/out_done) was moved earlier so it applies before the slurp path too. Tests follow harness conventions (spread bunEnv, tempDir with using, drain pipes concurrently, assert stderr/stdout before exit code, describe.concurrent). The evidence block confirms 11 pass with fix / 10 fail without on both ASAN-debug and release.
|
@robobun rebase |
9b10466 to
ecd40d7
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Rebased onto main (ecd40d7), squashed to a single commit. Three conflict hunks in |
ecd40d7 to
7ca0e91
Compare
7ca0e91 to
dcc885a
Compare
dcc885a to
472d7f5
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Rebased and squashed to 472d7f5. The last three CI builds (93556, 93559, 93563) all failed in the build step because the runners could not download vendored deps (WebKit, mimalloc, c-ares, lol-html: "Failed to download after 5 attempts"); every recent build on the pipeline including main (93528) is failing the same way, so this is an infra outage rather than anything in the diff. Locally on the rebased branch: |
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 458-461: Update the fstat handling in the surrounding cat
descriptor setup to propagate syscall failures instead of converting them to
None. Reserve None for successful fstat results whose mode is not regular, and
preserve the existing regular-file path and caller error reporting.
In `@test/js/bun/shell/commands/cat.test.ts`:
- Around line 124-128: Strengthen the test “directory as file argument with fd
stdout does not hang” by capturing stderr and reading the redirected out.txt
after runShell. Assert the platform-specific cat error text and expected nonzero
exit status, and verify out.txt is empty, so the test specifically validates the
directory-input failure and redirect behavior rather than any arbitrary command
failure.
🪄 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: 7813261e-b83d-4d60-9cad-02a3a9c4a653
📒 Files selected for processing (2)
src/runtime/shell/builtin/cat.rstest/js/bun/shell/commands/cat.test.ts
IOReader::start() on POSIX always registers the fd with epoll. Linux returns EPERM for regular files, which register_poll() surfaces through on_reader_error as a spurious read failure. With BUN_ENABLE_EXPERIMENTAL_SHELL_BUILTINS=1 (so cat runs as a builtin): - `cat < file` exited 1 with no output (EPERM became the exit code) - `cat file` hung forever: the ExecFilepathArgs error arm of Cat::on_io_reader_done gated completion on out_done instead of chunks_done >= chunks_queued, so an error before any chunk was queued suspended waiting on writer callbacks that never fire Cat::next now fstat()s the fd before handing it to IOReader::start. For S_IFREG only, it reads the file to EOF and reuses the existing in-memory-stdin shape (one enqueue on fd-backed stdout, write_no_io otherwise), so completion is returned to the enclosing trampoline rather than driven inline. Pipes, sockets, TTYs, and character devices keep the unchanged IOReader path. The file-arg branch is wrapped in a loop so many arguments with captured stdout iterate instead of recursing. The ExecFilepathArgs error arm is aligned with ExecStdin.
472d7f5 to
fa328bc
Compare
There was a problem hiding this comment.
LGTM — the earlier assertion-ordering nit and the two coderabbit points (fstat error propagation, directory-test strengthening) are all addressed in fa328bc.
What was reviewed:
- fd ownership across the new slurp path: file-arg fd is closed after slurp, stdin fd stays owned by
Builtin::stdin, and theNonebranch still hands ownership toIOReader::init— no leak or double-close. write_buf_to_stdoutnow incrementschunks_queuedwhere the old!stdin_needs_iobranch did not; tracedon_io_writer_chunkfor bothExecStdinandExecFilepathArgsand the completion condition is preserved.- State-counter reset moved before
shell_openat: equivalent for the IOReader path, and required for the slurp path so the increments land on zeroed counters. - Windows:
slurp_regular_filereturnsNoneso the loop nevercontinues; the only behavior change is theon_io_reader_donehang fix, which is a strict improvement.
Extended reasoning...
Overview
Two files: src/runtime/shell/builtin/cat.rs (~200 lines net) and a new test/js/bun/shell/commands/cat.test.ts (11 tests). The Rust change (1) adds slurp_regular_file, an fstat-gated synchronous read for S_IFREG fds on POSIX, (2) extracts the one-buffer stdout write into write_buf_to_stdout, (3) wraps Cat::next's FileArg handling in a loop so many captured-stdout file args iterate rather than recurse, and (4) adds chunks_done >= chunks_queued to the ExecFilepathArgs error arm of on_io_reader_done so a reader error before any chunk is queued completes instead of suspending.
Security risks
None. The change reads regular files the shell script already named; there is no new path handling, no external input parsing beyond what shell_openat already did, and the two new unsafe blocks are the standard spare_bytes_mut/commit_spare read-into-uninit-capacity idiom with accurate SAFETY comments. Allocation goes through try_reserve and surfaces ENOMEM as a catchable shell error rather than aborting.
Level of scrutiny
Moderate for the diff itself (state machine + fd lifecycle), but the blast radius is small: builtin cat is in Kind::DISABLED_ON_POSIX (Builtin.rs:210-215), so on POSIX the whole file is only reachable via BUN_ENABLE_EXPERIMENTAL_SHELL_BUILTINS=1. On Windows slurp_regular_file returns None, so the only production-visible change there is the on_io_reader_done hang fix and the behavior-preserving refactor around it. I traced each Windows-reachable change: the chunks_queued increment in write_buf_to_stdout keeps on_io_writer_chunk at Done (0≥0 → 1≥1), the loop is always single-iteration, and moving the counter reset before shell_openat is equivalent because a failed open transitions to WaitingWriteErr and never reads the counters.
Other factors
This PR has been through three review rounds. The initial IOReader change was backed out per review; comment-cop's paragraph-comment flags were tightened; coderabbit's whole-file-buffering concern was withdrawn (there was no working streaming baseline to regress from) and its two remaining points (propagate fstat error, strengthen the directory test) landed in fa328bc; my prior exitCode-ordering nit was also fixed there. The test suite is comprehensive (arg, stdin redirect, empty, multi-arg, 300 KB, pipe control, redirect-out, sequential ×2, directory error, 100 args), spawns with the env flag so it exercises the builtin on every platform, and was verified fails-before/passes-after on both ASAN and release. No bugs were found in the current bug-hunting pass. No CODEOWNERS entry for src/runtime/shell/.
2 participants
What does this PR do?
On POSIX,
IOReader::start()always registers the fd with epoll. Linux returnsEPERMfor regular files, whichregister_poll()surfaces throughon_reader_erroras a spurious read failure.With
BUN_ENABLE_EXPERIMENTAL_SHELL_BUILTINS=1(socatruns as a builtin rather than a subprocess on POSIX):Two things were going on:
cat < fileexited 1 with no output: theEPERMfromepoll_ctlreachedCat::on_io_reader_done(ExecStdinarm) and became the exit code.cat filehung: theExecFilepathArgserror arm ofCat::on_io_reader_donegated completion onout_donerather thanchunks_done >= chunks_queued, so an error that arrives before any chunk is queued suspends waiting on writer callbacks that will never fire (and the failedFilePoll's keep-alive pin kept the process up).Builtin
catis inKind::DISABLED_ON_POSIX, so the broken path was only reachable via the env flag (or on Windows, where reads go through libuv and this branch isn't taken). The same hardcodedis_pollable = trueis in the pre-RustIOReader.zig, so this has never worked.Fix
Cat::nextnowfstats the fd before handing it toIOReader::start. ForS_IFREGonly, it reads the file to EOF withbun_sys::readand reuses the existing in-memory-stdin shape (write_buf_to_stdout: one chunk enqueued on fd-backed stdout,write_no_iootherwise), so completion is returned to the enclosingYield::runtrampoline instead of being driven inline. This is the read-side analogue ofIOWriter::do_file_write.Pipes, sockets, TTYs, and character devices keep the unchanged
IOReader::start()path, soecho x | catandcat < /dev/zerobehave exactly as before.Cat::next'sBranch::FileArgis wrapped in a loop so many file arguments with captured stdout iterate rather than recurse. TheExecFilepathArgserror arm is aligned with theExecStdinarm so a read error before any chunk is queued finishes rather than suspending.Why is this fix correct?
read(2)on a regular file is finite and never returnsEAGAIN, so the synchronous slurp is bounded. It hands one buffer to the sameenqueue/write_no_iopath the existing!stdin_needs_iobranch already uses, so the downstream state machine is unchanged; each file becomes either oneYield::OnIoWriterChunktrampoline iteration (fd-backed stdout) or oneloopiteration insideCat::next(captured stdout), and sequential commands stay atDbgDepthGuarddepth 1. Windows returnsNonefromslurp_regular_fileand is untouched.Related
on_io_reader_done/on_io_writer_chunkcompletion logic incat.rs(covering read errors that arrive after chunks are already queued, which this PR does not touch). The one-lineExecFilepathArgserror-arm change here is a subset of it; whichever of the two lands second needs a small rebase incat.rsandcat.test.ts.IOReaderfor a secondcat. Independent of this PR: regular-file stdin never reachesIOReaderhere, and non-regular stdin is unchanged.How did you verify your code works?
Added
test/js/bun/shell/commands/cat.test.ts, which spawns a child withBUN_ENABLE_EXPERIMENTAL_SHELL_BUILTINS=1so the builtin path is taken on every platform. It coverscat file,cat < file, multi-file args, a multi-chunk 300 KB file,echo | cat(pollable control),cat file > out, four sequentialcatcommands (captured and redirected), an empty file, a directory argument with fd-backed stdout (pins the error-arm change), and a 100-file-argument case.[review] gate passed · iteration 3 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 3
evidence per changed file