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
This commit is contained in:
Teddy Ni
2026-07-13 17:33:13 +00:00
committed by copyberry
parent 088239294a
commit b36c0b1189
2 changed files with 52 additions and 1 deletions
+11 -1
View File
@@ -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;
@@ -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 {