From d6fb836f319085c47cb9400273cd8a92e5975b45 Mon Sep 17 00:00:00 2001 From: jif Date: Fri, 18 Sep 2026 14:43:41 +0000 Subject: [PATCH] Use macOS member fallback in shared process-group termination helpers (#46521) ## Why On macOS, process-group signals can be denied even when individual members can be signalled. Core execution cleanup used helpers that did not retry those signals against group members. ## What changed - Make `terminate_process_group` and `kill_process_group` use the existing member fallback on macOS, and simplify MCP and pipe callers to use the shared helpers. - Use the saved process-group ID when escalating cancellation after the termination grace period, so this path also uses the fallback. - Update the unsafe process-group ID test to exercise `terminate_process_group`. GitOrigin-RevId: 1bab28d3d53cd401b51ffe71d41fbece12aed66d --- codex-rs/core/src/exec.rs | 15 ++++++++------- codex-rs/rmcp-client/src/http_headers.rs | 4 +--- .../rmcp-client/src/stdio_server_launcher.rs | 8 ++------ codex-rs/utils/pty/src/pipe.rs | 7 +------ codex-rs/utils/pty/src/process_group.rs | 16 ++++++++++------ codex-rs/utils/pty/src/process_group_tests.rs | 4 ++-- 6 files changed, 24 insertions(+), 30 deletions(-) diff --git a/codex-rs/core/src/exec.rs b/codex-rs/core/src/exec.rs index 0a6771151f..60cfd00eff 100644 --- a/codex-rs/core/src/exec.rs +++ b/codex-rs/core/src/exec.rs @@ -57,6 +57,8 @@ use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_path_uri::PathUri; use codex_utils_pty::DEFAULT_OUTPUT_BYTES_CAP; use codex_utils_pty::process_group::kill_child_process_group; +use codex_utils_pty::process_group::kill_process_group; +use codex_utils_pty::process_group::terminate_process_group; pub const DEFAULT_EXEC_COMMAND_TIMEOUT_MS: u64 = 10_000; @@ -1051,7 +1053,7 @@ async fn consume_output( // remaining members of the original process group. let process_group_id = child.id(); let should_escalate = if let Some(process_group_id) = process_group_id { - codex_utils_pty::process_group::terminate_process_group(process_group_id)? + terminate_process_group(process_group_id)? } else { false }; @@ -1066,13 +1068,13 @@ async fn consume_output( if should_escalate && let Some(process_group_id) = process_group_id { - codex_utils_pty::process_group::kill_process_group( - process_group_id, - )?; + kill_process_group(process_group_id)?; } } Err(_) => { - kill_child_process_group(&mut child)?; + if let Some(process_group_id) = process_group_id { + kill_process_group(process_group_id)?; + } child.start_kill()?; } } @@ -1156,8 +1158,7 @@ async fn consume_output( match drained { Some(Ok(output)) => output, failure => { - let cleanup = process_group_id - .map_or(Ok(()), codex_utils_pty::process_group::kill_process_group); + let cleanup = process_group_id.map_or(Ok(()), kill_process_group); stdout_handle.abort(); stderr_handle.abort(); if !stdout_done { diff --git a/codex-rs/rmcp-client/src/http_headers.rs b/codex-rs/rmcp-client/src/http_headers.rs index d0b2d28c98..59667de368 100644 --- a/codex-rs/rmcp-client/src/http_headers.rs +++ b/codex-rs/rmcp-client/src/http_headers.rs @@ -19,10 +19,8 @@ use codex_exec_server::HttpRedirectPolicy; use codex_exec_server::HttpRequestParams; use codex_exec_server::HttpRequestResponse; use codex_exec_server::HttpResponseBodyStream; -#[cfg(all(unix, not(target_os = "macos")))] +#[cfg(unix)] use codex_utils_pty::process_group::kill_process_group; -#[cfg(target_os = "macos")] -use codex_utils_pty::process_group::kill_process_group_with_member_fallback as kill_process_group; use futures::FutureExt; use futures::future::BoxFuture; use futures::future::Shared; diff --git a/codex-rs/rmcp-client/src/stdio_server_launcher.rs b/codex-rs/rmcp-client/src/stdio_server_launcher.rs index 84cc008e0f..9ef3b18c4d 100644 --- a/codex-rs/rmcp-client/src/stdio_server_launcher.rs +++ b/codex-rs/rmcp-client/src/stdio_server_launcher.rs @@ -38,14 +38,10 @@ use codex_exec_server::ExecProcess; use codex_protocol::config_types::ShellEnvironmentPolicyInherit; use codex_utils_path_uri::LegacyAppPathString; use codex_utils_path_uri::PathUri; -#[cfg(all(unix, not(target_os = "macos")))] +#[cfg(unix)] use codex_utils_pty::process_group::kill_process_group; -#[cfg(target_os = "macos")] -use codex_utils_pty::process_group::kill_process_group_with_member_fallback as kill_process_group; -#[cfg(all(unix, not(target_os = "macos")))] +#[cfg(unix)] use codex_utils_pty::process_group::terminate_process_group; -#[cfg(target_os = "macos")] -use codex_utils_pty::process_group::terminate_process_group_with_member_fallback as terminate_process_group; use futures::FutureExt; use futures::future::BoxFuture; use rmcp::service::RoleClient; diff --git a/codex-rs/utils/pty/src/pipe.rs b/codex-rs/utils/pty/src/pipe.rs index e189c13be6..867d246452 100644 --- a/codex-rs/utils/pty/src/pipe.rs +++ b/codex-rs/utils/pty/src/pipe.rs @@ -63,16 +63,11 @@ impl ChildTerminator for PipeChildTerminator { } fn kill(&mut self) -> io::Result<()> { - #[cfg(all(unix, not(target_os = "macos")))] + #[cfg(unix)] { crate::process_group::kill_process_group(self.process_group_id) } - #[cfg(target_os = "macos")] - { - crate::process_group::kill_process_group_with_member_fallback(self.process_group_id) - } - #[cfg(windows)] { match &self.windows { diff --git a/codex-rs/utils/pty/src/process_group.rs b/codex-rs/utils/pty/src/process_group.rs index 3d2e3ca452..f403b7e647 100644 --- a/codex-rs/utils/pty/src/process_group.rs +++ b/codex-rs/utils/pty/src/process_group.rs @@ -13,6 +13,8 @@ //! `SIGTERM` when the parent exits, and re-checks the parent PID to avoid //! races during fork/exec. //! +//! On macOS, `terminate_process_group` and `kill_process_group` retry denied +//! group signals against individual members. //! On non-Unix platforms these helpers are no-ops. use std::io; @@ -222,7 +224,7 @@ fn signal_process_group_with_member_fallback( } } -#[cfg(unix)] +#[cfg(all(unix, not(target_os = "macos")))] /// Send SIGTERM to a specific process group ID (best-effort). /// /// Returns `Ok(true)` when SIGTERM was delivered to an existing group and @@ -232,8 +234,10 @@ pub fn terminate_process_group(process_group_id: u32) -> io::Result { } #[cfg(target_os = "macos")] -/// Retry a denied SIGTERM against the exact group's individual members. -pub fn terminate_process_group_with_member_fallback(process_group_id: u32) -> io::Result { +/// Send SIGTERM to a specific process group, retrying denied signals against its members. +/// +/// Returns `Ok(true)` when the group or at least one member was signalled. +pub fn terminate_process_group(process_group_id: u32) -> io::Result { signal_process_group_with_member_fallback( process_group_id, libc::SIGTERM, @@ -260,15 +264,15 @@ pub fn interrupt_process_group(_process_group_id: u32) -> io::Result<()> { Ok(()) } -#[cfg(unix)] +#[cfg(all(unix, not(target_os = "macos")))] /// Kill a specific process group ID (best-effort). pub fn kill_process_group(process_group_id: u32) -> io::Result<()> { signal_process_group_id(process_group_id as libc::pid_t, libc::SIGKILL).map(|_| ()) } #[cfg(target_os = "macos")] -/// Retry a denied SIGKILL against the exact group's individual members. -pub fn kill_process_group_with_member_fallback(process_group_id: u32) -> io::Result<()> { +/// Kill a specific process group, retrying denied signals against its members (best-effort). +pub fn kill_process_group(process_group_id: u32) -> io::Result<()> { signal_process_group_with_member_fallback( process_group_id, libc::SIGKILL, diff --git a/codex-rs/utils/pty/src/process_group_tests.rs b/codex-rs/utils/pty/src/process_group_tests.rs index 4b580ccb95..a3c8f3c3d0 100644 --- a/codex-rs/utils/pty/src/process_group_tests.rs +++ b/codex-rs/utils/pty/src/process_group_tests.rs @@ -12,7 +12,7 @@ use tokio::time::timeout; use super::signal_process_group_with_member_fallback; use super::signal_process_id; -use super::terminate_process_group_with_member_fallback; +use super::terminate_process_group; #[tokio::test] async fn denied_group_signal_terminates_owned_descendants_and_preserves_escalation() -> Result<()> { @@ -87,7 +87,7 @@ async fn denied_group_signal_terminates_owned_descendants_and_preserves_escalati #[test] fn denied_group_signal_rejects_unsafe_process_group_ids() { for process_group_id in [0, u32::MAX] { - let error = terminate_process_group_with_member_fallback(process_group_id) + let error = terminate_process_group(process_group_id) .expect_err("unsafe process group ID should be rejected"); assert_eq!(error.kind(), io::ErrorKind::InvalidInput); }