Skip to content

shell: finish the command when its buffered stdin is the last stdio to close #37799

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

Open
robobun wants to merge 8 commits into
base: main
Choose a base branch
from
+264 −148
Open

shell: finish the command when its buffered stdin is the last stdio to close #37799

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
06e57ce
shell: finish the command when its buffered stdin is the last stdio t…
robobun Aug 12, 2026
c87211c
shell: drive the completion Yields from raw-pointer subprocess callbacks
robobun Aug 12, 2026
db06282
shell: trim comments on the completion callbacks
robobun Aug 12, 2026
6a92b2a
spawn: document that on_close_io may free the process; pin the shell'…
robobun Aug 12, 2026
b794692
spawn: fold the on_close_io contract into its safety line
robobun Aug 12, 2026
ede94fd
shell tests: keep the stdin helper alive under BUN_FEATURE_FLAG_NO_OR…
robobun Aug 12, 2026
4372d29
shell tests: release the stdin helper and join the command on the fai…
robobun Aug 12, 2026
5b16549
ci: retrigger
robobun Aug 12, 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
97 changes: 35 additions & 62 deletions src/runtime/shell/states/Cmd.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,12 +68,7 @@ impl Cmd {
pub struct SubprocExec {
pub(crate) child: *mut ShellSubprocess,
pub(crate) buffered_closed: BufferedIoClosed,
/// NodeId-arena backrefs so the legacy `&mut self` subprocess callbacks
/// (`buffered_output_close` / `on_exit`) can hand a [`Yield`] back to the
/// trampoline. The `Cmd` lives inside `interp.nodes`, so we stash the
/// indices and
/// return `Yield::Next(this_id)` for the caller (`PipeReader::run_yield`)
/// to drive.
/// Null until the spawn has returned; gates the `Yield` built by `finish_if_done`.
pub(crate) interp: *mut Interpreter,
pub(crate) this_id: NodeId,
}
Expand Down Expand Up @@ -883,7 +878,7 @@ impl Cmd {
core::mem::take(&mut me.exec)
};
// `me`'s borrow ended above: the teardown below re-enters this Cmd
// via stdin `on_close_io` → `buffered_input_close` (a no-op once
// via stdin `on_stdin_writer_close` → `buffered_input_close` (a no-op once
// `exec` is taken).
match exec {
Exec::None => {}
Expand Down Expand Up @@ -919,11 +914,7 @@ impl Cmd {
}

// ── Subprocess callbacks (legacy `*Cmd` backref shape) ────────────────
// `ShellSubprocess` / `PipeReader` hold a `*mut Cmd` backref and call
// these via `&mut self`. The NodeId-arena port stashes `(interp, this_id)`
// on `SubprocExec` so the resulting `Yield` can be driven by the caller's
// `PipeReader::run_yield` without aliasing `&Interpreter` against
// `&mut self`.
// Each returns its Yield: running it can free this Cmd and its subprocess.

/// True once the command has both an exit code and (for subprocesses)
/// all buffered stdio closed.
Expand All @@ -939,11 +930,13 @@ impl Cmd {
}
}

/// Mark the subprocess's buffered stdin as closed.
pub(crate) fn buffered_input_close(&mut self) {
if let Exec::Subproc(sub) = &mut self.exec {
sub.buffered_closed.close_stdin();
}
/// Mark the subprocess's buffered stdin as closed; this may be the close that finishes it.
pub(crate) fn buffered_input_close(&mut self) -> Yield {
let Exec::Subproc(sub) = &mut self.exec else {
return Yield::suspended();
};
sub.buffered_closed.close_stdin();
self.finish_if_done()
}

/// Mark the subprocess's buffered stdout/stderr as closed (flushing the
Expand All @@ -958,27 +951,32 @@ impl Cmd {
OutKind::Stdout => self.buffered_output_close_stdout(err),
OutKind::Stderr => self.buffered_output_close_stderr(err),
}
if self.has_finished() {
// Set `state = Done` and hand the Yield back to the caller
// (`PipeReader::run_yield`), which drives the trampoline with the
// `*mut Interpreter` it already holds, landing in `Cmd::next` →
// `CmdState::Done` → `interp.child_done(...)`.
self.state = CmdState::Done;
let (interp, this_id) = match &self.exec {
Exec::Subproc(sub) => (sub.interp, sub.this_id),
// Only the subprocess path calls this; builtin output goes
// through `Builtin::done` → `on_exec_done`.
_ => return Yield::suspended(),
};
// Same gate as `on_exit`: `exec.interp` stays null until the spawn
// returns, so a Yield run here would reach `Cmd::deinit` and free the
// `ShellSubprocess` still on the spawn frame. `transition_to_exec` resumes.
if interp.is_null() {
return Yield::suspended();
}
return Yield::Next(this_id);
self.finish_if_done()
}

/// Called by `ShellSubprocess::on_process_exit`.
pub(crate) fn on_exit(&mut self, exit_code: ExitCode) -> Yield {
log!("cmd exit code={}", exit_code);
self.exit_code = Some(exit_code);
self.finish_if_done()
}

/// `Done` once the exit code and every piped stdio are in; the caller runs the Yield.
fn finish_if_done(&mut self) -> Yield {
if !self.has_finished() {
return Yield::suspended();
}
Yield::suspended()
self.state = CmdState::Done;
let (interp, this_id) = match &self.exec {
Exec::Subproc(sub) => (sub.interp, sub.this_id),
// Builtins finish through `Builtin::done` → `on_exec_done` instead.
_ => return Yield::suspended(),
};
// Still inside the spawn (see `transition_to_exec`, which resumes from `state`).
if interp.is_null() {
return Yield::suspended();
}
Yield::Next(this_id)
}

fn buffered_output_close_stdout(&mut self, err: Option<bun_sys::SystemError>) {
Expand Down Expand Up @@ -1050,31 +1048,6 @@ impl Cmd {
);
child.close_io(StdioKind::Stderr);
}

/// Called by `ShellSubprocess::on_process_exit`.
pub(crate) fn on_exit(&mut self, exit_code: ExitCode) {
self.exit_code = Some(exit_code);
let has_finished = self.has_finished();
log!("cmd exit code={} has_finished={}", exit_code, has_finished);
if has_finished {
self.state = CmdState::Done;
// `self` lives inside `interp.nodes`, so resume via the stashed
// backrefs.
let (interp, this_id) = match &self.exec {
Exec::Subproc(sub) => (sub.interp, sub.this_id),
_ => return,
};
if interp.is_null() {
return;
}
// SAFETY: `interp` outlives every spawned subprocess (it owns the
// arena slot containing `self`). `&mut self` is dead by NLL after
// this point so the `&Interpreter` borrow does not alias it.
// The caller (`ShellSubprocess::on_process_exit`) does not touch
// its `*mut Cmd` again after this returns.
Yield::Next(this_id).run(unsafe { &*interp });
}
}
}

fn set_stdio_from_redirect(stdio: &mut [Stdio; 3], flags: ast::RedirectFlags, fd: bun_sys::Fd) {
Expand Down
171 changes: 91 additions & 80 deletions src/runtime/shell/subproc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,7 @@ use std::sync::Arc;

#[cfg(unix)]
use crate::api::bun::process::SpawnResultExt as _;
use crate::api::bun::process::{
self as bun_process, Process, Rusage, SignalCodeExt, SpawnOptions, Status,
};
use crate::api::bun::process::{self as bun_process, Process, SignalCodeExt, SpawnOptions, Status};
#[cfg(windows)]
use crate::api::bun::process::{WindowsOptions, WindowsStdioResult};
use crate::api::bun::subprocess as JscSubprocess;
Expand Down Expand Up @@ -276,15 +274,19 @@ impl JscSubprocess::static_pipe_writer::StaticPipeWriterProcess for ShellSubproc
const POLL_OWNER_TAG: bun_io::PollTag =
bun_io::posix_event_loop::poll_tag::SHELL_STATIC_PIPE_WRITER;
unsafe fn on_close_io(this: *mut Self, kind: StdioKind) {
// SAFETY: caller (StaticPipeWriter) guarantees `this` is live.
unsafe { (*this).on_close_io(kind) }
// `Writable::init` only ever creates the writer for stdin.
debug_assert!(matches!(kind, StdioKind::Stdin));
// SAFETY: `StaticPipeWriter::on_close` passes its live process backref and does not
// touch it afterwards. Forwarded raw (not autoref'd): the callee may free `*this`.
unsafe { Self::on_stdin_writer_close(this) }
}
}

bun_spawn::link_impl_ProcessExit! {
Shell for ShellSubprocess => |this| {
on_process_exit(process, status, rusage) =>
(*this).on_process_exit(&*process, &status, rusage),
// Forwarded raw, not autoref'd: the callee may free `*this`.
on_process_exit(_process, status, _rusage) =>
ShellSubprocess::on_process_exit(this, &status),
}
}

Expand All @@ -299,15 +301,42 @@ impl ShellSubprocess {
unsafe { &mut *self.process }
}

pub(crate) fn on_static_pipe_writer_done(&mut self) {
/// The `< ${buffer}` stdin writer closed: release it and let the `Cmd` finish.
///
/// # Safety
/// `this` must be live and unborrowed. It may be freed (by `Cmd::deinit`, reached
/// through the Yield run here) by the time this returns, which is why it is raw.
Comment thread
robobun marked this conversation as resolved.
unsafe fn on_stdin_writer_close(this: *mut Self) {
{
// SAFETY: caller contract; the borrow ends with this block, before the Yield runs.
let slot = unsafe { &mut (*this).stdin };
match core::mem::replace(slot, Writable::Ignore) {
Writable::Buffer(buffer) => {
// Releases `create()`'s ref; `start()`'s keeps the writer alive until
// this callback returns.
// SAFETY: single-threaded; sole borrow of the payload.
unsafe { buffer_mut(&buffer) }.source.detach();
buffer.deref();
}
// The writer only exists while the slot holds it.
other => {
*slot = other;
return;
}
}
}
// SAFETY: caller contract; `CmdHandle` is `Copy`, no borrow is kept.
let handle = unsafe { (*this).cmd_parent };
log!(
"Subproc(0x{:x}) onStaticPipeWriterDone(cmd={})",
std::ptr::from_mut(self) as usize,
self.cmd_parent.id
"Subproc(0x{:x}) onStdinWriterClose(cmd={})",
this as usize,
handle.id
);
// SAFETY: cmd_parent backref resolves to the owning Cmd which outlives
// the subprocess (freed only in `Cmd::deinit` after all stdio closes).
unsafe { self.cmd_parent.cmd_mut() }.buffered_input_close();
// SAFETY: the owning Cmd outlives its subprocess (its `deinit` is what frees it);
// the `&mut Cmd` ends before the Yield runs.
let y = unsafe { handle.cmd_mut() }.buffered_input_close();
// May free `*this`.
y.run(&handle.interp);
}

pub(crate) fn has_exited(&self) -> bool {
Expand Down Expand Up @@ -392,62 +421,37 @@ impl ShellSubprocess {
self.close_io(StdioKind::Stderr);
}

/// A stdout/stderr `PipeReader` finished: swap it out of its slot for its buffered bytes.
pub(crate) fn on_close_io(&mut self, kind: StdioKind) {
match kind {
StdioKind::Stdin => match &mut self.stdin {
Writable::Pipe(pipe) => {
// DerefMut on the owning `&mut FileSinkPtr` encapsulates
// the access.
pipe.source.with_mut(|s| s.clear());
// FileSinkPtr::drop derefs.
self.stdin = Writable::Ignore;
}
Writable::Buffer(_) => {
self.on_static_pipe_writer_done();
// RefPtr has no Drop — move it out before reassigning so the
// create ref is actually released.
if let Writable::Buffer(buffer) =
core::mem::replace(&mut self.stdin, Writable::Ignore)
{
// SAFETY: single-threaded; sole borrow of the payload.
unsafe { buffer_mut(&buffer) }.source.detach();
buffer.deref();
}
}
_ => {}
},
StdioKind::Stdout | StdioKind::Stderr => {
let out: &mut Readable = match kind {
StdioKind::Stdout => &mut self.stdout,
StdioKind::Stderr => &mut self.stderr,
StdioKind::Stdin => unreachable!(),
};
if let Readable::Pipe(pipe) = core::mem::replace(out, Readable::Ignore) {
// The only callers reach here from inside
// `PipeReader::on_reader_done`/`on_reader_error`, which still
// hold a raw `*mut PipeReader` to this same allocation.
// Route every read/write through `Arc::as_ptr` (no `Deref`)
// so we never materialise a `&PipeReader` that would alias
// those callers' access; see `PipeReader::take_done_buffer`.
let pp = Arc::as_ptr(&pipe).cast_mut();
// SAFETY: `pp` projects from the Arc allocation's NonNull;
// raw place read of the discriminant + raw-ptr write
// through `take_done_buffer` (see its doc).
let buf = unsafe {
if matches!(&(*pp).state, PipeReaderState::Done(_)) {
Some(PipeReader::take_done_buffer(pp))
} else {
None
}
};
if let Some(buf) = buf {
*out = Readable::Buffer(buf);
} else {
*out = Readable::Ignore;
}
drop(pipe); // deref
let out: &mut Readable = match kind {
StdioKind::Stdout => &mut self.stdout,
StdioKind::Stderr => &mut self.stderr,
StdioKind::Stdin => unreachable!("stdin closes through on_stdin_writer_close"),
};
if let Readable::Pipe(pipe) = core::mem::replace(out, Readable::Ignore) {
// The only callers reach here from inside
// `PipeReader::on_reader_done`/`on_reader_error`, which still
// hold a raw `*mut PipeReader` to this same allocation.
// Route every read/write through `Arc::as_ptr` (no `Deref`)
// so we never materialise a `&PipeReader` that would alias
// those callers' access; see `PipeReader::take_done_buffer`.
Comment thread
robobun marked this conversation as resolved.
let pp = Arc::as_ptr(&pipe).cast_mut();
// SAFETY: `pp` projects from the Arc allocation's NonNull;
// raw place read of the discriminant + raw-ptr write
// through `take_done_buffer` (see its doc).
let buf = unsafe {
if matches!(&(*pp).state, PipeReaderState::Done(_)) {
Some(PipeReader::take_done_buffer(pp))
} else {
None
}
};
if let Some(buf) = buf {
*out = Readable::Buffer(buf);
} else {
*out = Readable::Ignore;
}
drop(pipe); // deref
}
}

Expand Down Expand Up @@ -503,11 +507,11 @@ impl ShellSubprocess {
/// # Safety
/// `this` must be the live `heap::alloc`'d subprocess with no outstanding
/// borrows; single-threaded shell. Raw (not `&mut self`) because the
/// stdin close re-enters `on_close_io(&mut Self)` through the writer's
/// stdin close re-enters `on_stdin_writer_close` through the writer's
/// process backref.
#[cfg(not(windows))]
pub(crate) unsafe fn deinit_in_flight_io(this: *mut Self) {
// Claim `start()`'s +1, `close()` (fires `on_close` → `on_close_io`:
// Claim `start()`'s +1, `close()` (fires `on_close` → `on_stdin_writer_close`:
// slot → `Ignore`, `create()`'s ref released), release the claimed
// ref — the JS `Subprocess::close_io` stdin shape.
// SAFETY: caller contract; the `stdin` borrow ends before `close()`.
Expand Down Expand Up @@ -958,8 +962,13 @@ impl ShellSubprocess {
Ok(())
}

pub(crate) fn on_process_exit(&mut self, _: &Process, status: &Status, _: &Rusage) {
log!("onProcessExit({:x})", std::ptr::from_mut(self) as usize);
/// Exit handler (`link_impl_ProcessExit!` above).
///
/// # Safety
/// Same contract as [`Self::on_stdin_writer_close`]: `this` must be live and unborrowed,
/// and may be freed by the time this returns.
Comment thread
robobun marked this conversation as resolved.
unsafe fn on_process_exit(this: *mut Self, status: &Status) {
log!("onProcessExit({:x})", this as usize);
let exit_code: Option<u8> = 'brk: {
if let Status::Exited(exited) = &status {
break 'brk Some(exited.code);
Expand All @@ -978,16 +987,18 @@ impl ShellSubprocess {
break 'brk None;
};

if let Some(code) = exit_code {
let handle = self.cmd_parent;
// SAFETY: cmd_parent backref outlives subprocess; resolved
// through the node arena so it survives `Vec<Node>` reallocation.
// `&mut self` is dead by NLL before `on_exit` re-enters interp.
let cmd = unsafe { handle.cmd_mut() };
if cmd.exit_code.is_none() {
cmd.on_exit(code.into());
}
let Some(code) = exit_code else { return };
// SAFETY: caller contract; `CmdHandle` is `Copy`, no borrow is kept.
let handle = unsafe { (*this).cmd_parent };
// SAFETY: the owning Cmd outlives its subprocess (its `deinit` is what frees it);
// the `&mut Cmd` ends before the Yield runs.
let cmd = unsafe { handle.cmd_mut() };
if cmd.exit_code.is_some() {
return;
}
let y = cmd.on_exit(code.into());
// May free `*this`.
y.run(&handle.interp);
}
}

Expand Down
Loading