From b36c0b11896b0708035b0778de5635b45e84fe0d Mon Sep 17 00:00:00 2001 From: Teddy Ni Date: Mon, 13 Jul 2026 17:28:26 +0000 Subject: [PATCH] Reap exited PID-managed app-server children (#32838) ## Why An app-server child started by a previous updater can remain as a zombie after termination. Its PID then still appears active, causing daemon shutdown to wait until its timeout instead of removing the stale PID file. ## What changed - Poll `waitpid` with `WNOHANG` while waiting for a PID-managed process to stop. - Check process state before enforcing the stop deadline so an exit at the deadline can still be observed and cleaned up. ## Testing Added a regression test that stops an untracked child and verifies that shutdown finishes promptly and removes its PID file. GitOrigin-RevId: b203cf860e4207716a6cfc36015f8ee77918d13a --- codex-rs/app-server-daemon/src/backend/pid.rs | 12 +++++- .../src/backend/pid_tests.rs | 41 +++++++++++++++++++ 2 files changed, 52 insertions(+), 1 deletion(-) diff --git a/codex-rs/app-server-daemon/src/backend/pid.rs b/codex-rs/app-server-daemon/src/backend/pid.rs index b525e61254..aa05e1db8e 100644 --- a/codex-rs/app-server-daemon/src/backend/pid.rs +++ b/codex-rs/app-server-daemon/src/backend/pid.rs @@ -254,13 +254,23 @@ impl PidBackend { let started_at = tokio::time::Instant::now(); let deadline = tokio::time::Instant::now() + STOP_TIMEOUT; let mut forced = false; - while tokio::time::Instant::now() < deadline { + loop { + #[cfg(unix)] + if let Ok(raw_pid) = libc::pid_t::try_from(pid) + && raw_pid > 0 + { + // A previous updater may have started this child; reap it if it has exited. + unsafe { libc::waitpid(raw_pid, std::ptr::null_mut(), libc::WNOHANG) }; + } if !self.record_is_active(&record).await? { match self.refresh_after_stale_record(&record).await? { PidFileState::Missing => return Ok(()), PidFileState::Starting | PidFileState::Running(_) => break, } } + if tokio::time::Instant::now() >= deadline { + break; + } if !forced && started_at.elapsed() >= STOP_GRACE_PERIOD { self.force_terminate_process(pid)?; forced = true; diff --git a/codex-rs/app-server-daemon/src/backend/pid_tests.rs b/codex-rs/app-server-daemon/src/backend/pid_tests.rs index 57b3d227e9..de67fa4e3d 100644 --- a/codex-rs/app-server-daemon/src/backend/pid_tests.rs +++ b/codex-rs/app-server-daemon/src/backend/pid_tests.rs @@ -1,3 +1,4 @@ +use std::process::Stdio; use std::time::Duration; use pretty_assertions::assert_eq; @@ -10,6 +11,7 @@ use super::PidCommandKind; use super::PidFileState; use super::PidLogTail; use super::PidRecord; +use super::read_process_start_time; use super::read_stderr_log_tail; use super::stderr_log_file_for_pid_file; use super::try_lock_file; @@ -147,6 +149,45 @@ async fn stale_record_cleanup_preserves_replacement_record() { ); } +#[tokio::test] +async fn stop_reaps_untracked_app_server_child() { + let temp_dir = TempDir::new().expect("temp dir"); + let pid_file = temp_dir.path().join("app-server.pid"); + let mut child = std::process::Command::new("sleep") + .arg("5") + .stdin(Stdio::null()) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .spawn() + .expect("spawn app-server shim"); + let pid = child.id(); + let record = PidRecord { + pid, + process_start_time: read_process_start_time(pid).await.expect("start time"), + }; + tokio::fs::write( + &pid_file, + serde_json::to_vec(&record).expect("serialize pid"), + ) + .await + .expect("write pid file"); + let backend = PidBackend::new( + temp_dir.path().join("codex"), + pid_file.clone(), + /*remote_control_enabled*/ false, + ); + + let result = tokio::time::timeout(Duration::from_secs(2), backend.stop()).await; + if matches!(child.try_wait(), Ok(None)) { + let _ = child.kill(); + let _ = child.wait(); + } + + // `sleep` is not tracked by Tokio, so stop must reap it instead of leaving a zombie. + result.expect("stop timed out").expect("stop"); + assert!(!pid_file.exists()); +} + #[test] fn update_loop_uses_hidden_app_server_subcommand() { let backend = PidBackend {