shell: support ReadableStream stdin redirect via SinkHandle - #36895
shell: support ReadableStream stdin redirect via SinkHandle#36895robobun wants to merge 11 commits into
Conversation
Bun.$`cmd < ${readableStream}` panicked with TODO SHELL READABLE
STREAM. Wrap the child's stdin pipe in a FileSink and call
FileSink::assign_to_stream, which tries wire_native_sink first
(ByteStream/FileReader wired directly via SinkHandle::FileSink, no JS)
and falls back to the JS pump for other streams.
- Cmd::init_subproc_redirections: validate (stdin only, not
locked/disturbed), collapse fully-buffered streams to a blob, else
set Stdio::ReadableStream.
- Writable::init (POSIX): create a FileSink around the stdin pipe fd
and start its writer; Windows already did the equivalent.
- spawn_maybe_sync_impl: after the subprocess is constructed, call
assign_to_stream on the FileSink. Skip the ShellWritable source
backref for ReadableStream stdin (source becomes the upstream
ByteStream/FileReader or stays None for JS streams).
- on_process_exit: for Writable::Pipe, call on_attached_process_exit
(cancels the stream, closes the writer, on_close detaches the native
source's SinkHandle) and mark buffered stdin closed so
has_finished() can complete. Re-check has_finished() when
stdout/stderr already set exit_code.
- Builtin.rs: throw a clear error when a ReadableStream is redirected
to a builtin.
Fixes #18262
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughShell commands now accept validated JavaScript ChangesReadableStream stdin support
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: 5
🤖 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/subproc.rs`:
- Around line 1151-1169: Update the Stdio::ReadableStream setup around FileSink
creation to handle the bun_sys::set_nonblocking result instead of discarding it,
and preserve any bun_sys::Error from either setup or writer.start(). On failure,
release pipe_ptr, tear down the spawned child, and return ShellErr::Sys with the
original error; do not continue to Writable::Pipe or return
UnexpectedCreatingStdin, preventing the caller’s panic path.
- Around line 920-928: Track the stdin origin in the subprocess state and update
the process-exit block around Writable::Pipe and
FileSink::on_attached_process_exit to run FileSink::on_attached_process_exit and
buffered_input_close() only when stdin originated from ReadableStream. Preserve
ordinary Windows Stdio::Pipe handling without replacing or dropping the active
FileSinkPtr during process exit.
In `@test/js/bun/shell/bunshell.test.ts`:
- Around line 3165-3170: Add a separate stderr-redirection assertion in the
“stdout/stderr redirect throws” test using the `2>` redirect flag with the same
ReadableStream and expected error. Keep the existing stdout case unchanged so
both redirect variants are explicitly covered.
- Around line 3148-3163: Update the test body around the stream’s pull source
and child script so the child loops on fs.readSync until all three bytes are
read or EOF occurs, ensuring stdout is reliably “xxx”. Add an underlying-source
cancel() resolver, await its promise after the shell command completes, and
assert cancellation occurred alongside the existing output and exit-code
expectations.
- Around line 3116-3119: Strengthen the assertion in the backpressure test by
comparing out.stdout byte-for-byte with the expected Buffer.alloc(size, "x")
payload, while retaining the existing length and exitCode checks.
🪄 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: 9dbd3f4e-9f3b-46f7-a00a-3550050ba60b
📒 Files selected for processing (4)
src/runtime/shell/Builtin.rssrc/runtime/shell/states/Cmd.rssrc/runtime/shell/subproc.rstest/js/bun/shell/bunshell.test.ts
…t, strengthen tests - Writable::init ReadableStream: propagate set_nonblocking/writer.start errors as WritableInitError::Sys so the caller returns ShellErr::Sys instead of the UnexpectedCreatingStdin panic. - on_process_exit: debug_assert the ShellWritable invariant (shell stdin is never Stdio::Pipe, only Fd/Ignore/Blob/ReadableStream). - Tests: compare the 256 KiB payload byte-for-byte; loop readSync in the early-exit child and assert the stream's cancel() fired; cover 2> as well as >; await all .resolves assertions.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…re, socket poll flag, unreachable guards, ByteStream test - spawn_maybe_sync_impl: on WritableInitError::Sys, construct the Process via to_process, kill + wait + close + deref it so the child is reaped and the pidfd closed (the other post-spawn error arms all do try_kill + abort_after_failed_start; this one fires before the Subprocess is written so it does the equivalent directly). - Writable::init (Windows): return Sys(e) on start_with_current_pipe failure so it takes the same teardown path instead of the UnexpectedCreatingStdin panic. - Writable::init (POSIX): tag the stdin FilePoll with FilePollFlag::Socket (the fd is a socketpair half, same as Bun.spawn's stdin path). - assign_to_stream wiring: the global/None and non-Pipe arms are unreachable (Cmd::init_subproc_redirections already .expect()s the global before setting Stdio::ReadableStream); encode as .expect()/unreachable!() instead of a silent no-op that would hang the child. - tests: add a fetch Response.body case (native ByteStream source); drop the inaccurate 'ByteStream' note on proc.stdout.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/subproc.rs`:
- Around line 723-739: In the WritableInitError::Sys branch, clear the published
child pointer in *out_subproc before returning the system error. Do this after
cleaning up the spawned process and before Err(ShellErr::Sys(...)), ensuring the
caller’s SubprocExec.child slot is null rather than dangling.
- Around line 828-850: Update the assign_to_stream failure path in
spawn_maybe_sync_impl to roll back the already-initialized subprocess on
Windows, reclaiming the subprocess, process reference, exit handler, and stdio
resources before clearing out_subproc. Reuse the existing
abort_after_failed_start cleanup mechanisms where appropriate, and ensure the
child slot is cleared after cleanup.
In `@test/js/bun/shell/bunshell.test.ts`:
- Around line 3094-3095: Replace the two-line regression comment above the
SinkHandle test with a single comment containing the full navigable issue URL
for issue `#18262`, preserving the test location and behavior unchanged.
🪄 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: 07202a1b-e5f5-4b0c-a3a4-6b42f31193d6
📒 Files selected for processing (2)
src/runtime/shell/subproc.rstest/js/bun/shell/bunshell.test.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/runtime/shell/subproc.rs (1)
830-861:⚠️ Potential issue | 🟠 MajorDuplicate: reclaim the Windows subprocess after
assign_to_streamfails.On Windows, this branch calls
abort_after_failed_start, but that helper returns without cleanup. The already-published subprocess can retain the child, exit handler, and stdio resources after the assignment error. Apply the prior rollback fix and clear the caller’s child slot after cleanup.🤖 Prompt for 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. In `@src/runtime/shell/subproc.rs` around lines 830 - 861, Update the ReadableStream assignment error path in the subprocess startup flow around assign_to_stream and abort_after_failed_start so Windows performs the same subprocess rollback cleanup as the prior fix, reclaiming the child, exit handler, and stdio resources. After cleanup, clear the caller’s child slot before returning the ShellErr::Custom result, while preserving the existing termination attempt and error message.Source: Coding guidelines
🤖 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.
Duplicate comments:
In `@src/runtime/shell/subproc.rs`:
- Around line 830-861: Update the ReadableStream assignment error path in the
subprocess startup flow around assign_to_stream and abort_after_failed_start so
Windows performs the same subprocess rollback cleanup as the prior fix,
reclaiming the child, exit handler, and stdio resources. After cleanup, clear
the caller’s child slot before returning the ShellErr::Custom result, while
preserving the existing termination attempt and error message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c6eb37a8-abf8-48c3-bcdf-c1893343eabd
📒 Files selected for processing (2)
src/runtime/shell/subproc.rstest/js/bun/shell/bunshell.test.ts
…e cleanup - WritableInitError::UnexpectedCreatingStdin: no producer remains after d6317bf made both the POSIX and Windows Writable::init arms return Sys(e); delete the variant and its panic match arm. - Sys(e) arm on Windows: put the taken-out stdout/stderr Buffer slots back into spawn_result so WindowsSpawnResult::drop routes them through uv_close (the slots were .take()'d out before Writable::init and WindowsStdioResult itself has no Drop).
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/runtime/shell/subproc.rs (2)
738-740: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winDo not discard non-
ESRCHkill failures.
Process::killnormalizes onlyESRCH, whileShellSubprocess::try_killpropagates other errors. Capture termination failures beforeabort_after_failed_startor process teardown, and retain them with the original error.🤖 Prompt for 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. In `@src/runtime/shell/subproc.rs` around lines 738 - 740, Update the process cleanup path around ShellSubprocess::try_kill to capture the result of Process::kill instead of discarding it. Preserve non-ESRCH termination errors and combine or retain them with the original error before invoking abort_after_failed_start or completing process teardown, while keeping expected ESRCH handling unchanged.Source: Coding guidelines
842-844: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReplace panics with recoverable errors for
Stdio::ReadableStreaminitialization failures.The
assert_stdio_result!(result)macro validates file descriptor validity in debug builds on Unix only; it does not guarantee the result will match the expected shape on either platform in release builds.On Windows (line 1033–1065), if
Stdio::ReadableStreamdoes not produceStdioResult::Buffer,Writable::initreturnsOk(Writable::Inherit)instead of panicking. The code at line 843 then encountersunreachable!("Writable::init returns Pipe for Stdio::ReadableStream"), which is reachable if the spawn layer fails.On Unix (line 1168),
Writable::initcallsresult.unwrap()onOption<Fd>. IfStdio::ReadableStreamdoes not produce a file descriptor, this panics.Both paths panic on user-reachable spawn failures. Return
WritableInitError::Sysinstead, propagating any spawn-layer error or a new error variant for unexpected result shapes.🤖 Prompt for 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. In `@src/runtime/shell/subproc.rs` around lines 842 - 844, Update the Stdio::ReadableStream initialization flow around Writable::init so unexpected result shapes and spawn-layer failures return recoverable WritableInitError::Sys errors instead of reaching unreachable! or unwrap() panics. Cover both the Windows handling near the Writable::Pipe match and the Unix Option<Fd> path, propagating existing errors and introducing an error variant only if needed for unexpected results.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@src/runtime/shell/subproc.rs`:
- Around line 738-740: Update the process cleanup path around
ShellSubprocess::try_kill to capture the result of Process::kill instead of
discarding it. Preserve non-ESRCH termination errors and combine or retain them
with the original error before invoking abort_after_failed_start or completing
process teardown, while keeping expected ESRCH handling unchanged.
- Around line 842-844: Update the Stdio::ReadableStream initialization flow
around Writable::init so unexpected result shapes and spawn-layer failures
return recoverable WritableInitError::Sys errors instead of reaching
unreachable! or unwrap() panics. Cover both the Windows handling near the
Writable::Pipe match and the Unix Option<Fd> path, propagating existing errors
and introducing an error variant only if needed for unexpected results.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3a87ee04-deb2-48be-b012-c13bfbc55546
📒 Files selected for processing (1)
src/runtime/shell/subproc.rs
|
Re: the two outside-diff findings on 0a1fc5b: 738-740 (discard kill error): the 842-844 ( |
… path A fresh Bun.file(path).stream() is in the Lazy::Blob state, so to_any_blob in Cmd::init_subproc_redirections collapses it to Stdio::Blob (the StaticPipeWriter path), not Stdio::ReadableStream. The subprocess-stdout and fetch-body tests are what exercise the wire_native_sink FileReader and ByteStream arms.
There was a problem hiding this comment.
No new findings — the earlier inline comments (child-process teardown on Writable::init failure, Windows uv-pipe cleanup on the Sys(e) path, dead UnexpectedCreatingStdin, and the Bun.file().stream() test naming) are all addressed. Deferring to a human because this is a nontrivial native change: new FileSink lifecycle for shell stdin, assign_to_stream wiring inside spawn_maybe_sync_impl, and new error-path teardown with a fair amount of unsafe.
What was reviewed:
Writable::initPOSIXReadableStreamarm: fd is set nonblocking,FileSink::create+ writer start mirrorsBun.spawn; error path derefs the sink and returnsSys(e).spawn_maybe_sync_implfailure paths:Sys(e)now nullsout_subproc, closes/re-seats stdout/stderr per platform, and reaps the child;assign_to_streamfailure uses the sametry_kill+abort_after_failed_startpattern as the sibling stdout/stderr start errors.on_process_exitWritable::Pipehandling:debug_assertguards the ReadableStream-only invariant;on_attached_process_exitruns beforebuffered_input_close, and the newhas_finished()re-check covers the stdout/stderr-already-closed ordering.- Tests cover native ByteStream (subprocess stdout, fetch body), the JS pump, early child exit with stream cancel, locked/disturbed rejection, and the builtin rejection.
Extended reasoning...
Overview
Implements Bun.$cmd < ${readableStream}`` (issue #18262), replacing a panic!("TODO SHELL READABLE STREAM"). Touches four files: `Cmd.rs` (redirect classification — stdin-only, locked/disturbed check, blob-collapse fast path), `subproc.rs` (~160 lines: new POSIX `Writable::init` arm building a `FileSink`, `assign_to_stream` wiring after subprocess construction, `on_process_exit` handling for the `Writable::Pipe` stdin, reshaped `WritableInitError`, and a new post-spawn error teardown that kills/reaps the child and cleans up stdout/stderr on both platforms), `Builtin.rs` (7-line rejection for ReadableStream → builtin), and 9 new tests in `bunshell.test.ts`.
Security risks
None identified. No parsing of untrusted input, no auth/crypto. The ReadableStream comes from the same JS realm that invoked the shell. Locked/disturbed streams are rejected up front with ERR_INVALID_STATE.
Level of scrutiny
High. This is core-runtime native code with substantial unsafe: raw *mut FileSink handling via a new FileSinkPtr::as_ptr, intrusive-refcount teardown of a spawned Process on the new error path, a self-referential SourceHandle skip, and a new on_process_exit branch that reaches back into the FileSink and the owning Cmd. It also has distinct POSIX/Windows paths. The failure modes here (leaked fds/pids, use-after-free of a FileSink whose source is a live ByteStream, hung shell command when has_finished() never fires) are exactly the categories REVIEW.md flags as most-blocked. That warrants a human pass even though automated review is clean.
Other factors
Three prior automated review rounds surfaced real issues (leaked spawned child on Writable::init failure, Windows uv-pipe cleanup, dead enum variant, mislabeled test) and all were fixed in follow-up commits d6317bf → ead44ad. The current bug-hunting run found nothing further. Test coverage is solid for the happy paths and the documented error cases; the PR body reports green on Linux debug ASAN and Windows plus rust:check-all across all targets, and a FileSink live-count delta of 0 over 20 iterations. This PR also supersedes four earlier attempts, so a maintainer should confirm this is the approach they want to land.
|
Updated 6:54 PM PT - Aug 4th, 2026
✅ @robobun, your commit b7363dfd0fb8fdf57622f699b8b48d01b710e530 passed in 🧪 To try this PR locally: bunx bun-pr 36895That installs a local version of the PR into your bun-36895 --bun |
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
Can you refactor this to not need unsafe and be sound?
- FileSinkPtr: safe create/create_with_pipe constructors (NonNull::new + expect over the non-null FileSink::create* postcondition) replace unsafe adopt; delete as_ptr; add a safe on_attached_process_exit wrapper whose single unsafe call is justified by the handle's RAII invariant (canonical pointer + owned ref). - Writable::init now owns the whole ReadableStream stdin setup, including assign_to_stream (mirrors Bun.spawn's Writable::init). The post-construction assign block in spawn_maybe_sync_impl, its raw (*subprocess).stdin access, the unreachable!, and the try_kill/abort_after_failed_start arm are deleted. Sync assign throws surface as WritableInitError::StreamAssign. - spawn_maybe_sync_impl publishes *out_subproc only after the Subprocess is fully initialised (still before watch/start/read_all, which are the first points a re-entrant callback can fire), so the error arm no longer needs to null a dangling pointer. - Writable::init failure teardown moved onto the spawn-result types as dispose_failed_spawn. The POSIX impl is safe code (raw kill on the pid because Process::kill no-ops while Poller::Detached, blocking wait4, pidfd close); this also fixes the previous arm, whose Process::kill call was a silent no-op pre-watch and whose blocking wait could therefore hang. Windows consumes the result so Drop uv_closes any stdio pipes still held. - on_process_exit: safe FileSinkPtr method + on_static_pipe_writer_done (the existing stdin-done path) instead of raw FileSink/cmd_mut calls; exit re-dispatch rewritten as a match without unwrap. - test: pin the sync assign-failure path (direct stream whose pull() throws).
|
Refactored in cfdfa8d (plus comment trims in 109c340) to drop the call-site unsafe:
Remaining unsafe in the diff: the relocated pre-existing Verified: all 11 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/spawn/process.rs`:
- Around line 1649-1672: The spawn cleanup path must return typed errors instead
of discarding failures. In src/spawn/process.rs:1649-1672, update
SpawnResultExt::dispose_failed_spawn and its POSIX implementation to report
kill, wait4, and pidfd-close failures, retry interrupted waits until the child
is reaped, and preserve the original stream-assignment error while attaching or
propagating cleanup failure. In src/spawn/process.rs:1479-1487, handle
Process::kill failure before releasing the Windows process handle and return the
combined typed cleanup result.
🪄 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: c7e1a0d1-7a0c-47a6-b819-cafaf6bce32c
📒 Files selected for processing (3)
src/runtime/shell/subproc.rssrc/spawn/process.rstest/js/bun/shell/bunshell.test.ts
Shell stdin is never Stdio::Pipe (InKind::to_subproc_stdio yields only Fd/Ignore; init_subproc_redirections never sets it), so Writable::Pipe for shell stdin is exclusively the Stdio::ReadableStream path, whose source is wired by Writable::init. The gated ShellWritable block in spawn_maybe_sync_impl could therefore never run. Delete it along with its only downstream consumers: SourceHandle::ShellWritable and the shell Writable::on_close, plus the now-vacuous debug_assert in on_process_exit.
…value fallibly A synchronously-thrown non-Error value comes back from assign_to_stream as the raw value, which to_error() does not recognize, so the failed assignment was treated as success and the child hung waiting on stdin. Classify the result by its success shapes instead (undefined/null/empty or a promise); anything else, or a pending exception, is a throw. Formatting the thrown value ran user toString() inside format!(), which panics when the Display impl returns Err; write via fmt::Write (which propagates the Err), fall back to the bare message, and clear the pending exception the throwing toString() left behind. Also relabel the FileSinkPtr SAFETY comments that still referenced the deleted adopt().
There was a problem hiding this comment.
No open findings — the earlier issues (dead ShellWritable wiring, the format!-over-fmt_string panic on a throwing toString(), and the stale adopt-contract SAFETY comments) are all resolved in b7363df. Deferring to a human because this is a substantive lifecycle change across the shell subprocess spawn path, FileSink ownership, and cross-platform process teardown — worth a maintainer's eyes on the on_process_exit re-dispatch and the new dispose_failed_spawn reap paths.
What was reviewed:
Writable::assign_streamresult classification and the falliblefmt::Writepath — the throwing-toString()case now clears the exception and falls back to a fixed message (covered by the three newpull()-throws tests).on_process_exit's newSome(prev) if cmd.has_finished()arm — confirmed it only re-fireson_exitwhen the stdin close just satisfiedhas_finished(), and thatWritable::Pipeis now reachable only viaStdio::ReadableStream(the sole shell producer).dispose_failed_spawn(POSIX) —posix_spawn::wait4retries EINTR internally; the best-effortkill/pidfd-close discards match the four siblingtry_killerror paths in this file.SourceHandle::ShellWritabledeletion — grepped every match arm instreams.rs; no remaining consumers.
Extended reasoning...
Overview
This PR replaces the panic!("TODO SHELL READABLE STREAM") in Bun.$ with real support for < ${readableStream} stdin redirection. It touches six files: Cmd.rs (redirect validation + blob-collapse fast path), Builtin.rs (catchable error when the target is a builtin), subproc.rs (~200 net-line rewrite of FileSinkPtr, Writable::init, a new Writable::assign_stream, on_process_exit stdin-close + completion re-check, and the Writable::init-failure teardown arm), streams.rs (deletes the now-dead SourceHandle::ShellWritable variant and its three match arms), process.rs (new dispose_failed_spawn on PosixSpawnResult / WindowsSpawnResult), and 12 new tests in bunshell.test.ts.
Over four prior review passes I raised (and robobun addressed): the provably-dead gated ShellWritable block, the user-reachable format!("…{}", err.fmt_string(global)) panic when the thrown value's toString() throws, and stale adopt-contract SAFETY comments after the constructor refactor. The final commit b7363df resolves all of them; this run's bug hunt found nothing.
Security risks
Low. Input surface is a JS ReadableStream the user already constructed; the only user-controlled data reaching native code is the stream's chunks (piped verbatim to the child's stdin, same as Bun.spawn) and the thrown value's toString() (now formatted fallibly with the pending exception cleared). Locked/disturbed streams throw ERR_INVALID_STATE before spawn. No path parsing, no shell metacharacter handling, no privilege boundaries crossed.
Level of scrutiny
High. This is subprocess-lifecycle and memory-ownership code: raw *mut FileSink behind an RAII wrapper, Arc interior-mutability via arc_as_mut_ptr, a re-entrant on_process_exit that now calls back into the FileSink (which can dispatch SourceHandle::on_close) while a &mut ShellSubprocess is on the stack, a new POSIX teardown path that bypasses Process::kill to signal the pid directly and blocking-wait4 it, and a Windows arm that hands stdio pipes back to WindowsSpawnResult::drop for uv_close. The on_process_exit completion match also changes when cmd.on_exit fires — a subtle ordering change that affects every shell subprocess, not just the ReadableStream path. These are exactly the areas REVIEW.md flags as most-blocked (paired acquisition/release, refcount balance on every terminal path, re-entrancy while holding raw pointers).
Other factors
- Test coverage is thorough for the feature (native ByteStream, 256 KiB backpressure, blob-collapse, fetch body, JS pump, early child exit + stream cancel, locked/disturbed rejection, stdout/stderr rejection, builtin rejection, three sync-throw variants including throwing
toString()), and the PR reports fullbunshell.test.ts+spawn-stdin-readable-stream.test.tsgreen on Linux ASAN + Windows with FileSink live-count delta 0. - The PR supersedes four earlier attempts and has already been through significant iteration (comment-cop, CodeRabbit, and my prior findings all resolved).
- Still, the
dispose_failed_spawndesign (directlibc_kill+ blockingwait4beforewatch()), the deletion of a publicSourceHandleenum variant, and the re-dispatch arm inon_process_exitare architectural choices a maintainer familiar with the shell/spawn subsystems should sign off on rather than a bot.
2 participants
What
Bun.$cmd < ${readableStream}`` now streams into the child's stdin instead of panicking withTODO SHELL READABLE STREAM.How
The child's stdin pipe is wrapped in a
FileSinkand handed toFileSink::assign_to_stream, which triesReadableStream::wire_native_sinkfirst. NativeByteStream/FileReadersources (proc.stdout,Bun.file().stream(),response.body) are wired directly viaSinkHandle::FileSinkwith backpressure and no JS round-trip; any other stream falls through to the existing JS pump.Cmd::init_subproc_redirections: a ReadableStream redirect is stdin-only (anything else throws). A locked or disturbed stream throwsERR_INVALID_STATE. A fully-buffered or not-yet-started file-backed stream collapses to a blob and takes the existingStaticPipeWriterpath; otherwiseStdio::ReadableStreamis set.Writable::init(POSIX): creates aFileSinkaround the stdin pipe fd and starts its writer (mirrorsBun.spawn). Windows already did the equivalent forStdio::Pipe | Stdio::ReadableStream.spawn_maybe_sync_impl: after the subprocess is constructed, the stream is wired in viaassign_to_stream. TheSourceHandle::ShellWritablebackref is skipped for ReadableStream stdin (the sink'ssourcebecomes the upstreamByteStream/FileReader, or staysNonefor the JS path), soFileSink::on_closenever reassignsself.stdin. A synchronous assignment failure kills the child, aborts throughabort_after_failed_start, and fails the command with the thrown value in the message.on_process_exit: forWritable::Pipe, callsFileSink::on_attached_process_exit(cancels the stream, closes the writer;on_closedetaches the native source'sSinkHandle) and marks buffered stdin closed soCmd::has_finished()can complete. Re-checkshas_finished()when stdout/stderr closing already setexit_code.Builtin.rs: a ReadableStream redirected to a builtin throws a catchable error (builtins read stdin synchronously from a buffer).Verification
test/js/bun/shell/bunshell.test.ts(redirect stdin from ReadableStream, 9 tests): subprocess stdout (native ByteStream), a 256 KiB payload,Bun.file().stream()(blob-collapse fast path),fetch(...).body(native ByteStream), a multi-chunk JS stream, a child that exits while the stream is still producing, locked/disturbed streams, stdout/stderr redirect rejection, and the builtin rejection.All of the above plus the full
bunshell.test.ts(427 pass) andspawn-stdin-readable-stream.test.ts(34 pass) are green on a Linux debug ASAN build and on Windows.FileSinklive count is delta 0 over 20 iterations.bun run rust:check-allpasses on all 10 targets;cargo clippy -p bun_runtimeis clean.Fixes #18262
Supersedes #30550, #33996, #35314, #35551 (this is the SinkHandle-native approach; #30550 predates
wire_native_sinkinFileSink::assign_to_streamfrom #36733 and has merge conflicts).no test proof · iteration 2 · 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