From 1e0deb5346dd25810cbb107df6d24fdca8dae3cc Mon Sep 17 00:00:00 2001 From: Tamir Duberstein Date: Fri, 26 Jun 2026 07:34:18 -0400 Subject: [PATCH] [codex] kill timed-out Git status process groups Codex limits `git status --porcelain` to five seconds. If `git` resolves to a wrapper, Tokio's drop cleanup kills only the wrapper. The wrapped Git process can keep scanning the worktree without an output reader. On Unix, run status in its own process group. When the timeout expires, kill the group and wait for the wrapper before returning. --- codex-rs/Cargo.lock | 1 + codex-rs/git-utils/Cargo.toml | 3 +- codex-rs/git-utils/src/info.rs | 101 +++++++++++++++++++++++++++------ 3 files changed, 88 insertions(+), 17 deletions(-) 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"); + } }