diff --git a/codex-rs/tui/docs.md b/codex-rs/tui/docs.md index 14ac7cb31..95b793524 100644 --- a/codex-rs/tui/docs.md +++ b/codex-rs/tui/docs.md @@ -149,9 +149,10 @@ When a turn is interrupted (ESC), the ACP backend emits `TurnLifecycle::Aborted` | Action | Method | Effect | |--------|--------|--------| | Interrupt received | `on_interrupted_turn()` | Increments `pending_stale_completes` | +| New turn starts | `on_task_started()` | Resets `pending_stale_completes` to 0 (drains orphaned counters from backend-suppressed Completeds) | | Stale Completed arrives | `on_task_complete()` | If counter > 0, decrements and returns early (skips turn finalization) | -This is complementary to the ACP backend's `turn_interrupted` flag (`@/codex-rs/acp/docs.md`), which suppresses the stale `Completed` at the source. The TUI counter provides a safety net in case the backend guard is bypassed. +This is complementary to the ACP backend's `turn_interrupted` flag (`@/codex-rs/acp/docs.md`), which suppresses the stale `Completed` at the source. In the common case the backend suppresses the stale event and the counter is never drained; the `on_task_started` reset ensures those orphaned counters don't consume the next turn's real Completed. The counter still provides defense-in-depth for the rare race where a stale Completed slips past the backend guard. **Turn-Boundary Cleanup of Incomplete Tool Cells** (`chatwidget/event_handlers.rs`): diff --git a/codex-rs/tui/src/chatwidget/event_handlers.rs b/codex-rs/tui/src/chatwidget/event_handlers.rs index eba35eb9e..f2b908319 100644 --- a/codex-rs/tui/src/chatwidget/event_handlers.rs +++ b/codex-rs/tui/src/chatwidget/event_handlers.rs @@ -182,6 +182,12 @@ impl ChatWidget { // Raw reasoning uses the same flow as summarized reasoning pub(super) fn on_task_started(&mut self) { + // When the ACP backend suppresses a stale Completed (via the + // turn_interrupted flag), the pending_stale_completes counter is + // never drained. Reset it here so leftover counters from previous + // interrupts don't consume this turn's real Completed. + self.pending_stale_completes = 0; + self.bottom_pane.clear_ctrl_c_quit_hint(); self.bottom_pane.set_task_running(true); self.retry_status_header = None; @@ -437,9 +443,10 @@ impl ChatWidget { /// When there are queued user messages, restore them into the composer /// separated by newlines rather than auto‑submitting the next one. pub(super) fn on_interrupted_turn(&mut self, _reason: TurnAbortReason) { - // The cancelled background task will eventually emit - // TurnLifecycle::Completed; record that we expect one stale - // Completed so on_task_complete can ignore it. + // The ACP backend usually suppresses the stale Completed via + // turn_interrupted, but if it races through, on_task_complete + // can use this counter to ignore it. The counter is reset by + // the next on_task_started as a safety net. self.pending_stale_completes += 1; // Finalize, log a gentle prompt, and clear running state. diff --git a/codex-rs/tui/src/chatwidget/mod.rs b/codex-rs/tui/src/chatwidget/mod.rs index 9fc9c2d94..e2a25c946 100644 --- a/codex-rs/tui/src/chatwidget/mod.rs +++ b/codex-rs/tui/src/chatwidget/mod.rs @@ -424,9 +424,10 @@ pub(crate) struct ChatWidget { // Gate: set when AgentMessage is received, cleared on next TaskStarted. // While true, late-arriving tool events are silently discarded. turn_finished: bool, - // Number of stale TurnLifecycle::Completed events expected after - // interrupts. Each on_interrupted_turn increments this; each - // on_task_complete decrements and skips processing while > 0. + // Defense-in-depth counter for stale TurnLifecycle::Completed events + // after interrupts. Incremented by on_interrupted_turn, decremented by + // on_task_complete, and reset to 0 by on_task_started (to drain orphaned + // counters when the ACP backend suppresses the stale Completed). pending_stale_completes: i32, /// Whether and how plan updates are rendered in a pinned drawer instead of /// history cells. diff --git a/codex-rs/tui/src/chatwidget/tests/part7.rs b/codex-rs/tui/src/chatwidget/tests/part7.rs index 5748becf4..b9498619d 100644 --- a/codex-rs/tui/src/chatwidget/tests/part7.rs +++ b/codex-rs/tui/src/chatwidget/tests/part7.rs @@ -1,19 +1,19 @@ use super::*; -/// When the stale Completed arrives during an active turn, tool events for the -/// active turn should NOT be discarded. +/// When the ACP backend suppresses the stale Completed (common case), the +/// next turn's real Completed must not be consumed as stale. /// -/// Race condition sequence: +/// Sequence: /// 1. Started(A) → task running -/// 2. Aborted(A) → task stopped (user pressed ESC) -/// 3. Started(B) → new task running (user submitted new message) -/// 4. Completed(A) → stale event from cancelled background task +/// 2. Aborted(A) → task stopped (user pressed ESC), counter = 1 +/// 3. Started(B) → counter reset to 0 +/// 4. Completed(B) → should finalize turn B normally /// -/// After step 4, turn B's tool events must still be processed. Before the fix, -/// the stale Completed prematurely gated tool events, causing them to be -/// silently discarded. +/// Before the fix, the counter from step 2 was never drained (because the +/// ACP backend suppressed the stale Completed), so the real Completed in +/// step 4 was consumed as stale, leaving the spinner running forever. #[test] -fn stale_completed_should_not_block_tool_events_for_next_turn() { +fn acp_suppressed_stale_should_not_block_next_turn_completion() { let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(); // Start and interrupt turn A @@ -22,27 +22,31 @@ fn stale_completed_should_not_block_tool_events_for_next_turn() { chat.on_interrupted_turn(TurnAbortReason::Interrupted); drain_insert_history(&mut rx); + // ACP backend suppresses the stale Completed → no on_task_complete call. + // Start turn B chat.on_task_started(); drain_insert_history(&mut rx); - // Stale Completed from turn A + // Real Completed from turn B should finalize the turn. chat.on_task_complete(None); drain_insert_history(&mut rx); - // Tool event for turn B should NOT be discarded - begin_exec(&mut chat, "turn-b-call", "echo hello from turn B"); - + // Task should be stopped. assert!( - chat.active_cell.is_some(), - "ExecCell should be created - stale Completed should not block turn B's tool events" + !chat.bottom_pane.is_task_running(), + "Task should be stopped after real Completed" + ); + assert!( + chat.turn_finished, + "Turn should be marked finished after real Completed" ); } -/// Multiple consecutive interrupts should each produce one stale Completed -/// that is correctly drained before the real turn's events arrive. +/// Multiple consecutive interrupts where ACP suppresses all stale Completeds. +/// The final real turn's Completed must still finalize normally. #[test] -fn multiple_interrupts_drain_stale_completes_in_order() { +fn multiple_interrupts_with_acp_suppression_should_not_hang() { let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(); // Interrupt twice in a row @@ -56,18 +60,25 @@ fn multiple_interrupts_drain_stale_completes_in_order() { chat.on_interrupted_turn(TurnAbortReason::Interrupted); drain_insert_history(&mut rx); + // ACP backend suppresses both stale Completeds. + // Start the real turn chat.on_task_started(); drain_insert_history(&mut rx); - // Two stale Completeds arrive - chat.on_task_complete(None); - chat.on_task_complete(None); - - // Real tool events should still work + // Tool events for the real turn should work begin_exec(&mut chat, "real-call", "echo real"); assert!( chat.active_cell.is_some(), - "ExecCell should be created after draining multiple stale Completeds" + "ExecCell should be created - counter was reset by on_task_started" + ); + + // Real Completed should finalize the turn + chat.on_task_complete(None); + drain_insert_history(&mut rx); + + assert!( + !chat.bottom_pane.is_task_running(), + "Task should be stopped after real Completed following multiple interrupts" ); }