fix(agent): reopen fallback chain after primary transport recovery - #54054
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
After
_try_recover_primary_transport()succeeds, a follow-on HTTP 429 now correctly activates the configuredfallback_providersinstead of burning the retry budget on the rebuilt primary client.Salvaged from #33846 by @sweetcornna. Closes #32646.
Root cause:
try_activate_fallback()does_fallback_index += 1before the fallback client resolves. A failed or skipped resolution leaves_fallback_index == len(chain)with_fallback_activatedstillFalse, which permanently shuts the eager-fallback gate (_fallback_index < len(_fallback_chain)) for the rest of the turn. In the reported timeout → primary-recovery → 429 sequence, the gate was already shut by the time the 429 arrived, so failover never fired. The recently merged #53911 extended that gate to transport failures but did not reset the burned index — this fills exactly that gap.Changes
agent/conversation_loop.py: in theretry_count >= max_retriesrecovery branch, when primary transport recovery succeeds, also resethas_retried_429 = False,_fallback_index = 0,_fallback_activated = False(alongside the existingretry_count = 0). This is gated byprimary_recovery_attempted, so it runs at most once per turn.tests/run_agent/test_32646_fallback_429_after_timeout.py: full-loop regression + direct state-reset unit tests.Validation
test_run_conversation_fallbacks_on_429_after_timeout_recoveryruff checkInfographic