From c9c7b73c4faa49b02a8f644c31670ff26276c29e Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Mon, 7 Sep 2026 17:10:07 +0000 Subject: [PATCH] Ensure the standalone updater runs on managed daemon starts (#43529) ## Why Starting a latest-channel standalone daemon required a separate `bootstrap` to enable automatic updates. A missing updater could also leave a running server without automatic updates until `bootstrap` was run again. ## What changed - Ensure a single supported updater after managed `start`, `restart`, and restarts caused by remote-control changes. Repeated starts recover a missing updater while reusing the running server. - Report updater failures as warnings so successful server starts and restarts still succeed. - Preserve an existing updater while the latest-channel marker and managed binary may be temporarily inconsistent during installation. Stop it when the latest-channel marker is removed. - Document automatic updater startup and the limitations of detecting out-of-band binary replacements. ## Testing Add Unix tests for updater reuse and recovery, invalid updater records, marker mismatches and release pinning, unmanaged servers, and bootstrap status for managed local builds. GitOrigin-RevId: 44148a8d65709f68def2072f542494620d7e574b --- codex-rs/app-server-daemon/README.md | 22 +-- codex-rs/app-server-daemon/src/lib.rs | 139 +++++++++++---- codex-rs/cli/tests/app_server_daemon.rs | 221 ++++++++++++++++++++++++ 3 files changed, 339 insertions(+), 43 deletions(-) create mode 100644 codex-rs/cli/tests/app_server_daemon.rs diff --git a/codex-rs/app-server-daemon/README.md b/codex-rs/app-server-daemon/README.md index 226969c383..95724b0f92 100644 --- a/codex-rs/app-server-daemon/README.md +++ b/codex-rs/app-server-daemon/README.md @@ -75,10 +75,10 @@ on Windows) and its managed binary under `CODEX_HOME/packages/standalone/current | Situation | What starts | Does this daemon fetch new binaries? | Does a running app-server eventually move to a newer binary on its own? | | --- | --- | --- | --- | -| Installer has run; only `start` is used | Managed binary | No | No; explicit restart is required. | -| Latest-channel installer has run; `bootstrap` is used | Managed binary and detached updater when supported | Yes; the platform's installer runs hourly. | Yes; after a successful update, a running app-server restarts with the new binary before the updater replaces itself. | +| Latest-channel installer has run; `start` is used | Managed binary and detached updater when supported | When supported, the platform's installer runs hourly. | When supported, the running server restarts with the new binary before the updater replaces itself. | +| Latest-channel installer has run; `bootstrap` is used | Managed binary and detached updater when supported | When supported, the platform's installer runs hourly. | When supported, the running server restarts with the new binary before the updater replaces itself. | | Installer selected an explicit release; `bootstrap` is used | Managed binary only | No; the selected release stays pinned. | No; an explicit restart uses the selected binary. | -| Another tool updates the managed binary | Next start or restart uses it | Only with `bootstrap`, on its normal cadence. | With `bootstrap`, the next successful installer pass compares binary contents and refreshes a running app-server before the updater. | +| Another tool updates the managed binary | A fresh start or explicit restart uses it; a running server is reused. | Yes, when a latest-channel updater is running. | An updater that was running through the change compares binary contents on its next successful installer pass and refreshes the server first. | ### Standalone installs @@ -86,8 +86,9 @@ For installs created by either platform's standalone installer: - lifecycle commands always use the standalone managed binary path - `bootstrap` is supported -- `bootstrap` starts a detached pid-backed updater loop only for a stable - latest-channel release whose managed binary supports the updater command +- managed `start`, `restart`, and `bootstrap` ensure a single detached pid-backed + updater loop only for a stable latest-channel release whose managed binary + supports the updater command - the installer records the latest-channel selection alongside `current`; selecting an explicit release clears it, even if that version is currently latest. The updater checks the selection again while holding the install lock @@ -98,20 +99,21 @@ For installs created by either platform's standalone installer: - after a successful refresh, if app-server is running and the managed binary contents changed, the updater restarts app-server with that binary first and only then replaces its own process image -- the updater loop is not reboot-persistent; it must be started again by - rerunning `bootstrap` after a reboot +- the updater loop is not reboot-persistent; a managed start after reboot + starts it again ### Out-of-band updates This daemon does not watch arbitrary executable files for replacement. If some other tool updates the managed binary path: -- without `bootstrap`, a currently running app-server remains on the old - executable image until an explicit `restart` -- with `bootstrap`, the detached updater loop notices the changed managed +- an updater that was already running notices a changed managed binary on its next successful scheduled installer pass; if app-server is running, it refreshes app-server first and then refreshes itself once that replacement starts successfully +- if the updater was absent during a same-version binary replacement, a later + managed start recovers it but cannot infer the running server's previous + executable identity; use `codex app-server daemon restart` to refresh the server ## Lifecycle semantics diff --git a/codex-rs/app-server-daemon/src/lib.rs b/codex-rs/app-server-daemon/src/lib.rs index 12b2c26839..e5d5208897 100644 --- a/codex-rs/app-server-daemon/src/lib.rs +++ b/codex-rs/app-server-daemon/src/lib.rs @@ -312,39 +312,38 @@ impl Daemon { async fn start(&self) -> Result { let settings = self.load_settings().await?; - if let Ok(info) = client::probe(&self.socket_path).await { - return Ok(self - .output( - LifecycleStatus::AlreadyRunning, - self.running_backend(&settings).await?, - /*pid*/ None, - Some(info.app_server_version), - ) - .await); - } - - if self.running_backend_instance(&settings).await?.is_some() { - let info = self.wait_until_ready().await?; - return Ok(self - .output( - LifecycleStatus::AlreadyRunning, - Some(BackendKind::Pid), - /*pid*/ None, - Some(info.app_server_version), - ) - .await); - } - - self.ensure_managed_codex_bin()?; - let pid = self.start_managed_backend(&settings).await?; - let info = self.wait_until_ready().await?; - Ok(self - .output( + let (status, backend, pid, info) = if let Ok(info) = client::probe(&self.socket_path).await + { + ( + LifecycleStatus::AlreadyRunning, + self.running_backend(&settings).await?, + None, + info, + ) + } else if self.running_backend_instance(&settings).await?.is_some() { + ( + LifecycleStatus::AlreadyRunning, + Some(BackendKind::Pid), + None, + self.wait_until_ready().await?, + ) + } else { + self.ensure_managed_codex_bin()?; + let pid = self.start_managed_backend(&settings).await?; + ( LifecycleStatus::Started, Some(BackendKind::Pid), pid, - Some(info.app_server_version), + self.wait_until_ready().await?, ) + }; + if backend.is_some() + && let Err(err) = self.ensure_managed_updater(&settings).await + { + eprintln!("warning: failed to ensure managed updater after app-server start: {err:#}"); + } + Ok(self + .output(status, backend, pid, Some(info.app_server_version)) .await) } @@ -365,6 +364,11 @@ impl Daemon { let pid = self.start_managed_backend(&settings).await?; let info = self.wait_until_ready().await?; + if let Err(err) = self.ensure_managed_updater(&settings).await { + eprintln!( + "warning: failed to ensure managed updater after app-server restart: {err:#}" + ); + } Ok(self .output( LifecycleStatus::Restarted, @@ -619,7 +623,13 @@ impl Daemon { let app_server_version = if let Some(backend) = backend { backend.stop().await?; let _ = self.start_managed_backend(&settings).await?; - Some(self.wait_until_ready().await?.app_server_version) + let info = self.wait_until_ready().await?; + if let Err(err) = self.ensure_managed_updater(&settings).await { + eprintln!( + "warning: failed to ensure managed updater after remote-control change: {err:#}" + ); + } + Some(info.app_server_version) } else { None }; @@ -708,13 +718,19 @@ impl Daemon { async fn ensure_managed_updater(&self, settings: &DaemonSettings) -> Result { let updater = backend::pid_update_loop_backend(self.backend_paths(settings)); if !self.is_stable_standalone_release()? { - updater.stop().await?; + // An installer publishes current and the latest marker separately. + // Keep its updater alive while that publication may be in progress. + if !self.has_latest_selection_marker() { + updater.stop().await?; + } return Ok(false); } let Ok(codex_bin) = managed_install::resolved_managed_codex_bin(&self.managed_codex_bin).await else { - updater.stop().await?; + if !self.has_latest_selection_marker() { + updater.stop().await?; + } return Ok(false); }; if !managed_install::supports_daemon_update_loop(&codex_bin).await @@ -723,7 +739,9 @@ impl Daemon { .await .is_ok_and(|selected| selected == codex_bin) { - updater.stop().await?; + if !self.has_latest_selection_marker() { + updater.stop().await?; + } return Ok(false); } backend::pid_update_loop_backend(self.backend_paths_with_bin(settings, &codex_bin)) @@ -744,6 +762,16 @@ impl Daemon { )) } + fn has_latest_selection_marker(&self) -> bool { + self.settings_file + .parent() + .and_then(Path::parent) + .is_some_and(|home| { + home.join("packages/standalone/auto-update-version") + .is_file() + }) + } + async fn is_bootstrapped(&self, settings: &DaemonSettings) -> Result { if !self.is_stable_standalone_release()? || !managed_install::supports_daemon_update_loop(&self.managed_codex_bin).await @@ -970,6 +998,8 @@ mod tests { use super::restart_decision; use super::should_reexec_updater; use crate::client::ProbeInfo; + #[cfg(unix)] + use crate::settings::DaemonSettings; #[test] fn remote_control_status_uses_camel_case_json() { @@ -1131,6 +1161,49 @@ mod tests { ); } + #[cfg(unix)] + #[tokio::test] + async fn managed_local_backend_counts_as_bootstrapped_without_updater() { + use std::os::unix::fs::PermissionsExt; + + let home = TempDir::new().expect("home"); + let standalone = home.path().join("packages/standalone"); + let local_bin = standalone.join("local-main/bin/codex"); + tokio::fs::create_dir_all(local_bin.parent().expect("bin parent")) + .await + .expect("local bin directory"); + tokio::fs::write(&local_bin, b"#!/bin/sh\nexec sleep 30\n") + .await + .expect("local bin"); + std::fs::set_permissions(&local_bin, std::fs::Permissions::from_mode(0o755)) + .expect("executable local bin"); + std::os::unix::fs::symlink("local-main", standalone.join("current")) + .expect("current local build"); + let state = home.path().join("app-server-daemon"); + let daemon = Daemon { + socket_path: home + .path() + .join("app-server-control/app-server-control.sock"), + pid_file: state.join("app-server.pid"), + update_pid_file: state.join("app-server-updater.pid"), + operation_lock_file: state.join("daemon.lock"), + settings_file: state.join("settings.json"), + managed_codex_bin: standalone.join("current/bin/codex"), + }; + let settings = DaemonSettings::default(); + assert!( + !daemon + .is_bootstrapped(&settings) + .await + .expect("not running") + ); + let backend = crate::backend::pid_backend(daemon.backend_paths(&settings)); + backend.start().await.expect("start local backend"); + let bootstrapped = daemon.is_bootstrapped(&settings).await; + backend.stop().await.expect("stop local backend"); + assert!(bootstrapped.expect("managed local backend")); + } + #[tokio::test] async fn not_ready_context_reports_daemon_app_server_before_stderr() { let temp_dir = TempDir::new().expect("temp dir"); diff --git a/codex-rs/cli/tests/app_server_daemon.rs b/codex-rs/cli/tests/app_server_daemon.rs new file mode 100644 index 0000000000..2539e55096 --- /dev/null +++ b/codex-rs/cli/tests/app_server_daemon.rs @@ -0,0 +1,221 @@ +#![cfg(unix)] + +use std::path::PathBuf; +use std::process::Child; +use std::process::Command; +use std::process::Stdio; +use std::time::Duration; +use std::time::Instant; + +use anyhow::Context; +use anyhow::Result; +use anyhow::ensure; +use pretty_assertions::assert_eq; +use serde_json::Value; +use tempfile::TempDir; + +struct TestDaemon { + home: TempDir, + codex: PathBuf, + unmanaged: Option, +} + +impl TestDaemon { + fn new() -> Result { + let home = tempfile::Builder::new().tempdir_in("/tmp")?; + let codex = codex_utils_cargo_bin::cargo_bin("codex")?; + let codex_source = std::fs::canonicalize(&codex)?; + let target = if cfg!(target_os = "macos") { + format!("{}-apple-darwin", std::env::consts::ARCH) + } else { + format!("{}-unknown-linux-musl", std::env::consts::ARCH) + }; + let standalone = home.path().join("packages/standalone"); + let release_name = format!("0.0.0-{target}"); + let managed = standalone + .join("releases") + .join(&release_name) + .join("bin/codex"); + std::fs::create_dir_all(managed.parent().context("managed bin parent")?)?; + std::fs::hard_link(&codex_source, &managed) + .or_else(|_| std::fs::copy(&codex_source, managed).map(|_| ()))?; + std::fs::write(standalone.join("auto-update-version"), &release_name)?; + std::os::unix::fs::symlink( + PathBuf::from("releases").join(release_name), + standalone.join("current"), + )?; + Ok(Self { + home, + codex, + unmanaged: None, + }) + } + + fn command(&self) -> Command { + let mut command = Command::new(&self.codex); + command.env("CODEX_HOME", self.home.path()); + command + } + + fn lifecycle(&self, action: &str) -> Result { + let output = self + .command() + .args(["app-server", "daemon", action]) + .output()?; + ensure!( + output.status.success(), + "daemon {action} failed: {}", + String::from_utf8_lossy(&output.stderr) + ); + Ok(serde_json::from_slice(&output.stdout)?) + } + + fn pid(&self, name: &str) -> Result { + let record = std::fs::read(self.home.path().join("app-server-daemon").join(name)) + .with_context(|| format!("failed to read {name}"))?; + Ok(serde_json::from_slice::(&record)?["pid"] + .as_u64() + .context("pid missing")? as u32) + } +} + +impl Drop for TestDaemon { + fn drop(&mut self) { + if let Some(mut child) = self.unmanaged.take() { + let _ = child.kill(); + let _ = child.wait(); + } + let _ = self.lifecycle("stop"); + if let Ok(pid) = self.pid("app-server-updater.pid") { + let _ = signal(pid, libc::SIGTERM); + } + } +} + +fn signal(pid: u32, signal: libc::c_int) -> Result<()> { + let raw_pid = libc::pid_t::try_from(pid).context("pid out of range")?; + ensure!( + unsafe { libc::kill(raw_pid, signal) } == 0, + "failed to signal pid {pid}: {}", + std::io::Error::last_os_error() + ); + Ok(()) +} + +fn wait_for_exit(pid: u32) -> Result<()> { + let deadline = Instant::now() + Duration::from_secs(30); + loop { + let output = Command::new("/bin/ps") + .args(["-p", &pid.to_string(), "-o", "stat="]) + .output() + .context("failed to invoke ps")?; + let state = String::from_utf8_lossy(&output.stdout); + if !output.status.success() || state.trim().starts_with('Z') { + return Ok(()); + } + ensure!(Instant::now() < deadline, "pid {pid} did not exit: {state}"); + std::thread::sleep(Duration::from_millis(50)); + } +} + +#[test] +fn managed_starts_ensure_one_updater_and_recover_a_missing_one() -> Result<()> { + let daemon = TestDaemon::new()?; + assert_eq!(daemon.lifecycle("start")?["status"], "started"); + let backend_pid = daemon.pid("app-server.pid")?; + let updater_pid = daemon.pid("app-server-updater.pid")?; + assert_ne!(backend_pid, updater_pid); + + assert_eq!(daemon.lifecycle("start")?["status"], "alreadyRunning"); + assert_eq!(daemon.pid("app-server.pid")?, backend_pid); + assert_eq!(daemon.pid("app-server-updater.pid")?, updater_pid); + + signal(updater_pid, libc::SIGTERM)?; + wait_for_exit(updater_pid)?; + assert_eq!(daemon.lifecycle("start")?["status"], "alreadyRunning"); + assert_eq!(daemon.pid("app-server.pid")?, backend_pid); + let replacement_pid = daemon.pid("app-server-updater.pid")?; + assert_ne!(replacement_pid, updater_pid); + assert_eq!(daemon.lifecycle("restart")?["status"], "restarted"); + assert_ne!(daemon.pid("app-server.pid")?, backend_pid); + assert_eq!(daemon.pid("app-server-updater.pid")?, replacement_pid); + Ok(()) +} + +#[test] +fn managed_start_succeeds_when_updater_record_is_invalid() -> Result<()> { + let daemon = TestDaemon::new()?; + let state_dir = daemon.home.path().join("app-server-daemon"); + std::fs::create_dir_all(&state_dir)?; + std::fs::write(state_dir.join("app-server-updater.pid"), "not a PID record")?; + + assert_eq!(daemon.lifecycle("start")?["status"], "started"); + let server_pid = daemon.pid("app-server.pid")?; + assert_eq!(daemon.lifecycle("start")?["status"], "alreadyRunning"); + assert_eq!(daemon.pid("app-server.pid")?, server_pid); + Ok(()) +} + +#[test] +fn managed_start_keeps_updater_on_marker_mismatch_but_stops_it_for_pin() -> Result<()> { + let daemon = TestDaemon::new()?; + assert_eq!(daemon.lifecycle("start")?["status"], "started"); + let updater_pid = daemon.pid("app-server-updater.pid")?; + let marker = daemon + .home + .path() + .join("packages/standalone/auto-update-version"); + std::fs::write(&marker, "0.1.0-other-target")?; + assert_eq!(daemon.lifecycle("start")?["status"], "alreadyRunning"); + assert_eq!(daemon.pid("app-server-updater.pid")?, updater_pid); + + std::fs::remove_file(marker)?; + + assert_eq!(daemon.lifecycle("start")?["status"], "alreadyRunning"); + wait_for_exit(updater_pid)?; + assert!( + !daemon + .home + .path() + .join("app-server-daemon/app-server-updater.pid") + .exists() + ); + Ok(()) +} + +#[test] +fn start_does_not_launch_updater_for_unmanaged_app_server() -> Result<()> { + let mut daemon = TestDaemon::new()?; + daemon.unmanaged = Some( + daemon + .command() + .args(["app-server", "--listen", "unix://"]) + .stdin(Stdio::null()) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .spawn()?, + ); + let deadline = Instant::now() + Duration::from_secs(30); + while !daemon + .command() + .args(["app-server", "daemon", "version"]) + .output()? + .status + .success() + { + ensure!(Instant::now() < deadline, "app server did not become ready"); + std::thread::sleep(Duration::from_millis(50)); + } + + let output = daemon.lifecycle("start")?; + assert_eq!(output["status"], "alreadyRunning"); + assert_eq!(output["backend"], Value::Null); + assert!( + !daemon + .home + .path() + .join("app-server-daemon/app-server-updater.pid") + .exists() + ); + Ok(()) +}