mirror of
https://github.com/openai/codex.git
synced 2026-09-20 12:47:38 +00:00
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
This commit is contained in:
@@ -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 {
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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<bool> {
|
||||
}
|
||||
|
||||
#[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<bool> {
|
||||
/// 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<bool> {
|
||||
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,
|
||||
|
||||
@@ -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);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user