Skip to content

fix(codex): accept thread-matching stale terminal events during same-thread recovery - #1189

Open
tiann wants to merge 1 commit into
mainfrom
fix/1070-codex-stale-terminal-thread-match
Open

fix(codex): accept thread-matching stale terminal events during same-thread recovery#1189
tiann wants to merge 1 commit into
mainfrom
fix/1070-codex-stale-terminal-thread-match

Conversation

@tiann

@tiann tiann commented Jul 27, 2026

Copy link
Copy Markdown
Owner

Fixes #1070

Root cause

After a same-thread retry/compact recovery opens a new Codex turn, a terminal event carrying the previous turn_id never finalizes the new turn: shouldIgnoreTerminalEvent() unconditionally ignores stale-turn events, and the launcher only passed allowMatchingThreadIdTerminalEvent for terminal_source === 'thread_status'. The session then stays non-ready (thinking stuck, no ready).

Fix

Following the fork patch referenced in the issue (mouriya-s-lab/hapi@6c675985):

  • cli/src/codex/utils/terminalEventGuard.ts: a stale-turn terminal event is accepted when the event thread matches the current thread and the caller explicitly allows it (allowMatchingThreadIdTerminalEvent). Events for a different thread are still ignored.
  • cli/src/codex/codexRemoteLauncher.ts: the allow condition is extended to terminal_source === 'thread_status' || sameThreadRetryAttempt > 0 || sameThreadCompactAttempt > 0, i.e. thread-matching stale terminal events are accepted while a same-thread recovery is in flight so the turn finalizes and ready is emitted.

Deviation from the fork patch (code drift since the issue was filed): on current main the stale event is dropped even earlier by the lastFinalizedTurnId duplicate-terminal check, before it ever reaches the guard. The same recovery exemption was therefore added to that check as well; without it the new regression tests time out exactly as the production session does.

Note on issue wording: the issue text frames stale terminal events as something that must stay scoped to the active turn, but the fork patch's actual semantics are the opposite for the recovery window — accept thread-matching stale terminal events so the session recovers ready. This PR follows the fork semantics.

Tests

  • New launcher regression tests in cli/src/codex/codexRemoteLauncher.test.ts: same-thread retry and same-thread compact recovery completing with a stale turn_id both emit ready and clear thinking.
  • New unit tests in cli/src/codex/utils/terminalEventGuard.test.ts for the thread-match exemption (accepted on matching thread, still ignored on different thread).
  • bun typecheck — passes.
  • bunx vitest run src/codex/codexRemoteLauncher.test.ts src/codex/utils/terminalEventGuard.test.ts — 79/79 pass.
  • bun run test (cli, full suite) — 154 files / 1482 tests pass.

Known trade-offs / risks

  • During the recovery window (retry/compact attempt counters > 0), a genuinely duplicate late terminal event from the old turn on the same thread is now accepted and finalizes the in-flight recovery turn. This is the fork's intended trade-off: the app-server only emits old-turn_id terminal events on this recovery path.
  • Unrelated pre-existing flake: src/modules/common/cursorModels.test.ts ("does not spawn agent --list-models while ACP transport is active") failed in 2 of 4 full-suite runs on this branch and passes standalone on both this branch and a clean origin/main baseline; it touches no Codex code and appears load/ordering sensitive.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Matching-thread fallback can accept an older terminal event as the active retry completion. shouldIgnoreTerminalEvent now returns false for any mismatched eventTurnId when the thread ids match, and codexRemoteLauncher enables that path while sameThreadRetryAttempt or sameThreadCompactAttempt is nonzero. If a duplicate/late terminal for turn-1 arrives after retry turn-2 has started, the terminal block clears currentTurnId/turnInFlight and schedules ready even though the retry turn is still running. Evidence: cli/src/codex/utils/terminalEventGuard.ts:24, cli/src/codex/codexRemoteLauncher.ts:2644.
    Suggested fix:
    if (input.eventTurnId) {
        if (!input.currentTurnId || input.eventTurnId === input.currentTurnId) {
            return false;
        }
        return true;
    }
    Then handle the known same-thread recovery terminal explicitly in codexRemoteLauncher, scoped to the stale lastFinalizedTurnId case, instead of letting the shared guard accept every turn-id mismatch for that thread.

Summary

  • Review mode: initial
  • Found one stale-terminal handling regression risk in the latest diff.

Testing

  • Not run (automation)

HAPI Bot

if (!input.currentTurnId || input.eventTurnId === input.currentTurnId) {
return false;
}
return !(allowMatchingThreadIdTerminalEvent && hasMatchingThreadId);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Major] This now accepts any mismatched eventTurnId when allowMatchingThreadIdTerminalEvent is true and the thread ids match. In the launcher that flag is enabled during same-thread retry/compact recovery, so a duplicate/late terminal for an older turn can pass while a new retry turn is active; the terminal handler then clears currentTurnId/turnInFlight and may emit ready before the retry actually finishes.

Suggested fix:

if (input.eventTurnId) {
    if (!input.currentTurnId || input.eventTurnId === input.currentTurnId) {
        return false;
    }
    return true;
}

Handle the intended stale lastFinalizedTurnId recovery case explicitly in codexRemoteLauncher, so the shared guard does not accept arbitrary turn-id mismatches for the current thread.

swear01 added a commit to swear01/hapi that referenced this pull request Jul 28, 2026
fix(codex): ignore stale terminal events after same-thread recovery
swear01 added a commit to swear01/hapi that referenced this pull request Jul 29, 2026
fix(codex): ignore stale terminal events after same-thread recovery
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Stale Codex terminal events can leave sessions non-ready

1 participant