Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 13 additions & 17 deletions codex-rs/core/tests/suite/unified_exec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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,
});
Expand Down Expand Up @@ -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(())
}
Expand Down
2 changes: 2 additions & 0 deletions codex-rs/exec-server/src/local_process.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1764,6 +1764,8 @@ mod tests {
terminator: None,
writer_handle: None,
resizer: None,
#[cfg(windows)]
tty: false,
})
.session
}
Expand Down
26 changes: 9 additions & 17 deletions codex-rs/exec-server/tests/exec_process.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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(())
}

Expand Down Expand Up @@ -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")]
Expand Down
7 changes: 6 additions & 1 deletion codex-rs/utils/pty/src/pipe.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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))
}
Expand Down
4 changes: 2 additions & 2 deletions codex-rs/utils/pty/src/pipe_tests.rs
Original file line number Diff line number Diff line change
@@ -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())
Expand All @@ -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(())
Expand Down
28 changes: 26 additions & 2 deletions codex-rs/utils/pty/src/process.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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))
}

Expand Down Expand Up @@ -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.
Expand All @@ -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);
Expand Down Expand Up @@ -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())
Expand Down
51 changes: 50 additions & 1 deletion codex-rs/utils/pty/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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);
Expand All @@ -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()
Expand All @@ -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,
Expand Down Expand Up @@ -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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,7 @@ pub(crate) async fn spawn_windows_sandbox_session_elevated_for_permission_profil
} else {
None
},
tty,
},
stdin_open,
))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down
Loading
Loading