From e3f481da98890580fdd9b1d8530cc1c89ee4eb6d Mon Sep 17 00:00:00 2001 From: Ruslan Nigmatullin Date: Mon, 11 May 2026 12:37:10 -0700 Subject: [PATCH] daemon: refresh updater after validated binary rollout (#21853) ## Why `bootstrap` starts a detached pid-backed updater loop, but before this change that updater could keep running an old executable image even after `install.sh` replaced the managed standalone binary under `CODEX_HOME`. That left the updater itself behind the binary it had just rolled out, especially when the app-server was stopped or when the managed binary changed without a version-string change. ## What changed - Track updater identity from the executable contents rather than only the reported CLI version. - Force the managed app-server restart path when the managed binary contents differ from the running updater image, then re-exec the updater from the managed binary once the rollout is in a safe state. - Distinguish a genuinely absent managed app-server from a managed process that exists but is not yet probeable, so self-refresh does not skip a required restart. - Keep the restart/re-exec decision under the daemon operation lock so `bootstrap` cannot race the handoff. - Update `app-server-daemon/README.md` to document the resulting standalone and out-of-band update behavior. ## Verification - `cargo test -p codex-app-server-daemon` - `just fix -p codex-app-server-daemon` Added focused unit coverage for: - content-based updater refresh decisions - safe updater re-exec outcomes across restart states --- codex-rs/Cargo.lock | 3 +- codex-rs/app-server-daemon/Cargo.toml | 3 +- codex-rs/app-server-daemon/README.md | 21 +- codex-rs/app-server-daemon/src/lib.rs | 198 ++++++++++++++++-- .../app-server-daemon/src/managed_install.rs | 37 ++++ .../src/managed_install_tests.rs | 11 + codex-rs/app-server-daemon/src/update_loop.rs | 73 ++++++- .../src/update_loop_tests.rs | 31 +++ 8 files changed, 344 insertions(+), 33 deletions(-) create mode 100644 codex-rs/app-server-daemon/src/update_loop_tests.rs diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 9d98806df..d15a1062d 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -1984,14 +1984,15 @@ dependencies = [ "anyhow", "codex-app-server-protocol", "codex-app-server-transport", - "codex-core", "codex-uds", + "codex-utils-home-dir", "futures", "libc", "pretty_assertions", "reqwest", "serde", "serde_json", + "sha2", "tempfile", "tokio", "tokio-tungstenite", diff --git a/codex-rs/app-server-daemon/Cargo.toml b/codex-rs/app-server-daemon/Cargo.toml index 5085d6bd7..24531b4c7 100644 --- a/codex-rs/app-server-daemon/Cargo.toml +++ b/codex-rs/app-server-daemon/Cargo.toml @@ -16,13 +16,14 @@ workspace = true anyhow = { workspace = true } codex-app-server-protocol = { workspace = true } codex-app-server-transport = { workspace = true } -codex-core = { workspace = true } +codex-utils-home-dir = { workspace = true } codex-uds = { workspace = true } futures = { workspace = true } libc = { workspace = true } reqwest = { workspace = true, features = ["rustls-tls"] } serde = { workspace = true, features = ["derive"] } serde_json = { workspace = true } +sha2 = { workspace = true } tokio = { workspace = true, features = [ "fs", "io-util", diff --git a/codex-rs/app-server-daemon/README.md b/codex-rs/app-server-daemon/README.md index c343d5c84..1d81193bb 100644 --- a/codex-rs/app-server-daemon/README.md +++ b/codex-rs/app-server-daemon/README.md @@ -52,8 +52,8 @@ the standalone managed binary under `CODEX_HOME`. | Situation | What starts | Does this daemon fetch new binaries? | Does a running app-server eventually move to a newer binary on its own? | | --- | --- | --- | --- | | `install.sh` has run, but only `start` is used | `start` uses `CODEX_HOME/packages/standalone/current/codex` | No | No. The managed path is used when starting or restarting, but no updater is installed. | -| `install.sh` has run, then `bootstrap` is used | The pidfile backend uses `CODEX_HOME/packages/standalone/current/codex` | Yes. Bootstrap launches a detached updater loop that runs `install.sh` hourly. | Yes, while that updater process is alive. After a successful fetch, it restarts a currently running app-server only when the managed binary reports a different version. | -| Some other tool updates the managed binary path | The next fresh start or restart uses the updated file at that path | No | Not automatically. The existing process keeps the old executable image until an explicit `restart`. | +| `install.sh` has run, then `bootstrap` is used | The pidfile backend uses `CODEX_HOME/packages/standalone/current/codex` | Yes. Bootstrap launches a detached updater loop that runs `install.sh` hourly. | Yes, while that updater process is alive and app-server is already running. After a successful fetch, the updater restarts app-server with the refreshed binary and only then replaces its own process image. | +| Some other tool updates the managed binary path | The next fresh start or restart uses the updated file at that path | Only if `bootstrap` is active, because the updater still runs `install.sh` on its normal cadence. | Without `bootstrap`, no. With `bootstrap`, the next successful updater pass compares the managed binary contents after `install.sh` runs; if app-server is running and they differ from the updater's current image, it refreshes app-server first and then itself. | ### Standalone installs @@ -62,19 +62,24 @@ For installs created by `install.sh`: - lifecycle commands always use the standalone managed binary path - `bootstrap` is supported - `bootstrap` starts a detached pid-backed updater loop that fetches via - `install.sh`, then restarts app-server if it is running on a different version + `install.sh` +- 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 ### Out-of-band updates This daemon does not watch arbitrary executable files for replacement. If some -other tool updates a binary that the daemon would use on its next launch: +other tool updates the managed binary path: -- a currently running app-server remains on the old executable image -- `restart` will launch the updated binary -- for bootstrapped daemons, the detached updater loop only reacts to updates it - fetched itself; it does not watch arbitrary file replacement +- 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 + binary on its next successful scheduled pass after running `install.sh`; if + app-server is running, it refreshes app-server first and then refreshes itself + once that replacement starts successfully ## Lifecycle semantics diff --git a/codex-rs/app-server-daemon/src/lib.rs b/codex-rs/app-server-daemon/src/lib.rs index 053bcdc35..b5b16b461 100644 --- a/codex-rs/app-server-daemon/src/lib.rs +++ b/codex-rs/app-server-daemon/src/lib.rs @@ -4,6 +4,7 @@ mod managed_install; mod settings; mod update_loop; +use std::path::Path; use std::path::PathBuf; use std::time::Duration; @@ -13,7 +14,7 @@ use anyhow::anyhow; pub use backend::BackendKind; use backend::BackendPaths; use codex_app_server_transport::app_server_control_socket_path; -use codex_core::config::find_codex_home; +use codex_utils_home_dir::find_codex_home; use managed_install::managed_codex_bin; #[cfg(unix)] use managed_install::managed_codex_version; @@ -123,9 +124,35 @@ pub struct RemoteControlOutput { } #[cfg(unix)] +#[derive(Debug, Clone, Copy, PartialEq, Eq)] pub(crate) enum RestartIfRunningOutcome { - Completed, Busy, + NotRunning, + NotReady, + AlreadyCurrent, + Restarted, +} + +#[cfg(unix)] +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum RestartMode { + IfVersionChanged, + Always, +} + +#[cfg(unix)] +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum UpdaterRefreshMode { + None, + ReexecIfManagedBinaryChanged, +} + +#[cfg(unix)] +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum RestartDecision { + NotReady, + AlreadyCurrent, + Restart, } pub async fn run(command: LifecycleCommand) -> Result { @@ -262,33 +289,49 @@ impl Daemon { } #[cfg(unix)] - pub(crate) async fn try_restart_if_running(&self) -> Result { + pub(crate) async fn try_restart_if_running( + &self, + mode: RestartMode, + updater_refresh_mode: UpdaterRefreshMode, + managed_codex_bin: &Path, + ) -> Result { let operation_lock = self.open_operation_lock_file().await?; if !try_lock_file(&operation_lock)? { return Ok(RestartIfRunningOutcome::Busy); } let settings = self.load_settings().await?; - if let Some(backend) = self.running_backend_instance(&settings).await? { - let Ok(info) = client::probe(&self.socket_path).await else { - return Ok(RestartIfRunningOutcome::Completed); + let outcome = if let Some(backend) = self.running_backend_instance(&settings).await? { + let info = client::probe(&self.socket_path).await.ok(); + let managed_version = if info.is_some() { + Some(managed_codex_version(managed_codex_bin).await?) + } else { + None }; - let managed_version = managed_codex_version(&self.managed_codex_bin).await?; - if info.app_server_version == managed_version { - return Ok(RestartIfRunningOutcome::Completed); + match restart_decision(mode, info.as_ref(), managed_version.as_deref()) { + RestartDecision::NotReady => return Ok(RestartIfRunningOutcome::NotReady), + RestartDecision::AlreadyCurrent => RestartIfRunningOutcome::AlreadyCurrent, + RestartDecision::Restart => { + backend.stop().await?; + let _ = self + .start_managed_backend_with_bin(&settings, managed_codex_bin) + .await?; + self.wait_until_ready().await?; + RestartIfRunningOutcome::Restarted + } } - backend.stop().await?; - let _ = self.start_managed_backend(&settings).await?; - self.wait_until_ready().await?; - return Ok(RestartIfRunningOutcome::Completed); - } - - if client::probe(&self.socket_path).await.is_ok() { + } else if client::probe(&self.socket_path).await.is_ok() { return Err(anyhow!( "app server is running but is not managed by codex app-server daemon" )); + } else { + RestartIfRunningOutcome::NotRunning + }; + + if should_reexec_updater(updater_refresh_mode, outcome) { + crate::update_loop::reexec_managed_updater(managed_codex_bin)?; } - Ok(RestartIfRunningOutcome::Completed) + Ok(outcome) } async fn stop(&self) -> Result { @@ -460,7 +503,17 @@ impl Daemon { } async fn start_managed_backend(&self, settings: &DaemonSettings) -> Result> { - let backend = backend::pid_backend(self.backend_paths(settings)); + self.start_managed_backend_with_bin(settings, &self.managed_codex_bin) + .await + } + + async fn start_managed_backend_with_bin( + &self, + settings: &DaemonSettings, + managed_codex_bin: &Path, + ) -> Result> { + let backend = + backend::pid_backend(self.backend_paths_with_bin(settings, managed_codex_bin)); backend.start().await } @@ -476,8 +529,16 @@ impl Daemon { } fn backend_paths(&self, settings: &DaemonSettings) -> BackendPaths { + self.backend_paths_with_bin(settings, &self.managed_codex_bin) + } + + fn backend_paths_with_bin( + &self, + settings: &DaemonSettings, + managed_codex_bin: &Path, + ) -> BackendPaths { BackendPaths { - codex_bin: self.managed_codex_bin.clone(), + codex_bin: managed_codex_bin.to_path_buf(), pid_file: self.pid_file.clone(), update_pid_file: self.update_pid_file.clone(), remote_control_enabled: settings.remote_control_enabled, @@ -575,6 +636,32 @@ fn already_remote_control_status(mode: RemoteControlMode) -> RemoteControlStatus } } +#[cfg(unix)] +fn restart_decision( + mode: RestartMode, + info: Option<&client::ProbeInfo>, + managed_version: Option<&str>, +) -> RestartDecision { + match (mode, info, managed_version) { + (RestartMode::IfVersionChanged, None, _) => RestartDecision::NotReady, + (RestartMode::IfVersionChanged, Some(info), Some(managed_version)) + if info.app_server_version == managed_version => + { + RestartDecision::AlreadyCurrent + } + _ => RestartDecision::Restart, + } +} + +#[cfg(unix)] +fn should_reexec_updater( + updater_refresh_mode: UpdaterRefreshMode, + outcome: RestartIfRunningOutcome, +) -> bool { + updater_refresh_mode == UpdaterRefreshMode::ReexecIfManagedBinaryChanged + && outcome == RestartIfRunningOutcome::Restarted +} + #[cfg(unix)] fn try_lock_file(file: &tokio::fs::File) -> Result { use std::os::fd::AsRawFd; @@ -603,6 +690,13 @@ mod tests { use super::BootstrapStatus; use super::LifecycleStatus; use super::RemoteControlStatus; + use super::RestartDecision; + use super::RestartIfRunningOutcome; + use super::RestartMode; + use super::UpdaterRefreshMode; + use super::restart_decision; + use super::should_reexec_updater; + use crate::client::ProbeInfo; #[test] fn lifecycle_status_uses_camel_case_json() { @@ -627,4 +721,70 @@ mod tests { "\"alreadyEnabled\"" ); } + + #[test] + fn updater_reexec_waits_for_validated_restart() { + assert_eq!( + [ + RestartIfRunningOutcome::Busy, + RestartIfRunningOutcome::NotReady, + RestartIfRunningOutcome::AlreadyCurrent, + RestartIfRunningOutcome::NotRunning, + RestartIfRunningOutcome::Restarted, + ] + .map(|outcome| { + should_reexec_updater(UpdaterRefreshMode::ReexecIfManagedBinaryChanged, outcome) + }), + [false, false, false, false, true] + ); + } + + #[test] + fn unchanged_updater_never_reexecs() { + assert_eq!( + [ + RestartIfRunningOutcome::Busy, + RestartIfRunningOutcome::NotReady, + RestartIfRunningOutcome::AlreadyCurrent, + RestartIfRunningOutcome::NotRunning, + RestartIfRunningOutcome::Restarted, + ] + .map(|outcome| should_reexec_updater(UpdaterRefreshMode::None, outcome)), + [false, false, false, false, false] + ); + } + + #[test] + fn restart_decision_preserves_forced_refreshes() { + let current_info = ProbeInfo { + app_server_version: "0.1.0".to_string(), + }; + + assert_eq!( + [ + restart_decision( + RestartMode::IfVersionChanged, + Some(¤t_info), + Some("0.1.0"), + ), + restart_decision( + RestartMode::IfVersionChanged, + /*info*/ None, + /*managed_version*/ None, + ), + restart_decision(RestartMode::Always, Some(¤t_info), Some("0.1.0")), + restart_decision( + RestartMode::Always, + /*info*/ None, + /*managed_version*/ None + ), + ], + [ + RestartDecision::AlreadyCurrent, + RestartDecision::NotReady, + RestartDecision::Restart, + RestartDecision::Restart, + ] + ); + } } diff --git a/codex-rs/app-server-daemon/src/managed_install.rs b/codex-rs/app-server-daemon/src/managed_install.rs index 83debb24e..d5ad85b98 100644 --- a/codex-rs/app-server-daemon/src/managed_install.rs +++ b/codex-rs/app-server-daemon/src/managed_install.rs @@ -8,6 +8,12 @@ use anyhow::Result; #[cfg(unix)] use anyhow::anyhow; #[cfg(unix)] +use sha2::Digest; +#[cfg(unix)] +use sha2::Sha256; +#[cfg(unix)] +use tokio::fs; +#[cfg(unix)] use tokio::process::Command; pub(crate) fn managed_codex_bin(codex_home: &Path) -> PathBuf { @@ -18,6 +24,16 @@ pub(crate) fn managed_codex_bin(codex_home: &Path) -> PathBuf { .join(managed_codex_file_name()) } +#[cfg(unix)] +pub(crate) async fn resolved_managed_codex_bin(codex_bin: &Path) -> Result { + fs::canonicalize(codex_bin).await.with_context(|| { + format!( + "failed to resolve managed Codex binary {}", + codex_bin.display() + ) + }) +} + #[cfg(unix)] pub(crate) async fn managed_codex_version(codex_bin: &Path) -> Result { let output = Command::new(codex_bin) @@ -47,6 +63,27 @@ pub(crate) async fn managed_codex_version(codex_bin: &Path) -> Result { parse_codex_version(&stdout) } +#[cfg(unix)] +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) struct ExecutableIdentity { + digest: [u8; 32], +} + +#[cfg(unix)] +pub(crate) async fn executable_identity(executable: &Path) -> Result { + let bytes = fs::read(executable) + .await + .with_context(|| format!("failed to read executable {}", executable.display()))?; + Ok(executable_identity_from_bytes(&bytes)) +} + +#[cfg(unix)] +pub(crate) fn executable_identity_from_bytes(bytes: &[u8]) -> ExecutableIdentity { + ExecutableIdentity { + digest: Sha256::digest(bytes).into(), + } +} + fn managed_codex_file_name() -> &'static str { if cfg!(windows) { "codex.exe" } else { "codex" } } diff --git a/codex-rs/app-server-daemon/src/managed_install_tests.rs b/codex-rs/app-server-daemon/src/managed_install_tests.rs index b7d5cccc4..93caa468e 100644 --- a/codex-rs/app-server-daemon/src/managed_install_tests.rs +++ b/codex-rs/app-server-daemon/src/managed_install_tests.rs @@ -1,5 +1,6 @@ use pretty_assertions::assert_eq; +use super::executable_identity_from_bytes; use super::parse_codex_version; #[test] @@ -14,3 +15,13 @@ fn parses_codex_cli_version_output() { fn rejects_malformed_codex_cli_version_output() { assert!(parse_codex_version("codex\n").is_err()); } + +#[test] +fn executable_identity_uses_binary_contents() { + let old = executable_identity_from_bytes(b"old"); + let same = executable_identity_from_bytes(b"old"); + let new = executable_identity_from_bytes(b"new"); + + assert_eq!(old, same); + assert_ne!(old, new); +} diff --git a/codex-rs/app-server-daemon/src/update_loop.rs b/codex-rs/app-server-daemon/src/update_loop.rs index 91193e0af..0fe21e0a6 100644 --- a/codex-rs/app-server-daemon/src/update_loop.rs +++ b/codex-rs/app-server-daemon/src/update_loop.rs @@ -1,4 +1,6 @@ #[cfg(unix)] +use std::process::Command as StdCommand; +#[cfg(unix)] use std::process::Stdio; #[cfg(unix)] use std::time::Duration; @@ -11,6 +13,8 @@ use anyhow::bail; #[cfg(unix)] use futures::FutureExt; #[cfg(unix)] +use std::os::unix::process::CommandExt; +#[cfg(unix)] use tokio::io::AsyncWriteExt; #[cfg(unix)] use tokio::process::Command; @@ -27,6 +31,16 @@ use tokio::time::sleep; use crate::Daemon; #[cfg(unix)] use crate::RestartIfRunningOutcome; +#[cfg(unix)] +use crate::RestartMode; +#[cfg(unix)] +use crate::UpdaterRefreshMode; +#[cfg(unix)] +use crate::managed_install::ExecutableIdentity; +#[cfg(unix)] +use crate::managed_install::executable_identity; +#[cfg(unix)] +use crate::managed_install::resolved_managed_codex_bin; #[cfg(unix)] const INITIAL_UPDATE_DELAY: Duration = Duration::from_secs(5 * 60); @@ -39,11 +53,12 @@ const UPDATE_INTERVAL: Duration = Duration::from_secs(60 * 60); pub(crate) async fn run() -> Result<()> { let mut terminate = signal(SignalKind::terminate()).context("failed to install updater shutdown handler")?; + let running_updater_identity = current_updater_identity().await?; if sleep_or_terminate(INITIAL_UPDATE_DELAY, &mut terminate).await { return Ok(()); } loop { - match update_once(&mut terminate).await { + match update_once(&running_updater_identity, &mut terminate).await { Ok(UpdateLoopControl::Continue) | Err(_) => {} Ok(UpdateLoopControl::Stop) => return Ok(()), } @@ -73,25 +88,71 @@ enum UpdateLoopControl { } #[cfg(unix)] -async fn update_once(terminate: &mut Signal) -> Result { +async fn update_once( + running_updater_identity: &ExecutableIdentity, + terminate: &mut Signal, +) -> Result { install_latest_standalone().await?; let daemon = Daemon::from_environment()?; + let managed_codex_bin = resolved_managed_codex_bin(&daemon.managed_codex_bin).await?; + let managed_identity = executable_identity(&managed_codex_bin).await?; + let (restart_mode, updater_refresh_mode) = + update_modes_for_identities(running_updater_identity, &managed_identity); + loop { if terminate.recv().now_or_never().flatten().is_some() { return Ok(UpdateLoopControl::Stop); } - match daemon.try_restart_if_running().await? { - RestartIfRunningOutcome::Completed => return Ok(UpdateLoopControl::Continue), + match daemon + .try_restart_if_running(restart_mode, updater_refresh_mode, &managed_codex_bin) + .await? + { RestartIfRunningOutcome::Busy => { if sleep_or_terminate(RESTART_RETRY_INTERVAL, terminate).await { return Ok(UpdateLoopControl::Stop); } } + _ => return Ok(UpdateLoopControl::Continue), } } } +#[cfg(unix)] +async fn current_updater_identity() -> Result { + let current_exe = + std::env::current_exe().context("failed to resolve current updater executable")?; + executable_identity(¤t_exe).await +} + +#[cfg(unix)] +fn update_modes_for_identities( + running_updater_identity: &ExecutableIdentity, + managed_identity: &ExecutableIdentity, +) -> (RestartMode, UpdaterRefreshMode) { + if running_updater_identity == managed_identity { + (RestartMode::IfVersionChanged, UpdaterRefreshMode::None) + } else { + ( + RestartMode::Always, + UpdaterRefreshMode::ReexecIfManagedBinaryChanged, + ) + } +} + +#[cfg(unix)] +pub(crate) fn reexec_managed_updater(managed_codex_bin: &std::path::Path) -> Result<()> { + let err = StdCommand::new(managed_codex_bin) + .args(["app-server", "daemon", "pid-update-loop"]) + .exec(); + Err(err).with_context(|| { + format!( + "failed to replace updater with managed Codex binary {}", + managed_codex_bin.display() + ) + }) +} + #[cfg(unix)] async fn install_latest_standalone() -> Result<()> { let script = reqwest::get("https://chatgpt.com/codex/install.sh") @@ -130,3 +191,7 @@ async fn install_latest_standalone() -> Result<()> { anyhow::bail!("standalone Codex updater exited with status {status}") } } + +#[cfg(all(test, unix))] +#[path = "update_loop_tests.rs"] +mod tests; diff --git a/codex-rs/app-server-daemon/src/update_loop_tests.rs b/codex-rs/app-server-daemon/src/update_loop_tests.rs new file mode 100644 index 000000000..cf270693a --- /dev/null +++ b/codex-rs/app-server-daemon/src/update_loop_tests.rs @@ -0,0 +1,31 @@ +use pretty_assertions::assert_eq; + +use super::update_modes_for_identities; +use crate::RestartMode; +use crate::UpdaterRefreshMode; +use crate::managed_install::executable_identity_from_bytes; + +#[test] +fn unchanged_updater_uses_version_based_restart() { + assert_eq!( + update_modes_for_identities( + &executable_identity_from_bytes(b"same"), + &executable_identity_from_bytes(b"same"), + ), + (RestartMode::IfVersionChanged, UpdaterRefreshMode::None) + ); +} + +#[test] +fn changed_updater_forces_refresh_even_when_version_may_match() { + assert_eq!( + update_modes_for_identities( + &executable_identity_from_bytes(b"old"), + &executable_identity_from_bytes(b"new"), + ), + ( + RestartMode::Always, + UpdaterRefreshMode::ReexecIfManagedBinaryChanged, + ) + ); +}