mirror of
https://github.com/openai/codex.git
synced 2026-10-19 17:28:46 +00:00
Terminate timed-out Git process trees (#36793)
## Why Timing out a Git metadata command must not leave helper processes running after the command wrapper exits. ## What changed - Run Git metadata commands in a dedicated process group on Unix and a Job Object on Windows so timeout cleanup terminates their full process trees. - Start Windows commands suspended, assign them to the Job Object, and then resume them so immediate descendants cannot escape containment. - Preserve descendants when a Git command completes normally, and retain the existing direct-spawn fallback if Windows Job Object setup fails. ## Testing Added cross-platform regression tests for cleanup both while the command wrapper is running and after it exits, plus Windows coverage for immediate-child Job Object containment. GitOrigin-RevId: 351851708e23ff06b89fe1894bd09a3558f67293
This commit is contained in:
@@ -22,6 +22,7 @@ log = { workspace = true }
|
||||
shared_library = "0.1.9"
|
||||
winapi = { version = "0.3.9", features = [
|
||||
"handleapi",
|
||||
"jobapi",
|
||||
"jobapi2",
|
||||
"minwinbase",
|
||||
"processthreadsapi",
|
||||
|
||||
@@ -4,15 +4,26 @@ use std::os::windows::io::AsRawHandle;
|
||||
use std::os::windows::io::FromRawHandle;
|
||||
use std::os::windows::io::RawHandle;
|
||||
use std::sync::Mutex;
|
||||
use tokio::process::Child;
|
||||
use tokio::process::Command;
|
||||
use winapi::shared::ntdef::NT_SUCCESS;
|
||||
use winapi::shared::ntdef::NTSTATUS;
|
||||
use winapi::um::jobapi2::AssignProcessToJobObject;
|
||||
use winapi::um::jobapi2::CreateJobObjectW;
|
||||
use winapi::um::jobapi2::SetInformationJobObject;
|
||||
use winapi::um::jobapi2::TerminateJobObject;
|
||||
use winapi::um::winbase::CREATE_SUSPENDED;
|
||||
use winapi::um::winnt::HANDLE;
|
||||
use winapi::um::winnt::JOB_OBJECT_LIMIT_BREAKAWAY_OK;
|
||||
use winapi::um::winnt::JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE;
|
||||
use winapi::um::winnt::JOBOBJECT_EXTENDED_LIMIT_INFORMATION;
|
||||
use winapi::um::winnt::JobObjectExtendedLimitInformation;
|
||||
|
||||
#[link(name = "ntdll")]
|
||||
unsafe extern "system" {
|
||||
fn NtResumeProcess(process_handle: HANDLE) -> NTSTATUS;
|
||||
}
|
||||
|
||||
/// Owns a Windows Job Object used to terminate a spawned process tree.
|
||||
#[derive(Debug)]
|
||||
pub struct JobObject {
|
||||
@@ -74,6 +85,25 @@ impl JobObject {
|
||||
}
|
||||
}
|
||||
|
||||
/// Starts a process only after assigning it to this Job Object.
|
||||
pub fn spawn_contained(&self, command: &mut Command) -> io::Result<Child> {
|
||||
command.creation_flags(CREATE_SUSPENDED).kill_on_drop(true);
|
||||
let child = command.spawn()?;
|
||||
let process_handle = child
|
||||
.raw_handle()
|
||||
.ok_or_else(|| io::Error::other("missing child process handle"))?;
|
||||
self.assign_process(process_handle)?;
|
||||
|
||||
let status = unsafe { NtResumeProcess(process_handle.cast()) };
|
||||
if !NT_SUCCESS(status) {
|
||||
return Err(io::Error::other(format!(
|
||||
"failed to resume contained process: NTSTATUS {status:#x}"
|
||||
)));
|
||||
}
|
||||
|
||||
Ok(child)
|
||||
}
|
||||
|
||||
/// Allows contained descendants to keep running after the root exits normally.
|
||||
///
|
||||
/// This disables both explicit job termination and kill-on-close for this
|
||||
|
||||
@@ -6,8 +6,18 @@ use crate::TerminalSize;
|
||||
use crate::spawn_pipe_process_no_stdin;
|
||||
use crate::spawn_pty_process;
|
||||
use std::collections::HashMap;
|
||||
use std::os::windows::io::AsRawHandle;
|
||||
use std::os::windows::io::FromRawHandle;
|
||||
use std::os::windows::io::OwnedHandle;
|
||||
use std::path::Path;
|
||||
use std::process::Stdio;
|
||||
use std::time::Duration;
|
||||
use tokio::io::AsyncBufReadExt;
|
||||
use tokio::io::BufReader;
|
||||
use tokio::process::Command;
|
||||
use winapi::um::jobapi::IsProcessInJob;
|
||||
use winapi::um::processthreadsapi::OpenProcess;
|
||||
use winapi::um::winnt::PROCESS_QUERY_LIMITED_INFORMATION;
|
||||
|
||||
const READY_MARKER: &str = "__CODEX_CHILD_READY__";
|
||||
const VALUE_MARKER: &str = "__CODEX_CHILD_VALUE__";
|
||||
@@ -183,6 +193,54 @@ async fn normal_exit_preserves_descendants_for_pipe_and_conpty() -> anyhow::Resu
|
||||
assert_normal_exit_preserves_descendant("ConPTY", &python, &env).await
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn contained_spawn_owns_immediate_descendant() -> anyhow::Result<()> {
|
||||
let Some(python) = find_python() else {
|
||||
eprintln!("python not found; skipping Windows contained-spawn test");
|
||||
return Ok(());
|
||||
};
|
||||
|
||||
let mut command = Command::new(&python);
|
||||
command
|
||||
.args([
|
||||
"-u",
|
||||
"-c",
|
||||
"import subprocess,sys; child=subprocess.Popen([sys.executable,'-c','import time; time.sleep(60)']); print(child.pid,flush=True); child.wait()",
|
||||
])
|
||||
.stdin(Stdio::null())
|
||||
.stdout(Stdio::piped())
|
||||
.stderr(Stdio::null());
|
||||
|
||||
let job = crate::JobObject::create()?;
|
||||
let mut root = job.spawn_contained(&mut command)?;
|
||||
let stdout = root
|
||||
.stdout
|
||||
.take()
|
||||
.ok_or_else(|| anyhow::anyhow!("missing contained process stdout"))?;
|
||||
let mut stdout = BufReader::new(stdout);
|
||||
let mut child_pid = String::new();
|
||||
tokio::time::timeout(Duration::from_secs(10), stdout.read_line(&mut child_pid)).await??;
|
||||
let child_pid: u32 = child_pid.trim().parse()?;
|
||||
|
||||
let process = unsafe { OpenProcess(PROCESS_QUERY_LIMITED_INFORMATION, 0, child_pid) };
|
||||
anyhow::ensure!(!process.is_null(), "failed to open immediate child process");
|
||||
let process = unsafe { OwnedHandle::from_raw_handle(process.cast()) };
|
||||
let mut in_job = 0;
|
||||
let checked = unsafe {
|
||||
IsProcessInJob(
|
||||
process.as_raw_handle().cast(),
|
||||
job.as_raw_handle().cast(),
|
||||
&mut in_job,
|
||||
)
|
||||
};
|
||||
anyhow::ensure!(checked != 0, "failed to inspect child Job Object");
|
||||
anyhow::ensure!(in_job != 0, "immediate child escaped its Job Object");
|
||||
|
||||
job.terminate()?;
|
||||
tokio::time::timeout(Duration::from_secs(10), root.wait()).await??;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn conpty_delivers_input_to_foreground_children() -> anyhow::Result<()> {
|
||||
let Some(python) = find_python() else {
|
||||
|
||||
Reference in New Issue
Block a user