From 8aa78c7198abf481e0676e3a67f6d0f9edc1e2ce Mon Sep 17 00:00:00 2001 From: Bill Njoroge Date: Thu, 20 Aug 2026 13:39:10 -0400 Subject: [PATCH] fix(runner): sweep the process group on cancel MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A cancelled step could leave background processes running on the runner host indefinitely. `invoke`'s wait loop calls `try_wait()` on every iteration, so the shell is reaped the moment it exits — which for `sh -c "cmd & echo ready"` is about a millisecond, long before cancellation arrives. Cancellation then signalled the group through the child handle, but a reaped handle reports no pid and addresses nothing, so surviving descendants were never signalled. `wait_for_exit` compounded it: it returned true as soon as the leader had a status, conflating "the shell exited" with "the group is gone". That reported success from the SIGINT stage, so the SIGTERM and SIGKILL stages never ran. A descendant ignoring SIGINT/SIGTERM was therefore unreachable by construction — reparented to init and outliving the job. Capture the group id at spawn, while the handle still reports one, and escalate against the group: signal it with `killpg`, treat the group as drained only when `killpg(pgid, 0)` reports it empty, and sweep it with SIGKILL when the grace windows expire. The leader is still reaped first so its zombie cannot hold the group open. This matches ProcessInvoker.cs, which kills the remaining process tree. The stream-drain grace path had the same defect — its `child.kill()` also ran after the leader was reaped — and now shares the group sweep. `nix` is unix-only here: the workspace forbids `unsafe`, ruling out raw `libc` calls, and `nix` does not build on Windows, where the existing `cfg(not(unix))` paths apply. It was already in the tree via command-group. The regression test now asserts the background child is actually dead rather than only checking the returned error string. Verified as a real guard: restoring the leader-only exit check fails it with "background child survived cancellation". --- CHANGELOG.md | 13 ++ Cargo.lock | 1 + crates/preloop-runner/Cargo.toml | 8 ++ crates/preloop-runner/src/process.rs | 196 +++++++++++++++++++++++---- 4 files changed, 193 insertions(+), 25 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a83c0b27..71477a92 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,19 @@ Releases before v0.27.0 predate the changelog. ### Fixed +- Cancelling a job no longer leaves background processes running on the + runner host. Cancellation signalled the process group through the child + handle, but the wait loop reaps the shell as soon as it exits, and a reaped + handle addresses nothing — so any descendant that outlived its shell was + never signalled. The leader's exit was also read as "the group is gone", + which returned success from the SIGINT stage and skipped the SIGTERM and + SIGKILL escalation entirely. A step that backgrounds a process ignoring + SIGINT/SIGTERM (databases, daemons, `nohup`) therefore survived + cancellation, was reparented to init, and accumulated on the host across + runs. The group id is now captured at spawn and the escalation runs against + the group itself, matching `ProcessInvoker.cs`, which kills the remaining + process tree. The same gap in the stream-drain grace path is fixed too. + - SmolVM compatibility now comes from the central `versions.toml` `smolvm_min_version` pin. `preloop-cli` and `preloop-vm` compile the same floor, and `preloop update --ensure-runtime` installs the latest stable diff --git a/Cargo.lock b/Cargo.lock index 63a2e8ba..17b71afd 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2056,6 +2056,7 @@ dependencies = [ "glob", "hostname", "indexmap", + "nix", "parking_lot", "preloop-dap", "preloop-gha-expressions", diff --git a/crates/preloop-runner/Cargo.toml b/crates/preloop-runner/Cargo.toml index 60003cf3..4dcb8b0c 100644 --- a/crates/preloop-runner/Cargo.toml +++ b/crates/preloop-runner/Cargo.toml @@ -49,6 +49,14 @@ serde_yaml.workspace = true regex.workspace = true zip.workspace = true +# Cancellation signals the process group by id, because the child handle stops +# addressing the group once the leader is reaped. `nix` is the safe wrapper: +# the workspace forbids `unsafe`, which rules out raw `libc` calls. Already in +# the tree via command-group. Unix-only — `nix` does not build on Windows, +# where the `cfg(not(unix))` paths in `process.rs` apply instead. +[target.'cfg(unix)'.dependencies] +nix = { version = "0.27", features = ["signal", "process"] } + [dev-dependencies] tempfile.workspace = true proptest.workspace = true diff --git a/crates/preloop-runner/src/process.rs b/crates/preloop-runner/src/process.rs index 818a9e8e..e239030e 100644 --- a/crates/preloop-runner/src/process.rs +++ b/crates/preloop-runner/src/process.rs @@ -122,6 +122,12 @@ pub async fn invoke<'a>( .group_spawn() .with_context(|| format!("spawning {program}"))?; + // Capture the group id now, while the handle still reports one: the wait + // loop below reaps the leader the moment it exits, and a reaped handle + // addresses nothing. The group outlives its leader for as long as any + // member is alive, so cancellation has to escalate against this id. + let group = process_group(&child); + let stdout = child.inner().stdout.take(); let stderr = child.inner().stderr.take(); @@ -177,9 +183,7 @@ pub async fn invoke<'a>( } _ = tokio::time::sleep_until(stream_deadline.unwrap_or_else(tokio::time::Instant::now)), if stream_deadline.is_some() => { tracing::info!("Killing process group for {program} after redirected streams remained open for {:?}", STREAM_DRAIN_GRACE); - if let Err(error) = child.kill().await { - tracing::warn!("Failed to kill process group after stream drain timeout: {error}"); - } + force_kill_group(&mut child, group, program).await; forced_stream_close = true; break; } @@ -200,9 +204,7 @@ pub async fn invoke<'a>( }, _ = tokio::time::sleep_until(stream_deadline.unwrap_or_else(tokio::time::Instant::now)), if stream_deadline.is_some() => { tracing::info!("Killing process group for {program} after redirected streams remained open for {:?}", STREAM_DRAIN_GRACE); - if let Err(error) = child.kill().await { - tracing::warn!("Failed to kill process group after stream drain timeout: {error}"); - } + force_kill_group(&mut child, group, program).await; forced_stream_close = true; break; } @@ -212,7 +214,7 @@ pub async fn invoke<'a>( } if cancel_requested { - terminate_process_group(&mut child, program).await; + terminate_process_group(&mut child, group, program).await; drain_chunks( stdout_handle, stderr_handle, @@ -389,21 +391,91 @@ async fn drain_chunks( // ── Process group termination ─────────────────────────────────────────── -async fn terminate_process_group(child: &mut AsyncGroupChild, program: &str) { - if graceful_signal(child, program, ProcessSignal::Interrupt, SIGINT_GRACE).await { +async fn terminate_process_group(child: &mut AsyncGroupChild, group: ProcessGroup, program: &str) { + if graceful_signal( + child, + group, + program, + ProcessSignal::Interrupt, + SIGINT_GRACE, + ) + .await + { tracing::info!("Process group for {program} exited after SIGINT"); return; } - if graceful_signal(child, program, ProcessSignal::Terminate, SIGTERM_GRACE).await { + if graceful_signal( + child, + group, + program, + ProcessSignal::Terminate, + SIGTERM_GRACE, + ) + .await + { tracing::info!("Process group for {program} exited after SIGTERM"); return; } tracing::info!("Killing process group for {program} after SIGINT/SIGTERM grace expired"); - if let Err(e) = child.kill().await { - tracing::warn!("Failed to kill process group: {e}"); + force_kill_group(child, group, program).await; +} + +/// Hard-kill every surviving member of the group, then reap the leader. +/// +/// `AsyncGroupChild::kill` can only reach a leader that is still unreaped, so +/// on its own it leaves a backgrounded descendant running — reparented to +/// init and outliving the job that spawned it. Sweeping the group is what +/// makes this match `ProcessInvoker.cs`, which kills the remaining tree. +async fn force_kill_group(child: &mut AsyncGroupChild, group: ProcessGroup, program: &str) { + #[cfg(unix)] + if let Some(group) = group { + signal_group(group, Signal::SIGKILL); + } + #[cfg(not(unix))] + let _ = group; + + if let Err(error) = child.kill().await { + // Routine once the wait loop has already collected the leader. + tracing::debug!("Leader for {program} was already reaped: {error}"); + } +} + +/// The process group cancellation escalates against, or `None` when none can +/// be addressed safely. +#[cfg(unix)] +type ProcessGroup = Option; +#[cfg(not(unix))] +type ProcessGroup = (); + +#[cfg(unix)] +fn process_group(child: &AsyncGroupChild) -> ProcessGroup { + let id = i32::try_from(child.id()?).ok()?; + // `killpg` reads a non-positive id as "my own group", which would signal + // the runner itself. `group_spawn` makes the child its own group leader, + // so an id matching our own group means it never got one. + if id <= 0 || id == nix::unistd::getpgrp().as_raw() { + return None; } + Some(nix::unistd::Pid::from_raw(id)) +} + +#[cfg(not(unix))] +fn process_group(_child: &AsyncGroupChild) -> ProcessGroup {} + +/// Deliver `signal` to every member of `group`. A failure means the group has +/// already drained, which is the outcome the caller wanted anyway. +#[cfg(unix)] +fn signal_group(group: nix::unistd::Pid, signal: Signal) { + let _ = nix::sys::signal::killpg(group, signal); +} + +/// Whether any member of `group` is still alive. The null signal runs the +/// existence and permission checks without delivering anything. +#[cfg(unix)] +fn group_alive(group: nix::unistd::Pid) -> bool { + nix::sys::signal::killpg(group, None).is_ok() } #[derive(Copy, Clone)] @@ -414,11 +486,12 @@ enum ProcessSignal { async fn graceful_signal( child: &mut AsyncGroupChild, + group: ProcessGroup, program: &str, signal: ProcessSignal, timeout: Duration, ) -> bool { - if let Err(e) = send_signal(child, signal) { + if let Err(e) = send_signal(child, group, signal) { tracing::warn!( "Failed to send {} to process group for {program}: {e}", signal.name() @@ -426,7 +499,7 @@ async fn graceful_signal( return false; } - wait_for_exit(child, timeout).await + wait_for_group_exit(child, group, timeout).await } impl ProcessSignal { @@ -439,29 +512,72 @@ impl ProcessSignal { } #[cfg(unix)] -fn send_signal(child: &AsyncGroupChild, signal: ProcessSignal) -> std::io::Result<()> { +fn send_signal( + child: &AsyncGroupChild, + group: ProcessGroup, + signal: ProcessSignal, +) -> std::io::Result<()> { let signal = match signal { ProcessSignal::Interrupt => Signal::SIGINT, ProcessSignal::Terminate => Signal::SIGTERM, }; + + // Address the group id directly: once the leader is reaped the child + // handle can no longer reach the members that are still running. + if let Some(group) = group { + signal_group(group, signal); + return Ok(()); + } + child.signal(signal) } #[cfg(not(unix))] -fn send_signal(child: &mut AsyncGroupChild, _signal: ProcessSignal) -> std::io::Result<()> { +fn send_signal( + child: &mut AsyncGroupChild, + _group: ProcessGroup, + _signal: ProcessSignal, +) -> std::io::Result<()> { child.start_kill() } -async fn wait_for_exit(child: &mut AsyncGroupChild, timeout: Duration) -> bool { +/// Wait until the whole group has drained, not merely the leader. +/// +/// The leader is reaped as soon as it exits so its zombie cannot keep the +/// group artificially alive, and only then is the group probed. Treating the +/// leader's exit as "the group is gone" is precisely what let a backgrounded +/// descendant outlive cancellation. +async fn wait_for_group_exit( + child: &mut AsyncGroupChild, + group: ProcessGroup, + timeout: Duration, +) -> bool { let deadline = tokio::time::Instant::now() + timeout; loop { - match child.try_wait() { - Ok(Some(_)) => return true, - Ok(None) => {} + let leader_gone = match child.try_wait() { + Ok(status) => status.is_some(), Err(e) => { tracing::warn!("Failed to poll process group status after signal: {e}"); return false; } + }; + + #[cfg(unix)] + { + match group { + Some(group) if !group_alive(group) => return true, + // With no addressable group the leader's status is all there is. + None if leader_gone => return true, + _ => {} + } + } + + #[cfg(not(unix))] + { + let _ = group; + if leader_gone { + return true; + } } let now = tokio::time::Instant::now(); @@ -703,6 +819,19 @@ mod tests { #[tokio::test] async fn cancellation_interrupts_background_child_after_shell_exit() { + // The shell exits within a millisecond, so cancellation always lands + // after the leader has been reaped, and the backgrounded child ignores + // both graceful signals — SIGKILL against the group is the only thing + // that can reap it. Assert the child actually died rather than + // trusting the error string: a survivor is reparented to init and + // silently outlives the job. + let dir = tempfile::tempdir().expect("tempdir"); + let pid_path = dir.path().join("background.pid"); + let script = format!( + "(trap '' TERM INT; while :; do sleep 1; done) & echo $! > {}; echo ready", + pid_path.display() + ); + let (cancel_tx, cancel_rx) = watch::channel(false); let cancel_task = tokio::spawn(async move { tokio::time::sleep(Duration::from_millis(150)).await; @@ -710,13 +839,10 @@ mod tests { }); let result = tokio::time::timeout( - Duration::from_secs(2), + Duration::from_secs(5), invoke( "sh", - &[ - "-c", - "(trap '' TERM; while :; do sleep 1; done) & echo ready", - ], + &["-c", &script], Path::new("."), &HashMap::new(), None, @@ -732,6 +858,26 @@ mod tests { .unwrap_err() .to_string() .contains("process cancelled")); + + let pid = nix::unistd::Pid::from_raw( + std::fs::read_to_string(&pid_path) + .expect("background pid file") + .trim() + .parse() + .expect("background pid"), + ); + + // The sweep, the reparent, and init's reap all race this assertion. + let mut survived = true; + for _ in 0..100 { + // The null signal only probes for existence. + if nix::sys::signal::kill(pid, None).is_err() { + survived = false; + break; + } + tokio::time::sleep(Duration::from_millis(20)).await; + } + assert!(!survived, "background child {pid} survived cancellation"); } #[tokio::test]