From 987b8dfc0fa33acd45a67944a20fdea361d52eb0 Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Mon, 26 Oct 2026 15:19:53 +0000 Subject: [PATCH] Allow slow daemon startup while detecting early process exits (#57253) ## Why On Linux, a cold start from a rotational disk can exceed the ten-second readiness timeout. Extending that timeout should still let startup failures surface promptly when the daemon exits. ## What changed - Increase the Linux readiness timeout to 30 seconds, matching macOS. - Stop waiting for readiness when the published daemon process exits, preserving the PID reservation and reporting startup diagnostics, including stderr. - Kill pending `ps` subprocesses when their process checks are dropped. ## Testing Add coverage for a managed daemon becoming ready after 15 seconds and for reporting an exited daemon's stderr within five seconds. GitOrigin-RevId: c663e52bea9039807f14ef8fc4b44aca9df3df21 --- codex-rs/app-server-daemon/src/backend/mod.rs | 1 + codex-rs/app-server-daemon/src/backend/pid.rs | 15 ++++++ codex-rs/app-server-daemon/src/lib.rs | 46 +++++++++++++------ .../src/update_loop_tests.rs | 23 ++++++++++ 4 files changed, 72 insertions(+), 13 deletions(-) diff --git a/codex-rs/app-server-daemon/src/backend/mod.rs b/codex-rs/app-server-daemon/src/backend/mod.rs index 1d2f4ecfac..1c2ea755e9 100644 --- a/codex-rs/app-server-daemon/src/backend/mod.rs +++ b/codex-rs/app-server-daemon/src/backend/mod.rs @@ -10,6 +10,7 @@ use std::path::PathBuf; use serde::Serialize; pub(crate) use pid::PidBackend; +pub(crate) use pid::wait_for_process_exit; #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize)] #[serde(rename_all = "camelCase")] diff --git a/codex-rs/app-server-daemon/src/backend/pid.rs b/codex-rs/app-server-daemon/src/backend/pid.rs index 60e793e18d..b80743523c 100644 --- a/codex-rs/app-server-daemon/src/backend/pid.rs +++ b/codex-rs/app-server-daemon/src/backend/pid.rs @@ -460,6 +460,20 @@ impl PidBackend { } } +/// Wait for a published process to exit without modifying its PID reservation. +pub(crate) async fn wait_for_process_exit(pid_file: &Path) { + loop { + // A missing or empty record can belong to a launch still publishing its PID. + if let Ok(contents) = fs::read(pid_file).await + && let Ok(record) = serde_json::from_slice::(&contents) + && matches!(process_matches_record(&record).await, Ok(false)) + { + return; + } + sleep(crate::START_POLL_INTERVAL).await; + } +} + pub(crate) async fn read_stderr_log_tail(pid_file: &Path) -> Result> { let path = stderr_log_file_for_pid_file(pid_file); let Some(contents) = read_log_tail(&path, STDERR_LOG_TAIL_BYTES).await? else { @@ -757,6 +771,7 @@ async fn read_process_start_time(pid: u32) -> Result { async fn read_process_details(pid: u32) -> Result<(String, String)> { let output = Command::new("ps") .args(["-p", &pid.to_string(), "-o", "stat=", "-o", "lstart="]) + .kill_on_drop(true) .output() .await .context("failed to invoke ps for pid-managed app server")?; diff --git a/codex-rs/app-server-daemon/src/lib.rs b/codex-rs/app-server-daemon/src/lib.rs index 0c4f95ff2f..51b9c72e72 100644 --- a/codex-rs/app-server-daemon/src/lib.rs +++ b/codex-rs/app-server-daemon/src/lib.rs @@ -50,10 +50,11 @@ use tokio::time::sleep; const START_POLL_INTERVAL: Duration = Duration::from_millis(50); // An older updater can pass background policy to a replacement daemon, so retain -// the longer readiness allowance during upgrades on macOS. -#[cfg(target_os = "macos")] +// the longer readiness allowance during upgrades on macOS. On Linux, a cold +// start from a rotational disk can take more than ten seconds. +#[cfg(any(target_os = "macos", target_os = "linux"))] const START_TIMEOUT: Duration = Duration::from_secs(30); -#[cfg(not(target_os = "macos"))] +#[cfg(not(any(target_os = "macos", target_os = "linux")))] const START_TIMEOUT: Duration = Duration::from_secs(10); // Leave room for the longest graceful stop, forced-exit check, and restart. const OPERATION_LOCK_TIMEOUT: Duration = @@ -701,20 +702,22 @@ impl Daemon { } async fn wait_until_ready(&self) -> Result { + let exited = backend::wait_for_process_exit(&self.pid_file); + tokio::pin!(exited); let deadline = tokio::time::Instant::now() + START_TIMEOUT; - loop { + let err = loop { match client::probe(&self.socket_path).await { Ok(info) => return Ok(info), Err(err) if tokio::time::Instant::now() < deadline => { - let _ = err; - sleep(START_POLL_INTERVAL).await; - } - Err(err) => { - let context = self.app_server_not_ready_context().await; - return Err(err).context(context); + tokio::select! { + () = &mut exited => break err, + () = sleep(START_POLL_INTERVAL) => {} + } } + Err(err) => break err, } - } + }; + Err(err).context(self.app_server_not_ready_context().await) } async fn app_server_not_ready_context(&self) -> String { @@ -1523,7 +1526,7 @@ mod tests { } #[tokio::test] - async fn not_ready_context_reports_daemon_app_server_before_stderr() { + async fn readiness_reports_stderr_promptly_when_daemon_is_gone() { let temp_dir = TempDir::new().expect("temp dir"); let daemon = Daemon { log_diagnostics: false, @@ -1539,8 +1542,25 @@ mod tests { .await .expect("write stderr log"); + // A daemon that exits during startup leaves its published PID record behind. + let mut child = std::process::Command::new(std::env::current_exe().unwrap()) + .arg("--help") + .stdout(std::process::Stdio::null()) + .spawn() + .unwrap(); + child.wait().unwrap(); + std::fs::write( + &daemon.pid_file, + serde_json::json!({ "pid": child.id(), "processStartTime": "exited" }).to_string(), + ) + .unwrap(); + let error = + tokio::time::timeout(std::time::Duration::from_secs(5), daemon.wait_until_ready()) + .await + .expect("an exited daemon should not exhaust the readiness timeout") + .expect_err("daemon is gone"); assert_eq!( - daemon.app_server_not_ready_context().await, + error.to_string(), format!( "app server did not become ready on {}\n\n\ Daemon used app-server:\n path: {}\n version: unknown\n\n\ diff --git a/codex-rs/app-server-daemon/src/update_loop_tests.rs b/codex-rs/app-server-daemon/src/update_loop_tests.rs index 02aca068a1..3eaf610e3d 100644 --- a/codex-rs/app-server-daemon/src/update_loop_tests.rs +++ b/codex-rs/app-server-daemon/src/update_loop_tests.rs @@ -829,6 +829,29 @@ async fn manual_update_restarts_managed_daemon_with_automatic_updates_disabled() check_manual_update_restart("standalone").await; } +#[cfg(any(target_os = "linux", target_os = "macos"))] +#[tokio::test] +async fn readiness_waits_for_a_slow_managed_daemon() { + let home = TempDir::new().unwrap(); + let (daemon, _) = manual_update_daemon(&home); + let settings = crate::settings::DaemonSettings::default(); + let backend = crate::backend::pid_backend(daemon.backend_paths(&settings)); + backend.start().await.unwrap(); + + tokio::time::pause(); + let (ready, listener) = tokio::join!(daemon.wait_until_ready(), async { + // Model a cold start that takes longer than the old Linux readiness deadline. + tokio::time::sleep(Duration::from_secs(15)).await; + tokio::time::resume(); + test_control_server(&daemon, home.path()) + .await + .abort_handle() + }); + backend.stop().await.unwrap(); + listener.abort(); + assert_eq!(ready.unwrap().app_server_version, "1.0.0"); +} + #[cfg(unix)] #[tokio::test] async fn daemon_start_and_restart_preserve_launch_features() {