fix: don't wait forever for a child that ignores SIGTERM

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)).

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.

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,16 +130,40 @@ 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<()> {
         let _span = span!(Level::DEBUG, "terminate_child", pid = self.child.id()).entered();
         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(())
     }

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);

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) {}