From d13db678cb5390f77d7fa6ba762f85b787fedcec Mon Sep 17 00:00:00 2001 From: Benjamin Carlsson Date: Sat, 17 Oct 2026 02:14:19 +0000 Subject: [PATCH] Stop applying macOS background priority to managed daemons (#54763) ## What changed Launch managed app-server and updater processes directly through the shared background command helper, removing the macOS `taskpolicy -b` wrapper and its availability probe. Remove the associated launch-policy tests and macOS daemon-startup test serialization. Retain the 30-second macOS readiness timeout because older updaters can pass background policy to replacement daemons during upgrades. GitOrigin-RevId: d8620a49139bd2a5c226c624cf7908e06ba7ba38 --- .../src/backend/pid_start.rs | 41 ++++------- .../src/background_command.rs | 64 +---------------- .../src/background_command_tests.rs | 70 ------------------- codex-rs/app-server-daemon/src/lib.rs | 4 +- codex-rs/cli/tests/daemon_startup.rs | 8 --- 5 files changed, 17 insertions(+), 170 deletions(-) delete mode 100644 codex-rs/app-server-daemon/src/background_command_tests.rs 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 0a959258ef..6eca5d8857 100644 --- a/codex-rs/app-server-daemon/src/backend/pid_start.rs +++ b/codex-rs/app-server-daemon/src/backend/pid_start.rs @@ -95,6 +95,7 @@ impl PidBackend { } else { None }; + let mut command = Command::from(codex_utils_process::background_command(&codex_bin)); let stderr_log = match self.open_stderr_log().await { Ok(stderr_log) => stderr_log, Err(err) => { @@ -104,10 +105,17 @@ 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 { .. }); - let managed_daemon_supported = managed_app_server + if managed_app_server && matches!( tokio::time::timeout( std::time::Duration::from_secs(5), @@ -120,8 +128,10 @@ impl PidBackend { .status(), ).await, Ok(Ok(status)) if status.success() - ); - if managed_app_server && !managed_daemon_supported { + ) + { + command.arg("--managed-daemon"); + } else if managed_app_server { let codex_home = self .pid_file .parent() @@ -136,31 +146,6 @@ 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); } diff --git a/codex-rs/app-server-daemon/src/background_command.rs b/codex-rs/app-server-daemon/src/background_command.rs index 362094dde8..1be0c8133f 100644 --- a/codex-rs/app-server-daemon/src/background_command.rs +++ b/codex-rs/app-server-daemon/src/background_command.rs @@ -1,67 +1,11 @@ -//! Prepares long-lived daemon commands for background execution and recovers -//! startup after the launching directory is removed. +//! Recovers background startup after the launching directory is removed. +//! Preserve process-scoped paths; retain usable Unix cwd defaults for clients. 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() { @@ -146,7 +90,3 @@ 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 deleted file mode 100644 index f62c0d6aad..0000000000 --- a/codex-rs/app-server-daemon/src/background_command_tests.rs +++ /dev/null @@ -1,70 +0,0 @@ -//! 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 851b9cfcf6..88a662486d 100644 --- a/codex-rs/app-server-daemon/src/lib.rs +++ b/codex-rs/app-server-daemon/src/lib.rs @@ -48,8 +48,8 @@ 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. +// An older updater can pass background policy to a replacement daemon, so retain +// the longer readiness allowance during upgrades on macOS. #[cfg(target_os = "macos")] const START_TIMEOUT: Duration = Duration::from_secs(30); #[cfg(not(target_os = "macos"))] diff --git a/codex-rs/cli/tests/daemon_startup.rs b/codex-rs/cli/tests/daemon_startup.rs index 62af8c5af5..a78093e73a 100644 --- a/codex-rs/cli/tests/daemon_startup.rs +++ b/codex-rs/cli/tests/daemon_startup.rs @@ -8,9 +8,6 @@ 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<()> { @@ -155,11 +152,6 @@ 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))]