mirror of
https://github.com/openai/codex.git
synced 2026-10-20 00:18:49 +00:00
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
This commit is contained in:
@@ -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`,
|
||||
|
||||
@@ -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)]
|
||||
|
||||
@@ -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() {
|
||||
|
||||
@@ -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<LifecycleOutput> {
|
||||
// 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));
|
||||
|
||||
@@ -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<u32>,
|
||||
}
|
||||
|
||||
#[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<Self> {
|
||||
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<Self> {
|
||||
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::<StopSettings>(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(|| {
|
||||
|
||||
@@ -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");
|
||||
|
||||
Reference in New Issue
Block a user