From 236cde79929c80016f65bce2475f55c832bf6ab6 Mon Sep 17 00:00:00 2001 From: Yuri Khrustalev Date: Tue, 15 Sep 2026 21:59:51 -0400 Subject: [PATCH 1/3] fix: don't wait forever for a child that ignores SIGTERM On a timeout, terminate_child() sent one SIGTERM and then blocked in wait() with no bound. A cargo process that ignores the signal, or that has been stopped and so never receives it, hung the whole run at that point. Wait only for a short grace period, then SIGKILL the process group. This needs a second signal, so the errno handling moves into a signal_group() helper rather than being duplicated. --- NEWS.md | 2 ++ book/src/timeouts.md | 5 +++++ src/process.rs | 38 ++++++++++++++++++++++++++++++++++---- src/process/unix.rs | 27 ++++++++++++++++++++++----- src/process/windows.rs | 6 ++++++ 5 files changed, 69 insertions(+), 9 deletions(-) diff --git a/NEWS.md b/NEWS.md index ed5bd2bb..f7619c39 100644 --- a/NEWS.md +++ b/NEWS.md @@ -2,6 +2,8 @@ ## Unreleased +- Fixed: After a timeout, cargo-mutants waits only a short grace period for the child to exit after `SIGTERM` before sending `SIGKILL`. Previously it waited indefinitely, so a process that ignored `SIGTERM`, or that had been stopped, hung the whole run. + - New: `#[mutants::exclude_re("pattern")]` attribute to exclude specific mutations by regex, without disabling all mutations on the function. The attribute can be placed on functions, `impl` blocks, `trait` blocks, modules, files, and on expressions that can carry an attribute (such as `match`, struct literals, call expressions, method calls, and unary expressions). Multiple patterns can be applied. Also supported within `cfg_attr`. Requires the [mutants](https://crates.io/crates/mutants) crate version `0.0.5` or later. - Fixed: `#[mutants::skip]` (and `#[cfg_attr(..., mutants::skip)]`) is now honoured when placed on `const` and `static` items, including associated constants in `impl` and `trait` blocks. Previously the attribute was silently ignored on these items and operator mutants inside the initializer expression were still generated ([#508](https://github.com/sourcefrog/cargo-mutants/issues/508)). diff --git a/book/src/timeouts.md b/book/src/timeouts.md index bff23255..3de52892 100644 --- a/book/src/timeouts.md +++ b/book/src/timeouts.md @@ -33,6 +33,11 @@ You can set an explicit timeouts with the `--timeout` option, also measured in s You can also set the test timeout as a multiple of the duration of the baseline test, with the `--timeout-multiplier` option and the `timeout_multiplier` configuration key. The multiplier only has an effect if the baseline is not skipped and if `--timeout` is not specified. +When a timeout expires, the process is sent `SIGTERM` so that it can clean up, and then +`SIGKILL` if it has not exited within a short grace period. A test process that ignores +`SIGTERM`, or that has been stopped and so never receives it, therefore cannot stall the +whole run. + ## Build timeouts `const` expressions may be evaluated at compile time. In the same way that mutations can cause tests to hang, mutations to const code may potentially cause the compiler to enter an infinite loop. diff --git a/src/process.rs b/src/process.rs index 0763e617..b8268611 100644 --- a/src/process.rs +++ b/src/process.rs @@ -26,15 +26,21 @@ use crate::output::ScenarioOutput; /// How frequently to check if a subprocess finished. const WAIT_POLL_INTERVAL: Duration = Duration::from_millis(50); +/// How long to let a process wind up after `SIGTERM` before sending `SIGKILL`. +const TERM_GRACE: Duration = Duration::from_millis(500); + +/// How often to check whether a signalled process has gone away. +const TERM_POLL_INTERVAL: Duration = Duration::from_millis(20); + #[cfg(windows)] mod windows; #[cfg(windows)] -use windows::{configure_command, terminate_child}; +use windows::{configure_command, kill_child, terminate_child}; #[cfg(unix)] mod unix; #[cfg(unix)] -use unix::{configure_command, terminate_child}; +use unix::{configure_command, kill_child, terminate_child}; pub struct Process { child: Child, @@ -124,6 +130,10 @@ impl Process { /// /// Blocks until the subprocess is terminated and then returns the exit status. /// + /// `SIGTERM` first, so it gets a chance to clean up, but only for a bounded grace + /// period: a process that ignores it, or that is stopped and so never receives it, + /// would otherwise hang the run here forever. + /// /// The status might not be `Timeout` if this raced with a normal exit. #[mutants::skip] // would leak processes from tests if skipped fn terminate(&mut self) -> Result<()> { @@ -131,9 +141,29 @@ impl Process { debug!("terminating child process"); terminate_child(&mut self.child)?; trace!("wait for child after termination"); + let deadline = Instant::now() + TERM_GRACE; + loop { + match self.child.try_wait() { + Ok(Some(exit)) => { + debug!("terminated child exit status {exit:?}"); + return Ok(()); + } + Ok(None) => {} + Err(err) => { + debug!(?err, "Failed to wait for child after termination"); + return Ok(()); + } + } + if Instant::now() >= deadline { + debug!("child did not exit after SIGTERM; killing it"); + kill_child(&mut self.child)?; + break; + } + sleep(TERM_POLL_INTERVAL); + } match self.child.wait() { - Err(err) => debug!(?err, "Failed to wait for child after termination"), - Ok(exit) => debug!("terminated child exit status {exit:?}"), + Err(err) => debug!(?err, "Failed to wait for child after kill"), + Ok(exit) => debug!("killed child exit status {exit:?}"), } Ok(()) } diff --git a/src/process/unix.rs b/src/process/unix.rs index 1eb1866f..6d9eefed 100644 --- a/src/process/unix.rs +++ b/src/process/unix.rs @@ -11,11 +11,10 @@ use crate::Result; use super::Exit; -#[allow(unknown_lints, clippy::needless_pass_by_ref_mut)] // To match Windows +/// Send a signal to a process group, treating "nothing there" as success. #[mutants::skip] // hard to exercise the ESRCH edge case -pub(super) fn terminate_child(child: &mut Child) -> Result<()> { - let pid = Pid::from_raw(child.id().try_into().unwrap()); - match killpg(pid, Signal::SIGTERM) { +fn signal_group(pgid: Pid, signal: Signal) -> Result<()> { + match killpg(pgid, signal) { Ok(()) => Ok(()), Err(Errno::ESRCH) => { Ok(()) // Probably already gone @@ -25,13 +24,31 @@ pub(super) fn terminate_child(child: &mut Child) -> Result<()> { } Err(errno) => { // TODO: Maybe strerror? - let message = format!("failed to terminate child: error {errno}"); + let message = format!("failed to signal process group {pgid}: error {errno}"); warn!("{}", message); bail!(message); } } } +/// The process group id of a child, which (because we start it with `process_group(0)`) +/// is the same as its pid. +fn child_pgid(child: &Child) -> Pid { + Pid::from_raw(child.id().try_into().expect("child pid fits in pid_t")) +} + +#[allow(unknown_lints, clippy::needless_pass_by_ref_mut)] // To match Windows +#[mutants::skip] // hard to exercise the ESRCH edge case +pub(super) fn terminate_child(child: &mut Child) -> Result<()> { + signal_group(child_pgid(child), Signal::SIGTERM) +} + +#[allow(unknown_lints, clippy::needless_pass_by_ref_mut)] // To match Windows +#[mutants::skip] // would leak processes from tests if skipped +pub(super) fn kill_child(child: &mut Child) -> Result<()> { + signal_group(child_pgid(child), Signal::SIGKILL) +} + #[mutants::skip] pub(super) fn configure_command(command: &mut Command) { command.process_group(0); diff --git a/src/process/windows.rs b/src/process/windows.rs index c37950fd..cca4981a 100644 --- a/src/process/windows.rs +++ b/src/process/windows.rs @@ -11,6 +11,12 @@ pub(super) fn terminate_child(child: &mut Child) -> Result<()> { child.kill().context("Kill child") } +/// Windows has no `SIGTERM`, so `terminate_child` already killed it outright. +#[mutants::skip] // would leak processes from tests if skipped +pub(super) fn kill_child(child: &mut Child) -> Result<()> { + child.kill().context("Kill child") +} + #[mutants::skip] pub(super) fn configure_command(_command: &mut Command) {} From 1ce1e5550d3f3ccbd94c372a67e1168ac310549b Mon Sep 17 00:00:00 2001 From: Yuri Khrustalev Date: Tue, 15 Sep 2026 22:01:21 -0400 Subject: [PATCH 2/3] fix: sweep the child's process group after every phase Tests can leave processes running after they exit: a helper a test spawned and forgot, or a test binary that was never reaped. Until now cargo-mutants only signalled the child's process group on a timeout, so on a normal exit those processes survived and kept allocating while later mutants were tested, in a window where no cargo phase is running at all. Sweep the group in the process layer, so every phase gets it: once the direct child exits with any status, SIGTERM the group, wait a bounded grace period, then SIGKILL whatever is left. What was reaped goes into the scenario log, and the pids into the debug log, enumerated from /proc on Linux. Windows has no process groups and the job object equivalent is out of scope, so the sweep is a no-op there. --- NEWS.md | 2 + book/src/timeouts.md | 17 +++ src/cargo.rs | 2 +- src/process.rs | 86 ++++++++++++-- src/process/unix.rs | 111 +++++++++++++++--- src/process/windows.rs | 9 +- .../spawns_background_child/Cargo_test.toml | 13 ++ testdata/spawns_background_child/src/lib.rs | 42 +++++++ tests/main.rs | 67 +++++++++++ 9 files changed, 321 insertions(+), 28 deletions(-) create mode 100644 testdata/spawns_background_child/Cargo_test.toml create mode 100644 testdata/spawns_background_child/src/lib.rs diff --git a/NEWS.md b/NEWS.md index f7619c39..b8315cf0 100644 --- a/NEWS.md +++ b/NEWS.md @@ -2,6 +2,8 @@ ## Unreleased +- Fixed: Each cargo invocation now runs as the leader of its own process group, and that group is swept after every phase, not only after a timeout. Processes left running by a scenario's tests are `SIGTERM`ed, given a short grace period, and then `SIGKILL`ed, so they can't keep consuming memory or CPU while later mutants are tested. Unix only. + - Fixed: After a timeout, cargo-mutants waits only a short grace period for the child to exit after `SIGTERM` before sending `SIGKILL`. Previously it waited indefinitely, so a process that ignored `SIGTERM`, or that had been stopped, hung the whole run. - New: `#[mutants::exclude_re("pattern")]` attribute to exclude specific mutations by regex, without disabling all mutations on the function. The attribute can be placed on functions, `impl` blocks, `trait` blocks, modules, files, and on expressions that can carry an attribute (such as `match`, struct literals, call expressions, method calls, and unary expressions). Multiple patterns can be applied. Also supported within `cfg_attr`. Requires the [mutants](https://crates.io/crates/mutants) crate version `0.0.5` or later. diff --git a/book/src/timeouts.md b/book/src/timeouts.md index 3de52892..660cdd59 100644 --- a/book/src/timeouts.md +++ b/book/src/timeouts.md @@ -50,6 +50,23 @@ In this case you can use the `--build-timeout` or `--build-timeout-multiplier` o You might also choose to skip mutants that can cause long-running const evaluation. +## Leftover processes + +A test can leave processes running after it returns: a daemon it started, a helper it +forgot to wait for, or a test binary that was not reaped. Those processes keep running +— and keep allocating — while cargo-mutants moves on to the next mutant, so they can +exhaust the machine's memory in a window where no cargo phase is running at all. + +To prevent this, cargo-mutants starts each cargo invocation as the leader of its own +process group, and sweeps that group after *every* phase, not only after a timeout. +Once the cargo process itself exits, anything left in the group is sent `SIGTERM`, given +a short grace period, and then `SIGKILL`ed. What was reaped is recorded in the +scenario's log, and the pids are shown at `--level=debug`. + +This has no effect on how a mutant is classified; it only stops work from one scenario +leaking into the next. Windows has no process groups, and cargo-mutants does not yet use +job objects, so this sweep is Unix-only. + ## Exceptions The multiplier timeout options cannot be used when the baseline is skipped diff --git a/src/cargo.rs b/src/cargo.rs index 909dd881..0309fd27 100644 --- a/src/cargo.rs +++ b/src/cargo.rs @@ -55,7 +55,7 @@ pub fn run_cargo( debug!(?encoded_rustflags); env.push(("CARGO_ENCODED_RUSTFLAGS".to_owned(), encoded_rustflags)); } - let process_status = Process::run( + let (process_status, _sweep) = Process::run( &argv, &env, build_dir.path(), diff --git a/src/process.rs b/src/process.rs index b8268611..1903bc9a 100644 --- a/src/process.rs +++ b/src/process.rs @@ -15,6 +15,7 @@ use std::time::{Duration, Instant}; use anyhow::Context; use camino::Utf8Path; +use itertools::Itertools; use serde::Serialize; use tracing::{Level, debug, span, trace}; @@ -26,21 +27,57 @@ use crate::output::ScenarioOutput; /// How frequently to check if a subprocess finished. const WAIT_POLL_INTERVAL: Duration = Duration::from_millis(50); -/// How long to let a process wind up after `SIGTERM` before sending `SIGKILL`. +/// How long to let a process, or a process group, wind up after `SIGTERM` before +/// sending `SIGKILL`. const TERM_GRACE: Duration = Duration::from_millis(500); -/// How often to check whether a signalled process has gone away. +/// How often to check whether a signalled process or group has gone away. const TERM_POLL_INTERVAL: Duration = Duration::from_millis(20); #[cfg(windows)] mod windows; #[cfg(windows)] -use windows::{configure_command, kill_child, terminate_child}; +use windows::{configure_command, kill_child, sweep_process_group, terminate_child}; #[cfg(unix)] mod unix; #[cfg(unix)] -use unix::{configure_command, kill_child, terminate_child}; +use unix::{configure_command, kill_child, sweep_process_group, terminate_child}; + +/// What sweeping a finished child's process group found and did. +/// +/// A scenario's tests can leave processes running: a test binary that was not waited +/// for, or anything a test spawned and forgot. Those processes stay in the child's +/// process group, and would otherwise keep running (and keep allocating) while +/// cargo-mutants moves on to later scenarios. +#[derive(Debug, Default, Clone, PartialEq, Eq, Serialize)] +pub struct Sweep { + /// The pids that were in the group, where the platform can enumerate them. + pub pids: Option>, + /// Whether any process was still in the group after the direct child exited. + pub strays: bool, + /// Whether `SIGTERM` was not enough, and `SIGKILL` had to be sent. + pub killed: bool, +} + +impl Sweep { + /// Describe what was reaped, or None if nothing was left over. + pub fn describe(&self) -> Option { + if !self.strays { + return None; + } + let how = if self.killed { "SIGKILLed" } else { "reaped" }; + Some(match &self.pids { + Some(pids) if !pids.is_empty() => format!( + "left {n} stray process{es} behind ({how}: {list})", + n = pids.len(), + es = if pids.len() == 1 { "" } else { "es" }, + list = pids.iter().join(", ") + ), + _ => format!("left stray processes behind ({how})"), + }) + } +} pub struct Process { child: Child, @@ -51,6 +88,9 @@ pub struct Process { impl Process { /// Run a subprocess to completion, watching for interrupts, with a timeout, while /// ticking the progress bar. + /// + /// Whatever the outcome, the child's process group is swept before returning, so + /// that nothing it left running survives into the next scenario. pub fn run( argv: &[String], env: &[(String, String)], @@ -59,17 +99,42 @@ impl Process { jobserver: Option<&jobserver::Client>, scenario_output: &mut ScenarioOutput, console: &Console, - ) -> Result { + ) -> Result<(Exit, Sweep)> { let mut child = Process::start(argv, env, cwd, timeout, jobserver, scenario_output)?; - let process_status = loop { - if let Some(exit_status) = child.poll()? { - break exit_status; + let result = loop { + match child.poll() { + Ok(Some(exit_status)) => break Ok(exit_status), + Ok(None) => {} + Err(err) => break Err(err), } console.tick(); sleep(WAIT_POLL_INTERVAL); }; + let sweep = child.sweep()?; + let process_status = result?; scenario_output.message(&format!("result: {process_status:?}"))?; - Ok(process_status) + if let Some(description) = sweep.describe() { + scenario_output.message(&description)?; + } + Ok((process_status, sweep)) + } + + /// Kill anything the child left running in its process group. + /// + /// This runs after every phase, not only after a timeout: a scenario that exited + /// cleanly can still have left a test binary or a process spawned by a test behind. + fn sweep(&mut self) -> Result { + let sweep = sweep_process_group(&self.child)?; + if sweep.strays { + debug!( + pids = ?sweep.pids, + killed = sweep.killed, + "swept processes left over in the child's process group" + ); + } else { + trace!("no processes left in the child's process group"); + } + Ok(sweep) } /// Launch a process, and return an object representing the child. @@ -132,7 +197,8 @@ impl Process { /// /// `SIGTERM` first, so it gets a chance to clean up, but only for a bounded grace /// period: a process that ignores it, or that is stopped and so never receives it, - /// would otherwise hang the run here forever. + /// would otherwise hang the run here forever. Whatever else is left in the process + /// group is dealt with by the sweep in [`Process::run`]. /// /// The status might not be `Timeout` if this raced with a normal exit. #[mutants::skip] // would leak processes from tests if skipped diff --git a/src/process/unix.rs b/src/process/unix.rs index 6d9eefed..61060255 100644 --- a/src/process/unix.rs +++ b/src/process/unix.rs @@ -1,5 +1,7 @@ use std::os::unix::process::{CommandExt, ExitStatusExt}; use std::process::{Child, Command, ExitStatus}; +use std::thread::sleep; +use std::time::Instant; use anyhow::bail; use nix::errno::Errno; @@ -9,18 +11,18 @@ use tracing::warn; use crate::Result; -use super::Exit; +use super::{Exit, Sweep, TERM_GRACE, TERM_POLL_INTERVAL}; -/// Send a signal to a process group, treating "nothing there" as success. +/// Send a signal to a process group, returning whether the group still had any member. +/// +/// A signal of `None` sends nothing and just probes for members. #[mutants::skip] // hard to exercise the ESRCH edge case -fn signal_group(pgid: Pid, signal: Signal) -> Result<()> { +fn signal_group(pgid: Pid, signal: Option) -> Result { match killpg(pgid, signal) { - Ok(()) => Ok(()), - Err(Errno::ESRCH) => { - Ok(()) // Probably already gone - } + Ok(()) => Ok(true), + Err(Errno::ESRCH) => Ok(false), // the group is empty Err(Errno::EPERM) if cfg!(target_os = "macos") => { - Ok(()) // If the process no longer exists then macos can return EPERM (maybe?) + Ok(false) // If the process no longer exists then macos can return EPERM (maybe?) } Err(errno) => { // TODO: Maybe strerror? @@ -31,22 +33,99 @@ fn signal_group(pgid: Pid, signal: Signal) -> Result<()> { } } +#[allow(unknown_lints, clippy::needless_pass_by_ref_mut)] // To match Windows +#[mutants::skip] // would leak processes from tests if skipped +pub(super) fn terminate_child(child: &mut Child) -> Result<()> { + signal_group(child_pgid(child), Some(Signal::SIGTERM))?; + Ok(()) +} + +#[allow(unknown_lints, clippy::needless_pass_by_ref_mut)] // To match Windows +#[mutants::skip] // would leak processes from tests if skipped +pub(super) fn kill_child(child: &mut Child) -> Result<()> { + signal_group(child_pgid(child), Some(Signal::SIGKILL))?; + Ok(()) +} + +/// Kill anything left in a child's process group once the child itself has exited. +/// +/// The child was started as the leader of its own process group, so anything it or its +/// descendants spawned and left running is still in that group, even after being +/// reparented. Send `SIGTERM`, give the group a moment to wind up, then `SIGKILL` +/// whatever is left. +#[mutants::skip] // would leak processes from tests if skipped +pub(super) fn sweep_process_group(child: &Child) -> Result { + let pgid = child_pgid(child); + // Probe before enumerating: almost every phase leaves nothing behind, and listing + // the group means reading every /proc//stat on the machine. + if !signal_group(pgid, None)? { + return Ok(Sweep::default()); + } + let pids = group_members(pgid); + signal_group(pgid, Some(Signal::SIGTERM))?; + let deadline = Instant::now() + TERM_GRACE; + loop { + if !signal_group(pgid, None)? { + return Ok(Sweep { + pids, + strays: true, + killed: false, + }); + } else if Instant::now() >= deadline { + break; + } + sleep(TERM_POLL_INTERVAL); + } + signal_group(pgid, Some(Signal::SIGKILL))?; + Ok(Sweep { + pids, + strays: true, + killed: true, + }) +} + /// The process group id of a child, which (because we start it with `process_group(0)`) /// is the same as its pid. +/// +/// Callers signal this after the child has been reaped, by which point the group may be +/// empty and the kernel free to hand the same number to something else. Hitting that +/// would take a full wrap of the pid space inside the microseconds between reaping and +/// signalling, so we live with it. fn child_pgid(child: &Child) -> Pid { Pid::from_raw(child.id().try_into().expect("child pid fits in pid_t")) } -#[allow(unknown_lints, clippy::needless_pass_by_ref_mut)] // To match Windows -#[mutants::skip] // hard to exercise the ESRCH edge case -pub(super) fn terminate_child(child: &mut Child) -> Result<()> { - signal_group(child_pgid(child), Signal::SIGTERM) +/// List the pids currently in a process group. +/// +/// Returns `None` on platforms where we can't enumerate processes: the sweep still +/// works there, it just can't say what it killed. +#[cfg(target_os = "linux")] +#[mutants::skip] // only affects what we can say in the debug log +fn group_members(pgid: Pid) -> Option> { + let members = std::fs::read_dir("/proc") + .ok()? + .flatten() + .filter_map(|dir_entry| dir_entry.file_name().to_string_lossy().parse::().ok()) + .filter(|pid| pgid_of(*pid) == Some(pgid.as_raw())) + .collect(); + Some(members) } -#[allow(unknown_lints, clippy::needless_pass_by_ref_mut)] // To match Windows -#[mutants::skip] // would leak processes from tests if skipped -pub(super) fn kill_child(child: &mut Child) -> Result<()> { - signal_group(child_pgid(child), Signal::SIGKILL) +/// Read a process's group id out of `/proc//stat`. +#[cfg(target_os = "linux")] +#[mutants::skip] // only affects what we can say in the debug log +fn pgid_of(pid: i32) -> Option { + let stat = std::fs::read_to_string(format!("/proc/{pid}/stat")).ok()?; + // The second field is the command name in parentheses, and may itself contain spaces + // and parentheses, so only split the fields after its closing paren. Counting from + // there, the fields are: state, ppid, pgrp. + let (_, after_comm) = stat.rsplit_once(')')?; + after_comm.split_whitespace().nth(2)?.parse().ok() +} + +#[cfg(not(target_os = "linux"))] +fn group_members(_pgid: Pid) -> Option> { + None } #[mutants::skip] diff --git a/src/process/windows.rs b/src/process/windows.rs index cca4981a..b23708ee 100644 --- a/src/process/windows.rs +++ b/src/process/windows.rs @@ -4,7 +4,7 @@ use anyhow::Context; use crate::Result; -use super::Exit; +use super::{Exit, Sweep}; #[mutants::skip] // hard to exercise the ESRCH edge case pub(super) fn terminate_child(child: &mut Child) -> Result<()> { @@ -17,6 +17,13 @@ pub(super) fn kill_child(child: &mut Child) -> Result<()> { child.kill().context("Kill child") } +/// Windows has no process groups; the equivalent would be a job object, which we don't +/// use yet, so there is nothing to sweep. +#[allow(clippy::unnecessary_wraps)] // To match Unix +pub(super) fn sweep_process_group(_child: &Child) -> Result { + Ok(Sweep::default()) +} + #[mutants::skip] pub(super) fn configure_command(_command: &mut Command) {} diff --git a/testdata/spawns_background_child/Cargo_test.toml b/testdata/spawns_background_child/Cargo_test.toml new file mode 100644 index 00000000..5857de59 --- /dev/null +++ b/testdata/spawns_background_child/Cargo_test.toml @@ -0,0 +1,13 @@ +# A tree whose test leaves a background process running after the test binary exits. +# +# cargo-mutants should sweep the cargo process group after every phase, so that no +# `sleep` process survives the scenario that spawned it. + +[package] +name = "cargo-mutants-testdata-spawns-background-child" +version = "0.1.0" +edition = "2018" +publish = false + +[lib] +doctest = false diff --git a/testdata/spawns_background_child/src/lib.rs b/testdata/spawns_background_child/src/lib.rs new file mode 100644 index 00000000..78ed86c0 --- /dev/null +++ b/testdata/spawns_background_child/src/lib.rs @@ -0,0 +1,42 @@ +//! A tree whose test spawns a background process and then exits successfully. +//! +//! The test records the pid of the background process in the file named by +//! `$BACKGROUND_CHILD_PID_FILE`, so that the cargo-mutants test suite can check that +//! the process group sweep reaps it. + +pub fn triple(x: i32) -> i32 { + x * 3 +} + +#[cfg(test)] +mod test { + use std::fs::OpenOptions; + use std::io::Write; + use std::process::{Command, Stdio}; + + /// Spawn a process that outlives this test, and record its pid. + fn leave_a_process_running() { + let child = Command::new("sleep") + .arg("300") + .stdin(Stdio::null()) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .spawn() + .expect("spawn sleep"); + if let Ok(path) = std::env::var("BACKGROUND_CHILD_PID_FILE") { + let mut file = OpenOptions::new() + .create(true) + .append(true) + .open(&path) + .expect("open pid file"); + writeln!(file, "{}", child.id()).expect("write pid"); + } + } + + #[test] + fn triple_triples() { + leave_a_process_running(); + // 3 is chosen so that every mutant of `x * 3` gives a different answer. + assert_eq!(super::triple(3), 9); + } +} diff --git a/tests/main.rs b/tests/main.rs index 0fb8973d..627b41cf 100644 --- a/tests/main.rs +++ b/tests/main.rs @@ -3846,3 +3846,70 @@ fn in_diff_with_nonexistent_file_returns_exit_code_6() { .code(6) .stderr(contains("Failed to read diff file").or(contains("Failed to open diff file"))); } + +/// A test can leave a background process running after it exits. cargo-mutants puts +/// each cargo invocation in its own process group and sweeps that group after every +/// phase, so nothing spawned by a scenario outlives it. +/// +/// The `spawns_background_child` tree records the pids it leaves behind, so we can +/// probe them once cargo-mutants has finished. +#[cfg(unix)] +#[test] +fn processes_spawned_by_tests_are_swept_after_each_scenario() +-> Result<(), Box> { + use nix::sys::signal::{SIGKILL, kill}; + use nix::unistd::Pid; + + let tmp_src_dir = copy_of_testdata("spawns_background_child"); + let pid_dir = tempdir()?; + let pid_file = pid_dir.path().join("pids.txt"); + let assert = run() + .arg("mutants") + .args(["--timeout=60", "--build-timeout=120", "-L", "debug"]) + .env("BACKGROUND_CHILD_PID_FILE", &pid_file) + .current_dir(tmp_src_dir.path()) + .timeout(OUTER_TIMEOUT) + .assert(); + let stdout = String::from_utf8_lossy(&assert.get_output().stdout).into_owned(); + println!("stdout:\n{stdout}"); + assert.success(); + + let pids: Vec = read_to_string(&pid_file)? + .lines() + .map(str::trim) + .map(str::parse) + .collect::>()?; + let survivors: Vec = pids + .iter() + .copied() + .filter(|pid| kill(Pid::from_raw(*pid), None).is_ok()) + .collect(); + // Before any assertion, so that a failure doesn't also leave these running. + // (clippy::needless_for_each rules out the iterator form here.) + for pid in &survivors { + let _ = kill(Pid::from_raw(*pid), SIGKILL); + } + assert!( + !pids.is_empty(), + "the tree's test should have spawned background processes" + ); + assert_eq!( + survivors, + Vec::::new(), + "processes spawned by the tests were still running after cargo-mutants finished" + ); + + // Sweeping the process group must not change any verdict. + assert_eq!( + outcome_json_counts(&tmp_src_dir), + json!({ + "total_mutants": 5, + "caught": 5, + "missed": 0, + "timeout": 0, + "unviable": 0, + "success": 0, + }) + ); + Ok(()) +} From bd0372ced758628568759e6df0410b79188949c8 Mon Sep 17 00:00:00 2001 From: Yuri Khrustalev Date: Tue, 15 Sep 2026 22:06:52 -0400 Subject: [PATCH 3/3] feat: say why a scenario's processes died A mutant caught because its tests were killed is indistinguishable, in the counts, from one caught by a failing assertion -- which is exactly what you want to know when diagnosing a run. Carry the process group sweep's findings on each phase result, and render them, with the name of any signal that killed the phase, in parentheses on the outcome line, in the scenario log, and in outcomes.json. Classification is deliberately untouched: this only makes the reason visible. --- NEWS.md | 2 + book/src/timeouts.md | 18 ++++++++ src/cargo.rs | 3 +- src/console.rs | 4 ++ src/outcome.rs | 98 ++++++++++++++++++++++++++++++++++++++++++-- src/process.rs | 2 + src/process/unix.rs | 8 ++++ tests/main.rs | 9 +++- 8 files changed, 139 insertions(+), 5 deletions(-) diff --git a/NEWS.md b/NEWS.md index b8315cf0..3622110f 100644 --- a/NEWS.md +++ b/NEWS.md @@ -2,6 +2,8 @@ ## Unreleased +- New: Outcome lines, scenario logs, and `outcomes.json` now say when a phase's process was killed by a signal, and when processes left running by the tests had to be reaped. The caught / missed / unviable / timeout classification is unchanged. + - Fixed: Each cargo invocation now runs as the leader of its own process group, and that group is swept after every phase, not only after a timeout. Processes left running by a scenario's tests are `SIGTERM`ed, given a short grace period, and then `SIGKILL`ed, so they can't keep consuming memory or CPU while later mutants are tested. Unix only. - Fixed: After a timeout, cargo-mutants waits only a short grace period for the child to exit after `SIGTERM` before sending `SIGKILL`. Previously it waited indefinitely, so a process that ignored `SIGTERM`, or that had been stopped, hung the whole run. diff --git a/book/src/timeouts.md b/book/src/timeouts.md index 660cdd59..48a98566 100644 --- a/book/src/timeouts.md +++ b/book/src/timeouts.md @@ -67,6 +67,24 @@ This has no effect on how a mutant is classified; it only stops work from one sc leaking into the next. Windows has no process groups, and cargo-mutants does not yet use job objects, so this sweep is Unix-only. +## Why a scenario died + +A mutant caught because the kernel killed its tests looks, in the summary counts, exactly +like a mutant caught by a failing assertion. When there is more to say, cargo-mutants says +it in parentheses on the outcome line: + +``` +caught src/server.rs:88:5: replace listen -> bool with false (test killed by SIGABRT; test left 1 stray process behind (SIGKILLed: 30411)) in 2s build + 9s test +``` + +Two things get reported this way: the signal that killed a phase's cargo process, if it +died by one, and anything the process group sweep had to clean up. The same information +is written to the scenario's log and, in `mutants.out/outcomes.json`, to a `sweep` field +on each phase result. + +This does not change the caught / missed / unviable / timeout classification. It only +makes the reason visible. + ## Exceptions The multiplier timeout options cannot be used when the baseline is skipped diff --git a/src/cargo.rs b/src/cargo.rs index 0309fd27..1b094002 100644 --- a/src/cargo.rs +++ b/src/cargo.rs @@ -55,7 +55,7 @@ pub fn run_cargo( debug!(?encoded_rustflags); env.push(("CARGO_ENCODED_RUSTFLAGS".to_owned(), encoded_rustflags)); } - let (process_status, _sweep) = Process::run( + let (process_status, sweep) = Process::run( &argv, &env, build_dir.path(), @@ -79,6 +79,7 @@ pub fn run_cargo( duration: start.elapsed(), process_status, argv, + sweep, }) } diff --git a/src/console.rs b/src/console.rs index 3c2c0cc2..970b4516 100644 --- a/src/console.rs +++ b/src/console.rs @@ -90,6 +90,10 @@ impl Console { style_outcome(outcome), style_scenario(scenario, true), ); + let death_reasons = outcome.death_reasons(); + if !death_reasons.is_empty() { + write!(s, " ({})", death_reasons.join("; ")).expect("format death reasons"); + } if options.show_times { let prs: Vec = outcome .phase_results() diff --git a/src/outcome.rs b/src/outcome.rs index ec5b7861..ed3a1700 100644 --- a/src/outcome.rs +++ b/src/outcome.rs @@ -17,7 +17,9 @@ use tracing::warn; use crate::console::{format_duration, plural}; use crate::exit_code::ExitCode; -use crate::process::Exit; +#[cfg(unix)] +use crate::process::signal_name; +use crate::process::{Exit, Sweep}; use crate::{Options, Result, Scenario, output}; /// What phase of running a scenario. @@ -245,6 +247,19 @@ impl ScenarioOutcome { .any(|pr| pr.phase != Phase::Test && pr.process_status.is_failure()) } + /// Say, for each phase that has something unusual to report, how its process tree + /// ended: killed by a signal, or leaving stray processes behind. + /// + /// This has no bearing on how the mutant is classified. It exists so that a mutant + /// caught because the kernel killed its tests can be told apart from one caught by a + /// failing assertion. + pub fn death_reasons(&self) -> Vec { + self.phase_results + .iter() + .flat_map(PhaseResult::death_reasons) + .collect() + } + /// True if this outcome is a caught mutant: it's a mutant and the tests failed. pub fn mutant_caught(&self) -> bool { self.scenario.is_mutant() @@ -303,12 +318,27 @@ pub struct PhaseResult { pub process_status: Exit, /// What command was run, as an argv list. pub argv: Vec, + /// What the sweep of the child's process group found and did. + pub sweep: Sweep, } impl PhaseResult { pub fn is_success(&self) -> bool { self.process_status.is_success() } + + fn death_reasons(&self) -> Vec { + let phase = self.phase.name(); + let mut reasons = Vec::new(); + #[cfg(unix)] + if let Exit::Signalled(signal) = self.process_status { + reasons.push(format!("{phase} killed by {}", signal_name(signal))); + } + if let Some(sweep) = self.sweep.describe() { + reasons.push(format!("{phase} {sweep}")); + } + reasons + } } impl Serialize for PhaseResult { @@ -316,11 +346,12 @@ impl Serialize for PhaseResult { where S: Serializer, { - let mut ss = serializer.serialize_struct("PhaseResult", 4)?; + let mut ss = serializer.serialize_struct("PhaseResult", 5)?; ss.serialize_field("phase", &self.phase)?; ss.serialize_field("duration", &self.duration.as_secs_f64())?; ss.serialize_field("process_status", &self.process_status)?; ss.serialize_field("argv", &self.argv)?; + ss.serialize_field("sweep", &self.sweep)?; ss.end() } } @@ -341,10 +372,68 @@ pub enum SummaryOutcome { mod test { use std::time::Duration; - use crate::process::Exit; + use crate::process::{Exit, Sweep}; use super::{Phase, PhaseResult, Scenario, ScenarioOutcome}; + fn phase_result(phase: Phase, process_status: Exit, sweep: Sweep) -> PhaseResult { + PhaseResult { + phase, + duration: Duration::from_secs(1), + process_status, + argv: vec!["cargo".into(), "test".into()], + sweep, + } + } + + fn outcome_of(phase_results: Vec) -> ScenarioOutcome { + ScenarioOutcome { + output_dir: "output".into(), + log_path: "log".into(), + diff_path: None, + scenario: Scenario::Baseline, + phase_results, + } + } + + #[test] + fn no_death_reasons_for_an_ordinary_test_failure() { + let outcome = outcome_of(vec![phase_result( + Phase::Test, + Exit::Failure(101), + Sweep::default(), + )]); + assert_eq!(outcome.death_reasons(), Vec::::new()); + } + + #[cfg(unix)] + #[test] + fn death_reasons_name_the_signal_that_killed_the_phase() { + let outcome = outcome_of(vec![phase_result( + Phase::Test, + Exit::Signalled(9), + Sweep::default(), + )]); + assert_eq!(outcome.death_reasons(), ["test killed by SIGKILL"]); + } + + #[test] + fn death_reasons_name_processes_left_behind_by_the_tests() { + let outcome = outcome_of(vec![phase_result( + Phase::Test, + Exit::Success, + Sweep { + pids: Some(vec![101, 102]), + strays: true, + killed: true, + }, + )]); + assert_eq!( + outcome.death_reasons(), + ["test left 2 stray processes behind (SIGKILLed: 101, 102)"] + ); + } + #[test] fn find_phase_result() { let outcome = ScenarioOutcome { @@ -358,12 +447,14 @@ mod test { duration: Duration::from_secs(2), process_status: Exit::Success, argv: vec!["cargo".into(), "build".into()], + sweep: Sweep::default(), }, PhaseResult { phase: Phase::Test, duration: Duration::from_secs(3), process_status: Exit::Success, argv: vec!["cargo".into(), "test".into()], + sweep: Sweep::default(), }, ], }; @@ -374,6 +465,7 @@ mod test { duration: Duration::from_secs(2), process_status: Exit::Success, argv: vec!["cargo".into(), "build".into()], + sweep: Sweep::default(), }) ); assert_eq!( diff --git a/src/process.rs b/src/process.rs index 1903bc9a..9db74ec1 100644 --- a/src/process.rs +++ b/src/process.rs @@ -42,6 +42,8 @@ use windows::{configure_command, kill_child, sweep_process_group, terminate_chil #[cfg(unix)] mod unix; #[cfg(unix)] +pub use unix::signal_name; +#[cfg(unix)] use unix::{configure_command, kill_child, sweep_process_group, terminate_child}; /// What sweeping a finished child's process group found and did. diff --git a/src/process/unix.rs b/src/process/unix.rs index 61060255..2098dae3 100644 --- a/src/process/unix.rs +++ b/src/process/unix.rs @@ -84,6 +84,14 @@ pub(super) fn sweep_process_group(child: &Child) -> Result { }) } +/// The name of a signal, like `SIGKILL`, or its number if we don't recognize it. +pub fn signal_name(signal: i32) -> String { + Signal::try_from(signal).map_or_else( + |_| format!("signal {signal}"), + |signal| signal.as_str().to_owned(), + ) +} + /// The process group id of a child, which (because we start it with `process_group(0)`) /// is the same as its pid. /// diff --git a/tests/main.rs b/tests/main.rs index 627b41cf..8241caa3 100644 --- a/tests/main.rs +++ b/tests/main.rs @@ -3865,7 +3865,7 @@ fn processes_spawned_by_tests_are_swept_after_each_scenario() let pid_file = pid_dir.path().join("pids.txt"); let assert = run() .arg("mutants") - .args(["--timeout=60", "--build-timeout=120", "-L", "debug"]) + .args(["--timeout=60", "--build-timeout=120", "-L", "debug", "-v"]) .env("BACKGROUND_CHILD_PID_FILE", &pid_file) .current_dir(tmp_src_dir.path()) .timeout(OUTER_TIMEOUT) @@ -3899,6 +3899,13 @@ fn processes_spawned_by_tests_are_swept_after_each_scenario() "processes spawned by the tests were still running after cargo-mutants finished" ); + // The outcome line should say what was left behind, so that a scenario that leaks + // processes is visible rather than silent. + assert!( + stdout.contains("stray process"), + "no mention of the stray processes in:\n{stdout}" + ); + // Sweeping the process group must not change any verdict. assert_eq!( outcome_json_counts(&tmp_src_dir),