fix(codex): stalled HTTP 429 retries end with an explicit failure instead of hanging#23842

🟡 Medium Adapters/CodexAdapterV2.ts:4891

A descendant started while the root interrupt is in flight is finalized with a completedAt earlier than its startedAt, so provider-turn, node, and task updates report an impossible completion time. now was captured before the interrupt and is reused here; capture a fresh completion timestamp after interrupting the descendants, as the abandonment path does.

Fixed in a77e1c8f9. Stopped descendants now settle with the time their own interrupt finished, matching the bounded-wait path, so a nested turn started while the root interrupt was in flight no longer gets a completedAt earlier than its startedAt. Added a regression test that fails before the change and passes after.

Sorry, I'm unable to act on this request because you do not have permissions within this repository.

🟠 High Adapters/CodexAdapterV2.ts:6510

When an exhausted root’s turn/completed event arrives during the unconfirmed-stop wait, this path finalizes the root and cancels rateLimitContainmentTimer without stopping descendants that started after the initial deadline scan. Their native work keeps running, and emitOrDeferRootTerminal reports childTurnsContinue: true; run the descendant containment pass before finalizing the root and canceling the timer.

Fixed in ac46a1e62. turn/completed for an unconfirmed root now stops the active lineage first, rescanning under the terminalization permit like the bounded wait does, so a nested turn started during the wait is either stopped before the root fails or starts after the root is terminal. Added a regression test that fails before the change and passes after.

Sorry, I'm unable to act on this request because you do not have permissions within this repository.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Read the time unit that follows a retry after N value.

The retry[-_ ]?after regex captures only the number and always multiplies it by 1,000. A prose message such as "Please retry after 2 minutes" therefore gives 2,000 ms, not 120,000 ms. The phrase fallback at Line 125-132 never runs, because the seconds match returns first.

The consequence: decideCodexRateLimitRetry gets a delay that is far too short. endStalledRateLimitedTurn then interrupts and fails the turn after 62 s, even though the provider asked for 2 minutes. A turn that would have recovered ends as rateLimitRetryExhausted, and resetAt is wrong.

To fix this, capture an optional unit on the same line and use UNIT_MS to scale the value. Seconds stay the default, so a bare header value still parses the same way.

Add a test case such as parseCodexRetryAfterMs("Please retry after 2 minutes") that expects 120_000.

✅ Addressed in commits ac46a1e to 8608489

Fixed in bf6a2fa2e. The retry after N match now takes an optional unit, as try again in already did, so "retry after 2 minutes" and "retry after 500ms" are read correctly; a retry-after-ms header is read as milliseconds, and a bare value stays delay-seconds. Added test cases that fail before the change and pass after.

@Hylouis233 Thanks for fixing this in bf6a2fa2e and adding regression tests. The changes you described address the unit-parsing issue and preserve seconds as the default for bare values.

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

✅ Review thread resolved.

_{You are interacting with an AI system.}

_{P2 Badge} Enforce the timeout across descendant teardown

When the root interrupt is unacknowledged and any descendant is still active at the 60-second deadline, abandonUncontainedRateLimitedTurn calls this loop after its sleep, and each interruptRateLimitedDescendant can spend another 10 seconds in interruptRateLimitedNativeTurn; the loop does so serially and rescans for newly spawned descendants. Thus one silent child makes the advertised 60-second bound at least 70 seconds and enough children can keep the root running arbitrarily longer, reproducing the hang for subagent-heavy turns. Use the same absolute deadline for lineage cleanup, or settle at the bound and make child interrupts concurrent or best-effort.

Useful? React with 👍 / 👎.

Addressed in c8156276a for the 60s unconfirmed bound. At that bound, interrupts for the nested turns are sent without waiting, and the lineage and root settle under one permit right at the bound, so one silent subagent no longer stretches this cap. A new adapter test fails before the change and passes after. Known limits: this is deliberately conservative, so a nested turn that confirms its interrupt only after the bound is still published as broken. The two other cleanup paths (the initial deadline path and the path after the root turn ends) still wait on descendants serially; they are outside this 60s bound and are unchanged in this commit.

_{P1 Badge} Evict broken root threads from the live-session cache

When the 429 interrupt is unacknowledged and the same provider session stays live, returning only threadDisposition: "broken" updates the persisted projection but does not invalidate ProviderSessionManager's loaded-thread cache. Its event pump deletes loadedProviderThreadKeyByThread only for a provider_thread.updated event whose status is error (ProviderSessionManager.ts:1973-1988), and the resume wrapper skips the native thread/resume RPC on a cache hit (ProviderSessionManager.ts:1789-1792). Consequently an immediate follow-up can issue turn/start against the still-retrying native thread, defeating the containment this branch promises; emit the root thread error/update (as the subagent path does) or teach the manager to evict on a broken terminal.

Useful? React with 👍 / 👎.

Addressed in 386516b80. An uncontained 429 root now publishes its provider thread as error before the broken terminal, so the session drops the loaded thread and the next turn resumes it instead of reusing it. A new adapter test fails before the change and passes after. Scope note: this is verified at the adapter layer and with the existing session-manager tests only; I have not run an end-to-end check.

_{P1 Badge} Wait for the authoritative interrupt completion

When a normal turn/interrupt response arrives before its later turn/completed notification, rootInterrupted is true and this branch finalizes the turn as reusable immediately. The probe-derived contract says the request returns first and only turn/completed is authoritative (docs/orchestration-v2/README.md:87-88); the ordinary Stop path accordingly waits on turnWaiters (CodexAdapterV2.ts:7684-7692). A queued follow-up can therefore issue turn/start during the gap while the native retry turn is still active, and descendants are finalized on the same premature criterion; wait boundedly for the completion event, then mark the thread broken if it never arrives.

Useful? React with 👍 / 👎.

Addressed in f0478d229. After the 429 deadline interrupts the root or a descendant, a turn/interrupt reply no longer counts as confirmation; the adapter waits up to 10s for that turn's turn/completed.

• A root that does not end in time moves to the existing unconfirmed bound, then fails with the thread disposition broken.
• A descendant that does not end in time is abandoned, and its thread is published as error.

New regression tests, both failing on the previous head and passing now:

• "waits for turn/completed after a 429 interrupt reply before ending the turn"
• "marks a descendant errored when Codex only replies to its 429 interrupt"

A third test, "ends a 429 turn once turn/completed follows its interrupt reply", covers the confirmed path (passes before and after). The existing 429 lineage test transcripts now include the turn/completed events Codex emits after an interrupt, since a bare reply no longer settles them.

Not verified:

• The real ordering of the interrupt reply and turn/completed, especially for subagent threads, has only been exercised in replay transcripts, not against a live Codex.
• The 10s confirmation budget is a design choice, not something from the Codex docs.
• The user-initiated Stop path is unchanged and still treats the reply as confirmation.
• A nested turn that starts while the root is waiting is interrupted on the rescan after the root confirms (or after 10s), not immediately.

_{P2 Badge} Preserve live thread fields in the error update

If thread/goal/updated changes a root thread after the turn was registered, rootProviderThreads and the persisted projection contain the new goal while context.providerThread is still the turn-start snapshot. An uncontained 429 then spreads that stale snapshot here; provider-thread.updated replaces the whole object (ProjectionStore.ts:871-883), and RunExecutionService.ts:1411-1414 adopts it as the latest thread for terminalization, so goal status/token progress is silently rewound while marking the thread errored. Build this update from the current rootProviderThreads entry rather than the context snapshot.

Useful? React with 👍 / 👎.

Addressed in 837b02b50. The errored provider_thread.updated is now built from the live root thread rather than the turn-start snapshot, so the latest goal is kept. Regression test: "keeps the latest goal when an unconfirmed 429 marks the thread errored" (fails without the fix, passes with it).

Not verified: behavior against a real Codex app-server (coverage is replay transcripts under TestClock) and CI. Whether Codex emits goal updates after a deadline interrupt in practice, and end-to-end projection/UI handling, are also unverified.

_{P2 Badge} Start the containment timer before waiting on descendants

When a root 429 interrupt is unconfirmed and one or more descendants never emit turn/completed, interruptRateLimitedDescendant can consume its full 10-second timeout and this loop awaits those descendants serially. The 60-second abandonment fiber is not created until line 4975, so even one silent child extends the advertised unconfirmed bound and N children add N×10 seconds; descendants spawned during the rescans can extend it further. Although the later abandonment loop now sends interrupts best-effort, this earlier pre-timer loop is fresh evidence that teardown is still unbounded; enter containment before scanning and share one absolute deadline or interrupt children concurrently.

Useful? React with 👍 / 👎.

Addressed in fc0ffbc26. Descendants are now interrupted concurrently under one shared interrupt deadline instead of being awaited one by one. Regression test: "starts the unconfirmed 429 bound one interrupt bound after silent descendants" (fails without the fix, passes with it).

Not verified: behavior against a real Codex app-server (coverage is replay transcripts under TestClock) and CI. Real wall-clock timing of the shared deadline is only asserted with TestClock.

_{P1 Badge} Terminate background terminals before failing the retry turn

When the 429 occurs while a commandExecution with a processId is still running in the background, this path only sends turn/interrupt and then finalizes the turn as failed. Failed finalization marks the command item failed and drops its runningCommandItemsByTurn tracking, but Codex background terminals outlive a turn—the ordinary Stop path separately invokes thread/backgroundTerminals/terminate at lines 7705-7759—so the process can keep modifying files after the UI reports failure and the adapter can no longer observe or stop it. Terminate the tracked terminals, or retain their context, before publishing this terminal.

Useful? React with 👍 / 👎.

Not changing this in this PR. Background terminal cleanup is separate from the 429 retry bound: an ordinary failed turn/completed does not terminate background terminals either, and Stop is still the path that terminates them. A change here would reach beyond the retry adapter and helper, so it seems better handled separately.

_{P1 Badge} Treat in-flight exhausted turns as uncontained

When Stop is already waiting for turn/completed as the retry deadline fires, rateLimitRetryExhausted is assigned at line 4883 but rateLimitContainment remains undefined until the automatic interrupt wait finishes at line 4925. If Stop's own 10-second wait expires first, it finalizes the context while this predicate returns false, so the terminal is reusable and no provider_thread.updated error evicts the loaded-session cache (ProviderSessionManager.ts:1973-1988), even though neither interrupt produced the authoritative completion required by docs/orchestration-v2/README.md:87-88; an immediate follow-up can therefore reuse the still-retrying native thread. The fresh current-code evidence is this pre-unconfirmed Stop window, so an exhausted native turn should be considered broken unless it has reached ended.

Useful? React with 👍 / 👎.

Addressed in 0a3f6703e. An exhausted 429 turn with a native thread now counts as uncontained until turn/completed confirms it ended, so if Stop's wait expires first the thread stays broken. Regression test: "keeps a 429 thread broken when Stop ends it before the deadline's stop is confirmed" (fails without the fix, passes with it).

Not verified: behavior against a real Codex app-server (coverage is replay transcripts under TestClock) and CI.