mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
[codex] Handle Ctrl-C for non-TTY unified exec (#26734)
## Why A long-running unified exec process started with `tty: false` could not be interrupted via `write_stdin`: ordinary non-TTY stdin writes are rejected once stdin is closed, but an exact U+0003 payload should still map to a process interrupt. The interrupt should flow through the same process lifecycle path as a real signal so Codex preserves process-reported output and exit metadata instead of fabricating a Ctrl-C exit code or tearing down the session early. ## What Changed - Add `process/signal` to exec-server with `ProcessSignal::Interrupt` and an empty response. - Add a non-consuming `ProcessHandle::signal` path for spawned processes; on Unix it sends SIGINT to the process group and leaves terminate/hard-kill unchanged. - Route non-TTY U+0003 `write_stdin` through `process.signal(...)` instead of `terminate`, then let the normal post-write collection path drain output and observe exit. - Add exec-server coverage where a shell `trap INT` handler prints the signal and exits with its own code. - Add unified exec coverage where a `tty: false` process traps SIGINT, emits output, and exits with its own code. ## Validation - `just test -p codex-exec-server exec_process_signal_interrupts_process` - `just test -p codex-exec-server` - `just test -p codex-core write_stdin_ctrl_c_interrupts_non_tty_session`
This commit is contained in:
committed by
GitHub
Unverified
parent
f574946960
commit
f2969f36e8
@@ -1,5 +1,3 @@
|
||||
#![cfg(unix)]
|
||||
|
||||
mod common;
|
||||
|
||||
use std::sync::Arc;
|
||||
@@ -13,6 +11,7 @@ use codex_exec_server::ExecParams;
|
||||
use codex_exec_server::ExecProcess;
|
||||
use codex_exec_server::ExecProcessEvent;
|
||||
use codex_exec_server::ProcessId;
|
||||
use codex_exec_server::ProcessSignal;
|
||||
use codex_exec_server::ReadResponse;
|
||||
use codex_exec_server::StartedExecProcess;
|
||||
use codex_exec_server::WriteStatus;
|
||||
@@ -505,6 +504,98 @@ async fn assert_exec_process_rejects_write_without_pipe_stdin(use_remote: bool)
|
||||
Ok(())
|
||||
}
|
||||
|
||||
async fn assert_exec_process_signal_interrupts_process(use_remote: bool) -> Result<()> {
|
||||
let context = create_process_context(use_remote).await?;
|
||||
let process_id = "proc-signal".to_string();
|
||||
let session = context
|
||||
.backend
|
||||
.start(ExecParams {
|
||||
process_id: process_id.clone().into(),
|
||||
argv: vec![
|
||||
"/bin/sh".to_string(),
|
||||
"-c".to_string(),
|
||||
"trap 'printf \"signal:2\\n\"; exit 7' INT; printf 'ready\\n'; while :; do :; done".to_string(),
|
||||
],
|
||||
cwd: std::env::current_dir()?,
|
||||
env_policy: /*env_policy*/ None,
|
||||
env: Default::default(),
|
||||
tty: false,
|
||||
pipe_stdin: false,
|
||||
arg0: None,
|
||||
})
|
||||
.await?;
|
||||
assert_eq!(session.process.process_id().as_str(), process_id);
|
||||
|
||||
let StartedExecProcess { process } = session;
|
||||
let mut wake_rx = process.subscribe_wake();
|
||||
let mut ready_output = String::new();
|
||||
let mut after_seq = None;
|
||||
loop {
|
||||
let response =
|
||||
read_process_until_change(Arc::clone(&process), &mut wake_rx, after_seq).await?;
|
||||
for chunk in response.chunks {
|
||||
ready_output.push_str(&String::from_utf8_lossy(&chunk.chunk.into_inner()));
|
||||
after_seq = Some(chunk.seq);
|
||||
}
|
||||
if ready_output.contains("ready\n") {
|
||||
break;
|
||||
}
|
||||
if response.closed {
|
||||
anyhow::bail!("process closed before readiness marker: {ready_output:?}");
|
||||
}
|
||||
after_seq = response.next_seq.checked_sub(1).or(after_seq);
|
||||
}
|
||||
|
||||
process.signal(ProcessSignal::Interrupt).await?;
|
||||
let (output, exit_code, closed) = collect_process_output_from_reads(process, wake_rx).await?;
|
||||
|
||||
assert!(
|
||||
output.contains("signal:2"),
|
||||
"expected signal handler output, got {output:?}"
|
||||
);
|
||||
assert_eq!(exit_code, Some(7));
|
||||
assert!(closed);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
async fn assert_exec_process_signal_reports_unsupported_on_windows(use_remote: bool) -> Result<()> {
|
||||
let context = create_process_context(use_remote).await?;
|
||||
let session = context
|
||||
.backend
|
||||
.start(ExecParams {
|
||||
process_id: ProcessId::from("proc-windows-signal"),
|
||||
argv: vec![
|
||||
"cmd".to_string(),
|
||||
"/C".to_string(),
|
||||
"echo ready && ping -n 30 127.0.0.1 >NUL".to_string(),
|
||||
],
|
||||
cwd: std::env::current_dir()?,
|
||||
env_policy: /*env_policy*/ None,
|
||||
env: Default::default(),
|
||||
tty: false,
|
||||
pipe_stdin: false,
|
||||
arg0: None,
|
||||
})
|
||||
.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}"
|
||||
);
|
||||
|
||||
session.process.terminate().await?;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
async fn assert_exec_process_preserves_queued_events_before_subscribe(
|
||||
use_remote: bool,
|
||||
) -> Result<()> {
|
||||
@@ -539,6 +630,7 @@ async fn assert_exec_process_preserves_queued_events_before_subscribe(
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
#[cfg_attr(not(unix), ignore = "Unix-only exec-server process test")]
|
||||
// Serialize tests that launch a real exec-server process through the full CLI.
|
||||
#[serial_test::serial(remote_exec_server)]
|
||||
async fn remote_exec_process_reports_transport_disconnect() -> Result<()> {
|
||||
@@ -630,6 +722,7 @@ async fn remote_exec_process_reports_transport_disconnect() -> Result<()> {
|
||||
|
||||
#[test_case(false ; "local")]
|
||||
#[test_case(true ; "remote")]
|
||||
#[cfg_attr(not(unix), ignore = "Unix-only exec-server process test")]
|
||||
#[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)]
|
||||
@@ -639,6 +732,7 @@ async fn exec_process_starts_and_exits(use_remote: bool) -> Result<()> {
|
||||
|
||||
#[test_case(false ; "local")]
|
||||
#[test_case(true ; "remote")]
|
||||
#[cfg_attr(not(unix), ignore = "Unix-only exec-server process test")]
|
||||
#[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)]
|
||||
@@ -648,6 +742,7 @@ async fn exec_process_streams_output(use_remote: bool) -> Result<()> {
|
||||
|
||||
#[test_case(false ; "local")]
|
||||
#[test_case(true ; "remote")]
|
||||
#[cfg_attr(not(unix), ignore = "Unix-only exec-server process test")]
|
||||
#[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)]
|
||||
@@ -657,6 +752,7 @@ async fn exec_process_pushes_events(use_remote: bool) -> Result<()> {
|
||||
|
||||
#[test_case(false ; "local")]
|
||||
#[test_case(true ; "remote")]
|
||||
#[cfg_attr(not(unix), ignore = "Unix-only exec-server process test")]
|
||||
#[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)]
|
||||
@@ -666,6 +762,7 @@ async fn exec_process_replays_events_after_close(use_remote: bool) -> Result<()>
|
||||
|
||||
#[test_case(false ; "local")]
|
||||
#[test_case(true ; "remote")]
|
||||
#[cfg_attr(not(unix), ignore = "Unix-only exec-server process test")]
|
||||
#[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)]
|
||||
@@ -677,6 +774,7 @@ async fn exec_process_retains_output_after_exit_until_streams_close(
|
||||
|
||||
#[test_case(false ; "local")]
|
||||
#[test_case(true ; "remote")]
|
||||
#[cfg_attr(not(unix), ignore = "Unix-only exec-server process test")]
|
||||
#[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)]
|
||||
@@ -686,6 +784,7 @@ async fn exec_process_write_then_read(use_remote: bool) -> Result<()> {
|
||||
|
||||
#[test_case(false ; "local")]
|
||||
#[test_case(true ; "remote")]
|
||||
#[cfg_attr(not(unix), ignore = "Unix-only exec-server process test")]
|
||||
#[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)]
|
||||
@@ -695,6 +794,7 @@ async fn exec_process_write_then_read_without_tty(use_remote: bool) -> Result<()
|
||||
|
||||
#[test_case(false ; "local")]
|
||||
#[test_case(true ; "remote")]
|
||||
#[cfg_attr(not(unix), ignore = "Unix-only exec-server process test")]
|
||||
#[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)]
|
||||
@@ -704,6 +804,27 @@ async fn exec_process_rejects_write_without_pipe_stdin(use_remote: bool) -> Resu
|
||||
|
||||
#[test_case(false ; "local")]
|
||||
#[test_case(true ; "remote")]
|
||||
#[cfg_attr(not(unix), ignore = "Unix-only exec-server process test")]
|
||||
#[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_interrupts_process(use_remote: bool) -> Result<()> {
|
||||
assert_exec_process_signal_interrupts_process(use_remote).await
|
||||
}
|
||||
|
||||
#[test_case(false ; "local")]
|
||||
#[test_case(true ; "remote")]
|
||||
#[cfg_attr(not(windows), ignore = "Windows-only exec-server process test")]
|
||||
#[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
|
||||
}
|
||||
|
||||
#[test_case(false ; "local")]
|
||||
#[test_case(true ; "remote")]
|
||||
#[cfg_attr(not(unix), ignore = "Unix-only exec-server process test")]
|
||||
#[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)]
|
||||
|
||||
Reference in New Issue
Block a user