Repository navigation
feat(providers): carry opt-in API key round-robin and per-key pacing (#6937) - #7135
Conversation
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
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 |
Capture native Responses key ownership at the physical HTTP boundary and settle terminal distributed rejections after same-key retries. Revalidate all queued turn admissions and fence CLI spawn after asynchronous setup. Preserve local pacing refusal metadata without cooling synthetic responses. Co-authored-by: GSL-R <174104911+GSL-R@users.noreply.github.com>
Separate already-sent failure ownership from dispatch eligibility so a late 401 extends an earlier short cooldown without admitting a cooled key. Preserve genuine rejection evidence when a replay returns a synthetic refusal. Cover both orderings with native Responses regressions. Co-authored-by: GSL-R <174104911+GSL-R@users.noreply.github.com>
|
✅ Deterministic PR hygiene checks passed. |
✅ READY
Hygiene✅ Deterministic PR hygiene checks passed. |
The passthrough pacing-error mapping reaches pacing-overload.ts from the Responses core import graph. Include it in the source-oracle inventory so coverage, dependency and strict below-2000-line guards cover the new owner. Co-authored-by: GSL-R <174104911+GSL-R@users.noreply.github.com>
…nd-robin # Conflicts: # scripts/test-layout/layout.json # tests/fixtures/test-layout-expected.json
Move the request-pacing status handler, including key cooldown diagnostics, to a cohesive management sibling. Preserve the route position, validation and response shape while leaving the provider router below the size limit after integration with dev. Document the new route owner. Co-authored-by: GSL-R <174104911+GSL-R@users.noreply.github.com>
# Conflicts: # scripts/test-layout/layout.json
# Conflicts: # scripts/test-layout/layout.json # tests/fixtures/test-layout-expected.json
…route registry passthrough-dispatch.ts reached 2001 lines after the dev merge; drop two blank lines. The pacing GET route lives in provider-key-pacing-routes, so declare that owner.
# Conflicts: # scripts/test-layout/layout.json # tests/fixtures/test-layout-expected.json
|
Maintainer integration decision (dev, MAINTAINERS.md maintainer-integration policy): landing this PR at head 3326138 as part of the 2026-10-16 PR-reduction batch. Hosted GitHub Actions are stalled (runs sit queued), and the maintainer ruled out local test runs for this round, so the evidence is remote and exact-tree:
|
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. |
![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: 3326138276
ℹ️ 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".
| let released = false; | ||
| let bodyTracked = false; | ||
| const leased = waiter.providerMaxConcurrent > 0 || waiter.modelMaxConcurrent > 0; | ||
| const leased = (!!waiter.keyId && waiter.keyLease) || waiter.providerMaxConcurrent > 0 || waiter.modelMaxConcurrent > 0; |
There was a problem hiding this comment.
Avoid treating every keyed request as concurrency-leased: keyLease is true whenever round-robin or any per-key rule creates a keyId, even when keyMaxConcurrent is zero. Consequently, enabling only keySelection: "round-robin" or a per-key start interval makes trackProviderRequestSlotBody wrap every response and cancel it after the 30-second unconsumed or five-minute inactivity deadline. Long quiet streams can therefore be truncated despite the operator configuring no concurrency limit; make the body-lifecycle lease conditional on an actual per-key concurrency cap, or decouple diagnostic in-flight accounting from lease enforcement.
Useful? React with 👍 / 👎.
| const ownsDistributedKey = !activeProvider._apiKeyAttempt?.distributed | ||
| || headers.get("authorization") === `Bearer ${activeProvider.apiKey}`; |
There was a problem hiding this comment.
When a custom Chat provider supplies an x-api-key or api-key static header alongside the generated bearer header, this check still declares the distributed bearer key to own a 401/429. A gateway may authenticate using the alternate header, so the refusal can belong to that credential; the recovery loop then cools and rotates otherwise unused pool keys, potentially exhausting the pool. Apply the same alternate-header exclusions used by distributedKeyPhysicalFetch before assigning ownership.
Useful? React with 👍 / 👎.
2 participants
Summary
Carry of contributor PR #6937 by @GSL-R, retaining its commits through an ordinary merge of source head
b8d48e7227a504586039f9a9a8a25779af42ce45onto dev7640a942b3. The feature starts at636a7d2e3342d9f07141c8116bf29d3caae5a7d8; subsequent contributor and maintainer fixes are retained, including the final merge's failed-key refusal before turn readmission. Current dev does not contain this opt-in feature.requestPacing.keySelection: "round-robin"allocates healthy API keys per request without persisting an active-key change.requestPacing.keysadds uniform per-credential spacing/concurrency gates alongside provider/model gates. Default active-key selection, OAuth and forwarded credentials retain their existing behavior. Queued stale selections rebuild and acquire replacement-key capacity before sending; terminal distributed 401/429 outcomes retain cooldowns. WebSocket paths fall back to HTTP/SSE when these controls require a guarded send.Preserves fixes for all original CodeRabbit findings: translated invalid-limit feedback, Portuguese/Russian active-first copy, distributed 401 cooling, replacement-key admission, notification-error lease release, longest cooldown retention, and queued-key documentation. Ingwannu's review was a scoped compact/CI inspection and grants no broad security approval.
Images retains one non-idempotent upstream POST, distributed selection and cooldown recording. Its standalone relay remains outside inference pacing; per-key/provider pacing limits do not cover that relay. Counters/cursors are process-local and credential-reference-based, not project quota or cost balancing.
Maintainer security review is required before merge under MAINTAINERS.md because this changes credential selection and recovery. A GUI screenshot is still required before review readiness; no screenshot is committed to this branch. Independent transport audit found native Responses cooldown and CLI dispatch gaps; the follow-up fixes and regressions are included. Independent static re-reviews passed at
3e7270678aand final head70d2473332; these are bounded code reviews, not maintainer security approval or independent test execution. No labels or changes to the source PR are requested by this carry.Co-authored-by: GSL-R 174104911+GSL-R@users.noreply.github.com
Screenshots
Round-robin API-key selection and provider/per-key pacing limits.
Captured from this PR worktree in light theme using disposable homes and fake provider data. Fresh evidence for the screenshot requirement; earlier screenshot-pending notes below are historical.
Verification
All tests/checks ran in this worktree with Bun 1.4.3 and temporary HOME, OPENCODEX_HOME and CODEX_HOME via an ignored scratch wrapper. Installation used
bun installand(cd gui && bun install)first; no dependency or lockfile changes. No live account/config/keychain access, service restart or provider preset edits.691 focused runtime/guard cases plus 8 GUI cases passed. Structured comparisons retained every mapping from both parents in the shard-aware layout and expected fixture.
git diff --checkpassed. No full local suite,prepush, whole tests directory or whole GUI suite was run: the maintainer explicitly prohibits heavy local suites on this shared host. Repository-wide/platform coverage is left to exact-head CI. PR and manual cross-platform run results will be recorded after pushing; no unrun CI is claimed as passed.Follow-up validation at
3e7270678a482313cec547e8fc229daa838108e3Native Responses now captures immutable credential/destination ownership after connection-policy rewrites and settles terminal distributed failures after configured same-key retries. Pending genuine 429 evidence survives a local replay-admission refusal; cancellation, synthetic responses, header overrides and stale revisions do not cool unused keys. CLI turns revalidate every admission and fence spawn after asynchronous tool setup. The passthrough catch also preserves pacing 429/Retry-After. Initial and replay queue-exhaustion/expiry end-to-end cases already returned 429 before this catch hardening; no reproduced 502 is claimed.
Regression evidence: native 401/429 cases failed before the fix (10 pass, 2 fail in the original 12-case batch); final native file is 15 pass, 0 fail. Disabling the admission/spawn fences produced 2 pass, 6 fail, and restored-code focused validation passed. New tests live in sibling files and are registered in both layout inventories. No ratchet baseline changed. GUI and public docs are unchanged by the follow-up, so their passing builds were not rerun. Hosted CI is still queued; final-head results remain pending.
Concurrent outcome validation at
70d24733328f6520463e109f84280cf016c1f164Failure ownership now permits an already cooling, still-configured credential to extend its cooldown. The separate physical-send check continues to refuse that credential. A synthetic replay refusal retains preceding genuine rejection evidence. Both new native regression cases failed before their fixes and pass afterward (15 pass / 1 fail, then 16 pass / 1 fail in the respective red runs).
The prior independent focused re-review passed at
3e7270678a; this final concurrent-outcome delta passed a separate narrow independent static re-review at70d2473332, with no remaining blockers. Hosted executable CI remains pending/queued; cancelled or older-head checks do not qualify as passing.ci.ymldispatch input islane=all; the full cross-platform dispatch remains contingent on green PR CI, as requested. Screenshot and explicit maintainer security review remain required.Exact-head hosted state
At
70d24733328f6520463e109f84280cf016c1f164, PR Cross-platform CI is pending; no executable job result is available. Catalog async contracts, React Doctor, Enforce PR target branch and PR Labeler are queued; PR hygiene is pending. These are not passing CI evidence. Fullworkflow_dispatch(lane=all) has not run because the requested green PR CI prerequisite is pending. GOAT rejected the implementation subagent's PR watch; the coordinator must own ongoing monitoring and dispatch after green.Checklist
Pending: explicit maintainer security review, GUI screenshot and exact-head CI.