fix(responses): bound API-key 429 rotations across continuations - #4621
fix(responses): bound API-key 429 rotations across continuations#4621luvs01 wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (11)
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe change bounds API-key 429 failover for generic Responses requests. Initial recovery, sidecar operations, and continuations share a fixed pool-based rotation limit. Failover preserves cooldown recording when no replacement is selected. Tests and documentation cover the contract. ChangesResponses key failover
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant Client
participant ResponsesAdapter
participant Provider
participant KeyFailover
Client->>ResponsesAdapter: submit Responses request
ResponsesAdapter->>Provider: send request with current API key
Provider-->>ResponsesAdapter: return 429
ResponsesAdapter->>KeyFailover: request bounded rotation
KeyFailover-->>ResponsesAdapter: record cooldown and return replacement or no replacement
ResponsesAdapter->>Provider: send recovery or continuation request
Provider-->>ResponsesAdapter: return response or final error
ResponsesAdapter-->>Client: return Responses result
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The documented behavior matches the affected Responses request path, with no remaining actionable merge risk identified. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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. Automatic ready-for-review conversion failed; please mark the pull request ready manually if it is still a draft. |
|
@codex review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
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. |
리뷰 · 우선순위 64 / 80설명 이 PR(작성자 luvs01, draft)은 API 키 풀에서 429가 났을 때 키가 무한히 돌 수 있는 구멍을 막는다. 짧은 쿨다운이 끝나면 이미 시도한 키를 다시 고를 수 있고, reset-only 전송은 공유 transient retry budget을 안 켜서 요청이 키만 바꾸며 계속 도는 장면이 나온다. 고치는 방법은 단순하다. 현재 테스트는 핵심을 잘 잡는다. 문서 쪽은 영어 providers.md와 8개 로케일, 그리고 우선순위 64는 '실제 비용/루프 버그를 막는 코드'라서 문서 PR보다 높고, 동시에 draft·임팩트 스위트 미완료·structure 산포·tip 뒤처짐 때문에 70대로 올리지 않은 점수다. 콤보 전체 예산이 아니고 '키를 한 번씩만' 보장도 아니라고 본문이 솔직히 말한다. 그 범위를 메인테이너가 받아들일지가 머지 판단의 핵심이다. src/server/responses/core.ts keyPool429RetryAllowed - continuation=false일 때만 auth-recovery reserve를 엿보는데, countedExternally:true 로 reserveDispatch를 호출한다. '검사만 하고 소비하지 않는다'는 주석과 실제 reserve API 의미가 같은지 tip 기준으로 한 번 더 확인이 필요하다. 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 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: 78957f72f0
ℹ️ 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".
|
@codex review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e7dc2d7343
ℹ️ 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".
|
@codex review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Updated author evidence for the current head 9336e38. Both automated reviews are complete and their two findings are resolved, including the initially unpooled key and sidecar sites. CI completed with the explicitly documented Windows exceptions; this is author-ready, not a green-CI claim. The reserve question was rechecked: reserveDispatch returns a permit, and shared state is charged only by permit.use(); the allowance query does not call it. Pools with fewer than two entries already decline key rotation in rotateKeyAfterFailure, while existing same-target retry and OAuth fallback policies remain separate. The referenced dev changes from 62f0222 to 4f788f9 are a version bump and release evidence, with no diff in core.ts, key-failover.ts or request-execution-budget.ts. Rebasing solely to move the base label would restart validation without changing those semantics. The ownership manifest was not expanded: the short links satisfy its existing source-owner mapping while the contract stays in one canonical section. Maintainer acceptance of the invocation-level design remains a merge decision. The four author-checklist boxes and review-ready label are complete. GitHub denied the author account permission to mark this PR ready, and the repository automation also reported a failed draft conversion. It therefore remains technically draft; a maintainer must perform that state transition. I have not retried the denied mutation. |
2 participants
Summary
An API-key provider with a short 429 cooldown can select an already-tried key again after its cooldown expires. Default reset-only transports do not enable the optional shared transient retry budget, so repeated 429s can keep the request rotating.
Bound successful key-pool failovers within each generic response invocation to the initial pool size minus one when the initially selected key reference belongs to the pool, or the full pool size when it is outside the pool. Initial recovery, web-search/image/video bridge rounds and terminal continuations share that counter. Initialization precedes sidecar dispatch. The sidecar callback applies the API-key count while preserving its existing reset/same-target policies and OAuth/Anthropic account fallback; it does not gain the ordinary core physical-send budget. Existing physical-send budgets still stop dispatch earlier when applicable. On exhaustion, record the final proven failed key's cooldown without selecting or persisting another key, and preserve the final upstream failure. A superseded key-selection snapshot cannot alter key health.
This is an invocation-level bound, not a combo-wide bound or a guarantee that each distinct key is tried only once. Source-owner documentation and all eight provider-configuration translations describe that scope.
Verification
Based on upstream dev
62f02223a0d1b2f71d8bc8a07d29703ad77a7ca9; Bun 1.4.2 on Windows.The bounded pre-fix regression failed in all three original scenarios: default 429 recovery, recovery followed by a terminal continuation, and a transient-retry route. The fixture explicitly stops after six sends, preventing an unbounded test.
Current-source focused verification: 186 passing cases / 669 assertions in the key-failover, transient-retry, web-search and image-loop files. The combined invocation also reported one module-load error when the image-loop test mock omitted
imageFulfillmentTailSnapshotneeded by the server suite. Running that server file in a separate process passed all 22 cases / 149 assertions; the separate Anthropic sidecar fallback regression passed 1 case / 8 assertions. The failed combined invocation is retained as such, not relabeled green. Existing module mock replacement and the actual production export establish the isolation issue.Typecheck, structure ownership check, privacy scan and diff check passed. The updated documentation build generated all 441 pages.
bun scripts/test.ts --changed=62f02223a0d1b2f71d8bc8a07d29703ad77a7ca9reached the runner's 900-second limit (exit 124). Collected output contains 6,589 passing and 57 failing cases across the broad import graph; the run did not complete. Failures include timeouts and assertions in management, compact, routing, CLI and response paths. They have not all been attributed to the baseline or this change, so this is not a successful impact-suite result. The later current-head hosted run and automated reviews have now been assessed below; this historical incomplete local run is not relabeled as successful.Current-head CI 34839412597 completed on
9336e38f99b2a65a062113d5ccba4eb559974956with a failure conclusion. All other jobs passed; Windows 4/6 and 6/6 contain the exceptions below and therefore the aggregate gate also failed. This is author-ready with documented validation exceptions, not a claim that hosted CI is green.Review regressions: an initially unpooled key failed before its correction (two sends instead of three), then passed with
outside, A, Band final cooldown/error preservation. Actual web-search and image bridge requests then exposed a second missing rotation site: each alternated A/B until the six-send fixture ceiling on the preceding head. Both now stop after A/B, with bridge activation verified by the emitted synthetic tool and the final provider 429/cooldown preserved (2 cases, 14 assertions). The shared callback also serves the video bridge. Both identified findings have been addressed. Codex and CodeRabbit completed review of the current head without additional findings; all review threads are resolved. Current-head CI has been assessed with the documented exceptions.Windows 4/6: nine unchanged desktop restart failures match the known Windows/POSIX fixture issue addressed separately in fix(codex): repair desktop restart membership and POSIX-only cases on Windows #4564. One additional
native-profile-crash-boundariescase timed out waiting for its startup port file before reaching its teardown assertion. On this exact head, that case passed independently on Windows (1 test, 3 assertions, 5.59 seconds); the helper publishes its port before any Responses dispatch, outside the changed rotation path. The CI-only startup delay remains unattributed.Windows 6/6: the existing paginated catalog-path assertion compares escaped TOML text to a raw Windows path (test(codex): compare the injected catalog path as a decoded TOML value #4568), and an unchanged auth-context fixture cleanup reports EPERM, matching an earlier observed failure. The specific cleanup handle/locking cause remains unknown. Neither failure exercises the changed rotation logic. No passing test was rerun merely to relabel this CI result.
Checklist
Review readiness checklist
Author readiness includes the explicit validation exceptions above; the hosted CI conclusion remains failure.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation