Attempt OAuth token refresh on 429 with retry_after=0 (SMA-513)#321
Attempt OAuth token refresh on 429 with retry_after=0 (SMA-513)#321botfarm-coder merged 2 commits intomainfrom
Conversation
429 responses with Retry-After: 0 or absent header often indicate an expired OAuth token rather than genuine rate limiting. Add token refresh logic to the 429 handler: if refresh produces a new fingerprint, retry the API call; otherwise fall through to normal backoff.
| self._attempt_records = [] | ||
| try: | ||
| data = self._fetch(refreshed_token) | ||
| except Exception: |
There was a problem hiding this comment.
CODEX: This retry path catches every failure from the refreshed request and converts it into _handle_429(). If the refreshed token comes back with 401 (or any other non-429 HTTP error), we silently classify an auth failure as rate limiting, skip _handle_401(), and never trigger the existing auth-backoff/notification path. Please preserve the status-specific handling here instead of collapsing all retry failures into 429 backoff.
There was a problem hiding this comment.
Fixed — split the broad except Exception into except httpx.HTTPStatusError (routes 401s to _handle_401(), other HTTP errors to _handle_429()) and a separate except Exception for non-HTTP failures. Added a test for 401-after-refresh routing.
| # --------------------------------------------------------------------------- | ||
|
|
||
|
|
||
| class Test429TokenRefresh: |
There was a problem hiding this comment.
Nit: there's an existing _make_429_error(retry_after=...) helper (line ~1239) that wraps both the response and error construction. All other 429 tests in this file use it. These tests manually create the response + error — switch to _make_429_error for consistency.
| class Test429TokenRefresh: | |
| error = _make_429_error(retry_after="0") |
There was a problem hiding this comment.
Fixed — switched all tests to use _make_429_error().
botfarm-reviewer
left a comment
There was a problem hiding this comment.
Review iteration 1
Claude: changes_requested
Codex: changes_requested
Overall: changes_requested
- Replace broad `except Exception` in 429 refresh retry path with separate `httpx.HTTPStatusError` handler that routes 401s to `_handle_401()` instead of collapsing all failures into 429 backoff - Switch Test429TokenRefresh tests to use `_make_429_error()` helper for consistency with other 429 tests - Add test for 401-after-refresh routing to auth handler
botfarm-reviewer
left a comment
There was a problem hiding this comment.
Clean implementation with comprehensive test coverage.
What I checked:
- Logic flow: 429 with
retry_after=0/absent → refresh → fingerprint check → retry is correct and follows the established pattern from the 401 handler - All error paths (refresh failure, same fingerprint, retry failure, retry-401, retry-other-error) are handled and fall through appropriately
- Success path correctly resets both rate-limit and auth-failure state before parsing
parse_retry_after("0")returns0(int), soretry_after == 0matches as expected; past HTTP-dates return0.0(float) which also matches via==- Test coverage is thorough: 7 test cases covering success, fallthrough, and edge-case scenarios
- Existing test fixtures correctly updated with
refresh_token.return_value = Noneto prevent false-positive refresh attempts
Minor observations (not blocking):
- The inner
except httpx.HTTPStatusErrorcatches all non-401 HTTP errors on retry (e.g. 500, 503) and routes them through_handle_429— technically this increments the 429 counter for non-429 errors, but the effect is just conservative backoff, which is harmless - The fallback
_handle_429uses the originalretry_after_header(0 or None), not the retry response's header — acceptable since the original header is what triggered the refresh path
Summary
Retry-After: 0(or absent), attempt an OAuth token refresh before falling back to backoff — this pattern indicates an expired token, not genuine rate limitingTest plan
retry_after=0→ refresh → new token → retry succeedsretry_after=0→ refresh → same fingerprint → falls through to backoffretry_after=0→ refresh returns None → falls through to backoffretry_after>0→ no refresh attempted → normal backoff