From a51608398d53b6d23ed98b8287de415b35f1eea5 Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Mon, 7 Sep 2026 19:38:07 +0000 Subject: [PATCH] Make the managed app-server shutdown grace period configurable (#43572) ## Why Managed app-server shutdowns use a fixed 60-second grace period. Allow users to adjust how long shutdown waits before forcing the process to exit. ## What changed - Add `shutdownGraceSeconds` to daemon settings, accepting integers from 0 through 300 and defaulting to 60. Zero requests graceful shutdown and then forces termination immediately. - Apply the setting to managed app-server stops and restarts, including updater-triggered restarts, and extend the lifecycle lock timeout to accommodate the maximum grace period. - Keep `stop` usable with unreadable or invalid settings by falling back to the default grace period. ## Testing Add tests for setting defaults and bounds, invalid-value fallback, preservation when saving remote-control settings, graceful process exit, and forced termination with zero or finite grace periods. GitOrigin-RevId: 8627b391323cf27b40df96935a82e66aa715f572 --- codex-rs/app-server-daemon/README.md | 11 +- codex-rs/app-server-daemon/src/backend/pid.rs | 14 ++- .../src/backend/pid_tests.rs | 105 +++++++++++++++++- codex-rs/app-server-daemon/src/lib.rs | 28 +++-- codex-rs/app-server-daemon/src/settings.rs | 51 +++++++++ .../app-server-daemon/src/settings_tests.rs | 59 +++++++++- 6 files changed, 251 insertions(+), 17 deletions(-) diff --git a/codex-rs/app-server-daemon/README.md b/codex-rs/app-server-daemon/README.md index 913e794cbf..7707b9e9a4 100644 --- a/codex-rs/app-server-daemon/README.md +++ b/codex-rs/app-server-daemon/README.md @@ -50,6 +50,7 @@ default. Edit `CODEX_HOME/app-server-daemon/settings.json` to change this: ```json {"remoteControlEnabled": false, + "shutdownGraceSeconds": 60, "updater": {"autoUpdateEnabled": false, "updateIntervalMinutes": 120}} ``` @@ -66,6 +67,12 @@ interrupted. Accepted updates continue if the CLI exits. Installer errors return nonzero. The updater uses saved network settings; CLI `-c` overrides do not reach it. +For all managed app-server shutdowns, including explicit stop and restart and +updater-triggered restarts, `shutdownGraceSeconds` defaults to 60 and accepts +an integer from 0 through 300. Zero forces shutdown immediately after requesting +a graceful exit; the five-minute maximum bounds the wait even if a turn is still +running. + ## Bootstrap flow For a new Linux or macOS machine: @@ -155,8 +162,8 @@ the managed daemon without clearing its saved remote-control preference. `daemon start` and `daemon restart` use that saved preference. `daemon bootstrap` sets it according to `--remote-control` (disabled when omitted). -`stop` sends a graceful termination request first, then sends a second -termination signal after the grace window if the process is still alive. +`stop` sends a graceful termination request first, then force-terminates the +process after the configured grace window if it is still alive. All mutating lifecycle commands are serialized per `CODEX_HOME`, so a concurrent `start`, `restart`, `enable-remote-control`, `disable-remote-control`, `stop`, diff --git a/codex-rs/app-server-daemon/src/backend/pid.rs b/codex-rs/app-server-daemon/src/backend/pid.rs index 393b5c9798..eeb0512850 100644 --- a/codex-rs/app-server-daemon/src/backend/pid.rs +++ b/codex-rs/app-server-daemon/src/backend/pid.rs @@ -20,9 +20,10 @@ use tokio::io::AsyncSeekExt; use tokio::process::Command; use tokio::time::sleep; +use crate::settings::DEFAULT_SHUTDOWN_GRACE_SECONDS; + const STOP_POLL_INTERVAL: Duration = Duration::from_millis(50); -const STOP_GRACE_PERIOD: Duration = Duration::from_secs(60); -const STOP_TIMEOUT: Duration = Duration::from_secs(70); +const STOP_FORCE_TIMEOUT: Duration = Duration::from_secs(10); const START_TIMEOUT: Duration = Duration::from_secs(10); const STDERR_LOG_TAIL_BYTES: u64 = 4096; @@ -138,6 +139,10 @@ impl PidBackend { } pub(crate) async fn stop(&self) -> Result<()> { + self.stop_with_grace(DEFAULT_SHUTDOWN_GRACE_SECONDS).await + } + + pub(crate) async fn stop_with_grace(&self, grace_seconds: u32) -> Result<()> { loop { let Some(record) = self.wait_for_pid_start().await? else { return Ok(()); @@ -151,7 +156,8 @@ impl PidBackend { let pid = record.pid; let started_at = tokio::time::Instant::now(); - let deadline = started_at + STOP_TIMEOUT; + let force_after = Duration::from_secs(grace_seconds.into()); + let deadline = started_at + force_after + STOP_FORCE_TIMEOUT; #[cfg(unix)] self.terminate_process(pid)?; #[cfg(windows)] @@ -203,7 +209,7 @@ impl PidBackend { if tokio::time::Instant::now() >= deadline { break; } - if !forced && started_at.elapsed() >= STOP_GRACE_PERIOD { + if !forced && started_at.elapsed() >= force_after { #[cfg(unix)] self.force_terminate_process(pid)?; #[cfg(windows)] diff --git a/codex-rs/app-server-daemon/src/backend/pid_tests.rs b/codex-rs/app-server-daemon/src/backend/pid_tests.rs index ce4e9d1bda..98223cb820 100644 --- a/codex-rs/app-server-daemon/src/backend/pid_tests.rs +++ b/codex-rs/app-server-daemon/src/backend/pid_tests.rs @@ -1,10 +1,10 @@ -#[cfg(unix)] +#[cfg(any(unix, windows))] use std::process::Stdio; use std::time::Duration; use pretty_assertions::assert_eq; use tempfile::TempDir; -#[cfg(unix)] +#[cfg(any(unix, windows))] use tokio::time::sleep; use codex_app_server_transport::REMOTE_CONTROL_DISABLED_ENV_VAR; @@ -287,6 +287,107 @@ async fn stop_reaps_untracked_app_server_child() { assert!(!pid_file.exists()); } +#[cfg(any(unix, windows))] +#[tokio::test] +async fn shutdown_grace_child() { + let Some(ready) = std::env::var_os("CODEX_TEST_SHUTDOWN_GRACE_READY") else { + return; + }; + let ready = std::path::PathBuf::from(ready); + #[cfg(unix)] + let mut terminate = tokio::signal::unix::signal(tokio::signal::unix::SignalKind::terminate()) + .expect("install shutdown handler"); + tokio::fs::write(&ready, "").await.expect("ready marker"); + let _ = tokio::time::timeout(Duration::from_secs(8), async { + #[cfg(unix)] + terminate.recv().await; + #[cfg(windows)] + loop { + if ready.with_extension("shutdown").exists() { + break; + } + sleep(Duration::from_millis(10)).await; + } + if std::env::var_os("CODEX_TEST_SHUTDOWN_GRACE_EXIT").is_some() { + sleep(Duration::from_millis(150)).await; + tokio::fs::write(ready.with_extension("exited"), "") + .await + .expect("graceful exit marker"); + return; + } + std::future::pending::<()>().await; + }) + .await; +} + +#[cfg(any(unix, windows))] +#[tokio::test] +async fn shutdown_grace_handles_process_exit() { + let temp = TempDir::new().expect("temp dir"); + for (name, grace_seconds, exits) in [ + ("zero", 0, false), + ("finite_exit", 1, true), + ("finite_force", 1, false), + ] { + let ready = temp.path().join(format!("{name}.ready")); + let mut child = std::process::Command::new(std::env::current_exe().expect("test binary")) + .args(["--exact", "backend::pid::tests::shutdown_grace_child"]) + .env("CODEX_TEST_SHUTDOWN_GRACE_READY", &ready) + .envs(exits.then_some(("CODEX_TEST_SHUTDOWN_GRACE_EXIT", "1"))) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .spawn() + .expect("spawn daemon shim"); + let pid = child.id(); + let wait_until = tokio::time::Instant::now() + Duration::from_secs(3); + while !ready.exists() { + assert!( + tokio::time::Instant::now() < wait_until, + "shim did not start" + ); + sleep(Duration::from_millis(10)).await; + } + let pid_file = temp.path().join(format!("{name}.pid")); + let record = PidRecord { + pid, + process_start_time: super::read_process_start_time(pid) + .await + .expect("start time"), + executable_identity: None, + }; + tokio::fs::write( + &pid_file, + serde_json::to_vec(&record).expect("serialize pid"), + ) + .await + .expect("write pid file"); + #[cfg(unix)] + let backend = PidBackend::new( + temp.path().join("codex"), + pid_file, + /*remote_control_enabled*/ false, + ); + #[cfg(windows)] + let backend = PidBackend::new_update_loop(temp.path().join("codex"), pid_file); + let result = tokio::time::timeout( + Duration::from_secs(3), + backend.stop_with_grace(grace_seconds), + ) + .await; + let still_running = backend + .record_is_active(&record) + .await + .expect("child status"); + if still_running { + child.kill().expect("clean up daemon shim"); + } + let _ = child.wait(); // The backend may already have reaped a Unix child. + result.expect("stop deadline").expect("stop outcome"); + assert_eq!(ready.with_extension("exited").exists(), exits); + assert!(!still_running, "{name}"); + } +} + #[cfg(unix)] #[tokio::test] async fn stopping_updater_signals_its_installer_process_group() { diff --git a/codex-rs/app-server-daemon/src/lib.rs b/codex-rs/app-server-daemon/src/lib.rs index 8d0cca3d9b..49774272cf 100644 --- a/codex-rs/app-server-daemon/src/lib.rs +++ b/codex-rs/app-server-daemon/src/lib.rs @@ -27,11 +27,14 @@ use managed_install::managed_codex_bin; use managed_install::managed_codex_version; use serde::Serialize; use settings::DaemonSettings; +use settings::MAX_SHUTDOWN_GRACE_SECONDS; use tokio::time::sleep; const START_POLL_INTERVAL: Duration = Duration::from_millis(50); const START_TIMEOUT: Duration = Duration::from_secs(10); -const OPERATION_LOCK_TIMEOUT: Duration = Duration::from_secs(75); +// Leave room for the longest graceful stop, forced-exit check, and restart. +const OPERATION_LOCK_TIMEOUT: Duration = + Duration::from_secs(MAX_SHUTDOWN_GRACE_SECONDS as u64 + 75); const PID_FILE_NAME: &str = "app-server.pid"; const UPDATE_PID_FILE_NAME: &str = "app-server-updater.pid"; const OPERATION_LOCK_FILE_NAME: &str = "daemon.lock"; @@ -383,7 +386,9 @@ impl Daemon { self.ensure_managed_codex_bin()?; if let Some(backend) = self.running_backend_instance(&settings).await? { - backend.stop().await?; + backend + .stop_with_grace(settings.shutdown_grace_seconds) + .await?; } let pid = self.start_managed_backend(&settings).await?; @@ -451,7 +456,9 @@ impl Daemon { RestartDecision::Restart => { #[cfg(windows)] backend::windows::ensure_detached_launch(managed_codex_bin)?; - backend.stop().await?; + backend + .stop_with_grace(settings.shutdown_grace_seconds) + .await?; let _ = self .start_managed_backend_with_bin(&settings, managed_codex_bin) .await?; @@ -480,10 +487,11 @@ impl Daemon { } async fn stop(&self) -> Result { - // Stopping a managed process does not require readable settings. - let settings = DaemonSettings::default(); + let settings = DaemonSettings::load_for_stop(&self.settings_file).await; if let Some(backend) = self.running_backend_instance(&settings).await? { - backend.stop().await?; + backend + .stop_with_grace(settings.shutdown_grace_seconds) + .await?; return Ok(self .output( LifecycleStatus::Stopped, @@ -646,7 +654,9 @@ impl Daemon { settings.save(&self.settings_file).await?; let app_server_version = if let Some(backend) = backend { - backend.stop().await?; + backend + .stop_with_grace(settings.shutdown_grace_seconds) + .await?; let _ = self.start_managed_backend(&settings).await?; let info = self.wait_until_ready().await?; if let Err(err) = self.ensure_managed_updater(&settings).await { @@ -685,7 +695,9 @@ impl Daemon { .stop() .await?; if let Some(backend) = self.running_backend_instance(&settings).await? { - backend.stop().await?; + backend + .stop_with_grace(settings.shutdown_grace_seconds) + .await?; } let backend = backend::pid_backend(self.backend_paths(&settings)); diff --git a/codex-rs/app-server-daemon/src/settings.rs b/codex-rs/app-server-daemon/src/settings.rs index 384b3bc709..b9af0e1cdd 100644 --- a/codex-rs/app-server-daemon/src/settings.rs +++ b/codex-rs/app-server-daemon/src/settings.rs @@ -11,12 +11,15 @@ use serde_json::Value; use tokio::fs; pub(crate) const DEFAULT_UPDATE_INTERVAL_MINUTES: u32 = 60; +pub(crate) const DEFAULT_SHUTDOWN_GRACE_SECONDS: u32 = 60; +pub(crate) const MAX_SHUTDOWN_GRACE_SECONDS: u32 = 5 * 60; #[derive(Debug, Clone, PartialEq, Eq)] pub(crate) struct DaemonSettings { pub(crate) remote_control_enabled: bool, pub(crate) auto_update_enabled: bool, pub(crate) update_interval_minutes: u32, + pub(crate) shutdown_grace_seconds: u32, } impl Default for DaemonSettings { @@ -25,19 +28,38 @@ impl Default for DaemonSettings { remote_control_enabled: false, auto_update_enabled: true, update_interval_minutes: DEFAULT_UPDATE_INTERVAL_MINUTES, + shutdown_grace_seconds: DEFAULT_SHUTDOWN_GRACE_SECONDS, } } } #[derive(Default, Deserialize)] #[serde(rename_all = "camelCase")] +struct StopSettings { + shutdown_grace_seconds: Option, +} + +#[derive(Deserialize)] +#[serde(rename_all = "camelCase")] struct StoredSettings { #[serde(default)] remote_control_enabled: bool, + #[serde(default = "default_shutdown_grace_seconds")] + shutdown_grace_seconds: u32, #[serde(default)] updater: UpdaterSettings, } +impl Default for StoredSettings { + fn default() -> Self { + Self { + remote_control_enabled: false, + shutdown_grace_seconds: DEFAULT_SHUTDOWN_GRACE_SECONDS, + updater: UpdaterSettings::default(), + } + } +} + #[derive(Debug, Deserialize)] #[serde(rename_all = "camelCase")] pub(crate) struct UpdaterSettings { @@ -64,9 +86,22 @@ fn default_update_interval_minutes() -> u32 { DEFAULT_UPDATE_INTERVAL_MINUTES } +fn default_shutdown_grace_seconds() -> u32 { + DEFAULT_SHUTDOWN_GRACE_SECONDS +} + +fn validate_shutdown_grace(seconds: u32) -> Result<()> { + ensure!( + seconds <= MAX_SHUTDOWN_GRACE_SECONDS, + "shutdown grace must be between 0 and {MAX_SHUTDOWN_GRACE_SECONDS} seconds" + ); + Ok(()) +} + impl UpdaterSettings { pub(crate) async fn load(settings_file: &Path) -> Result { let settings: StoredSettings = read_settings(settings_file).await?; + validate_shutdown_grace(settings.shutdown_grace_seconds)?; let settings = settings.updater; settings.validate()?; Ok(settings) @@ -89,13 +124,29 @@ impl DaemonSettings { pub(crate) async fn load(path: &Path) -> Result { let settings: StoredSettings = read_settings(path).await?; settings.updater.validate()?; + validate_shutdown_grace(settings.shutdown_grace_seconds)?; Ok(Self { remote_control_enabled: settings.remote_control_enabled, auto_update_enabled: settings.updater.auto_update_enabled, update_interval_minutes: settings.updater.update_interval_minutes, + shutdown_grace_seconds: settings.shutdown_grace_seconds, }) } + pub(crate) async fn load_for_stop(path: &Path) -> Self { + // Stop must work even when settings are unreadable or partially edited. + let shutdown_grace_seconds = read_settings::(path) + .await + .ok() + .and_then(|settings| settings.shutdown_grace_seconds) + .filter(|&seconds| seconds <= MAX_SHUTDOWN_GRACE_SECONDS) + .unwrap_or(DEFAULT_SHUTDOWN_GRACE_SECONDS); + Self { + shutdown_grace_seconds, + ..Self::default() + } + } + pub(crate) async fn save(&self, path: &Path) -> Result<()> { if let Some(parent) = path.parent() { fs::create_dir_all(parent).await.with_context(|| { diff --git a/codex-rs/app-server-daemon/src/settings_tests.rs b/codex-rs/app-server-daemon/src/settings_tests.rs index 64d2588510..487759a0f5 100644 --- a/codex-rs/app-server-daemon/src/settings_tests.rs +++ b/codex-rs/app-server-daemon/src/settings_tests.rs @@ -2,6 +2,7 @@ use pretty_assertions::assert_eq; use tempfile::TempDir; use super::DaemonSettings; +use super::MAX_SHUTDOWN_GRACE_SECONDS; #[tokio::test] async fn remote_control_save_preserves_updater_settings() { @@ -22,7 +23,7 @@ async fn remote_control_save_preserves_updater_settings() { tokio::fs::write( &path, - r#"{"remoteControlEnabled":true,"updater":{"autoUpdateEnabled":false,"updateIntervalMinutes":17},"futureSetting":42}"#, + r#"{"remoteControlEnabled":true,"shutdownGraceSeconds":25,"updater":{"autoUpdateEnabled":false,"updateIntervalMinutes":17},"futureSetting":42}"#, ) .await .expect("write settings"); @@ -40,12 +41,68 @@ async fn remote_control_save_preserves_updater_settings() { .expect("parse settings"), serde_json::json!({ "remoteControlEnabled": false, + "shutdownGraceSeconds": 25, "updater": {"autoUpdateEnabled": false, "updateIntervalMinutes": 17}, "futureSetting": 42, }) ); } +#[tokio::test] +async fn shutdown_grace_accepts_zero_through_five_minutes() { + let temp = TempDir::new().expect("temp dir"); + let path = temp.path().join("settings.json"); + assert_eq!( + DaemonSettings::load(&path).await.expect("missing settings"), + DaemonSettings::default() + ); + assert_eq!( + DaemonSettings::load_for_stop(&path) + .await + .shutdown_grace_seconds, + 60 + ); + + for seconds in [ + 0, + MAX_SHUTDOWN_GRACE_SECONDS, + MAX_SHUTDOWN_GRACE_SECONDS + 1, + ] { + tokio::fs::write( + &path, + serde_json::to_vec(&serde_json::json!({"shutdownGraceSeconds": seconds})) + .expect("serialize settings"), + ) + .await + .expect("write settings"); + let expected = (seconds <= MAX_SHUTDOWN_GRACE_SECONDS).then_some(seconds); + assert_eq!( + DaemonSettings::load(&path) + .await + .ok() + .map(|settings| settings.shutdown_grace_seconds), + expected + ); + assert_eq!( + DaemonSettings::load_for_stop(&path) + .await + .shutdown_grace_seconds, + expected.unwrap_or(60) + ); + } + + tokio::fs::write(&path, r#"{"shutdownGraceSeconds":"unlimited"}"#) + .await + .expect("write invalid settings"); + assert!(DaemonSettings::load(&path).await.is_err()); + assert_eq!( + DaemonSettings::load_for_stop(&path) + .await + .shutdown_grace_seconds, + 60 + ); +} + #[tokio::test] async fn update_interval_accepts_long_values_and_rejects_zero() { let temp = TempDir::new().expect("temp dir");