From 05ef27e4332d9f0e41e9410684687837063a5f5a Mon Sep 17 00:00:00 2001 From: Sebastien Tardif Date: Wed, 2 Sep 2026 13:12:22 -0700 Subject: [PATCH 1/4] fix(supervisor): bound run_once child wait ocm service --once blocked forever on child.wait() if an env child never exited. Wait with a deadline and kill the child on timeout, matching the handoff wait helper. Signed-off-by: Sebastien Tardif (cherry picked from commit 40ce43ff592c9aebd95b88cb97b7d84848590065) --- src/supervisor/mod.rs | 101 ++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 97 insertions(+), 4 deletions(-) diff --git a/src/supervisor/mod.rs b/src/supervisor/mod.rs index 63f1ea6c..e1af6671 100644 --- a/src/supervisor/mod.rs +++ b/src/supervisor/mod.rs @@ -5,7 +5,7 @@ use std::os::unix::fs::{MetadataExt, PermissionsExt}; #[cfg(unix)] use std::os::unix::process::CommandExt; use std::path::{Path, PathBuf}; -use std::process::{Child, Command, Output, Stdio}; +use std::process::{Child, Command, ExitStatus, Output, Stdio}; use std::sync::{ Arc, atomic::{AtomicBool, Ordering}, @@ -73,6 +73,7 @@ const SUPERVISED_CHILD_RUNTIME_ENV_KEYS: [&str; 4] = const SERVICE_EXECUTABLE_OVERRIDE: &str = "OCM_SERVICE_EXECUTABLE"; pub(crate) const SERVICE_EXECUTABLE_IDENTITY: &str = "ocm-service-supervisor"; const SERVICE_EXECUTABLE_IDENTITY_TIMEOUT_MS: u64 = 1_000; +const SERVICE_ONCE_CHILD_TIMEOUT_MS: u64 = 15_000; const SERVICE_EXECUTABLE_IDENTITY_BUSY_ATTEMPTS: usize = 5; const SERVICE_EXECUTABLE_IDENTITY_BUSY_RETRY_MS: u64 = 20; #[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)] @@ -1067,9 +1068,11 @@ impl<'a> SupervisorService<'a> { // admission until the child exits instead of exposing an unseen child. let _admission = admit_supervisor_child_start(&spec, self.env, self.cwd)?; let mut child = spawn_supervisor_child(&spec)?; - let status = child.wait().map_err(|error| { - format!("failed waiting for env \"{}\": {error}", spec.env_name) - })?; + let status = wait_child_with_timeout( + &mut child, + Duration::from_millis(SERVICE_ONCE_CHILD_TIMEOUT_MS), + &format!("env \"{}\"", spec.env_name), + )?; child_results.push(child_run_result(&spec, status.code(), 0)); } @@ -1359,6 +1362,56 @@ fn spawn_supervisor_child(spec: &SupervisorChildSpec) -> Result { }) } +fn wait_child_with_timeout( + child: &mut Child, + timeout: Duration, + label: &str, +) -> Result { + let started_at = Instant::now(); + loop { + match child.try_wait() { + Ok(Some(status)) => return Ok(status), + Ok(None) if started_at.elapsed() < timeout => { + sleep(Duration::from_millis(25)); + } + Ok(None) => { + terminate_child(child); + return Err(format!("{label} timed out after {}ms", timeout.as_millis())); + } + Err(error) => { + terminate_child(child); + return Err(format!("failed waiting for {label}: {error}")); + } + } + } +} + +fn terminate_child(child: &mut Child) { + #[cfg(unix)] + { + let process_group = format!("-{}", child.id()); + let _ = Command::new("kill") + .args(["-TERM", "--", &process_group]) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .status(); + for _ in 0..20 { + match child.try_wait() { + Ok(Some(_)) => return, + Ok(None) => sleep(Duration::from_millis(25)), + Err(_) => break, + } + } + let _ = Command::new("kill") + .args(["-KILL", "--", &process_group]) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .status(); + } + let _ = child.kill(); + let _ = child.wait(); +} + fn prepare_supervisor_child_tmpdir() -> Result { let preferred = std::env::var_os("TMPDIR") .map(PathBuf::from) @@ -4224,4 +4277,44 @@ mod tests { assert!(!process_env.contains_key("GH_TOKEN")); assert!(!process_env.contains_key("PWD")); } + + #[cfg(unix)] + #[test] + fn wait_child_with_timeout_kills_sleep_after_deadline() { + let started = Instant::now(); + let mut child = Command::new("/bin/sleep") + .arg("30") + .spawn() + .expect("spawn /bin/sleep 30"); + let pid = child.id(); + let error = wait_child_with_timeout(&mut child, Duration::from_millis(200), "sleep") + .expect_err("sleep should be killed at the deadline"); + let elapsed = started.elapsed(); + assert!( + elapsed < Duration::from_secs(1), + "timed wait should return within 1s, took {elapsed:?}" + ); + assert!( + error.contains("timed out"), + "expected timeout error, got {error}" + ); + let still_alive = Command::new("kill") + .args(["-0", "--", &pid.to_string()]) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .status() + .is_ok_and(|status| status.success()); + assert!(!still_alive, "child {pid} should be dead after timeout"); + } + + #[cfg(unix)] + #[test] + fn wait_child_with_timeout_returns_status_when_child_exits() { + let mut child = Command::new("/usr/bin/true") + .spawn() + .expect("spawn /usr/bin/true"); + let status = wait_child_with_timeout(&mut child, Duration::from_millis(200), "true") + .expect("exited child should not time out"); + assert!(status.success()); + } } From d9e51cbe1ae75f66978c43226af7f1cc9771ca83 Mon Sep 17 00:00:00 2001 From: Sebastien Tardif Date: Wed, 2 Sep 2026 15:49:11 -0700 Subject: [PATCH 2/4] test(supervisor): give macOS more time to reap a timed-out sleep The 200ms deadline plus SIGTERM grace can exceed 1s on macos-latest. Keep proving we do not wait the full 30s sleep. Signed-off-by: Sebastien Tardif (cherry picked from commit c85f0e65004d99f6d74d9ec0fd60d39e6047b0fd) --- src/supervisor/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/supervisor/mod.rs b/src/supervisor/mod.rs index e1af6671..45e38053 100644 --- a/src/supervisor/mod.rs +++ b/src/supervisor/mod.rs @@ -4291,8 +4291,8 @@ mod tests { .expect_err("sleep should be killed at the deadline"); let elapsed = started.elapsed(); assert!( - elapsed < Duration::from_secs(1), - "timed wait should return within 1s, took {elapsed:?}" + elapsed < Duration::from_secs(5), + "timed wait should return well before the 30s sleep, took {elapsed:?}" ); assert!( error.contains("timed out"), From 07333f28756d2ca5f7854169e5756b5a8ffd8b0f Mon Sep 17 00:00:00 2001 From: Sebastien Tardif Date: Fri, 11 Sep 2026 09:41:58 -0700 Subject: [PATCH 3/4] ci: retrigger after macos Test flake dev_stop_acknowledgement_refuses_live_recorded_ownership failed once on macos-latest; the same test passed on #117 and #136 from the same main. This PR does not touch that test. Signed-off-by: Sebastien Tardif From a8df5b3b2a43f5b77a0171305531be01c417b816 Mon Sep 17 00:00:00 2001 From: Sebastien Tardif Date: Fri, 18 Sep 2026 10:55:11 -0700 Subject: [PATCH 4/4] fix(supervisor): KILL leftover descendants on run_once timeout terminate_child returned when the group leader exited after TERM, so a SIGTERM-ignoring grandchild stayed alive after run_once released admission. Reuse the existing process-group shutdown used by stop_supervisor_child. The 15s once-mode default is unchanged. Signed-off-by: Sebastien Tardif --- src/supervisor/mod.rs | 84 ++++++++++++++++++++++++++++--------------- 1 file changed, 56 insertions(+), 28 deletions(-) diff --git a/src/supervisor/mod.rs b/src/supervisor/mod.rs index 45e38053..79714712 100644 --- a/src/supervisor/mod.rs +++ b/src/supervisor/mod.rs @@ -1387,29 +1387,7 @@ fn wait_child_with_timeout( } fn terminate_child(child: &mut Child) { - #[cfg(unix)] - { - let process_group = format!("-{}", child.id()); - let _ = Command::new("kill") - .args(["-TERM", "--", &process_group]) - .stdout(Stdio::null()) - .stderr(Stdio::null()) - .status(); - for _ in 0..20 { - match child.try_wait() { - Ok(Some(_)) => return, - Ok(None) => sleep(Duration::from_millis(25)), - Err(_) => break, - } - } - let _ = Command::new("kill") - .args(["-KILL", "--", &process_group]) - .stdout(Stdio::null()) - .stderr(Stdio::null()) - .status(); - } - let _ = child.kill(); - let _ = child.wait(); + terminate_process_group(child); } fn prepare_supervisor_child_tmpdir() -> Result { @@ -2360,16 +2338,20 @@ fn start_due_children( } fn stop_supervisor_child(running_child: &mut RunningSupervisorChild) { + terminate_process_group(&mut running_child.child); +} + +fn terminate_process_group(child: &mut Child) { #[cfg(unix)] { - let process_group = format!("-{}", running_child.child.id()); + let process_group = format!("-{}", child.id()); let _ = Command::new("kill") .args(["-TERM", "--", &process_group]) .stdout(Stdio::null()) .stderr(Stdio::null()) .status(); for _ in 0..20 { - let _ = running_child.child.try_wait(); + let _ = child.try_wait(); if !supervisor_process_group_exists(&process_group) { break; } @@ -2382,7 +2364,7 @@ fn stop_supervisor_child(running_child: &mut RunningSupervisorChild) { .stderr(Stdio::null()) .status(); for _ in 0..20 { - let _ = running_child.child.try_wait(); + let _ = child.try_wait(); if !supervisor_process_group_exists(&process_group) { break; } @@ -2393,8 +2375,8 @@ fn stop_supervisor_child(running_child: &mut RunningSupervisorChild) { // Always wait on the process-group leader. It can exit after try_wait() // reports None but before the group-existence probe; returning in that // window leaves a zombie that can block OpenClaw's single-instance lock. - let _ = running_child.child.kill(); - let _ = running_child.child.wait(); + let _ = child.kill(); + let _ = child.wait(); } #[cfg(unix)] @@ -4317,4 +4299,50 @@ mod tests { .expect("exited child should not time out"); assert!(status.success()); } + + #[cfg(unix)] + #[test] + fn wait_child_with_timeout_kills_term_resistant_descendant() { + use std::os::unix::process::CommandExt; + + let pidfile = + std::env::temp_dir().join(format!("ocm-once-descendant-{}.pid", std::process::id())); + let _ = fs::remove_file(&pidfile); + let mut child = Command::new("perl") + .env("OCM_DESCENDANT_PIDFILE", &pidfile) + .args([ + "-e", + "if (fork()) { sleep 30; exit 0 } $SIG{TERM}='IGNORE'; open F, '>', $ENV{OCM_DESCENDANT_PIDFILE} or die; print F \"$$\\n\"; close F; sleep 30", + ]) + .process_group(0) + .spawn() + .expect("spawn perl descendant holder"); + let error = wait_child_with_timeout(&mut child, Duration::from_millis(800), "pipe-hold") + .expect_err("leader should time out"); + assert!( + error.contains("timed out"), + "expected timeout error, got {error}" + ); + let pid = fs::read_to_string(&pidfile) + .unwrap_or_default() + .trim() + .parse::() + .expect("descendant pid file"); + let _ = fs::remove_file(&pidfile); + let alive = Command::new("kill") + .args(["-0", "--", &pid.to_string()]) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .status() + .is_ok_and(|status| status.success()); + if alive { + let _ = Command::new("kill") + .args(["-KILL", "--", &pid.to_string()]) + .status(); + } + assert!( + !alive, + "TERM-resistant descendant {pid} should be gone after group KILL" + ); + } }