shell: stream the seq builtin's output in chunks instead of rendering it whole - #38029
shell: stream the seq builtin's output in chunks instead of rendering it whole#38029robobun wants to merge 4 commits into
Conversation
|
Warning Review limit reached
Next review available in: 53 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 (4)
Comment |
|
Warning Review limit reached
Next review available in: 3 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 (2)
Comment |
|
Updated 12:30 PM PT - Aug 13th, 2026
❌ @robobun, your commit 5537978 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 38029That installs a local version of the PR into your bun-38029 --bun |
|
Status: fix, shared helpers and tests pushed (branch rebased onto current main); waiting on CI. Reproduced on bun 1.4.0 and on an unfixed ASAN debug build with the memory test in |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this and didn't find any bugs. The change follows the yes builtin's chunked-enqueue pattern closely and the tests cover all three stdout paths, the 64 KiB boundary, EPIPE early-exit, and the memory bound. Since it reworks a native state machine with an on_io_writer_chunk re-arm loop and an unsafe split-borrow write path, a maintainer glance would still be worthwhile.
Checked: render_chunk never produces an empty buffer on the fd path (each non-final chunk leaves has_next() true, so the next chunk writes ≥1 value), and the saturation guard fires inside a chunk that returns true, so it can't loop.
Checked: the no-IO > ${buffer} path preserves the old truncation semantics — partial write returns Ok, next chunk hits ENOSPC, loop exits with code 0.
Checked: /dev/null in the RSS test maps to NUL on Windows via the shell's path rewrite, same as the test it replaces.
Extended reasoning...
Overview
This PR changes the seq shell builtin (src/runtime/shell/builtin/seq.rs) from rendering the entire sequence into one Vec before writing, to streaming it in ~64 KiB chunks. It adds two fields to Seq (current cursor, reusable buf), a Writing state, and three helpers (render_chunk, enqueue_chunk, split_stdout_state). The fd path queues one chunk and re-arms from on_io_writer_chunk; the no-IO path loops synchronously. test/js/bun/shell/commands/seq.test.ts gains a seq long output block covering captured/file/pipe stdout, exact chunk-boundary sequences, two EPIPE early-exit cases, and an RSS-delta test that replaces the old ASAN-only 152 MB bound with a 16 MB bound.
Security risks
None identified. Argument parsing is unchanged; the new code only reshapes how already-computed values are buffered and written. The one unsafe block (write_no_io_to) satisfies its documented contract (shell is cmd_node.base.shell, live for the Cmd) and mirrors how Builtin::write_no_io already invokes it.
Level of scrutiny
Medium-high. This is native runtime code with a small state machine driven by async IOWriter callbacks — the class of change where a mis-ordered state transition can hang a pipeline or double-fire Builtin::done. I traced every state transition: do_ → first enqueue_chunk sets Writing or Done; on_io_writer_chunk on Writing re-arms, on Done finishes with 0, on Err finishes with 1, on error param finishes with 1 without rendering more. render_chunk only cuts between values (the >= CHUNK_SIZE check runs before writing the next value), so concatenated chunks are byte-identical to the old single buffer. The saturation guard (next == current) breaks inside the chunk and returns true, so it terminates in one place as before. The split_stdout_state / enqueue_chunk / OutputNeedsIOSafeGuard::OutputNeedsIo-with-debug_assert shape is lifted verbatim from yes.rs.
Other factors
Test coverage is strong: exact-boundary sequences (8192×8 bytes = 64 KiB, and one past), all three stdout kinds, | true and a one-line-then-exit reader for EPIPE, and an RSS test whose 16 MB bound is well below the unfixed ~87–128 MB and well above the fixed ~2 MB (no ASAN branching needed at that margin). The old test's 152 MB bound is correctly retired since it no longer measures anything. No prior human review comments; CI was still building at timeline read. Deferring rather than approving because a ~100-line native state-machine rewrite with an unsafe write path and callback re-entry is worth a maintainer's eyes even when it looks right.
… it whole The seq builtin rendered the entire sequence into one Vec and handed it to stdout in a single write, so writing N bytes to an fd needed more than 2N bytes of memory (the Vec plus IOWriter's copy) and a consumer such as `seq 1 N | head -1` saw nothing until the whole sequence existed. Render about 64 KiB at a time into a buffer kept on the builtin's state. On an fd stdout the next chunk is rendered and queued from on_io_writer_chunk once the previous one has been written, the same shape as the yes builtin; on a captured or JS-buffer stdout the chunks are written in a loop. Output, exit codes and the f32 saturation guard are unchanged.
Both chunked builtins need stdout and their own state borrowed at the same
time so the chunk is written straight from the state. Put that split on
BuiltinState (split_stdout for the fd path, split_stdout_no_io for the
synchronous targets) instead of each builtin carrying a copy of it, and
pair the synchronous stream with its shell env in NoIoOutput so the one
unsafe write_no_io_to call stays inside Builtin.rs; Builtin::write_no_io
is built on the same pieces, so every synchronous builtin write still goes
through one place.
seq tests: pin the `> ${buffer}` behaviour across several chunks (fits,
and truncated with exit 0 like the other builtins), and make the EPIPE
tests observe seq's own exit status rather than only the reader's.
9658177 to
e0217c6
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
40cd346 to
f44cd27
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it reworks the seq builtin's state machine, adds a new raw-pointer wrapper (NoIoOutput) to shared Builtin.rs infrastructure, and refactors yes onto it, a human look would still be worthwhile.
What was reviewed:
render_chunkboundary logic — chunks cut only between values, saturation guard preserved, terminator appended once on the last chunk.IOWriter::enqueuecopies its input (s.buf.extend_from_slice), so reusingme.bufacross chunks is safe.write_no_io_tovisibility narrowed to private — no remaining external callers;yesnow goes throughNoIoOutput::write.- No-io loop terminates on
lastor ENOSPC for every non-fdBuiltinIOarm; captured/ignore/blob always return Ok so they run tolast.
Extended reasoning...
Overview
The PR converts the seq shell builtin from rendering its entire output into one Vec to streaming it in ~64 KiB chunks, mirroring the structure yes already uses. It touches four files: seq.rs (new Writing state, current/buf fields, render_chunk/enqueue_chunk), Builtin.rs (new split_stdout/split_stdout_no_io on the BuiltinState trait, new NoIoOutput<'a> wrapper, new of_mut_with_shell, refactored write_no_io), yes.rs (deletes its private split_stdout_state in favour of the shared helpers), and seq.test.ts (replaces the old ASAN-gated 152 MB RSS test with a comprehensive multi-target/boundary/EPIPE/memory suite).
Security risks
None identified. The change is internal to the shell interpreter's output buffering and does not touch parsing of untrusted input, auth, or crypto. The one unsafe block is the pre-existing write_no_io_to deref of the shell env pointer, now confined to NoIoOutput::write with the same invariant (env outlives the Cmd) that the old Builtin::write_no_io relied on.
Level of scrutiny
Moderate-to-high. This is runtime shell execution: state-machine transitions driven by on_io_writer_chunk callbacks, a new safe wrapper over a raw *mut ShellExecEnv, and a shared-infrastructure refactor that both seq and yes now depend on. The logic is careful and well-tested, but the interaction between the Writing/Done states and the IOWriter callback path, plus the borrow-splitting helpers, are the kind of change a maintainer familiar with the shell trampoline should sign off on.
Other factors
- Verified
IOWriter::enqueuecopies bytes into its own buffer before returning, so clearing/reusingme.buffor the next chunk is sound. - The no-io synchronous loop is not a regression: the old code also rendered and wrote the whole sequence synchronously; chunking there is for code uniformity, not memory (captured output is inherently full-size).
write_no_io_tois now module-private; grep confirms no other callers remain.- Test coverage is thorough (captured/file/pipe/Buffer targets, exact chunk-boundary sequences, EPIPE with three reader behaviours, RSS regression test with a 16 MB bound well below the 61–127 MB unfixed delta).
- The comment-cop bot flagged long comments in earlier commits; all were shortened in f44cd27 and the threads are resolved.
- No CODEOWNERS entry for
src/runtime/shell/. - CI on f44cd27 shows only unrelated Windows infra/cluster failures so far.
There was a problem hiding this comment.
I reviewed this and the bug hunter found no issues. Because it reworks the seq builtin's async state machine and adds shared split-borrow / NoIoOutput abstractions on BuiltinState (with a raw *mut ShellExecEnv behind them), a human look is still worthwhile.
Checked: IOWriter::enqueue copies its input, so reusing me.buf across chunks is safe; render_chunk cuts only between values so concatenated output is byte-identical, and the next == current saturation guard terminates within its chunk.
Checked: write_no_io_to narrowed to module-private — the only external caller was yes.rs, now on NoIoOutput; Builtin::write_no_io still routes through the same code.
Checked: the no-io loop runs synchronously to completion for captured/Buffer stdout — same as before, not a regression.
Extended reasoning...
Overview
Streams the seq shell builtin's output in ~64 KiB chunks instead of rendering the whole sequence into one Vec first. seq.rs gains a current cursor, a reusable chunk buffer, a new State::Writing variant, render_chunk / enqueue_chunk helpers, and a Writing arm in on_io_writer_chunk. Builtin.rs gains shared helpers on BuiltinState (split_stdout, split_stdout_no_io), a NoIoOutput wrapper that confines the one unsafe call to write_no_io_to inside the module, and of_mut_with_shell; Builtin::write_no_io and of_mut are rebuilt on those pieces. yes.rs drops its private split_stdout_state and the manual split-borrow + unsafe in write_no_io_loop in favor of the shared helpers. Tests add byte-for-byte multi-chunk coverage across capture/file/pipe/Buffer, chunk-boundary cases, EPIPE handling with early-exiting readers, and an RSS regression test replacing the old ASAN-only one.
Security risks
None identified. No user-controlled parsing was added; the sequence bounds and separator are already validated by the existing argument parser. The unsafe surface shrank: write_no_io_to is now module-private and reached only through NoIoOutput, whose shell pointer is derived from the same cmd_node.base.shell the old write_no_io used, with the Cmd kept borrowed through io for the wrapper's lifetime.
Level of scrutiny
Medium-high. This is async state-machine code in the shell interpreter: a mis-sequenced Writing/Done transition would hang or drop output, and the new trait methods sit on BuiltinState where every builtin can reach them. It is not a mechanical change. That said, it closely mirrors the existing yes streaming pattern, the concatenation invariant is easy to verify (chunks cut only at value boundaries, terminator appended exactly once when render_chunk returns true), and the test matrix is thorough.
Other factors
- Verified
IOWriter::enqueue(IOWriter.rs:1024) copies its input into the writer's own buffer, so clearing and refillingme.bufbetween chunks is sound even though the write completes asynchronously. - Verified
write_no_io_tohas no remaining callers outsideBuiltin.rsafter the visibility change. - The captured-stdout /
> ${buffer}path loops synchronously without yielding to the event loop; the old code also ran the whole sequence synchronously, so this is not a regression, and the ENOSPC break preserves the existing exit-0 truncation behavior (pinned by the new "Buffer smaller than the sequence" test, with #34698 referenced for the future fix). - comment-cop feedback was addressed in f44cd27 and all threads are resolved. CI build #94742 was still running at review time.
- Deferring rather than approving because the change introduces new shared abstractions across the builtin layer and reworks an async state machine with
unsafepointer handling — worth a maintainer's eyes even though no defects were found.
1 participant
Problem
seqbuiltin inBun.$rendered the whole sequence into one localVecand wrote it with a singleenqueue/write_no_iocall (src/runtime/shell/builtin/seq.rs,Seq::do_).IOWriter::enqueuecopies what it is handed, so writing an N byte sequence to an fd held more than 2N bytes:seq -s <100 bytes> 1 300000 > /dev/null(30 MB of output) grows the process by 60 to 87 MB on the release build and 127 MB under ASAN, linearly in the length of the sequence.seq 1 16777216is about 150 MB of output, and both the line count and the line length (-s) are chosen by the caller.seq 1 N | head -1waited for (and paid the memory for) all N values.Fix
Seqkeeps a cursor (current) and one reusable chunk buffer;render_chunkappends values until the buffer holds 64 KiB, or the terminator once the sequence ends.do_queues the first chunk;on_io_writer_chunkrenders and queues the next one after the previous one is written (stateWriting) and finishes when the chunk that was in flight was the last one (stateDone). A write error still ends the command with exit 1 and nothing further is rendered, soseq ... | headstops at the first EPIPE.> ${buffer}stdout: the chunks are written in a loop; a chunk the target refuses (a full> ${buffer}, the only error these targets produce) ends the loop, since the rest of the sequence would be refused as well.next == currentsaturation guard ends the sequence inside the chunk in which it fires, so no value is rendered twice across a boundary. One chunk is in flight at a time, so the builtin plus theIOWriterhold about two chunks however long the sequence is. It is the same structure theyesbuiltin already uses, with a finite sequence instead of an endless one.yesandseqnow both need, stdout and the builtin's own state borrowed together so the chunk is written straight from the state, lives onBuiltinState(split_stdoutfor the fd path,split_stdout_no_iofor synchronous targets) instead of being copied into each builtin;yesis switched to it and its private copy deleted.split_stdout_no_iohands out aNoIoOutput, the stream paired with its shell env, so the oneunsafecall intowrite_no_io_tostays inBuiltin.rs, andBuiltin::write_no_iois built on the same pieces, so every synchronous builtin write still goes through one place.> ${buffer}still truncates with exit 0: seq, like the other builtins, leaves reporting that to the shared write layer (shell: fail the command when a> ${buf}redirect overflows the target Buffer #34698 is the open change doing so for all of them, and seq's writes still go through the layer it instruments), and the new tests pin today's behaviour so that interaction is visible.yes, which reports ENOSPC itself, is unchanged too.test/js/bun/shell/commands/seq.test.ts. Newseq long outputblock: multi-chunk output compared byte for byte on a captured stdout, a file, a pipe and aBuffer; aBuffersmaller than the output; the two sequences ending exactly at and one value past the 64 KiB boundary on all three stdout kinds; seq's own exit status (made visible with(seq ... || echo seq-failed 1>&2) | reader) when the reader never reads, exits after the first line, or drains everything; and a memory test running the 30 MB sequence in a child whose RSS may grow by at most 16 MB. The memory test fails on the unfixed build (release: 61 to 87 MB over five runs, unfixed ASAN debug build: 127 MB) and passes with this change (about 2 MB under ASAN, still 0 to 3 MB at 121 MB of output); every other test passes on both builds, i.e. pins pre-existing behaviour. It replaces the ASAN-only RSS test whose 152 MB bound this change makes meaningless.commands/*.test.ts(includingyes, which covers bothsplit_stdoutusers on both paths),bunshell,shell-seq-condexpr,pipeline_stack,exec,epipe,yieldandfile-iopass apart from tests that fail identically with the released binary in this container (root bypassing thelspermission tests, a 5 s budget on thermsymlink race);cargo clippy -p bun_runtimeandcargo fmtare clean.Background
Yieldnaming what runs next instead of calling it. A builtin that writes more than once queues one piece on theIOWriterand is called back throughon_io_writer_chunkwhen it has been written (synchronously, as a returnedYield, for regular files and/dev/null; from the event loop for pipes and ttys). Queuing the next piece from that callback is how output is streamed;yesworks this way.IOWriteris the shared queue in front of a file descriptor;enqueueappends a copy of the bytes to its own buffer and frees that buffer once every queued chunk is written. That copy is why buffering the whole output first cost twice its size..text()/.quiet()capture,> ${buffer}, a Blob, or ignored) it is written synchronously: appended to the shell env's capture buffer, or copied into the JS buffer withENOSPConce that is full. The capture buffer belongs to the shell env, which is why a synchronous write needs the env alongside the stream; the env outlives the command, which is whatNoIoOutputrelies on.BuiltinStateis the per-builtin state downcast (Impl::Seq(v) => v) that every builtin module already uses throughstate_mut; the new methods are the same projection with stdout borrowed next to it.seqfollows the BSD variant: the-sseparator follows every value, including the last, and the-tterminator is written once after the sequence. A pipeline's exit code is its last command's, which is why the EPIPE tests chain anechooff seq to see seq's own status. The operands aref32, so the pre-existingnext == currentguard is what ends sequences whose increment no longer changes the value; shell: make the seq builtin count in f64 at the operands' decimal precision #37923 moves the arithmetic tof64and is independent of the buffering changed here.