diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 9f3e69e3cc..8dbef91ed5 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -3104,6 +3104,7 @@ dependencies = [ "codex-protocol", "codex-utils-absolute-path", "codex-utils-path-uri", + "codex-utils-pty", "futures", "serde", ] diff --git a/codex-rs/git-utils/Cargo.toml b/codex-rs/git-utils/Cargo.toml index 18ba488252..c6a33f9bf5 100644 --- a/codex-rs/git-utils/Cargo.toml +++ b/codex-rs/git-utils/Cargo.toml @@ -15,6 +15,7 @@ codex-file-system = { workspace = true } codex-protocol = { workspace = true } codex-utils-absolute-path = { workspace = true } codex-utils-path-uri = { workspace = true } +codex-utils-pty = { workspace = true } futures = { workspace = true, features = ["alloc"] } gix = { workspace = true } once_cell = { workspace = true } @@ -24,7 +25,7 @@ serde = { workspace = true, features = ["derive"] } similar = { workspace = true } tempfile = { workspace = true } thiserror = { workspace = true } -tokio = { workspace = true, features = ["macros", "process", "rt", "time"] } +tokio = { workspace = true, features = ["io-util", "macros", "process", "rt", "time"] } ts-rs = { workspace = true, features = [ "uuid-impl", "serde-json-impl", diff --git a/codex-rs/git-utils/src/info.rs b/codex-rs/git-utils/src/info.rs index d14cd1e0b1..1964502382 100644 --- a/codex-rs/git-utils/src/info.rs +++ b/codex-rs/git-utils/src/info.rs @@ -3,6 +3,7 @@ use std::collections::HashSet; use std::ffi::OsStr; use std::path::Path; use std::path::PathBuf; +use std::process::Stdio; use codex_file_system::ExecutorFileSystem; use codex_file_system::FindUpErrorPolicy; @@ -13,6 +14,7 @@ use futures::future::join_all; use schemars::JsonSchema; use serde::Deserialize; use serde::Serialize; +use tokio::io::AsyncReadExt; use tokio::process::Command; use tokio::time::Duration as TokioDuration; use tokio::time::timeout; @@ -291,13 +293,7 @@ fn trim_git_suffix(value: &str) -> &str { pub async fn get_has_changes(cwd: &Path) -> Option { let git = Path::new("git"); let fsmonitor = detect_local_fsmonitor_override(git, cwd).await; - let output = - run_git_command_with_timeout_from(git, &["status", "--porcelain"], cwd, fsmonitor).await?; - if !output.status.success() { - return None; - } - - Some(!output.stdout.is_empty()) + run_git_status_with_timeout_from(git, cwd, fsmonitor, GIT_COMMAND_TIMEOUT).await } fn parse_git_remote_urls(stdout: &str) -> Option> { @@ -443,6 +439,54 @@ async fn run_git_command_with_timeout_from( cwd: &Path, fsmonitor: crate::FsmonitorOverride, ) -> Option { + let mut command = git_command(git, cwd, fsmonitor); + command.args(args).kill_on_drop(true); + match timeout(GIT_COMMAND_TIMEOUT, command.output()).await { + Ok(Ok(output)) => Some(output), + _ => None, + } +} + +async fn run_git_status_with_timeout_from( + git: &Path, + cwd: &Path, + fsmonitor: crate::FsmonitorOverride, + duration: TokioDuration, +) -> Option { + let mut command = git_command(git, cwd, fsmonitor); + command + .args(["status", "--porcelain"]) + .kill_on_drop(true) + .stdin(Stdio::null()) + .stdout(Stdio::piped()) + .stderr(Stdio::null()); + #[cfg(unix)] + command.process_group(0); + + let mut child = command.spawn().ok()?; + #[cfg(unix)] + let process_group_id = child.id().expect("spawned child has a process ID"); + let mut stdout = child.stdout.take()?; + let mut output = Vec::new(); + let status = match timeout(duration, async { + stdout.read_to_end(&mut output).await?; + child.wait().await + }) + .await + { + Ok(Ok(status)) => status, + _ => { + #[cfg(unix)] + let _ = codex_utils_pty::process_group::kill_process_group(process_group_id); + let _ = child.kill().await; + return None; + } + }; + + status.success().then_some(!output.is_empty()) +} + +fn git_command(git: &Path, cwd: &Path, fsmonitor: crate::FsmonitorOverride) -> Command { let mut command = Command::new(git); command .env("GIT_OPTIONAL_LOCKS", "0") @@ -450,15 +494,8 @@ async fn run_git_command_with_timeout_from( // and fsmonitor helpers while preserving built-in fsmonitor acceleration. .args(["-c", &format!("core.hooksPath={DISABLED_HOOKS_PATH}")]) .args(["-c", fsmonitor.git_config_arg()]) - .args(args) - .current_dir(cwd) - .kill_on_drop(true); - let result = timeout(GIT_COMMAND_TIMEOUT, command.output()).await; - - match result { - Ok(Ok(output)) => Some(output), - _ => None, // Timeout or error - } + .current_dir(cwd); + command } async fn get_git_remotes(cwd: &Path) -> Option> { @@ -1086,4 +1123,36 @@ mod tests { ] ); } + + #[cfg(unix)] + #[tokio::test] + async fn git_status_timeout_kills_wrapped_processes() { + let temp_dir = tempfile::tempdir().expect("create temp dir"); + let git = temp_dir.path().join("git"); + let ready = temp_dir.path().join("git.ready"); + let survived = temp_dir.path().join("git.survived"); + std::fs::write( + &git, + "#!/bin/sh\n\ + (sleep 1; : >\"$0.survived\") &\n\ + : >\"$0.ready\"\n\ + wait\n", + ) + .expect("write fake Git"); + std::fs::set_permissions(&git, std::fs::Permissions::from_mode(0o755)) + .expect("mark fake Git executable"); + + let result = run_git_status_with_timeout_from( + &git, + temp_dir.path(), + crate::FsmonitorOverride::Disabled, + TokioDuration::from_millis(500), + ) + .await; + tokio::time::sleep(TokioDuration::from_millis(1100)).await; + + assert_eq!(result, None); + assert!(ready.exists(), "fake Git did not start its child"); + assert!(!survived.exists(), "wrapped process survived the timeout"); + } }