feat(codex): add opt-in provider-host circuit breaker - #1243
Conversation
|
✅ Deterministic PR hygiene checks passed. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds an opt-in provider-host circuit breaker for native Responses and compact requests. It adds configuration validation, lease-based cooldown and recovery state, request-path integration, quota-priming checks, tests, and localized documentation. ChangesProvider host circuit breaking
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Responses
participant UpstreamHostHealth
participant AccountSelection
participant ProviderUpstream
Client->>Responses: Submit Responses or compact request
Responses->>UpstreamHostHealth: Acquire host admission
UpstreamHostHealth-->>Responses: Lease or 503 circuit-open response
Responses->>AccountSelection: Resolve account after admission
Responses->>ProviderUpstream: Send request
ProviderUpstream-->>Responses: Success or connection failure
Responses->>UpstreamHostHealth: Settle, record failure, or release lease
Responses-->>Client: Return provider response or error
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
|
@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: 5
🤖 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 `@docs-site/src/content/docs/reference/configuration/providers.md`:
- Line 28: Update the upstreamHostCircuitThreshold documentation to state that
the circuit applies only to Codex pool routing with no pinned account and
remains inert for codexAccountMode: "direct" or account-qualified selectors. Add
equivalent wording to
docs-site/src/content/docs/reference/configuration/providers.md:28-28,
docs-site/src/content/docs/ja/reference/configuration/providers.md:26-26,
docs-site/src/content/docs/ko/reference/configuration/providers.md:26-26,
docs-site/src/content/docs/ru/reference/configuration/providers.md:27-27, and
docs-site/src/content/docs/zh-cn/reference/configuration/providers.md:26-26.
In `@docs-site/src/content/docs/zh-cn/reference/configuration/providers.md`:
- Line 25: Remove the locale-only clause “未确认的失败仍归属账户。” from the
upstreamFailoverThreshold row in the zh-cn provider configuration table, leaving
the shared behavioral description consistent with the English, ja, ko, and ru
locale rows.
In `@src/codex/upstream-host-health.ts`:
- Around line 302-307: The cooldown-generation change in
recordUpstreamHostFailure invalidates concurrent leases, preventing
resetUpstreamHostHealth from honoring a later HTTP response. Update the
lease-matching/reset flow so a fenced-out lease issued before the cooldown can
still clear that cooldown, preserving newer-generation safety; add a regression
test in the existing upstream-host circuit concurrency tests covering
failure/open first followed by a concurrent successful response.
In `@src/server/responses/core.ts`:
- Around line 2485-2490: Update the request-building flow around
adapter.buildRequest() to catch pre-send failures, release the Codex probe lease
via releaseCodexAuthContextProbeLease(authCtx), and rethrow the original error.
Keep the existing finally behavior conditional so streaming responses retain
probe ownership, and add a regression test covering this failure when host
admission is disabled.
In `@tests/upstream-host-circuit.test.ts`:
- Around line 19-50: Add focused tests in the existing opt-in upstream host
circuit describe block for failure-window expiry and retention-cap eviction,
using the explicit now values and constants shown in the review; also cover the
all-active-lease retention path so pruneTo’s early return is exercised. Extend
the threshold normalization assertions to include 0, the string "3", and 20,
verifying invalid values normalize to 0 and valid values remain bounded. Add the
required imports for UPSTREAM_HOST_FAILURE_WINDOW_MS and
UPSTREAM_HOST_HEALTH_MAX_ENTRIES, and keep the tests near the existing
upstream-host circuit coverage.
🪄 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: 3a4c9388-fffd-4e5c-af31-a3a712b967eb
📒 Files selected for processing (13)
docs-site/src/content/docs/ja/reference/configuration/providers.mddocs-site/src/content/docs/ko/reference/configuration/providers.mddocs-site/src/content/docs/reference/configuration/providers.mddocs-site/src/content/docs/ru/reference/configuration/providers.mddocs-site/src/content/docs/zh-cn/reference/configuration/providers.mdsrc/codex/upstream-host-health.tssrc/config.tssrc/server/responses/compact.tssrc/server/responses/core.tssrc/types.tstests/responses-compaction-routing.test.tstests/upstream-host-circuit-config.test.tstests/upstream-host-circuit.test.ts
|
@coderabbitai review |
|
![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: 745b3384d6
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/upstream-reachability.test.ts`:
- Around line 349-356: Strengthen the “retention pressure never evicts an active
admission lease” test by asserting that releaseUpstreamHostAdmission returns
true for every lease, including leases[0]. Replace the discard-only release loop
over leases.slice(1) with per-lease assertions while preserving the existing
release timestamps and coverage of all 129 leases.
- Around line 185-212: Add a final positive-control invocation in the test using
a closed or otherwise healthy upstream circuit, then assert primeCalls is 1.
Keep this step after the existing blocked assertions so
maybePrimeSubagentQuota’s Date.now()-based quotaPrimedAt does not affect earlier
checks, and preserve the existing setSubagentQuotaPrimeForTests wiring.
🪄 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: e1ff9a67-1265-46c2-b935-9b44f69d92ce
📒 Files selected for processing (3)
src/codex/subagent-model-fallback.tstests/config-user-edits.test.tstests/upstream-reachability.test.ts
|
Maintainer takeover/review complete.
No known merge-blocking issue remains from this review. |
✅ READY
|
|
no gui |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 36c4058158
ℹ️ 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 hostCircuitEnabled = hostKey !== null | ||
| && normalizeUpstreamHostCircuitThreshold(config.upstreamHostCircuitThreshold) > 0; |
There was a problem hiding this comment.
When upstreamHostCircuitThreshold is nonzero, this condition enables the circuit for every forward-auth passthrough route, including codexAccountMode: "direct", account-qualified routes with route.codexAccountId, and noncanonical forward providers. After enough qualifying connection failures, those routes therefore start returning 503, despite docs-site/src/content/docs/reference/configuration/providers.md documenting the option as limited to unpinned Codex Pool routing and inert for Direct/account-qualified requests; the compact path repeats the same unrestricted check. Include the same pool, unfixed-account, and canonical-provider restrictions used by the pre-auth admission helper, or update the documented contract if this broader behavior is intended.
AGENTS.md reference: AGENTS.md:L231-L232
Useful? React with 👍 / 👎.
2 participants
Supersedes #1039.
This is a maintainer-owned port of the provider-host circuit work from #1039 onto current
dev. The original feature commit was cherry-picked cleanly so the contributor attribution is preserved, then the implementation was reviewed and hardened on the current routing/auth/config code.What changed
upstreamHostCircuitThresholdfor native OpenAI forward Responses and native compact sends.0disables the circuit; live writes accept only integer values1–20when enabled.connect_neutralDNS/TCP failures.503+Retry-Afterbefore final Pool account selection/upstream send when possible.Review findings carried forward from #1039
All four unresolved CodeRabbit findings from #1039 were addressed:
503builder;Additional maintainer fixes
The current-tree review also fixed:
adapter.buildRequest()throws after Codex auth selection, any claimed quota probe lease is released before propagating the error.epoll_ctl EEXISTisolate corruption caused by changing the test-file count./backend-api/wham/usageprobes to the same unavailable origin. The single-flight path re-checks the circuit after taking ownership, while the final route-dependent host admission remains in place after fallback selection.Security / privacy review
Validation
Final maintainer validation on head
36c4058:31224500702, attempt 2)REQUEST_CHANGESreview remainsThe first shard-4 attempt hit the repository's known Bun 1.3.14 isolate/runtime corruption (
EEXIST: file already exists, epoll_ctl, followed by test registration after the run had already terminated) and timed out. Re-running that unchanged shard on the same head completed successfully, confirming the failure was runtime flakiness rather than a code change.Summary by CodeRabbit
503responses withRetry-Afterand allow recovery probes after 30 seconds.