From 97ebf2d94d62e2ff2045f3ea17b6e11eae9a21fb Mon Sep 17 00:00:00 2001 From: Benjamin Carlsson Date: Thu, 15 Oct 2026 19:54:27 +0000 Subject: [PATCH] Run managed daemon processes at background priority on macOS (#54401) ## What changed - Launch managed processes through `taskpolicy -b` on macOS when a five-second probe succeeds, preserving the managed process PID. Fall back to direct launch if the probe fails or times out. - Increase the macOS startup timeout from 10 to 30 seconds to accommodate background CPU and I/O scheduling, and serialize daemon startup test fixtures on macOS. - Use the shared background command helper and retain `CREATE_NO_WINDOW` alongside Windows detachment flags. ## Testing Add macOS tests for PID preservation, background priority when available, and direct-launch fallback after a probe timeout. GitOrigin-RevId: ab3328e2e8f62218ac9602e4172563c00056875b --- codex-rs/Cargo.lock | 1 + codex-rs/app-server-daemon/Cargo.toml | 1 + .../src/backend/pid_start.rs | 44 ++++++++---- .../src/background_command.rs | 64 ++++++++++++++++- .../src/background_command_tests.rs | 70 +++++++++++++++++++ codex-rs/app-server-daemon/src/lib.rs | 5 ++ codex-rs/cli/tests/daemon_startup.rs | 8 +++ 7 files changed, 177 insertions(+), 16 deletions(-) create mode 100644 codex-rs/app-server-daemon/src/background_command_tests.rs diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index ac8dd51de8..41e6cd96ac 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -2227,6 +2227,7 @@ dependencies = [ "codex-uds", "codex-utils-cargo-bin", "codex-utils-home-dir", + "codex-utils-process", "futures", "libc", "pretty_assertions", diff --git a/codex-rs/app-server-daemon/Cargo.toml b/codex-rs/app-server-daemon/Cargo.toml index 053d788348..3721f88b7d 100644 --- a/codex-rs/app-server-daemon/Cargo.toml +++ b/codex-rs/app-server-daemon/Cargo.toml @@ -20,6 +20,7 @@ codex-features = { workspace = true } codex-http-client = { workspace = true } codex-install-context = { workspace = true } codex-utils-home-dir = { workspace = true } +codex-utils-process = { workspace = true } codex-uds = { workspace = true } futures = { workspace = true } libc = { workspace = true } diff --git a/codex-rs/app-server-daemon/src/backend/pid_start.rs b/codex-rs/app-server-daemon/src/backend/pid_start.rs index 2c091aba30..0a959258ef 100644 --- a/codex-rs/app-server-daemon/src/backend/pid_start.rs +++ b/codex-rs/app-server-daemon/src/backend/pid_start.rs @@ -95,7 +95,6 @@ impl PidBackend { } else { None }; - let mut command = Command::new(&codex_bin); let stderr_log = match self.open_stderr_log().await { Ok(stderr_log) => stderr_log, Err(err) => { @@ -105,17 +104,10 @@ impl PidBackend { return Err(err); } }; - command - // Handoff suppression belongs to the foreground CLI, not its long-lived children. - .env_remove(crate::telemetry::HANDOFF_ENV) - .args(self.command_args().iter().map(std::borrow::Cow::as_ref)) - .stdin(Stdio::null()) - .stdout(Stdio::null()) - .stderr(Stdio::from(stderr_log.into_std().await)); // Older or pinned managed binaries may predate this optional startup flag. let managed_app_server = matches!(self.command_kind, super::PidCommandKind::AppServer { .. }); - if managed_app_server + let managed_daemon_supported = managed_app_server && matches!( tokio::time::timeout( std::time::Duration::from_secs(5), @@ -128,10 +120,8 @@ impl PidBackend { .status(), ).await, Ok(Ok(status)) if status.success() - ) - { - command.arg("--managed-daemon"); - } else if managed_app_server { + ); + if managed_app_server && !managed_daemon_supported { let codex_home = self .pid_file .parent() @@ -146,6 +136,31 @@ impl PidBackend { } } } + let command = crate::background_command::managed_process(&codex_bin).await; + let mut command = match command { + Ok(command) => Command::from(command), + Err(err) => { + if replacement.is_none() { + let _ = fs::remove_file(&self.pid_file).await; + } + return Err(err).with_context(|| { + format!( + "failed to spawn detached app-server process using {}", + self.codex_bin.display() + ) + }); + } + }; + command + // Handoff suppression belongs to the foreground CLI, not its long-lived children. + .env_remove(crate::telemetry::HANDOFF_ENV) + .args(self.command_args().iter().map(std::borrow::Cow::as_ref)) + .stdin(Stdio::null()) + .stdout(Stdio::null()) + .stderr(Stdio::from(stderr_log.into_std().await)); + if managed_daemon_supported { + command.arg("--managed-daemon"); + } if let Some((key, value)) = self.command_env() { command.env(key, value); } @@ -172,6 +187,7 @@ impl PidBackend { #[cfg(windows)] { use windows_sys::Win32::System::Threading::CREATE_BREAKAWAY_FROM_JOB; + use windows_sys::Win32::System::Threading::CREATE_NO_WINDOW; use windows_sys::Win32::System::Threading::DETACHED_PROCESS; // A Windows process pins its working directory for its lifetime. // Keep both managed children out of the launching project's directory. @@ -179,7 +195,7 @@ impl PidBackend { command.current_dir(&workdir); // Never retry inside the parent's Job Object: that would report a // successful launch that dies when the terminal/SSH session closes. - command.creation_flags(DETACHED_PROCESS | CREATE_BREAKAWAY_FROM_JOB); + command.creation_flags(CREATE_NO_WINDOW | DETACHED_PROCESS | CREATE_BREAKAWAY_FROM_JOB); if matches!(self.command_kind, PidCommandKind::UpdateLoop { .. }) { match fs::remove_file(self.pid_file.with_extension("ready")).await { Ok(()) => {} diff --git a/codex-rs/app-server-daemon/src/background_command.rs b/codex-rs/app-server-daemon/src/background_command.rs index 1be0c8133f..362094dde8 100644 --- a/codex-rs/app-server-daemon/src/background_command.rs +++ b/codex-rs/app-server-daemon/src/background_command.rs @@ -1,11 +1,67 @@ -//! Recovers background startup after the launching directory is removed. -//! Preserve process-scoped paths; retain usable Unix cwd defaults for clients. +//! Prepares long-lived daemon commands for background execution and recovers +//! startup after the launching directory is removed. use std::path::Path; use std::process::Command; +#[cfg(target_os = "macos")] +use std::process::Stdio; +#[cfg(target_os = "macos")] +use std::time::Duration; use anyhow::Result; +pub(crate) async fn managed_process(program: &Path) -> std::io::Result { + #[cfg(target_os = "macos")] + { + let mut probe = tokio::process::Command::new("/usr/sbin/taskpolicy"); + probe.args(["-b", "/usr/bin/true"]); + return managed_process_with_probe(program, probe, Duration::from_secs(5)).await; + } + #[cfg(not(target_os = "macos"))] + Ok(codex_utils_process::background_command(program)) +} + +#[cfg(target_os = "macos")] +async fn managed_process_with_probe( + program: &Path, + mut probe: tokio::process::Command, + timeout: Duration, +) -> std::io::Result { + use std::os::unix::fs::PermissionsExt; + + let metadata = program.metadata()?; + if !metadata.is_file() || metadata.permissions().mode() & 0o111 == 0 { + return Err(std::io::Error::new( + std::io::ErrorKind::PermissionDenied, + "managed Codex path is not executable", + )); + } + + // Preserve daemon availability when taskpolicy is denied by a restricted + // environment such as a test sandbox, or when the probe cannot finish promptly. + let available = matches!( + tokio::time::timeout( + timeout, + probe + .stdin(Stdio::null()) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .kill_on_drop(true) + .status(), + ) + .await, + Ok(Ok(status)) if status.success() + ); + if available { + // taskpolicy uses POSIX_SPAWN_SETEXEC, so the managed process keeps this PID. + let mut command = codex_utils_process::background_command("/usr/sbin/taskpolicy"); + command.arg("-b").arg(program); + return Ok(command); + } + + Ok(codex_utils_process::background_command(program)) +} + pub(crate) fn set_working_directory(command: &mut Command, state_dir: &Path) -> Result<()> { #[cfg(unix)] let cwd_missing = match std::env::current_dir() { @@ -90,3 +146,7 @@ pub(crate) fn set_working_directory(command: &mut Command, state_dir: &Path) -> command.current_dir(state_dir); Ok(()) } + +#[cfg(all(test, target_os = "macos"))] +#[path = "background_command_tests.rs"] +mod tests; diff --git a/codex-rs/app-server-daemon/src/background_command_tests.rs b/codex-rs/app-server-daemon/src/background_command_tests.rs new file mode 100644 index 0000000000..f62c0d6aad --- /dev/null +++ b/codex-rs/app-server-daemon/src/background_command_tests.rs @@ -0,0 +1,70 @@ +//! Behavioral coverage for managed macOS process launch policy. + +use std::process::Stdio; +use std::time::Duration; +use std::time::Instant; + +use pretty_assertions::assert_eq; +use tokio::io::AsyncWriteExt; + +#[tokio::test] +async fn managed_process_preserves_pid_and_uses_background_priority_when_available() { + let command = super::managed_process(std::path::Path::new("/usr/bin/ruby")) + .await + .expect("managed command"); + let taskpolicy_available = + command.get_program() == std::ffi::OsStr::new("/usr/sbin/taskpolicy"); + let mut command = tokio::process::Command::from(command); + let mut child = command + .args([ + "-e", + "puts \"#{Process.pid} #{Process.getpriority(4, 0)}\"; STDOUT.flush; STDIN.read(1)", + ]) + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .spawn() + .expect("start managed process"); + let spawned_pid = child.id().expect("managed process pid"); + + child + .stdin + .take() + .expect("managed process stdin") + .write_all(b"x") + .await + .expect("release managed process"); + let output = child.wait_with_output().await.expect("managed process"); + + assert!(output.status.success()); + let output = String::from_utf8(output.stdout).expect("utf-8 managed process output"); + let (pid, priority) = output.trim().split_once(' ').expect("pid and priority"); + assert_eq!(pid.parse::().expect("numeric pid"), spawned_pid); + if taskpolicy_available { + assert_eq!(priority, "1"); + } +} + +#[tokio::test] +async fn timed_out_taskpolicy_probe_falls_back_to_direct_launch() { + let mut probe = tokio::process::Command::new("/bin/sleep"); + probe.arg("60"); + let started = Instant::now(); + let command = super::managed_process_with_probe( + std::path::Path::new("/usr/bin/ruby"), + probe, + Duration::from_millis(20), + ) + .await + .expect("managed command"); + + assert!(started.elapsed() < Duration::from_secs(1)); + assert_eq!(command.get_program(), std::ffi::OsStr::new("/usr/bin/ruby")); + + let output = tokio::process::Command::from(command) + .args(["-e", "puts 'ready'"]) + .output() + .await + .expect("direct launch"); + assert!(output.status.success()); + assert_eq!(output.stdout, b"ready\n"); +} diff --git a/codex-rs/app-server-daemon/src/lib.rs b/codex-rs/app-server-daemon/src/lib.rs index 0b65a9a7b8..851b9cfcf6 100644 --- a/codex-rs/app-server-daemon/src/lib.rs +++ b/codex-rs/app-server-daemon/src/lib.rs @@ -48,6 +48,11 @@ use settings::MAX_SHUTDOWN_GRACE_SECONDS; use tokio::time::sleep; const START_POLL_INTERVAL: Duration = Duration::from_millis(50); +// The managed process starts with background CPU and I/O policy on macOS, so +// foreground load can legitimately delay readiness. +#[cfg(target_os = "macos")] +const START_TIMEOUT: Duration = Duration::from_secs(30); +#[cfg(not(target_os = "macos"))] 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 = diff --git a/codex-rs/cli/tests/daemon_startup.rs b/codex-rs/cli/tests/daemon_startup.rs index a78093e73a..62af8c5af5 100644 --- a/codex-rs/cli/tests/daemon_startup.rs +++ b/codex-rs/cli/tests/daemon_startup.rs @@ -8,6 +8,9 @@ use std::fs; use std::process::Command; use std::time::Duration; +#[cfg(target_os = "macos")] +static DAEMON_STARTUP_SERIAL: tokio::sync::Semaphore = tokio::sync::Semaphore::const_new(1); + #[tokio::test] #[cfg(unix)] async fn auto_daemon_start_attaches_to_shared_server() -> Result<()> { @@ -152,6 +155,11 @@ async fn bedrock_onboarding_leaves_a_running_daemon_untouched() -> Result<()> { } async fn daemon_startup(command: &str) -> Result<()> { + // Background QoS is intentionally allowed to reduce throughput. Avoid + // making many independent daemon fixtures compete during this test binary. + #[cfg(target_os = "macos")] + let _serial = DAEMON_STARTUP_SERIAL.acquire().await?; + #[cfg(windows)] let elevated = codex_app_server_daemon::is_elevated()?; #[cfg(not(windows))]