mirror of
https://github.com/openai/codex.git
synced 2026-10-17 17:08:55 +00:00
Fall back to per-process MCP cleanup on macOS (#37068)
## Why Signaling an MCP server's process group can fail with `EPERM` on macOS, preventing owned descendants from being terminated. ## What changed - When a group signal is denied, enumerate the group's processes and signal members that still belong to the expected process group. - Apply the fallback to both `SIGTERM` and `SIGKILL` during MCP stdio server cleanup, while retaining the existing group signaling behavior elsewhere. - Reject invalid process group IDs and avoid signaling processes that have moved to another group. ## Testing Added macOS tests covering cleanup with a live or exited group leader, `SIGTERM`-resistant descendants, escalation to `SIGKILL`, and unsafe group IDs. GitOrigin-RevId: db722475b94dece74244ce8109b3e92f6cfb202a
This commit is contained in:
@@ -38,10 +38,14 @@ use codex_exec_server::ExecProcess;
|
||||
use codex_protocol::config_types::ShellEnvironmentPolicyInherit;
|
||||
use codex_utils_path_uri::LegacyAppPathString;
|
||||
use codex_utils_path_uri::PathUri;
|
||||
#[cfg(unix)]
|
||||
#[cfg(all(unix, not(target_os = "macos")))]
|
||||
use codex_utils_pty::process_group::kill_process_group;
|
||||
#[cfg(unix)]
|
||||
#[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")))]
|
||||
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;
|
||||
|
||||
@@ -133,6 +133,95 @@ fn signal_process_group_id(pgid: libc::pid_t, signal: libc::c_int) -> io::Result
|
||||
Ok(true)
|
||||
}
|
||||
|
||||
#[cfg(target_os = "macos")]
|
||||
fn signal_process_id(pid: libc::pid_t, signal: libc::c_int) -> io::Result<bool> {
|
||||
if unsafe { libc::kill(pid, signal) } == -1 {
|
||||
let error = io::Error::last_os_error();
|
||||
if error.raw_os_error() == Some(libc::ESRCH) {
|
||||
return Ok(false);
|
||||
}
|
||||
return Err(error);
|
||||
}
|
||||
|
||||
Ok(true)
|
||||
}
|
||||
|
||||
#[cfg(target_os = "macos")]
|
||||
fn signal_process_group_with_member_fallback(
|
||||
process_group_id: u32,
|
||||
signal: libc::c_int,
|
||||
signal_group: impl FnOnce(libc::pid_t, libc::c_int) -> io::Result<bool>,
|
||||
mut signal_member: impl FnMut(libc::pid_t, libc::c_int) -> io::Result<bool>,
|
||||
) -> io::Result<bool> {
|
||||
let process_group_id = libc::pid_t::try_from(process_group_id)
|
||||
.ok()
|
||||
.filter(|process_group_id| *process_group_id > 0)
|
||||
.ok_or_else(|| io::Error::new(io::ErrorKind::InvalidInput, "invalid process group ID"))?;
|
||||
|
||||
match signal_group(process_group_id, signal) {
|
||||
Err(error) if error.kind() == io::ErrorKind::PermissionDenied => {}
|
||||
result => return result,
|
||||
}
|
||||
|
||||
let mut process_ids: Vec<libc::pid_t> = vec![0; 16];
|
||||
loop {
|
||||
let buffer_size = libc::c_int::try_from(std::mem::size_of_val(process_ids.as_slice()))
|
||||
.map_err(|_| {
|
||||
io::Error::new(io::ErrorKind::InvalidData, "process group is too large")
|
||||
})?;
|
||||
let count = unsafe {
|
||||
libc::proc_listpgrppids(
|
||||
process_group_id,
|
||||
process_ids.as_mut_ptr().cast(),
|
||||
buffer_size,
|
||||
)
|
||||
};
|
||||
if count < 0 {
|
||||
return Err(io::Error::last_os_error());
|
||||
}
|
||||
let count = count as usize;
|
||||
if count < process_ids.len() {
|
||||
process_ids.truncate(count);
|
||||
break;
|
||||
}
|
||||
let capacity = process_ids.len().checked_mul(2).ok_or_else(|| {
|
||||
io::Error::new(io::ErrorKind::InvalidData, "process group is too large")
|
||||
})?;
|
||||
process_ids.resize(capacity, 0);
|
||||
}
|
||||
process_ids.sort_unstable_by_key(|process_id| *process_id == process_group_id);
|
||||
|
||||
let mut signalled = false;
|
||||
let mut first_error = None;
|
||||
for process_id in process_ids {
|
||||
if process_id <= 0 {
|
||||
continue;
|
||||
}
|
||||
let current_group_id = unsafe { libc::getpgid(process_id) };
|
||||
if current_group_id == -1 {
|
||||
let error = io::Error::last_os_error();
|
||||
if error.raw_os_error() != Some(libc::ESRCH) && first_error.is_none() {
|
||||
first_error = Some(error);
|
||||
}
|
||||
continue;
|
||||
}
|
||||
if current_group_id != process_group_id {
|
||||
continue;
|
||||
}
|
||||
match signal_member(process_id, signal) {
|
||||
Ok(delivered) => signalled |= delivered,
|
||||
Err(error) if first_error.is_none() => first_error = Some(error),
|
||||
Err(_) => {}
|
||||
}
|
||||
}
|
||||
|
||||
if signalled {
|
||||
Ok(true)
|
||||
} else {
|
||||
first_error.map_or(Ok(false), Err)
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(unix)]
|
||||
/// Send SIGTERM to a specific process group ID (best-effort).
|
||||
///
|
||||
@@ -142,6 +231,17 @@ pub fn terminate_process_group(process_group_id: u32) -> io::Result<bool> {
|
||||
signal_process_group_id(process_group_id as libc::pid_t, libc::SIGTERM)
|
||||
}
|
||||
|
||||
#[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> {
|
||||
signal_process_group_with_member_fallback(
|
||||
process_group_id,
|
||||
libc::SIGTERM,
|
||||
signal_process_group_id,
|
||||
signal_process_id,
|
||||
)
|
||||
}
|
||||
|
||||
#[cfg(not(unix))]
|
||||
/// No-op on non-Unix platforms.
|
||||
pub fn terminate_process_group(_process_group_id: u32) -> io::Result<bool> {
|
||||
@@ -166,6 +266,18 @@ 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<()> {
|
||||
signal_process_group_with_member_fallback(
|
||||
process_group_id,
|
||||
libc::SIGKILL,
|
||||
signal_process_group_id,
|
||||
signal_process_id,
|
||||
)
|
||||
.map(|_| ())
|
||||
}
|
||||
|
||||
#[cfg(not(unix))]
|
||||
/// No-op on non-Unix platforms.
|
||||
pub fn kill_process_group(_process_group_id: u32) -> io::Result<()> {
|
||||
@@ -187,3 +299,7 @@ pub fn kill_child_process_group(child: &mut Child) -> io::Result<()> {
|
||||
pub fn kill_child_process_group(_child: &mut Child) -> io::Result<()> {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[cfg(all(test, target_os = "macos"))]
|
||||
#[path = "process_group_tests.rs"]
|
||||
mod tests;
|
||||
|
||||
@@ -0,0 +1,94 @@
|
||||
use std::io;
|
||||
use std::process::Stdio;
|
||||
use std::time::Duration;
|
||||
|
||||
use anyhow::Context;
|
||||
use anyhow::Result;
|
||||
use pretty_assertions::assert_eq;
|
||||
use tokio::io::AsyncBufReadExt;
|
||||
use tokio::io::BufReader;
|
||||
use tokio::process::Command;
|
||||
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;
|
||||
|
||||
#[tokio::test]
|
||||
async fn denied_group_signal_terminates_owned_descendants_and_preserves_escalation() -> Result<()> {
|
||||
for leader_exited in [false, true] {
|
||||
let mut wrapper = Command::new("/bin/sh")
|
||||
.args([
|
||||
"-c",
|
||||
"trap '' TERM; /bin/sleep 30 & resistant=$!; trap - TERM; /bin/sleep 30 & sibling=$!; printf '%s %s\\n' \"$resistant\" \"$sibling\"; wait",
|
||||
])
|
||||
.stdout(Stdio::piped())
|
||||
.stderr(Stdio::null())
|
||||
.kill_on_drop(true)
|
||||
.process_group(0)
|
||||
.spawn()?;
|
||||
let process_group_id =
|
||||
wrapper.id().context("wrapper process has no process ID")? as libc::pid_t;
|
||||
let stdout = wrapper.stdout.take().context("wrapper has no stdout")?;
|
||||
let line = timeout(
|
||||
Duration::from_secs(5),
|
||||
BufReader::new(stdout).lines().next_line(),
|
||||
)
|
||||
.await??
|
||||
.context("missing descendant IDs")?;
|
||||
let (resistant_pid, sibling_pid) =
|
||||
line.split_once(' ').context("invalid descendant IDs")?;
|
||||
let resistant_pid = resistant_pid.parse::<libc::pid_t>()?;
|
||||
let sibling_pid = sibling_pid.parse::<libc::pid_t>()?;
|
||||
|
||||
if leader_exited {
|
||||
wrapper.kill().await?;
|
||||
}
|
||||
|
||||
let mut denied_leader = false;
|
||||
for signal in [libc::SIGTERM, libc::SIGKILL] {
|
||||
assert!(signal_process_group_with_member_fallback(
|
||||
process_group_id as u32,
|
||||
signal,
|
||||
|_, _| Err(io::Error::from_raw_os_error(libc::EPERM)),
|
||||
|process_id, signal| {
|
||||
if process_id == process_group_id {
|
||||
denied_leader = true;
|
||||
Err(io::Error::from_raw_os_error(libc::EPERM))
|
||||
} else {
|
||||
signal_process_id(process_id, signal)
|
||||
}
|
||||
},
|
||||
)?);
|
||||
if signal == libc::SIGTERM {
|
||||
assert_eq!(denied_leader, !leader_exited);
|
||||
assert!(signal_process_id(resistant_pid, /*signal*/ 0)?);
|
||||
}
|
||||
}
|
||||
|
||||
for process_id in [resistant_pid, sibling_pid] {
|
||||
timeout(Duration::from_secs(5), async move {
|
||||
while signal_process_id(process_id, /*signal*/ 0)? {
|
||||
tokio::time::sleep(Duration::from_millis(20)).await;
|
||||
}
|
||||
Ok::<(), io::Error>(())
|
||||
})
|
||||
.await??;
|
||||
}
|
||||
|
||||
if !leader_exited {
|
||||
timeout(Duration::from_secs(5), wrapper.wait()).await??;
|
||||
}
|
||||
}
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[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)
|
||||
.expect_err("unsafe process group ID should be rejected");
|
||||
assert_eq!(error.kind(), io::ErrorKind::InvalidInput);
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user