From ef3228b58fbc16ac99b34ac84cfef91f073af00f Mon Sep 17 00:00:00 2001 From: Sebastien Tardif Date: Wed, 2 Sep 2026 13:01:19 -0700 Subject: [PATCH 1/2] fix: bound git worktree helper subprocesses ensure_openclaw_worktree and related helpers used Command::output() with no deadline. A stuck git lock blocked ocm setup and cleanup. Wait with a timeout and kill the child. Signed-off-by: Sebastien Tardif (cherry picked from commit da84e0265a82f36bf9b319bd7eeb3aa108f05879) --- src/infra/process.rs | 100 ++++++++++++++++++- src/openclaw_repo.rs | 155 +++++++++++++++++------------ src/supervisor/openclaw_handoff.rs | 57 ++--------- 3 files changed, 198 insertions(+), 114 deletions(-) diff --git a/src/infra/process.rs b/src/infra/process.rs index 3dc7ce2a..dd6331f6 100644 --- a/src/infra/process.rs +++ b/src/infra/process.rs @@ -1,6 +1,8 @@ use std::collections::BTreeMap; use std::path::Path; -use std::process::{Command, Stdio}; +use std::process::{Child, Command, ExitStatus, Output, Stdio}; +use std::thread; +use std::time::{Duration, Instant}; pub fn run_direct( command: &str, @@ -28,3 +30,99 @@ pub fn run_shell(command: &str, env: &BTreeMap, cwd: &Path) -> R run_direct("sh", &["-lc".to_string(), command.to_string()], env, cwd) } } + +/// Run a command, capture output, and kill the child if it exceeds `timeout`. +pub(crate) fn command_output( + mut command: Command, + timeout: Duration, + label: &str, +) -> Result { + command.stdin(Stdio::null()); + command.stdout(Stdio::piped()); + command.stderr(Stdio::piped()); + #[cfg(unix)] + { + use std::os::unix::process::CommandExt; + command.process_group(0); + } + + let mut child = command + .spawn() + .map_err(|error| format!("failed to run {label}: {error}"))?; + let stdout = child + .stdout + .take() + .ok_or_else(|| format!("{label} stdout was not captured"))?; + let stderr = child + .stderr + .take() + .ok_or_else(|| format!("{label} stderr was not captured"))?; + + let stdout_reader = thread::spawn(move || read_pipe(stdout)); + let stderr_reader = thread::spawn(move || read_pipe(stderr)); + + let status = wait_for_child(&mut child, timeout, label); + let stdout = stdout_reader + .join() + .map_err(|_| format!("{label} stdout reader panicked"))?; + let stderr = stderr_reader + .join() + .map_err(|_| format!("{label} stderr reader panicked"))?; + Ok(Output { + status: status?, + stdout, + stderr, + }) +} + +fn read_pipe(mut reader: impl std::io::Read) -> Vec { + let mut buf = Vec::new(); + let _ = reader.read_to_end(&mut buf); + buf +} + +pub(crate) fn wait_for_child( + 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 => { + thread::sleep(Duration::from_millis(25)); + } + Ok(None) => { + terminate_child(child); + return Err(format!("{label} timed out after {timeout:?}")); + } + 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]) + .status(); + for _ in 0..20 { + match child.try_wait() { + Ok(Some(_)) => return, + Ok(None) => thread::sleep(Duration::from_millis(25)), + Err(_) => break, + } + } + let _ = Command::new("kill") + .args(["-KILL", "--", &process_group]) + .status(); + } + let _ = child.kill(); + let _ = child.wait(); +} diff --git a/src/openclaw_repo.rs b/src/openclaw_repo.rs index f37b207d..a984fcaa 100644 --- a/src/openclaw_repo.rs +++ b/src/openclaw_repo.rs @@ -1,15 +1,17 @@ use std::collections::BTreeMap; -use std::ffi::OsString; +use std::ffi::{OsStr, OsString}; use std::fs; use std::io::Read; use std::path::{Path, PathBuf}; -use std::process::{Command, Stdio}; +use std::process::{Command, Output, Stdio}; +use std::time::Duration; #[cfg(unix)] use std::os::unix::ffi::OsStringExt; use serde_json::Value; +use crate::infra::process::command_output; use crate::store::{clean_path, dev_sources::path_identity, display_path}; const SOURCE_DEPENDENCY_PROBE: &str = r#"import fs from "node:fs"; @@ -65,6 +67,17 @@ for (const [name, specifier] of requirements) { } process.stdout.write(JSON.stringify(issues));"#; +const GIT_COMMAND_TIMEOUT: Duration = Duration::from_secs(15); + +fn git_output( + cwd: &Path, + args: impl IntoIterator>, +) -> Result { + let mut command = git_command(); + command.arg("-C").arg(cwd).args(args); + command_output(command, GIT_COMMAND_TIMEOUT, "git") +} + pub(crate) fn detect_openclaw_checkout(path: &Path) -> Option { let package_json = path.join("package.json"); let scripts_dir = path.join("scripts"); @@ -483,13 +496,16 @@ pub(crate) fn ensure_openclaw_worktree( let worktree_argument = worktree_root .strip_prefix(&repo_root) .map_err(|_| "OCM-owned worktree destination is outside its repository".to_string())?; - let output = git_command() - .arg("-C") - .arg(&repo_root) - .args(["worktree", "add", "--detach"]) - .arg(worktree_argument) - .output() - .map_err(|error| format!("failed to run git worktree add: {error}"))?; + let output = git_output( + &repo_root, + [ + OsStr::new("worktree"), + OsStr::new("add"), + OsStr::new("--detach"), + worktree_argument.as_os_str(), + ], + ) + .map_err(|error| format!("failed to run git worktree add: {error}"))?; if !output.status.success() { let stderr = String::from_utf8_lossy(&output.stderr).trim().to_string(); let stdout = String::from_utf8_lossy(&output.stdout).trim().to_string(); @@ -579,10 +595,9 @@ fn remove_generated_simulation_outputs(worktree_root: &Path) -> Result<(), Strin return Ok(()); } - let output = git_command() - .arg("-C") - .arg(worktree_root) - .args([ + let output = git_output( + worktree_root, + [ "clean", "-ffdX", "--", @@ -596,9 +611,9 @@ fn remove_generated_simulation_outputs(worktree_root: &Path) -> Result<(), Strin "extensions/diffs-language-pack/assets", "extensions/diffs/assets", "extensions/discord/assets", - ]) - .output() - .map_err(|error| format!("failed to remove generated simulation output: {error}"))?; + ], + ) + .map_err(|error| format!("failed to remove generated simulation output: {error}"))?; if output.status.success() { return Ok(()); } @@ -614,13 +629,16 @@ fn remove_generated_simulation_outputs(worktree_root: &Path) -> Result<(), Strin fn remove_registered_worktree(repo_root: &Path, worktree_root: &Path) -> Result<(), String> { ensure_worktree_clean(worktree_root)?; - let output = git_command() - .arg("-C") - .arg(repo_root) - .args(["worktree", "remove", "--force"]) - .arg(worktree_root) - .output() - .map_err(|error| format!("failed to run git worktree remove: {error}"))?; + let output = git_output( + repo_root, + [ + OsStr::new("worktree"), + OsStr::new("remove"), + OsStr::new("--force"), + worktree_root.as_os_str(), + ], + ) + .map_err(|error| format!("failed to run git worktree remove: {error}"))?; if output.status.success() { return Ok(()); } @@ -636,18 +654,18 @@ fn ensure_worktree_clean(worktree_root: &Path) -> Result<(), String> { return Ok(()); } - let output = git_command() - .args(["-c", "status.showUntrackedFiles=all"]) - .arg("-C") - .arg(worktree_root) - .args([ + let output = git_output( + worktree_root, + [ + "-c", + "status.showUntrackedFiles=all", "status", "--porcelain=v1", "--untracked-files=all", "--ignore-submodules=none", - ]) - .output() - .map_err(|error| format!("failed to inspect git worktree status: {error}"))?; + ], + ) + .map_err(|error| format!("failed to inspect git worktree status: {error}"))?; if !output.status.success() { let stderr = String::from_utf8_lossy(&output.stderr).trim().to_string(); let stdout = String::from_utf8_lossy(&output.stdout).trim().to_string(); @@ -666,18 +684,17 @@ fn ensure_worktree_clean(worktree_root: &Path) -> Result<(), String> { } fn ensure_no_ignored_local_files(worktree_root: &Path) -> Result<(), String> { - let worktree_output = git_command() - .arg("-C") - .arg(worktree_root) - .args([ + let worktree_output = git_output( + worktree_root, + [ "ls-files", "--others", "--ignored", "--exclude-standard", "-z", - ]) - .output() - .map_err(|error| format!("failed to inspect ignored worktree files: {error}"))?; + ], + ) + .map_err(|error| format!("failed to inspect ignored worktree files: {error}"))?; if !worktree_output.status.success() { let stderr = String::from_utf8_lossy(&worktree_output.stderr) .trim() @@ -689,18 +706,17 @@ fn ensure_no_ignored_local_files(worktree_root: &Path) -> Result<(), String> { return Err(format!("git ignored-file inspection failed: {detail}")); } - let submodule_output = git_command() - .arg("-C") - .arg(worktree_root) - .args([ + let submodule_output = git_output( + worktree_root, + [ "submodule", "foreach", "--quiet", "--recursive", "git ls-files --others --ignored --exclude-standard -z", - ]) - .output() - .map_err(|error| format!("failed to inspect ignored submodule files: {error}"))?; + ], + ) + .map_err(|error| format!("failed to inspect ignored submodule files: {error}"))?; if !submodule_output.status.success() { let stderr = String::from_utf8_lossy(&submodule_output.stderr) .trim() @@ -739,28 +755,23 @@ fn is_disposable_ignored_path(path: &Path) -> bool { } fn registered_worktree_paths(repo_root: &Path) -> Result, String> { - let output = git_command() - .arg("-C") - .arg(repo_root) - .args(["worktree", "list", "--porcelain", "-z"]) - .output() + let output = git_output(repo_root, ["worktree", "list", "--porcelain", "-z"]) .map_err(|error| format!("failed to run git worktree list: {error}"))?; if output.status.success() { return parse_registered_worktree_paths(&output.stdout); } - let fallback = git_command() - .arg("-C") - .arg(repo_root) - .args([ + let fallback = git_output( + repo_root, + [ "-c", "core.quotePath=false", "worktree", "list", "--porcelain", - ]) - .output() - .map_err(|error| format!("failed to run compatible git worktree list: {error}"))?; + ], + ) + .map_err(|error| format!("failed to run compatible git worktree list: {error}"))?; if fallback.status.success() { return parse_legacy_registered_worktree_paths(&fallback.stdout); } @@ -1021,12 +1032,7 @@ fn git_command() -> Command { } fn git_rev_parse_path(path: &Path, selector: &str) -> Option { - let output = git_command() - .arg("-C") - .arg(path) - .args(["rev-parse", selector]) - .output() - .ok()?; + let output = git_output(path, ["rev-parse", selector]).ok()?; if !output.status.success() { return None; } @@ -1083,11 +1089,34 @@ mod tests { #[cfg(unix)] use super::parse_registered_worktree_paths; + use crate::infra::process::command_output; + use super::{ ensure_openclaw_worktree, parse_legacy_registered_worktree_paths, prepare_openclaw_simulation_worktree_cleanup, remove_openclaw_worktree, }; + #[cfg(unix)] + #[test] + fn git_timeout_kills_sleep_after_deadline() { + use std::time::{Duration, Instant}; + + let started = Instant::now(); + let mut command = Command::new("/bin/sleep"); + command.arg("30"); + let error = command_output(command, 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 runner should return within 1s, took {elapsed:?}" + ); + assert!( + error.contains("timed out"), + "expected timeout error, got {error}" + ); + } + fn run_git(repo: &std::path::Path, args: &[&str]) { let output = Command::new("git") .arg("-C") diff --git a/src/supervisor/openclaw_handoff.rs b/src/supervisor/openclaw_handoff.rs index 82c59545..98fb95df 100644 --- a/src/supervisor/openclaw_handoff.rs +++ b/src/supervisor/openclaw_handoff.rs @@ -2,9 +2,9 @@ use std::io::Read; #[cfg(unix)] use std::os::unix::process::CommandExt; use std::path::Path; -use std::process::{Child, Command, ExitStatus, Stdio}; +use std::process::{Command, ExitStatus, Stdio}; use std::thread; -use std::time::{Duration, Instant}; +use std::time::Duration; use serde::Deserialize; @@ -240,7 +240,11 @@ fn run_openclaw_machine_command( let stdout_reader = thread::spawn(move || read_bounded(stdout)); let stderr_reader = thread::spawn(move || read_bounded(stderr)); - let status = wait_for_child(&mut child, COMMAND_TIMEOUT); + let status = crate::infra::process::wait_for_child( + &mut child, + COMMAND_TIMEOUT, + "restart-handoff command", + ); let stdout = stdout_reader .join() .map_err(|_| "restart-handoff stdout reader panicked".to_string())? @@ -340,53 +344,6 @@ fn read_bounded(mut reader: R) -> std::io::Result> { Ok(kept) } -fn wait_for_child(child: &mut Child, timeout: Duration) -> Result { - let started_at = Instant::now(); - loop { - match child.try_wait() { - Ok(Some(status)) => return Ok(status), - Ok(None) if started_at.elapsed() < timeout => { - thread::sleep(Duration::from_millis(25)); - } - Ok(None) => { - terminate_child(child); - return Err(format!( - "restart-handoff command timed out after {} seconds", - timeout.as_secs() - )); - } - Err(error) => { - terminate_child(child); - return Err(format!( - "failed waiting for restart-handoff command: {error}" - )); - } - } - } -} - -fn terminate_child(child: &mut Child) { - #[cfg(unix)] - { - let process_group = format!("-{}", child.id()); - let _ = Command::new("kill") - .args(["-TERM", "--", &process_group]) - .status(); - for _ in 0..20 { - match child.try_wait() { - Ok(Some(_)) => return, - Ok(None) => thread::sleep(Duration::from_millis(25)), - Err(_) => break, - } - } - let _ = Command::new("kill") - .args(["-KILL", "--", &process_group]) - .status(); - } - let _ = child.kill(); - let _ = child.wait(); -} - fn command_failure_detail(status: ExitStatus) -> String { match status.code() { Some(code) => format!("command exited {code}"), From 6a6dfd11a91ef28f351e23fe245850ddbf535855 Mon Sep 17 00:00:00 2001 From: Sebastien Tardif Date: Wed, 9 Sep 2026 13:42:11 -0700 Subject: [PATCH 2/2] test: give macOS more time to reap a timed-out git helper 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 db422a742c9453788f4e2ac0c847c42b74754866) --- src/openclaw_repo.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/openclaw_repo.rs b/src/openclaw_repo.rs index a984fcaa..043e6623 100644 --- a/src/openclaw_repo.rs +++ b/src/openclaw_repo.rs @@ -1108,8 +1108,8 @@ mod tests { .expect_err("sleep should be killed at the deadline"); let elapsed = started.elapsed(); assert!( - elapsed < Duration::from_secs(1), - "timed runner should return within 1s, took {elapsed:?}" + elapsed < Duration::from_secs(5), + "timed runner should return well before the 30s sleep, took {elapsed:?}" ); assert!( error.contains("timed out"),