From 9d126ede20c2b6a38ad660d1f5c27fb48f9535a1 Mon Sep 17 00:00:00 2001 From: Val Alexander Date: Fri, 14 Aug 2026 09:39:41 -0500 Subject: [PATCH] fix: terminate timed-out worker process groups Signed-off-by: Codex --- Cargo.lock | 1 + Cargo.toml | 1 + crates/worker/Cargo.toml | 1 + crates/worker/src/backend.rs | 57 +++++++++++++++++++++++++++++------- crates/worker/src/lib.rs | 11 ++++++- 5 files changed, 59 insertions(+), 12 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 49fc478..f046e99 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -402,6 +402,7 @@ dependencies = [ "coven-github-config", "coven-github-gardener", "coven-github-store", + "libc", "serde", "serde_json", "tokio", diff --git a/Cargo.toml b/Cargo.toml index 9c1fa1c..35276bf 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -25,6 +25,7 @@ clap = { version = "4", features = ["derive"] } chrono = "0.4" hmac = "0.12" jsonwebtoken = "9" +libc = "0.2" octocrab = "0.41" reqwest = { version = "0.12", features = ["json"] } rusqlite = { version = "0.32", features = ["bundled"] } diff --git a/crates/worker/Cargo.toml b/crates/worker/Cargo.toml index 5ec06a2..e37bfca 100644 --- a/crates/worker/Cargo.toml +++ b/crates/worker/Cargo.toml @@ -6,6 +6,7 @@ license.workspace = true [dependencies] anyhow.workspace = true +libc.workspace = true serde.workspace = true serde_json.workspace = true tokio.workspace = true diff --git a/crates/worker/src/backend.rs b/crates/worker/src/backend.rs index 839bc20..2bb7524 100644 --- a/crates/worker/src/backend.rs +++ b/crates/worker/src/backend.rs @@ -96,7 +96,8 @@ impl Backend { // Git auth is injected via the environment, never written // to the session brief or any durable artifact (issue #4). .env("COVEN_GIT_TOKEN", git_token); - await_child(command, timeout, None).await + configure_host_process_group(&mut command); + await_child(command, timeout, KillTarget::ProcessGroup).await } Backend::Container(container) => { let name = container_name(task_id); @@ -109,7 +110,7 @@ impl Backend { docker_bin: container.docker_bin.clone(), name, }; - await_child(command, timeout, Some(kill)).await + await_child(command, timeout, KillTarget::Container(kill)).await } } } @@ -121,6 +122,23 @@ struct KillSpec { name: String, } +/// What must be terminated when the wall-clock limit expires. +enum KillTarget { + /// The host process is started as its own process-group leader, so killing + /// the group also terminates every descendant it spawned. + ProcessGroup, + /// Docker needs both its CLI process and the named container terminated. + Container(KillSpec), +} + +#[cfg(unix)] +fn configure_host_process_group(command: &mut Command) { + command.process_group(0); +} + +#[cfg(not(unix))] +fn configure_host_process_group(_command: &mut Command) {} + /// Container name for one task attempt. Unique per attempt: docker rejects /// duplicate names, so a stale name must never collide with a retry. fn container_name(task_id: &str) -> String { @@ -181,11 +199,7 @@ pub fn docker_run_args( args } -async fn await_child( - mut command: Command, - timeout: Duration, - kill: Option, -) -> LaunchOutcome { +async fn await_child(mut command: Command, timeout: Duration, kill: KillTarget) -> LaunchOutcome { let mut child = match command.spawn() { Ok(child) => child, Err(e) => return LaunchOutcome::Failed(format!("failed to spawn session: {e}")), @@ -194,12 +208,15 @@ async fn await_child( Ok(Ok(status)) => LaunchOutcome::Exited(status.code()), Ok(Err(e)) => LaunchOutcome::Failed(format!("failed to await session: {e}")), Err(_) => { - // Kill the CLI process first… - let _ = child.kill().await; + if matches!(kill, KillTarget::ProcessGroup) { + kill_process_group(&mut child).await; + } else { + let _ = child.kill().await; + } let _ = child.wait().await; - // …then the container itself: killing the docker CLI does not + // Killing the docker CLI does not // reliably stop the container it launched. - if let Some(kill) = kill { + if let KillTarget::Container(kill) = kill { match Command::new(&kill.docker_bin) .args(["kill", &kill.name]) .output() @@ -214,6 +231,24 @@ async fn await_child( } } +#[cfg(unix)] +async fn kill_process_group(child: &mut tokio::process::Child) { + if let Some(pid) = child.id() { + // SAFETY: `pid` belongs to the child we spawned as a new process-group + // leader. A negative pid asks kill(2) to signal that entire group. + unsafe { + libc::kill(-(pid as i32), libc::SIGKILL); + } + } else { + let _ = child.kill().await; + } +} + +#[cfg(not(unix))] +async fn kill_process_group(child: &mut tokio::process::Child) { + let _ = child.kill().await; +} + #[cfg(test)] mod tests { use super::*; diff --git a/crates/worker/src/lib.rs b/crates/worker/src/lib.rs index 796a3e9..0257054 100644 --- a/crates/worker/src/lib.rs +++ b/crates/worker/src/lib.rs @@ -2092,7 +2092,10 @@ mod process_tests { #[tokio::test] async fn coven_code_process_is_stopped_after_configured_timeout() { - let (root, script) = scratch("timeout-test", "#!/usr/bin/env bash\nsleep 5\n"); + let (root, script) = scratch( + "timeout-test", + "#!/usr/bin/env bash\n(sleep 2; touch \"$(dirname \"$5\")/descendant-survived\") &\nwait\n", + ); let mut config = test_config(script, root.clone(), 0); // This test specifically exercises the kill-on-timeout path. config.worker.timeout_secs = 1; @@ -2108,6 +2111,12 @@ mod process_tests { "process should stop close to the configured timeout" ); + tokio::time::sleep(Duration::from_secs(2)).await; + assert!( + !root.join("descendant-survived").exists(), + "timeout must terminate the entire process group" + ); + let _ = fs::remove_dir_all(root); }