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
3 changes: 2 additions & 1 deletion codex-rs/tui/docs.md
Original file line number Diff line number Diff line change
Expand Up @@ -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`):

Expand Down
13 changes: 10 additions & 3 deletions codex-rs/tui/src/chatwidget/event_handlers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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.
Expand Down
7 changes: 4 additions & 3 deletions codex-rs/tui/src/chatwidget/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
61 changes: 36 additions & 25 deletions codex-rs/tui/src/chatwidget/tests/part7.rs
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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
Expand All @@ -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"
);
}
Loading