diff --git a/server/src/agents/pool/pipeline/completion/server.rs b/server/src/agents/pool/pipeline/completion/server.rs index 438e68fd..aaf5a371 100644 --- a/server/src/agents/pool/pipeline/completion/server.rs +++ b/server/src/agents/pool/pipeline/completion/server.rs @@ -74,25 +74,11 @@ pub(in crate::agents::pool) async fn run_server_owned_completion( // Kill any in-flight cargo test processes for this worktree so they don't // hold the build lock while gates try to run. - if let Some(wt_path) = worktree_path.as_ref() - && let Ok(output) = std::process::Command::new("pgrep") - .args([ - "-f", - &format!("--manifest-path {}/Cargo.toml", wt_path.display()), - ]) - .output() - { - let pids = String::from_utf8_lossy(&output.stdout); - for pid_str in pids.lines() { - if let Ok(pid) = pid_str.trim().parse::() { - crate::slog!( - "[agents] Killing stale cargo process (pid {pid}) for '{story_id}' before running gates" - ); - unsafe { - libc::kill(pid, libc::SIGKILL); - } - } - } + if let Some(wt_path) = worktree_path.as_ref() { + let pattern = format!("--manifest-path {}/Cargo.toml", wt_path.display()); + let _ = crate::process_kill::sigkill_pids_and_verify(&crate::process_kill::pids_matching( + &pattern, + )); } // Run acceptance gates. Third element of the tuple is `needs_commit_recovery`: diff --git a/server/src/process_kill.rs b/server/src/process_kill.rs index 261b43fa..83451020 100644 --- a/server/src/process_kill.rs +++ b/server/src/process_kill.rs @@ -91,8 +91,10 @@ pub fn sigkill_pids_and_verify(pids: &[u32]) -> Result> { /// "every process running in `/`" or "every cargo invocation /// against this `Cargo.toml`". pub fn pids_matching(pattern: &str) -> Vec { + // `--` prevents pgrep (getopt_long-based) from interpreting patterns that + // start with `--` (e.g. `--manifest-path ...`) as unrecognised long options. let Ok(output) = std::process::Command::new("pgrep") - .args(["-f", pattern]) + .args(["-f", "--", pattern]) .output() else { return Vec::new(); @@ -267,6 +269,50 @@ mod tests { let _ = reaper.join(); } + /// Verify that the merge-gate's stale-cargo kill pattern actually kills + /// processes whose command line contains `--manifest-path .../Cargo.toml`. + #[test] + fn stale_cargo_pattern_kills_matching_process() { + use std::os::unix::process::CommandExt; + let unique = format!("{}-{}", std::process::id(), rand_u64()); + let fake_manifest = format!("/tmp/huskies-test-{unique}/Cargo.toml"); + let argv0 = format!("cargo test --manifest-path {fake_manifest}"); + + let child: Child = Command::new("sleep") + .arg0(&argv0) + .arg("60") + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .stdin(Stdio::null()) + .spawn() + .expect("failed to spawn sleeper"); + let pid = child.id(); + let reaper = std::thread::spawn(move || { + let mut c = child; + let _ = c.wait(); + }); + + std::thread::sleep(std::time::Duration::from_millis(100)); + + let pattern = format!("--manifest-path {fake_manifest}"); + let pids = pids_matching(&pattern); + assert!( + pids.contains(&pid), + "pids_matching must find the stale-cargo-like process (pid {pid}); got {pids:?}" + ); + + let result = sigkill_pids_and_verify(&pids); + assert!( + result.is_ok(), + "sigkill_pids_and_verify must succeed for stale cargo pids: {result:?}" + ); + assert!( + !pid_is_alive(pid), + "stale cargo-like process (pid {pid}) must be dead after kill" + ); + let _ = reaper.join(); + } + #[test] fn pids_matching_returns_empty_when_no_match() { let pattern = format!("nonexistent-pattern-{}-{}", std::process::id(), rand_u64());