fix(codex): retain quota cooldown after account retry - #1353
Conversation
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAlternate Codex account retries defer reset-derived quota outcomes until the alternate request returns a successful HTTP response. Transport failures return before host-health reset. Auth tests cover cooldown persistence and multi-account, multi-model failover. ChangesCodex retry health
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant FirstAccount
participant AlternateAccount
participant QuotaHealth
FirstAccount->>AlternateAccount: Send account retry
AlternateAccount-->>FirstAccount: Return transport failure or HTTP response
AlternateAccount->>QuotaHealth: Reset host health after success
FirstAccount->>QuotaHealth: Commit deferred quota outcome after success
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Deterministic PR hygiene checks passed. |
✅ READY
Review readiness checklist
✅ 4/4 boxes ticked. This pull request has been marked Ready for Review. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/server/responses/core.ts`:
- Around line 507-512: Update the alternate-response retry flow around
retryHostKey so a successful circuit-enabled request retains its
UpstreamHostAdmissionLease and passes that matching lease to
resetUpstreamHostHealth instead of null. If no lease exists, perform the
required host-admission flow before resetting health; preserve the existing
no-lease reset behavior when the circuit threshold is disabled.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f252f095-9c4a-4a67-94d9-75c7025c9e51
📒 Files selected for processing (2)
src/server/responses/core.tstests/server-auth.test.ts
|
Please rebase this PR onto the latest |
96cdf4d to
ef3977c
Compare
|
✅ Action performedReview finished.
|
Rate Limit Exceeded
|
Rate Limit Exceeded
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/server-auth.test.ts`:
- Around line 2324-2345: Strengthen the assertions in the test using
getCodexUpstreamHealth so cooldownUntil is verified to be greater than the
current assertion time, while preserving the existing numeric and retry-after
source checks. Capture the current time immediately before asserting the health
entry to ensure the stored cooldown is still active.
🪄 Autofix
❌ Autofix failed (check again to retry)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 00e78aed-bf38-4d08-a315-acb8d977231f
📒 Files selected for processing (1)
tests/server-auth.test.ts
|
An unexpected error occurred while generating fixes: Not Found - https://docs.github.com/rest/git/refs#get-a-reference |
ef3977c to
a274792
Compare
Rate Limit Exceeded
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server/responses/core.ts (1)
1729-1741: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDelete the same credential-bearing headers that the openai-responses adapter forwards.
openai-responses.tsforwards allFORWARD_HEADERSinto the outgoing request atsrc/adapters/openai-responses.ts:1183-1185; this strip only removesauthorizationandchatgpt-account-idinsrc/server/responses/core.ts:1735-1736, leaving headers likeopenai-beta,originator,session_id,session-id, and thex-codex-*headers on the third-party route. Delete the same headers here, or derive the strip set from the adapter’s forwarded list.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/responses/core.ts` around lines 1729 - 1741, The stripClaudeMainAuth branch in the response handling flow must remove every credential-bearing header that the openai-responses adapter forwards, not only authorization and chatgpt-account-id. Update the selectedForwardHeaders cleanup to delete or derive the complete FORWARD_HEADERS set, including openai-beta, originator, session_id, session-id, and x-codex-* headers, while preserving the existing route and provider sanitization.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/server-auth.test.ts`:
- Around line 2420-2427: Update the test around the request in the
preserved-first-account scenario to assert the cooldown state directly using the
imported getCodexQuotaHealthSnapshot helper. Verify the snapshot for pool-a
retains the expected cooldown/reset-derived state, while preserving the existing
response and dispatch assertions and cleanup.
---
Outside diff comments:
In `@src/server/responses/core.ts`:
- Around line 1729-1741: The stripClaudeMainAuth branch in the response handling
flow must remove every credential-bearing header that the openai-responses
adapter forwards, not only authorization and chatgpt-account-id. Update the
selectedForwardHeaders cleanup to delete or derive the complete FORWARD_HEADERS
set, including openai-beta, originator, session_id, session-id, and x-codex-*
headers, while preserving the existing route and provider sanitization.
🪄 Autofix
❌ Autofix failed (check again to retry)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: dda2b2d8-9831-45a8-92a7-3858ee303d88
📒 Files selected for processing (2)
src/server/responses/core.tstests/server-auth.test.ts
|
An unexpected error occurred while generating fixes: Not Found - https://docs.github.com/rest/git/refs#get-a-reference |
a274792 to
d9119fc
Compare
|
|
|
Thanks @luvs01! Really appreciate the focused fix, the regression coverage, and the careful follow-up on the review feedback. Merged. 🙏 |
2 participants
Summary
Retry-Aftercooldowns, which are still recorded before the alternate send.The combo deferral was designed to let a later model target reuse the same account, but it also suppressed the first account's quota outcome after another account had successfully served the request. That could leave an exhausted account active or thread-affined and send the next request back to it.
Verification
bun run typecheck— passed.bun run privacy:scan— passed.git diff --check— passed.tests/server-auth.test.tspass before the final focused refinement reported 71 passed and one transient existing timeout; that timeout passed alone on both this branch and cleandev.Checklist
Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
All CI tests are green on my local testing.
I pushed my PR to the latest dev commit.
I resolved all correct Codex and CodeRabbit findings.
My PR is ready for review.
Summary by CodeRabbit
Bug Fixes
Tests