mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Fix stale background terminal poll events (#23231)
## Why Issue #23214 reports `/ps` showing no background terminals while the status line still says it is waiting for a background terminal. The race is in core: `write_stdin` can poll a process that exits before the response returns. The process manager correctly returns `process_id: None`, but the handler still emitted a `TerminalInteraction` event using the requested session id, causing clients to believe a dead process was still being polled. Fixes #23214. ## What changed - Suppress `TerminalInteraction` events for empty `write_stdin` polls once `response.process_id` is `None`. - Continue emitting interactions for non-empty stdin, even if that input causes the process to exit before the response returns. - Extend the unified exec integration test to assert completed empty polls do not emit terminal interactions. ## Verification - `cargo test -p codex-core --test all unified_exec_emits_one_begin_and_one_end_event` - `cargo test -p codex-core --test all unified_exec_emits_terminal_interaction_for_write_stdin` `cargo test -p codex-core` currently aborts in unrelated `agent::control::tests::resume_agent_from_rollout_uses_edge_data_when_descendant_metadata_source_is_stale` with a reproducible stack overflow.
This commit is contained in:
committed by
GitHub
Unverified
parent
532b9c83ae
commit
e43a2e297f
@@ -78,14 +78,21 @@ impl ToolExecutor<ToolInvocation> for WriteStdinHandler {
|
||||
FunctionCallError::RespondToModel(format!("write_stdin failed: {err}"))
|
||||
})?;
|
||||
|
||||
let interaction = TerminalInteractionEvent {
|
||||
call_id: response.event_call_id.clone(),
|
||||
process_id: args.session_id.to_string(),
|
||||
stdin: args.chars.clone(),
|
||||
};
|
||||
session
|
||||
.send_event(turn.as_ref(), EventMsg::TerminalInteraction(interaction))
|
||||
.await;
|
||||
// Empty stdin is a background poll, so emit it only while there is
|
||||
// still a live process for the UI to wait on. Non-empty stdin is a real
|
||||
// terminal interaction and should remain visible even if it completes
|
||||
// the process before the response returns.
|
||||
if !args.chars.is_empty() || response.process_id.is_some() {
|
||||
let process_id = response.process_id.unwrap_or(args.session_id);
|
||||
let interaction = TerminalInteractionEvent {
|
||||
call_id: response.event_call_id.clone(),
|
||||
process_id: process_id.to_string(),
|
||||
stdin: args.chars.clone(),
|
||||
};
|
||||
session
|
||||
.send_event(turn.as_ref(), EventMsg::TerminalInteraction(interaction))
|
||||
.await;
|
||||
}
|
||||
|
||||
Ok(boxed_tool_output(response))
|
||||
}
|
||||
|
||||
@@ -691,8 +691,8 @@ impl UnifiedExecProcessManager {
|
||||
|
||||
// After polling, refresh_process_state tells us whether the PTY is
|
||||
// still alive or has exited and been removed from the store; we thread
|
||||
// that through so the handler can tag TerminalInteraction with an
|
||||
// appropriate process_id and exit_code.
|
||||
// that through so the handler can tag or suppress TerminalInteraction
|
||||
// with an appropriate process_id and exit_code.
|
||||
let status = if let Some(status) = status_after_write {
|
||||
status
|
||||
} else {
|
||||
|
||||
@@ -1322,6 +1322,7 @@ async fn unified_exec_emits_one_begin_and_one_end_event() -> Result<()> {
|
||||
|
||||
let mut begin_events = Vec::new();
|
||||
let mut end_events = Vec::new();
|
||||
let mut terminal_interactions = Vec::new();
|
||||
let mut task_completed = false;
|
||||
loop {
|
||||
let event_msg = wait_for_event(&test.codex, |_| true).await;
|
||||
@@ -1332,6 +1333,9 @@ async fn unified_exec_emits_one_begin_and_one_end_event() -> Result<()> {
|
||||
EventMsg::ExecCommandEnd(event) if event.call_id == open_call_id => {
|
||||
end_events.push(event);
|
||||
}
|
||||
EventMsg::TerminalInteraction(event) if event.call_id == open_call_id => {
|
||||
terminal_interactions.push(event);
|
||||
}
|
||||
EventMsg::TurnComplete(_) => {
|
||||
task_completed = true;
|
||||
}
|
||||
@@ -1353,6 +1357,10 @@ async fn unified_exec_emits_one_begin_and_one_end_event() -> Result<()> {
|
||||
1,
|
||||
"expected end event for the write_stdin call"
|
||||
);
|
||||
assert!(
|
||||
terminal_interactions.is_empty(),
|
||||
"completed empty polls should not emit terminal interactions: {terminal_interactions:?}"
|
||||
);
|
||||
|
||||
let open_event = &begin_events[0];
|
||||
|
||||
|
||||
@@ -76,12 +76,16 @@ impl ChatWidget {
|
||||
if !self.bottom_pane.is_task_running() {
|
||||
return;
|
||||
}
|
||||
self.flush_answer_stream_with_separator();
|
||||
let command_display = self
|
||||
.unified_exec_processes
|
||||
.iter()
|
||||
.find(|process| process.key == process_id)
|
||||
.map(|process| process.command_display.clone());
|
||||
if stdin.is_empty() && command_display.is_none() {
|
||||
return;
|
||||
}
|
||||
|
||||
self.flush_answer_stream_with_separator();
|
||||
if stdin.is_empty() {
|
||||
// Empty stdin means we are polling for background output.
|
||||
// Surface this in the status indicator (single "waiting" surface) instead of
|
||||
|
||||
@@ -719,6 +719,22 @@ async fn unified_exec_wait_status_header_updates_on_late_command_display() {
|
||||
assert_eq!(status.details(), Some("sleep 5"));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn unified_exec_empty_poll_for_finished_process_does_not_show_waiting_status() {
|
||||
let (mut chat, _rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await;
|
||||
chat.on_task_started();
|
||||
|
||||
terminal_interaction(&mut chat, "call-finished", "proc-finished", "");
|
||||
|
||||
assert_eq!(chat.status_state.current_status.header, "Working");
|
||||
let status = chat
|
||||
.bottom_pane
|
||||
.status_widget()
|
||||
.expect("task status indicator should remain visible");
|
||||
assert_eq!(status.header(), "Working");
|
||||
assert!(chat.unified_exec_wait_streak.is_none());
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn unified_exec_waiting_multiple_empty_snapshots() {
|
||||
let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await;
|
||||
|
||||
Reference in New Issue
Block a user