mirror of
https://github.com/openai/codex.git
synced 2026-04-27 09:51:03 +03:00
feat: detach non-tty childs (#9477)
Thanks to the investigations made by * @frantic-openai https://github.com/openai/codex/pull/9403 * @kfiramar https://github.com/openai/codex/pull/9388
This commit is contained in:
@@ -135,6 +135,13 @@ async fn run_shell_script_with_timeout(
|
||||
// returns a ref of handler.
|
||||
let mut handler = Command::new(&args[0]);
|
||||
handler.args(&args[1..]);
|
||||
#[cfg(unix)]
|
||||
unsafe {
|
||||
handler.pre_exec(|| {
|
||||
codex_utils_pty::process_group::detach_from_tty()?;
|
||||
Ok(())
|
||||
});
|
||||
}
|
||||
handler.kill_on_drop(true);
|
||||
let output = timeout(snapshot_timeout, handler.output())
|
||||
.await
|
||||
|
||||
@@ -66,12 +66,12 @@ pub(crate) async fn spawn_child_async(
|
||||
|
||||
#[cfg(unix)]
|
||||
unsafe {
|
||||
let set_process_group = matches!(stdio_policy, StdioPolicy::RedirectForShellTool);
|
||||
let detach_from_tty = matches!(stdio_policy, StdioPolicy::RedirectForShellTool);
|
||||
#[cfg(target_os = "linux")]
|
||||
let parent_pid = libc::getpid();
|
||||
cmd.pre_exec(move || {
|
||||
if set_process_group {
|
||||
codex_utils_pty::process_group::set_process_group()?;
|
||||
if detach_from_tty {
|
||||
codex_utils_pty::process_group::detach_from_tty()?;
|
||||
}
|
||||
|
||||
// This relies on prctl(2), so it only works on Linux.
|
||||
|
||||
@@ -118,7 +118,7 @@ async fn spawn_process_with_stdin_mode(
|
||||
#[cfg(unix)]
|
||||
unsafe {
|
||||
command.pre_exec(move || {
|
||||
crate::process_group::set_process_group()?;
|
||||
crate::process_group::detach_from_tty()?;
|
||||
#[cfg(target_os = "linux")]
|
||||
crate::process_group::set_parent_death_signal(parent_pid)?;
|
||||
Ok(())
|
||||
|
||||
@@ -4,6 +4,8 @@
|
||||
//! command can be cleaned up reliably:
|
||||
//! - `set_process_group` is called in `pre_exec` so the child starts its own
|
||||
//! process group.
|
||||
//! - `detach_from_tty` starts a new session so non-interactive children do not
|
||||
//! inherit the controlling TTY.
|
||||
//! - `kill_process_group_by_pid` targets the whole group (children/grandchildren)
|
||||
//! - `kill_process_group` targets a known process group ID directly
|
||||
//! instead of a single PID.
|
||||
@@ -42,6 +44,26 @@ pub fn set_parent_death_signal(_parent_pid: i32) -> io::Result<()> {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[cfg(unix)]
|
||||
/// Detach from the controlling TTY by starting a new session.
|
||||
pub fn detach_from_tty() -> io::Result<()> {
|
||||
let result = unsafe { libc::setsid() };
|
||||
if result == -1 {
|
||||
let err = io::Error::last_os_error();
|
||||
if err.raw_os_error() == Some(libc::EPERM) {
|
||||
return set_process_group();
|
||||
}
|
||||
return Err(err);
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[cfg(not(unix))]
|
||||
/// No-op on non-Unix platforms.
|
||||
pub fn detach_from_tty() -> io::Result<()> {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[cfg(unix)]
|
||||
/// Put the calling process into its own process group.
|
||||
///
|
||||
|
||||
@@ -152,6 +152,49 @@ async fn pipe_process_round_trips_stdin() -> anyhow::Result<()> {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[cfg(unix)]
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn pipe_process_detaches_from_parent_session() -> anyhow::Result<()> {
|
||||
let parent_sid = unsafe { libc::getsid(0) };
|
||||
if parent_sid == -1 {
|
||||
anyhow::bail!("failed to read parent session id");
|
||||
}
|
||||
|
||||
let env_map: HashMap<String, String> = std::env::vars().collect();
|
||||
let script = "echo $$; sleep 0.2";
|
||||
let (program, args) = shell_command(script);
|
||||
let spawned = spawn_pipe_process(&program, &args, Path::new("."), &env_map, &None).await?;
|
||||
|
||||
let mut output_rx = spawned.output_rx;
|
||||
let pid_bytes =
|
||||
tokio::time::timeout(tokio::time::Duration::from_millis(500), output_rx.recv()).await??;
|
||||
let pid_text = String::from_utf8_lossy(&pid_bytes);
|
||||
let child_pid: i32 = pid_text
|
||||
.split_whitespace()
|
||||
.next()
|
||||
.ok_or_else(|| anyhow::anyhow!("missing child pid output: {pid_text:?}"))?
|
||||
.parse()?;
|
||||
|
||||
let child_sid = unsafe { libc::getsid(child_pid) };
|
||||
if child_sid == -1 {
|
||||
anyhow::bail!("failed to read child session id");
|
||||
}
|
||||
|
||||
assert_eq!(child_sid, child_pid, "expected child to be session leader");
|
||||
assert_ne!(
|
||||
child_sid, parent_sid,
|
||||
"expected child to be detached from parent session"
|
||||
);
|
||||
|
||||
let exit_code = spawned.exit_rx.await.unwrap_or(-1);
|
||||
assert_eq!(
|
||||
exit_code, 0,
|
||||
"expected detached pipe process to exit cleanly"
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn pipe_and_pty_share_interface() -> anyhow::Result<()> {
|
||||
let env_map: HashMap<String, String> = std::env::vars().collect();
|
||||
|
||||
Reference in New Issue
Block a user