From e43a2e297fe72285890e0f04f9f1aece843c2338 Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Tue, 19 May 2026 20:48:37 -0700 Subject: [PATCH] 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. --- .../handlers/unified_exec/write_stdin.rs | 23 ++++++++++++------- .../core/src/unified_exec/process_manager.rs | 4 ++-- codex-rs/core/tests/suite/unified_exec.rs | 8 +++++++ .../tui/src/chatwidget/command_lifecycle.rs | 6 ++++- .../tui/src/chatwidget/tests/exec_flow.rs | 16 +++++++++++++ 5 files changed, 46 insertions(+), 11 deletions(-) diff --git a/codex-rs/core/src/tools/handlers/unified_exec/write_stdin.rs b/codex-rs/core/src/tools/handlers/unified_exec/write_stdin.rs index 3dea7092f..843f9e638 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec/write_stdin.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec/write_stdin.rs @@ -78,14 +78,21 @@ impl ToolExecutor 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)) } diff --git a/codex-rs/core/src/unified_exec/process_manager.rs b/codex-rs/core/src/unified_exec/process_manager.rs index 8c0cc8968..c6fc2ab3e 100644 --- a/codex-rs/core/src/unified_exec/process_manager.rs +++ b/codex-rs/core/src/unified_exec/process_manager.rs @@ -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 { diff --git a/codex-rs/core/tests/suite/unified_exec.rs b/codex-rs/core/tests/suite/unified_exec.rs index d61e42738..9eb831b08 100644 --- a/codex-rs/core/tests/suite/unified_exec.rs +++ b/codex-rs/core/tests/suite/unified_exec.rs @@ -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]; diff --git a/codex-rs/tui/src/chatwidget/command_lifecycle.rs b/codex-rs/tui/src/chatwidget/command_lifecycle.rs index efe752296..6f13d3798 100644 --- a/codex-rs/tui/src/chatwidget/command_lifecycle.rs +++ b/codex-rs/tui/src/chatwidget/command_lifecycle.rs @@ -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 diff --git a/codex-rs/tui/src/chatwidget/tests/exec_flow.rs b/codex-rs/tui/src/chatwidget/tests/exec_flow.rs index d54783615..ffc883377 100644 --- a/codex-rs/tui/src/chatwidget/tests/exec_flow.rs +++ b/codex-rs/tui/src/chatwidget/tests/exec_flow.rs @@ -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;