mirror of
https://github.com/openai/codex.git
synced 2026-10-18 17:18:52 +00:00
Terminate Windows non-TTY processes on interrupt (#35655)
## Why Windows non-TTY exec sessions reported interrupts as unsupported, so sending Ctrl-C through `write_stdin` did not stop the running process. ## What changed - Route interrupt requests for Windows non-TTY processes through their existing termination callback, including pipe-backed processes. - Track whether driver-backed Windows sessions use a TTY so PTY interrupts keep their existing behavior. - Consume the terminator after a successful interrupt to avoid invoking it again when the process handle is dropped. ## Testing Added coverage for local and remote exec-server sessions, unified exec, pipe fallbacks, and legacy Windows sandbox processes. GitOrigin-RevId: 34504d01f091ef57bb961e98ad5a8d9f1acee4ee
This commit is contained in:
@@ -2199,9 +2199,14 @@ async fn assert_write_stdin_ctrl_c_interrupts_non_tty_session(
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
#[cfg_attr(not(windows), ignore = "Windows-only unified exec interrupt test")]
|
||||
async fn write_stdin_ctrl_c_reports_unsupported_interrupt_to_model_on_windows() -> Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
async fn write_stdin_ctrl_c_terminates_non_tty_session_on_windows() -> Result<()> {
|
||||
if core_test_support::test_target_os() != core_test_support::TestTargetOs::Windows {
|
||||
return Ok(());
|
||||
}
|
||||
skip_if_wine_exec!(
|
||||
Ok(()),
|
||||
"Wine exits Windows non-TTY shell processes immediately"
|
||||
);
|
||||
skip_if_sandbox!(Ok(()));
|
||||
|
||||
let server = start_mock_server().await;
|
||||
@@ -2218,8 +2223,7 @@ async fn write_stdin_ctrl_c_reports_unsupported_interrupt_to_model_on_windows()
|
||||
let interrupt_call_id = "uexec-windows-interrupt";
|
||||
|
||||
let start_args = serde_json::json!({
|
||||
"shell": "cmd",
|
||||
"cmd": "echo READY && ping -n 30 127.0.0.1 >NUL",
|
||||
"cmd": "Start-Sleep -Seconds 30",
|
||||
"yield_time_ms": 250,
|
||||
"tty": false,
|
||||
});
|
||||
@@ -2277,23 +2281,15 @@ async fn write_stdin_ctrl_c_reports_unsupported_interrupt_to_model_on_windows()
|
||||
Some("1000"),
|
||||
"exec_command should leave a running non-TTY session"
|
||||
);
|
||||
assert!(
|
||||
start_output.output.contains("READY"),
|
||||
"start output should include command readiness marker, got {:?}",
|
||||
start_output.output
|
||||
);
|
||||
|
||||
let interrupt_output = request_log
|
||||
.function_call_output_text(interrupt_call_id)
|
||||
.expect("missing interrupt output for write_stdin");
|
||||
let interrupt_output = parse_unified_exec_output(&interrupt_output)?;
|
||||
assert!(
|
||||
interrupt_output.contains("write_stdin failed"),
|
||||
"model-visible write_stdin output should report failure, got {interrupt_output:?}"
|
||||
);
|
||||
assert!(
|
||||
interrupt_output.contains("process interrupt is not supported by this process backend"),
|
||||
"model-visible write_stdin output should explain unsupported interrupt, got {interrupt_output:?}"
|
||||
interrupt_output.process_id.is_none(),
|
||||
"interrupted process should be cleared from the session map"
|
||||
);
|
||||
assert_eq!(interrupt_output.exit_code, Some(1));
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
@@ -1764,6 +1764,8 @@ mod tests {
|
||||
terminator: None,
|
||||
writer_handle: None,
|
||||
resizer: None,
|
||||
#[cfg(windows)]
|
||||
tty: false,
|
||||
})
|
||||
.session
|
||||
}
|
||||
|
||||
@@ -932,7 +932,7 @@ async fn assert_exec_process_signal_interrupts_process(use_remote: bool) -> Resu
|
||||
Ok(())
|
||||
}
|
||||
|
||||
async fn assert_exec_process_signal_reports_unsupported_on_windows(use_remote: bool) -> Result<()> {
|
||||
async fn assert_exec_process_signal_terminates_on_windows(use_remote: bool) -> Result<()> {
|
||||
let context = create_process_context(use_remote).await?;
|
||||
let session = context
|
||||
.backend
|
||||
@@ -956,21 +956,13 @@ async fn assert_exec_process_signal_reports_unsupported_on_windows(use_remote: b
|
||||
})
|
||||
.await?;
|
||||
|
||||
let err = match session.process.signal(ProcessSignal::Interrupt).await {
|
||||
Ok(()) => anyhow::bail!("Windows non-TTY signal should report unsupported"),
|
||||
Err(err) => err,
|
||||
};
|
||||
let message = err.to_string();
|
||||
assert!(
|
||||
message.contains("failed to signal process"),
|
||||
"unexpected signal error: {message}"
|
||||
);
|
||||
assert!(
|
||||
message.contains("process interrupt is not supported by this process backend"),
|
||||
"unexpected signal error: {message}"
|
||||
);
|
||||
let StartedExecProcess { process, .. } = session;
|
||||
let wake_rx = process.subscribe_wake();
|
||||
process.signal(ProcessSignal::Interrupt).await?;
|
||||
let (_output, exit_code, closed) = collect_process_output_from_reads(process, wake_rx).await?;
|
||||
|
||||
session.process.terminate().await?;
|
||||
assert_eq!(exit_code, Some(1));
|
||||
assert!(closed);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
@@ -1268,8 +1260,8 @@ async fn exec_process_signal_interrupts_process(use_remote: bool) -> Result<()>
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
// Serialize tests that launch a real exec-server process through the full CLI.
|
||||
#[serial_test::serial(remote_exec_server)]
|
||||
async fn exec_process_signal_reports_unsupported_on_windows(use_remote: bool) -> Result<()> {
|
||||
assert_exec_process_signal_reports_unsupported_on_windows(use_remote).await
|
||||
async fn exec_process_signal_terminates_on_windows(use_remote: bool) -> Result<()> {
|
||||
assert_exec_process_signal_terminates_on_windows(use_remote).await
|
||||
}
|
||||
|
||||
#[test_case(false ; "local")]
|
||||
|
||||
@@ -48,7 +48,12 @@ impl ChildTerminator for PipeChildTerminator {
|
||||
crate::process_group::interrupt_process_group(self.process_group_id)
|
||||
}
|
||||
|
||||
#[cfg(not(unix))]
|
||||
#[cfg(windows)]
|
||||
{
|
||||
self.kill()
|
||||
}
|
||||
|
||||
#[cfg(not(any(unix, windows)))]
|
||||
{
|
||||
Err(crate::process::unsupported_signal(signal))
|
||||
}
|
||||
|
||||
@@ -1,7 +1,7 @@
|
||||
use super::*;
|
||||
|
||||
#[test]
|
||||
fn process_fallback_terminates_root() -> anyhow::Result<()> {
|
||||
fn process_fallback_interrupt_terminates_root() -> anyhow::Result<()> {
|
||||
let mut child = std::process::Command::new("ping.exe")
|
||||
.args(["-n", "60", "127.0.0.1"])
|
||||
.stdin(Stdio::null())
|
||||
@@ -12,7 +12,7 @@ fn process_fallback_terminates_root() -> anyhow::Result<()> {
|
||||
windows: WindowsChildTerminator::Process(child.id()),
|
||||
};
|
||||
|
||||
terminator.kill()?;
|
||||
terminator.signal(ProcessSignal::Interrupt)?;
|
||||
|
||||
assert!(!child.wait()?.success());
|
||||
Ok(())
|
||||
|
||||
@@ -234,7 +234,12 @@ impl ProcessHandle {
|
||||
return Ok(());
|
||||
};
|
||||
|
||||
killer.signal(signal)
|
||||
let result = killer.signal(signal);
|
||||
#[cfg(windows)]
|
||||
if result.is_ok() {
|
||||
killer_opt.take();
|
||||
}
|
||||
result
|
||||
}
|
||||
|
||||
/// Attempts to kill the child and abort helper tasks.
|
||||
@@ -273,10 +278,17 @@ impl Drop for ProcessHandle {
|
||||
/// Adapts a closure into a `ChildTerminator` implementation.
|
||||
struct ClosureTerminator {
|
||||
inner: Option<Box<dyn FnMut() + Send + Sync>>,
|
||||
#[cfg(windows)]
|
||||
interrupt_terminates: bool,
|
||||
}
|
||||
|
||||
impl ChildTerminator for ClosureTerminator {
|
||||
fn signal(&mut self, signal: ProcessSignal) -> io::Result<()> {
|
||||
#[cfg(windows)]
|
||||
if self.interrupt_terminates {
|
||||
return self.kill();
|
||||
}
|
||||
|
||||
Err(unsupported_signal(signal))
|
||||
}
|
||||
|
||||
@@ -356,6 +368,9 @@ pub struct ProcessDriver {
|
||||
pub terminator: Option<Box<dyn FnMut() + Send + Sync>>,
|
||||
pub writer_handle: Option<JoinHandle<()>>,
|
||||
pub resizer: Option<ResizeFn>,
|
||||
/// Whether this Windows process is attached to a pseudo-console.
|
||||
#[cfg(windows)]
|
||||
pub tty: bool,
|
||||
}
|
||||
|
||||
/// Build a `SpawnedProcess` from a driver that supplies stdin/output/exit channels.
|
||||
@@ -368,8 +383,13 @@ pub fn spawn_from_driver(driver: ProcessDriver) -> SpawnedProcess {
|
||||
terminator,
|
||||
writer_handle,
|
||||
resizer,
|
||||
#[cfg(windows)]
|
||||
tty,
|
||||
} = driver;
|
||||
|
||||
#[cfg(windows)]
|
||||
let interrupt_terminates = terminator.is_some() && !tty;
|
||||
|
||||
let (stdout_tx, stdout_rx) = mpsc::channel::<Vec<u8>>(256);
|
||||
let (stderr_tx, stderr_rx) = mpsc::channel::<Vec<u8>>(256);
|
||||
let (exit_seen_tx, exit_seen_rx) = watch::channel(false);
|
||||
@@ -433,7 +453,11 @@ pub fn spawn_from_driver(driver: ProcessDriver) -> SpawnedProcess {
|
||||
|
||||
let handle = ProcessHandle::new(
|
||||
writer_tx,
|
||||
Box::new(ClosureTerminator { inner: terminator }),
|
||||
Box::new(ClosureTerminator {
|
||||
inner: terminator,
|
||||
#[cfg(windows)]
|
||||
interrupt_terminates,
|
||||
}),
|
||||
reader_handle,
|
||||
stderr_reader_handle
|
||||
.map(|handle| handle.abort_handle())
|
||||
|
||||
@@ -4,6 +4,7 @@ use std::path::Path;
|
||||
use pretty_assertions::assert_eq;
|
||||
|
||||
use crate::ProcessDriver;
|
||||
use crate::ProcessSignal;
|
||||
use crate::SpawnedProcess;
|
||||
use crate::TerminalSize;
|
||||
use crate::combine_output_receivers;
|
||||
@@ -614,7 +615,14 @@ async fn driver_backed_process_can_expose_split_stdout_and_stderr() -> anyhow::R
|
||||
terminator: None,
|
||||
writer_handle: None,
|
||||
resizer: None,
|
||||
#[cfg(windows)]
|
||||
tty: false,
|
||||
});
|
||||
let error = spawned
|
||||
.session
|
||||
.signal(ProcessSignal::Interrupt)
|
||||
.expect_err("interrupting a driver without a terminator should remain unsupported");
|
||||
assert_eq!(error.kind(), std::io::ErrorKind::Unsupported);
|
||||
|
||||
let SpawnedProcess {
|
||||
session: _session,
|
||||
@@ -650,6 +658,37 @@ async fn driver_backed_process_can_expose_split_stdout_and_stderr() -> anyhow::R
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[cfg(windows)]
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn driver_backed_interrupt_terminates_once() {
|
||||
let terminations = std::sync::Arc::new(std::sync::atomic::AtomicUsize::new(0));
|
||||
let callback_terminations = std::sync::Arc::clone(&terminations);
|
||||
let (writer_tx, _writer_rx) = tokio::sync::mpsc::channel::<Vec<u8>>(1);
|
||||
let (_stdout_tx, stdout_rx) = tokio::sync::broadcast::channel::<Vec<u8>>(1);
|
||||
let (_exit_tx, exit_rx) = tokio::sync::oneshot::channel::<i32>();
|
||||
let spawned = spawn_from_driver(ProcessDriver {
|
||||
writer_tx,
|
||||
stdout_rx,
|
||||
stderr_rx: None,
|
||||
exit_rx,
|
||||
terminator: Some(Box::new(move || {
|
||||
callback_terminations.fetch_add(1, std::sync::atomic::Ordering::SeqCst);
|
||||
})),
|
||||
writer_handle: None,
|
||||
resizer: None,
|
||||
#[cfg(windows)]
|
||||
tty: false,
|
||||
});
|
||||
|
||||
spawned
|
||||
.session
|
||||
.signal(ProcessSignal::Interrupt)
|
||||
.expect("interrupt should terminate the driver-backed process");
|
||||
drop(spawned.session);
|
||||
|
||||
assert_eq!(terminations.load(std::sync::atomic::Ordering::SeqCst), 1);
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn driver_backed_process_can_resize_via_resizer_hook() -> anyhow::Result<()> {
|
||||
let (writer_tx, _writer_rx) = tokio::sync::mpsc::channel::<Vec<u8>>(1);
|
||||
@@ -663,7 +702,7 @@ async fn driver_backed_process_can_resize_via_resizer_hook() -> anyhow::Result<(
|
||||
stdout_rx: stdout_driver_rx,
|
||||
stderr_rx: None,
|
||||
exit_rx,
|
||||
terminator: None,
|
||||
terminator: Some(Box::new(|| {})),
|
||||
writer_handle: None,
|
||||
resizer: Some(Box::new(move |size| {
|
||||
if let Ok(mut guard) = size_tx.lock()
|
||||
@@ -673,8 +712,16 @@ async fn driver_backed_process_can_resize_via_resizer_hook() -> anyhow::Result<(
|
||||
}
|
||||
Ok(())
|
||||
})),
|
||||
#[cfg(windows)]
|
||||
tty: true,
|
||||
});
|
||||
|
||||
let error = spawned
|
||||
.session
|
||||
.signal(ProcessSignal::Interrupt)
|
||||
.expect_err("interrupting a PTY-backed driver should remain unsupported");
|
||||
assert_eq!(error.kind(), std::io::ErrorKind::Unsupported);
|
||||
|
||||
spawned.session.resize(TerminalSize {
|
||||
rows: 40,
|
||||
cols: 120,
|
||||
@@ -711,6 +758,8 @@ async fn driver_backed_process_drains_output_that_arrives_after_exit_signal() ->
|
||||
terminator: None,
|
||||
writer_handle: None,
|
||||
resizer: None,
|
||||
#[cfg(windows)]
|
||||
tty: false,
|
||||
});
|
||||
|
||||
let SpawnedProcess {
|
||||
|
||||
@@ -232,6 +232,7 @@ pub(crate) async fn spawn_windows_sandbox_session_elevated_for_permission_profil
|
||||
} else {
|
||||
None
|
||||
},
|
||||
tty,
|
||||
},
|
||||
stdin_open,
|
||||
))
|
||||
|
||||
@@ -470,6 +470,7 @@ pub(crate) async fn spawn_windows_sandbox_session_legacy(
|
||||
Box::new(move |size| resize_conpty_handle(&hpc, size))
|
||||
as Box<dyn FnMut(TerminalSize) -> Result<()> + Send>
|
||||
}),
|
||||
tty,
|
||||
};
|
||||
|
||||
Ok(finish_driver_spawn(driver, stdin_open))
|
||||
|
||||
@@ -12,6 +12,7 @@ use base64::engine::general_purpose::STANDARD as BASE64;
|
||||
use codex_protocol::models::PermissionProfile;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use codex_utils_pty::ProcessDriver;
|
||||
use codex_utils_pty::ProcessSignal;
|
||||
use pretty_assertions::assert_eq;
|
||||
use std::collections::HashMap;
|
||||
use std::fs;
|
||||
@@ -345,7 +346,7 @@ fn legacy_non_tty_cmd_rejects_deny_read_overrides() {
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn legacy_non_tty_powershell_emits_output() {
|
||||
fn legacy_non_tty_powershell_interrupt_terminates_process() {
|
||||
let Some(pwsh) = pwsh_path() else {
|
||||
return;
|
||||
};
|
||||
@@ -364,11 +365,11 @@ fn legacy_non_tty_powershell_emits_output() {
|
||||
pwsh.display().to_string(),
|
||||
"-NoProfile".to_string(),
|
||||
"-Command".to_string(),
|
||||
"Write-Output LEGACY-NONTTY-DIRECT".to_string(),
|
||||
"Write-Output LEGACY-NONTTY-DIRECT; [System.Threading.ManualResetEvent]::new($false).WaitOne()".to_string(),
|
||||
],
|
||||
cwd.as_path(),
|
||||
HashMap::new(),
|
||||
Some(5_000),
|
||||
/*timeout_ms*/ None,
|
||||
&[],
|
||||
&[],
|
||||
/*tty*/ false,
|
||||
@@ -378,12 +379,41 @@ fn legacy_non_tty_powershell_emits_output() {
|
||||
.await
|
||||
.expect("spawn legacy non-tty powershell session");
|
||||
println!("pwsh spawn returned");
|
||||
let (stdout, exit_code) =
|
||||
collect_stdout_and_exit(spawned, codex_home.path(), Duration::from_secs(10)).await;
|
||||
println!("pwsh collect returned exit_code={exit_code}");
|
||||
let stdout = String::from_utf8_lossy(&stdout);
|
||||
assert_eq!(exit_code, 0, "stdout={stdout:?}");
|
||||
assert!(stdout.contains("LEGACY-NONTTY-DIRECT"), "stdout={stdout:?}");
|
||||
let codex_utils_pty::SpawnedProcess {
|
||||
session,
|
||||
mut stdout_rx,
|
||||
stderr_rx: _stderr_rx,
|
||||
exit_rx,
|
||||
} = spawned;
|
||||
timeout(Duration::from_secs(5), async {
|
||||
let mut stdout = Vec::new();
|
||||
while let Some(chunk) = stdout_rx.recv().await {
|
||||
stdout.extend(chunk);
|
||||
if String::from_utf8_lossy(&stdout).contains("LEGACY-NONTTY-DIRECT") {
|
||||
return;
|
||||
}
|
||||
}
|
||||
panic!(
|
||||
"PowerShell exited before emitting its readiness marker\n{}",
|
||||
sandbox_log(codex_home.path())
|
||||
);
|
||||
})
|
||||
.await
|
||||
.unwrap_or_else(|_| {
|
||||
panic!(
|
||||
"timed out waiting for PowerShell readiness marker\n{}",
|
||||
sandbox_log(codex_home.path())
|
||||
)
|
||||
});
|
||||
|
||||
session
|
||||
.signal(ProcessSignal::Interrupt)
|
||||
.expect("interrupt should terminate the restricted-token process job");
|
||||
let exit_code = timeout(Duration::from_secs(5), exit_rx)
|
||||
.await
|
||||
.unwrap_or_else(|_| panic!("timed out waiting for exit\n{}", sandbox_log(codex_home.path())))
|
||||
.expect("interrupted process should report an exit code");
|
||||
assert_eq!(exit_code, 1);
|
||||
});
|
||||
}
|
||||
|
||||
@@ -405,6 +435,7 @@ fn finish_driver_spawn_keeps_stdin_open_when_requested() {
|
||||
terminator: None,
|
||||
writer_handle: None,
|
||||
resizer: None,
|
||||
tty: false,
|
||||
},
|
||||
/*stdin_open*/ true,
|
||||
);
|
||||
@@ -437,6 +468,7 @@ fn finish_driver_spawn_closes_stdin_when_not_requested() {
|
||||
terminator: None,
|
||||
writer_handle: None,
|
||||
resizer: None,
|
||||
tty: false,
|
||||
},
|
||||
/*stdin_open*/ false,
|
||||
);
|
||||
|
||||
Reference in New Issue
Block a user