Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion brush-core/src/commands.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Expand Down
3 changes: 3 additions & 0 deletions brush-core/src/options.rs
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,8 @@ 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 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<usize>,
}
Expand Down Expand Up @@ -229,6 +231,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,
Expand Down
6 changes: 6 additions & 0 deletions brush-core/src/shell/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,12 @@ pub struct CreateOptions<SE: extensions::ShellExtensions = extensions::DefaultSh
/// Whether to launch external commands as session leaders.
#[builder(default)]
pub external_cmd_leads_session: bool,
/// Whether externally spawned commands should be killed when their spawning
/// shell is dropped. Defaults to `false` (children outlive the shell, as a
/// real shell requires); enable for embedded shells, where a surviving child
/// leaks and can hold the shell's output pipe open past teardown.
#[builder(default)]
pub kill_external_commands_on_drop: bool,
/// Initial working dir for the shell. If left unspecified, will be populated from
/// the host environment.
pub working_dir: Option<PathBuf>,
Expand Down
7 changes: 6 additions & 1 deletion brush-core/src/sys/stubs/process.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,12 @@ impl Child {
}
}

pub(crate) fn spawn(mut command: std::process::Command) -> std::io::Result<Child> {
pub(crate) fn spawn(
mut command: std::process::Command,
kill_on_drop: bool,
) -> std::io::Result<Child> {
// No kill-on-drop on this stub platform; accepted and ignored.
let _ = kill_on_drop;
let child = command.spawn()?;
Ok(Child { inner: child })
}
4 changes: 3 additions & 1 deletion brush-core/src/sys/tokio_process.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@
pub(crate) type ProcessId = i32;
pub(crate) use tokio::process::Child;

pub(crate) fn spawn(command: std::process::Command) -> std::io::Result<Child> {
// `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<Child> {
let mut command = tokio::process::Command::from(command);
command.kill_on_drop(kill_on_drop);
command.spawn()
}
161 changes: 161 additions & 0 deletions brush-core/tests/kill_on_drop_tests.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,161 @@
//! 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.
//!
//! 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.

#![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<u32> {
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(), &params)
.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::<u32>().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(())
}
Loading