diff --git a/src/supervisor/mod.rs b/src/supervisor/mod.rs index 63f1ea6c..79714712 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,34 @@ 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) { + terminate_process_group(child); +} + fn prepare_supervisor_child_tmpdir() -> Result { let preferred = std::env::var_os("TMPDIR") .map(PathBuf::from) @@ -2307,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; } @@ -2329,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; } @@ -2340,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)] @@ -4224,4 +4259,90 @@ 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(5), + "timed wait should return well before the 30s sleep, 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()); + } + + #[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" + ); + } }