mirror of
https://github.com/openai/codex.git
synced 2026-10-19 16:17:49 +00:00
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
This commit is contained in:
committed by
copyberry
parent
df8990e261
commit
d13db678cb
@@ -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);
|
||||
}
|
||||
|
||||
@@ -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<Command> {
|
||||
#[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<Command> {
|
||||
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;
|
||||
|
||||
@@ -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::<u32>().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");
|
||||
}
|
||||
@@ -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"))]
|
||||
|
||||
@@ -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))]
|
||||
|
||||
Reference in New Issue
Block a user