From 2f3cb70b33e335f566aa525538277c11a8070b43 Mon Sep 17 00:00:00 2001 From: hartsock Date: Sun, 19 Jul 2026 20:54:50 -0400 Subject: [PATCH 1/2] feat(core): add opt-in kill-on-drop for externally spawned commands `sys::process::spawn` never sets `kill_on_drop`, so a spawned child outlives the shell that started it. That is correct for a real shell -- it is what makes job control, disowned jobs, and `nohup`-style usage work -- and remains the default. It is wrong for an embedded shell, where the host creates and destroys the shell as an object. There a surviving child keeps running unattended and holds a duplicate of the shell's stdout/stderr pipe, so a host draining that output never observes EOF. Add a `kill_external_commands_on_drop` creation option, defaulting to `false`, plumbed through `RuntimeOptions` to `sys::process::spawn` alongside the existing `external_cmd_leads_session`. The stub backend accepts and ignores it. Process-group behavior is untouched, so this signals the immediate child only. Assisted-by: Claude Code:claude-opus-4-8 --- brush-core/src/commands.rs | 2 +- brush-core/src/options.rs | 6 + brush-core/src/shell/builder.rs | 16 +++ brush-core/src/sys/stubs/process.rs | 8 +- brush-core/src/sys/tokio_process.rs | 13 +- brush-core/tests/kill_on_drop_tests.rs | 179 +++++++++++++++++++++++++ 6 files changed, 221 insertions(+), 3 deletions(-) create mode 100644 brush-core/tests/kill_on_drop_tests.rs diff --git a/brush-core/src/commands.rs b/brush-core/src/commands.rs index 0bd9c3b30..64e0505a8 100644 --- a/brush-core/src/commands.rs +++ b/brush-core/src/commands.rs @@ -633,7 +633,7 @@ pub(crate) fn execute_external_command( .join(" ") ); - match sys::process::spawn(cmd) { + match sys::process::spawn(cmd, context.shell.options().kill_external_commands_on_drop) { Ok(child) => { // Retrieve the pid. #[expect(clippy::cast_possible_wrap)] diff --git a/brush-core/src/options.rs b/brush-core/src/options.rs index a2b2e5c6c..ee52bdaaa 100644 --- a/brush-core/src/options.rs +++ b/brush-core/src/options.rs @@ -198,6 +198,11 @@ pub struct RuntimeOptions { pub sh_mode: bool, /// Whether to treat external commands as session leaders. pub external_cmd_leads_session: bool, + /// Whether externally spawned commands should be killed when the shell that + /// spawned them is dropped. Defaults to `false` (a child outlives its + /// shell); see + /// [`CreateOptions::kill_external_commands_on_drop`](crate::CreateOptions::kill_external_commands_on_drop). + pub kill_external_commands_on_drop: bool, /// Maximum function call depth. pub max_function_call_depth: Option, } @@ -229,6 +234,7 @@ impl RuntimeOptions { treat_unset_variables_as_error: create_options.treat_unset_variables_as_error, exit_on_nonzero_command_exit: create_options.exit_on_nonzero_command_exit, external_cmd_leads_session: create_options.external_cmd_leads_session, + kill_external_commands_on_drop: create_options.kill_external_commands_on_drop, login_shell: create_options.login, disable_filename_globbing: create_options.disable_pathname_expansion, remember_command_locations: true, diff --git a/brush-core/src/shell/builder.rs b/brush-core/src/shell/builder.rs index 3f99a9249..b7e89aca5 100644 --- a/brush-core/src/shell/builder.rs +++ b/brush-core/src/shell/builder.rs @@ -182,6 +182,22 @@ pub struct CreateOptions, diff --git a/brush-core/src/sys/stubs/process.rs b/brush-core/src/sys/stubs/process.rs index ac44aad07..eb5d47dd7 100644 --- a/brush-core/src/sys/stubs/process.rs +++ b/brush-core/src/sys/stubs/process.rs @@ -28,7 +28,13 @@ impl Child { } } -pub(crate) fn spawn(mut command: std::process::Command) -> std::io::Result { +pub(crate) fn spawn( + mut command: std::process::Command, + kill_on_drop: bool, +) -> std::io::Result { + // This stub platform has no process reaping to speak of; `std::process::Child` + // has no kill-on-drop facility, so the request is accepted and ignored. + let _ = kill_on_drop; let child = command.spawn()?; Ok(Child { inner: child }) } diff --git a/brush-core/src/sys/tokio_process.rs b/brush-core/src/sys/tokio_process.rs index 3de0e38f6..65a89fd36 100644 --- a/brush-core/src/sys/tokio_process.rs +++ b/brush-core/src/sys/tokio_process.rs @@ -3,7 +3,18 @@ pub(crate) type ProcessId = i32; pub(crate) use tokio::process::Child; -pub(crate) fn spawn(command: std::process::Command) -> std::io::Result { +/// Spawns the given command. +/// +/// # Arguments +/// +/// * `command` - The command to spawn. +/// * `kill_on_drop` - Whether the spawned process should be killed when the +/// returned [`Child`] handle is dropped. See +/// [`CreateOptions::kill_external_commands_on_drop`](crate::CreateOptions::kill_external_commands_on_drop) +/// for when this is appropriate; it is `false` for ordinary shells, which must +/// outlive their children. +pub(crate) fn spawn(command: std::process::Command, kill_on_drop: bool) -> std::io::Result { let mut command = tokio::process::Command::from(command); + command.kill_on_drop(kill_on_drop); command.spawn() } diff --git a/brush-core/tests/kill_on_drop_tests.rs b/brush-core/tests/kill_on_drop_tests.rs new file mode 100644 index 000000000..096846cd1 --- /dev/null +++ b/brush-core/tests/kill_on_drop_tests.rs @@ -0,0 +1,179 @@ +//! Integration tests for the opt-in `kill_external_commands_on_drop` creation +//! option. +//! +//! By default a spawned child outlives the shell that spawned it. That is the +//! only correct behavior for a real shell — job control, disowned jobs, and +//! `nohup`-style usage all depend on it — and these tests pin it as the +//! default. An *embedded* shell has the opposite requirement: there the shell +//! is an object the host creates and destroys, so a child that survives +//! teardown is a leak. It keeps running unattended, and it holds a duplicate of +//! the shell's stdout/stderr pipe, so a host draining that output never sees +//! EOF. +//! +//! # What owns a spawned child +//! +//! These tests tear down the whole runtime rather than just dropping the +//! `Shell`, because that is what actually reaches a running child. A +//! backgrounded command (`cmd &`) is executed by a detached `tokio` task +//! operating on a *clone* of the shell, and the resulting `ChildProcess` is +//! owned by that task — not by the spawning shell's job table, which only holds +//! the task's `JoinHandle`. Dropping a `JoinHandle` detaches rather than +//! aborts, so dropping the parent shell alone leaves such a child untouched. +//! Shutting the runtime down drops the tasks, which drops the `Child`, which is +//! where `kill_on_drop` takes effect. +//! +//! The shell under test has no builtins registered; everything it runs here is +//! an external command named by absolute path. + +#![cfg(unix)] +#![cfg(test)] +#![allow(clippy::panic_in_result_fn, clippy::expect_used)] + +use std::time::{Duration, Instant}; + +use anyhow::Result; + +/// How long to wait for a polled condition to come true. +const TIMEOUT: Duration = Duration::from_secs(10); + +/// Returns whether a process is still running. +/// +/// A killed-but-unreaped process lingers as a zombie, and `kill -0` reports +/// success for one, so this inspects the process *state* and treats `Z` as not +/// running. +fn is_running(pid: u32) -> bool { + let output = std::process::Command::new("ps") + .args(["-o", "state=", "-p", pid.to_string().as_str()]) + .output() + .expect("failed to run `ps`"); + + let state = String::from_utf8_lossy(&output.stdout); + let state = state.trim(); + !state.is_empty() && !state.starts_with('Z') +} + +/// Polls `condition` until it holds, returning whether it did within [`TIMEOUT`]. +fn poll_until(mut condition: impl FnMut() -> bool) -> bool { + let deadline = Instant::now() + TIMEOUT; + loop { + if condition() { + return true; + } + if Instant::now() >= deadline { + return false; + } + std::thread::sleep(Duration::from_millis(25)); + } +} + +/// Builds a shell with the option set as requested, backgrounds a long-running +/// external child, tears the whole runtime down, and returns the child's pid so +/// the caller can check whether it survived. +fn spawn_child_then_tear_down(kill_on_drop: bool) -> Result { + let dir = tempfile::tempdir()?; + let pid_file = dir.path().join("pid"); + + // A multi-threaded runtime is required: the backgrounded command runs on a + // separate task, and the pid poll below blocks a worker. + let runtime = tokio::runtime::Builder::new_multi_thread() + .worker_threads(2) + .enable_io() + .build()?; + + // `/bin/sh` is the shell's direct child and `exec`s into `sleep`, so the pid + // it reports is the process the shell holds a handle to. No brush builtin is + // involved: the child is named by absolute path, and the inner `PATH` + // assignment is interpreted by `sh` itself. + let script = std::format!( + "/bin/sh -c 'PATH=/bin:/usr/bin; echo $$ > {}; exec sleep 300' &", + pid_file.display() + ); + + let pid = runtime.block_on(async { + let mut shell = brush_core::Shell::builder() + .do_not_inherit_env(true) + .skip_well_known_vars(true) + .kill_external_commands_on_drop(kill_on_drop) + .build() + .await?; + + let params = shell.default_exec_params(); + shell + .run_string(script.as_str(), &brush_core::SourceInfo::default(), ¶ms) + .await?; + + // Wait for the child to report its own pid. + let mut pid = None; + poll_until(|| { + pid = std::fs::read_to_string(pid_file.as_path()) + .ok() + .and_then(|contents| contents.trim().parse::().ok()); + pid.is_some() + }); + pid.ok_or_else(|| anyhow::anyhow!("child never reported its pid")) + })?; + + assert!( + is_running(pid), + "child {pid} should be running before teardown" + ); + + // Tear everything down: this drops the task that owns the child, and with it + // the `Child` handle that carries the kill-on-drop request. + drop(runtime); + + Ok(pid) +} + +/// With the option ON, tearing the shell down reaps the child it spawned. +#[test] +fn child_is_killed_on_teardown_with_option_on() -> Result<()> { + let pid = spawn_child_then_tear_down(true)?; + + assert!( + poll_until(|| !is_running(pid)), + "with the option enabled, child {pid} should have been killed on teardown" + ); + + Ok(()) +} + +/// With the option OFF (the default), the child outlives teardown, exactly as it +/// does today. This is the behavior real shells depend on. +#[test] +fn child_outlives_teardown_by_default() -> Result<()> { + let pid = spawn_child_then_tear_down(false)?; + + // Give the child every chance to die, so a regression to + // kill-on-drop-by-default is caught rather than raced past. + std::thread::sleep(Duration::from_millis(500)); + let survived = is_running(pid); + + // This test intentionally leaves a live process behind, so clean it up + // regardless of the assertion's outcome. + let _ = std::process::Command::new("kill") + .args(["-9", pid.to_string().as_str()]) + .status(); + + assert!(survived, "by default child {pid} must outlive its shell"); + + Ok(()) +} + +/// The option must default to off, so merely adding it changes nothing for +/// existing consumers. +#[tokio::test] +async fn option_defaults_to_disabled() -> Result<()> { + let shell = brush_core::Shell::builder() + .do_not_inherit_env(true) + .skip_well_known_vars(true) + .build() + .await?; + + assert!( + !shell.options().kill_external_commands_on_drop, + "kill_external_commands_on_drop must default to disabled" + ); + + Ok(()) +} From e2b33d1da94257b51f5781014fd4f3306677e9f9 Mon Sep 17 00:00:00 2001 From: hartsock Date: Mon, 20 Jul 2026 10:54:35 -0400 Subject: [PATCH 2/2] docs: tighten kill-on-drop comments to match house style The added doc comments were multi-paragraph prose where brush uses one-line field docs. Trim each to match its neighbors; drop the `# Arguments` block on the internal `spawn` (it had no doc before); keep only the non-obvious runtime- teardown note in the test header. Assisted-by: Claude Code:claude-opus-4-8 --- brush-core/src/options.rs | 5 +---- brush-core/src/shell/builder.rs | 18 ++++------------ brush-core/src/sys/stubs/process.rs | 3 +-- brush-core/src/sys/tokio_process.rs | 11 +--------- brush-core/tests/kill_on_drop_tests.rs | 30 ++++++-------------------- 5 files changed, 13 insertions(+), 54 deletions(-) diff --git a/brush-core/src/options.rs b/brush-core/src/options.rs index ee52bdaaa..4008f9179 100644 --- a/brush-core/src/options.rs +++ b/brush-core/src/options.rs @@ -198,10 +198,7 @@ pub struct RuntimeOptions { pub sh_mode: bool, /// Whether to treat external commands as session leaders. pub external_cmd_leads_session: bool, - /// Whether externally spawned commands should be killed when the shell that - /// spawned them is dropped. Defaults to `false` (a child outlives its - /// shell); see - /// [`CreateOptions::kill_external_commands_on_drop`](crate::CreateOptions::kill_external_commands_on_drop). + /// Whether externally spawned commands are killed when their spawning shell is dropped. pub kill_external_commands_on_drop: bool, /// Maximum function call depth. pub max_function_call_depth: Option, diff --git a/brush-core/src/shell/builder.rs b/brush-core/src/shell/builder.rs index b7e89aca5..b45491c2f 100644 --- a/brush-core/src/shell/builder.rs +++ b/brush-core/src/shell/builder.rs @@ -182,20 +182,10 @@ pub struct CreateOptions std::io::Result { - // This stub platform has no process reaping to speak of; `std::process::Child` - // has no kill-on-drop facility, so the request is accepted and ignored. + // No kill-on-drop on this stub platform; accepted and ignored. let _ = kill_on_drop; let child = command.spawn()?; Ok(Child { inner: child }) diff --git a/brush-core/src/sys/tokio_process.rs b/brush-core/src/sys/tokio_process.rs index 65a89fd36..9e4edbd52 100644 --- a/brush-core/src/sys/tokio_process.rs +++ b/brush-core/src/sys/tokio_process.rs @@ -3,16 +3,7 @@ pub(crate) type ProcessId = i32; pub(crate) use tokio::process::Child; -/// Spawns the given command. -/// -/// # Arguments -/// -/// * `command` - The command to spawn. -/// * `kill_on_drop` - Whether the spawned process should be killed when the -/// returned [`Child`] handle is dropped. See -/// [`CreateOptions::kill_external_commands_on_drop`](crate::CreateOptions::kill_external_commands_on_drop) -/// for when this is appropriate; it is `false` for ordinary shells, which must -/// outlive their children. +// `kill_on_drop`: see `CreateOptions::kill_external_commands_on_drop` (false for ordinary shells). pub(crate) fn spawn(command: std::process::Command, kill_on_drop: bool) -> std::io::Result { let mut command = tokio::process::Command::from(command); command.kill_on_drop(kill_on_drop); diff --git a/brush-core/tests/kill_on_drop_tests.rs b/brush-core/tests/kill_on_drop_tests.rs index 096846cd1..a82994066 100644 --- a/brush-core/tests/kill_on_drop_tests.rs +++ b/brush-core/tests/kill_on_drop_tests.rs @@ -1,29 +1,11 @@ -//! Integration tests for the opt-in `kill_external_commands_on_drop` creation -//! option. +//! Integration tests for the opt-in `kill_external_commands_on_drop` option: +//! that a child outlives its shell by default, and is reaped when the option is set. //! -//! By default a spawned child outlives the shell that spawned it. That is the -//! only correct behavior for a real shell — job control, disowned jobs, and -//! `nohup`-style usage all depend on it — and these tests pin it as the -//! default. An *embedded* shell has the opposite requirement: there the shell -//! is an object the host creates and destroys, so a child that survives -//! teardown is a leak. It keeps running unattended, and it holds a duplicate of -//! the shell's stdout/stderr pipe, so a host draining that output never sees -//! EOF. -//! -//! # What owns a spawned child -//! -//! These tests tear down the whole runtime rather than just dropping the -//! `Shell`, because that is what actually reaches a running child. A -//! backgrounded command (`cmd &`) is executed by a detached `tokio` task -//! operating on a *clone* of the shell, and the resulting `ChildProcess` is -//! owned by that task — not by the spawning shell's job table, which only holds -//! the task's `JoinHandle`. Dropping a `JoinHandle` detaches rather than -//! aborts, so dropping the parent shell alone leaves such a child untouched. -//! Shutting the runtime down drops the tasks, which drops the `Child`, which is +//! Note: these tear down the whole runtime, not just the `Shell`. A backgrounded +//! command runs in a detached `tokio` task owning a *clone* of the shell; the +//! shell's job table holds only the task's `JoinHandle`, and dropping that +//! detaches rather than aborts. Dropping the tasks is what drops the `Child`, //! where `kill_on_drop` takes effect. -//! -//! The shell under test has no builtins registered; everything it runs here is -//! an external command named by absolute path. #![cfg(unix)] #![cfg(test)]