fix(codex): throttle repeated failed pool quota primes (rebase of #3003) - #3185
Conversation
A pool account whose WHAM lookup fails stores no quota, so it stays "unknown" and every later prime trigger re-selects it as stale and repeats the same failing request. Successful lookups are already bounded by POOL_CACHE_TTL; failures had no backoff at all. Record the last prime attempt per account and give a failed lookup the same TTL window. The record is keyed by credential generation, so a re-authentication, refresh, or account removal retries immediately instead of waiting out a backoff earned by the previous credential.
…y return The prune sat after the provider-eligibility early return, so an account removed while the provider was disabled kept its failed-attempt marker. Restoring the same id inside POOL_CACHE_TTL then read that stale failure as current and skipped the retry the restored credential was entitled to. The existing coverage removed an account with the provider enabled, which is why this survived review. Add the disabled-window case.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
✅ Deterministic PR hygiene checks passed. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 80101192ba
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const q = getAccountQuota(a.id); | ||
| return !q || Date.now() - q.updatedAt >= POOL_CACHE_TTL; | ||
| if (q) return Date.now() - q.updatedAt >= POOL_CACHE_TTL; |
There was a problem hiding this comment.
In src/codex/auth-api.ts, when an account already has a quota older than POOL_CACHE_TTL, this branch selects it solely from q.updatedAt and never checks poolQuotaPrimeAttemptedAt. If its refresh then receives a 503, transport error, or unusable response, the old quota remains stale, so every subsequent prime trigger dispatches another WHAM request despite the newly recorded attempt. Apply the matching-generation attempt timestamp to stale cached accounts as well, rather than consulting it only when q is null.
Useful? React with 👍 / 👎.
2 participants
Summary
Maintainer rebase of #3003 by @luvs01 onto current
dev— all three commits cherry-picked with author credit preserved, no conflicts — plus the outstanding review fix applied.Throttles repeated failed pool quota primes so an unreachable or rejecting account cannot turn every prime trigger into another upstream request. A failed attempt is retried once per
POOL_CACHE_TTLwindow, and a newer credential invalidates the previous failure so a re-authenticated account retries immediately.The review finding, fixed rather than deferred
CodeRabbit left one unresolved thread on
src/codex/auth-api.ts, and it was correct:The branch already pruned stale markers, but the prune sat after the provider-eligibility early return, so a removal during a disabled window never reached it. Moved ahead of the return.
The existing test covered removal with the provider enabled, which is why this survived review. Added the case the reviewer actually described:
removal purges the backoff even while the provider is disabled— fail a prime to record the marker, remove the account while the provider is disabled, then restore both inside the TTL window and assert the prime dispatches instead of reading the stale failure as current.Verification
Exact head:
bun test ./tests/codex-quota-prime.test.ts— 22 pass, 0 fail, 81 expect() calls.Full-suite and typecheck coverage is left to CI on this exact head.
Checklist
dev