mirror of
https://github.com/openai/codex.git
synced 2026-11-11 21:10:39 +00:00
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
This commit is contained in:
@@ -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")]
|
||||
|
||||
@@ -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::<PidRecord>(&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<Option<PidLogTail>> {
|
||||
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<String> {
|
||||
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")?;
|
||||
|
||||
@@ -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<client::ProbeInfo> {
|
||||
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\
|
||||
|
||||
@@ -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() {
|
||||
|
||||
Reference in New Issue
Block a user