fix(providers): harden OpenRouter quota reset cooldowns - #4980
Conversation
…ared A free-tier quota exhaustion is dated by the upstream, but OpenRouter puts that date in the 429 BODY and sends no Retry-After. parseRetryAfterMs only reads the header, so the key was parked for DEFAULT_COOLDOWN_MS, came back, took another 429, and repeated for the rest of the quota window. parseQuotaResetAt reads `... will reset at <date>` out of a bounded 4KB prefix; readQuotaResetAt pulls it off a cloned response so the caller can still cancel the original to release the socket. When present it outranks both the header and the default, because it is the only one of the three that knows when the quota actually returns. MAX_QUOTA_COOLDOWN_MS is 32 days, separate from the 10-minute cap on undated 429s. That cap stays short on purpose: an undated 429 is a guess. A dated one is not. 32 rather than 8 days because the wording parsed is "Weekly/Monthly Limit Exhausted" — an 8-day cap silently clamped every monthly reset ~23 days early, back into the loop this removes. Caught by the cap's own test. Anything unreadable returns undefined and leaves today's behaviour exactly as it was: no body, consumed body, no match, unparsable date, a date already past, or one past the cap. Closes #4024
Review follow-up from @lidge-jun on #4733: readQuotaResetAt called .text() on a clone and sliced 4KB afterwards, so the parse was bounded and the read was not. The PR claimed a bounded prefix; that was wrong. Fixing it surfaced something worse than the unbounded read. clone() tees the body, and the caller leaves the original branch undrained while this runs — so the tee stalls once its buffer fills. A 5MB error body hangs the rotation path outright. Reproduced: the first bounded version still timed out at 5s against a finite 5MB stream. So it no longer clones. It pulls a bounded prefix from the original and returns a Response that replays those bytes ahead of the remainder, which the caller can read or cancel exactly as before. The signature is now { at, response } and adapter-dispatch rebinds upstreamResponse — the response is still needed on the !rotated path, so consuming it outright was not an option either. New test counts bytes actually PULLED, not bytes parsed: the two were different before, which is the whole point.
…in the dispatch path Two CodeRabbit findings. `Date.parse` does not reject an out-of-range day. Measured on Bun, `2026-02-30T00:00:00Z` yields March 2 and `2026-04-31T00:00:00Z` yields May 1, so a malformed upstream body parked a key past the instant it actually named. Only the month was caught (`2026-13-01` is NaN). Validated on the date text rather than by round-tripping the parsed instant, because a value carrying an explicit offset legitimately lands on a different UTC day than the one written. Century leap rule included: 2000 is a leap year, 2100 is not. The other finding is that nothing exercised the wiring. The unit tests covered parseQuotaResetAt and readQuotaResetAt in isolation, so a change that dropped quotaResetAt before rotateProviderTransportOn429, or flipped the precedence against Retry-After, kept every test green while the key came back after the header's 30s and took the same 429 again. Added an end-to-end test that serves a 429 carrying both a Retry-After and a body date and asserts the failed key is parked to the body's instant. Co-authored-by: Abhishek Sharma <abhicse24@gmail.com>
|
Warning Review limit reachedNext included review available in 27 minutes. View limit detailsLimit details: You’ve used all 10 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (7)
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 |
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. |
리뷰 · 우선순위 76 / 80이 PR은 OpenRouter 무료 티어가 주간/월간 한도를 다 썼을 때, OCX가 키를 너무 짧게만 식혔다가 다시 집어 들어 같은 429를 반복하는 문제를 고칩니다. 지금 원본은 기여자 @abhisheksharma2411의 #4733(초안)입니다. 그 PR은 아이디어는 맞았지만, 본문을 읽을 때 상한 없이 배선은 라인 - 키-failover.ts 중복 JSDoc - 테스트 레이아웃 - 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
![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: 316cf3800d
ℹ️ 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".
| export async function readQuotaResetAt( | ||
| response: Response, | ||
| nowOrOptions: number | QuotaResetReadOptions = {}, | ||
| ): Promise<{ at: number | undefined; response: Response }> { |
There was a problem hiding this comment.
This changes both src/providers/ and src/server/, but the commit updates only structure/transports/responses.md; structure/INDEX.md maps src/providers/ to four documents and src/server/ to multiple additional documents. The repository’s ownership contract requires every document listed for a changed area to be updated in the same change, so update the remaining mapped documents or narrow the manifest ownership where those documents do not actually describe this behavior.
AGENTS.md reference: structure/AGENTS.md:L44-L50
Useful? React with 👍 / 👎.
| let peeked: Awaited<ReturnType<typeof readQuotaResetAt>>; | ||
| try { | ||
| peeked = await readQuotaResetAt(upstreamResponse, { signal: options.abortSignal }); |
There was a problem hiding this comment.
The body peek is wired only into the initial Responses adapter-dispatch loop. An ordinary OpenRouter /v1/chat/completions request takes the native fast path and reaches the separate loop in src/server/chat-native.ts:406-413, while terminal continuations rotate in src/server/responses/adapter-continuation.ts:318-324; both still call rotateProviderTransportOn429 without reading or passing quotaResetAt. If either path receives the documented dated quota response, the failed key is parked only for Retry-After or the one-minute default and resumes the recurring 429 loop, so the body parsing should be shared by all raw-response key-rotation sites.
Useful? React with 👍 / 👎.
|
✅ Deterministic PR hygiene checks passed. |
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
I verified this against exact head . The cooldown parser itself is bounded, but the behavior is incomplete at two runtime boundaries:\n\n1. Dated OpenRouter reset evidence is consumed only in the initial Responses adapter-dispatch loop. The native OpenAI Chat path in and terminal continuation rotation in still call without . Those paths therefore fall back to Retry-After/one minute and can re-enter the same recurring 429 loop. Share the bounded peek/parse behavior across every raw-response 429 rotation boundary and add focused tests for native Chat and continuation.\n2. The source change spans and , while the ownership documentation update is incomplete for the mapped areas. Reconcile the required structure owners or narrow the ownership mapping with an explicit reason.\n\nThe current hosted suite also has real failures, so this is not mergeable yet. Please re-request review after these paths and CI are corrected.
Replacing this review because shell quoting stripped inline code formatting from the submitted body.
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
I verified this against exact head 316cf3800d. The cooldown parser itself is bounded, but the behavior is incomplete at two runtime boundaries:
- Dated OpenRouter reset evidence is consumed only in the initial Responses adapter-dispatch loop. The native OpenAI Chat path in
src/server/chat-native.tsand terminal continuation rotation insrc/server/responses/adapter-continuation.tsstill callrotateProviderTransportOn429withoutquotaResetAt. Those paths therefore fall back to Retry-After/one minute and can re-enter the same recurring 429 loop. Share the bounded peek/parse behavior across every raw-response 429 rotation boundary and add focused tests for native Chat and continuation. - The source change spans
src/providers/andsrc/server/, while the ownership documentation update is incomplete for the mapped areas. Reconcile the required structure owners or narrow the ownership mapping with an explicit reason.
The current hosted suite also has real failures, so this is not mergeable yet. Please re-request review after these paths and CI are corrected.
Implements the reviewed fixes for PR #4733 while preserving the original monthly-window and UTC contracts. Co-authored-by: Abhishek Sharma <abhicse24@gmail.com>
316cf38 to
613e224
Compare
|
Merging with macOS legs outstanding, and recording why rather than leaving it implicit. At this exact head the full Linux suite (test 1/4 through 4/4), This change is platform-neutral, so waiting on a queue that is both saturated and known-unreliable would delay the work without adding information. The evidence that governs the release is not per-PR macOS legs; it is the full-platform Stating the boundary plainly: this is merged on Linux, gates and cross-platform smoke evidence at its exact head, with macOS coverage deferred to the candidate run rather than claimed here. |
|
Agreeing with this carry too, and the provider-scope fix is the one I should have made myself. Accepting dated cooldowns only from the canonical OpenRouter quota-error JSON shape is right, and I had no business matching on prose alone. The stream-safety fixes are the half I got genuinely wrong. I bounded the parse and then claimed the read was bounded, and when I did bound the read I still left the boundary-chunk overflow unreplayed and the reader unreleased on some exits. Replaying the overflow, bounding the peek with cancellation and a deadline, and releasing or cancelling on every exit is the correct shape for reading a prefix of an upstream-controlled stream on the rotation path. The rotation-and-persistence-after-abort ordering is a real hazard I hadn't considered at all. Two things I'd flag, neither a disagreement: 1. Durability is still unstated in-product. Cooldowns live in 2. Dropping the closing reference was the right call given this is only the key-parking half. Worth a line on #4024 saying "keys landed, combo targets open" so the issue does not read as untouched. No objection to anything on top of my work. Thanks for carrying it rather than leaving it to rot on a round trip — and for the attribution. |
3 participants
Summary
Verification
git diff --check origin/dev...HEADcompleted with no findings.jq empty scripts/test-layout/layout.json tests/fixtures/test-layout-expected.jsoncompleted successfully.Checklist