feat(codex): preserve opt-in provider-host circuit follow-up - #1039
Draft
luvs01 wants to merge 1 commit into
Draft
feat(codex): preserve opt-in provider-host circuit follow-up#1039luvs01 wants to merge 1 commit into
luvs01 wants to merge 1 commit into
Conversation
Contributor
📝 WalkthroughWalkthroughAdds an opt-in provider-host circuit breaker for pre-connection DNS/TCP failures. Responses and compact requests now use admission leases, 30-second cooldowns, half-open probes, ChangesProvider-host circuit breaking
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ResponsesCore
participant CompactHandler
participant UpstreamHostHealth
participant Provider
Client->>ResponsesCore: Send Responses request
Client->>CompactHandler: Send native compact request
ResponsesCore->>UpstreamHostHealth: Acquire host admission
CompactHandler->>UpstreamHostHealth: Acquire host admission
UpstreamHostHealth-->>ResponsesCore: Lease or 503 retry timing
UpstreamHostHealth-->>CompactHandler: Lease or 503 retry timing
ResponsesCore->>Provider: Forward admitted request
CompactHandler->>Provider: Forward admitted request
Provider-->>ResponsesCore: Response or connection failure
Provider-->>CompactHandler: Response or connection failure
ResponsesCore->>UpstreamHostHealth: Reset, record, or release lease
CompactHandler->>UpstreamHostHealth: Reset, record, or release lease
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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`:
- Around line 25-26: Update the failover description beside
upstreamFailoverThreshold in all five listed provider configuration
files—docs-site/src/content/docs/reference/configuration/providers.md lines
25-26, docs-site/src/content/docs/ja/reference/configuration/providers.md lines
24-25, docs-site/src/content/docs/ko/reference/configuration/providers.md lines
24-25, docs-site/src/content/docs/ru/reference/configuration/providers.md lines
25-26, and docs-site/src/content/docs/zh-cn/reference/configuration/providers.md
lines 24-25—to replace “cooldowns” with the equivalent account-scoped wording
“account cooldowns,” while leaving the provider-host circuit description
unchanged.
In `@src/server/responses/compact.ts`:
- Around line 316-343: Extract the shared upstream-host circuit eligibility
predicate and 503 response builder currently defined in core.ts, export them,
and replace the inline ternary and blocked-admission response in the compact
handler with those helpers. Apply the same response-builder substitution to the
later blocked-admission path around the existing compact handler logic, while
preserving the compact endpoint’s current eligibility behavior and using the
shared helpers from both handlers.
In `@tests/responses-compaction-routing.test.ts`:
- Around line 936-960: Add an observable assertion that the second blocked
request in both circuit tests skips Pool account selection, such as spying on
the account-selection method or comparing Pool affinity/quota state before and
after that request. Keep the existing no-send and health assertions, and ensure
the test specifically verifies host admission prevents
resolveCodexAuthContext/account selection before returning 503.
In `@tests/upstream-host-circuit.test.ts`:
- Around line 142-173: Add a focused regression test near the existing
stale-completion tests that admits a lease for one upstream-host health key,
then attempts both failure recording and reset against a different key using
that lease. Assert the mismatched-key operations do not mutate the target host’s
state, and preserve coverage for both lease validation paths used by
recordUpstreamHostFailure and resetUpstreamHostHealth.
🪄 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: bf0019f5-7f39-46c7-b85b-3d267bc70df9
📒 Files selected for processing (11)
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/server/responses/compact.tssrc/server/responses/core.tssrc/types.tstests/responses-compaction-routing.test.tstests/upstream-host-circuit.test.ts
Comment on lines
+25
to
+26
| | `upstreamFailoverThreshold?` | `number` | `3` | Consecutive transient failures before future new sessions fail over. Set `0` to disable. For regular Responses and native compact sends, proven pre-connection DNS/TCP reachability failures are tracked at the provider-host level: they never affect account health, cooldowns, thread/session affinity, active-account selection, or Pool routing, and never count toward this threshold. | | ||
| | `upstreamHostCircuitThreshold?` | `number` | `0` | Opt-in circuit threshold for proven pre-connection DNS/TCP failures on native OpenAI forward Responses and compact sends. `0` disables it; `1`–`20` opens a 30-second provider-origin cooldown after that many terminal logical requests. While open, requests receive `503` with `Retry-After` before account selection or upstream send; after cooldown, one half-open request is admitted. Timeouts and HTTP responses never count, and any HTTP response closes the circuit. | |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Scope the failover statement to account cooldowns in every provider configuration page.
Each failover row says that the failures do not affect “cooldowns.” The following circuit row says that the same failures open a 30-second provider-origin cooldown. Replace the unqualified term with account-scoped wording.
docs-site/src/content/docs/reference/configuration/providers.md#L25-L26: Change “cooldowns” to “account cooldowns.”docs-site/src/content/docs/ja/reference/configuration/providers.md#L24-L25: Use the equivalent account-scoped term in Line 24.docs-site/src/content/docs/ko/reference/configuration/providers.md#L24-L25: Use the equivalent account-scoped term in Line 24.docs-site/src/content/docs/ru/reference/configuration/providers.md#L25-L26: Use the equivalent account-scoped term in Line 25.docs-site/src/content/docs/zh-cn/reference/configuration/providers.md#L24-L25: Use the equivalent account-scoped term in Line 24.
As per path instructions and the PR objective, the documentation must distinguish account failover state from the provider-host circuit state.
📍 Affects 5 files
docs-site/src/content/docs/reference/configuration/providers.md#L25-L26(this comment)docs-site/src/content/docs/ja/reference/configuration/providers.md#L24-L25docs-site/src/content/docs/ko/reference/configuration/providers.md#L24-L25docs-site/src/content/docs/ru/reference/configuration/providers.md#L25-L26docs-site/src/content/docs/zh-cn/reference/configuration/providers.md#L24-L25
🤖 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 `@docs-site/src/content/docs/reference/configuration/providers.md` around lines
25 - 26, Update the failover description beside upstreamFailoverThreshold in all
five listed provider configuration
files—docs-site/src/content/docs/reference/configuration/providers.md lines
25-26, docs-site/src/content/docs/ja/reference/configuration/providers.md lines
24-25, docs-site/src/content/docs/ko/reference/configuration/providers.md lines
24-25, docs-site/src/content/docs/ru/reference/configuration/providers.md lines
25-26, and docs-site/src/content/docs/zh-cn/reference/configuration/providers.md
lines 24-25—to replace “cooldowns” with the equivalent account-scoped wording
“account cooldowns,” while leaving the provider-host circuit description
unchanged.
Source: Path instructions
Comment on lines
+316
to
+343
| const preAuthCompactHostKey = normalizeUpstreamHostCircuitThreshold( | ||
| config.upstreamHostCircuitThreshold, | ||
| ) > 0 | ||
| && route.codexAccountMode === "pool" | ||
| && route.codexAccountId === undefined | ||
| && route.provider.authMode === "forward" | ||
| ? upstreamHostHealthKey( | ||
| route.providerName, | ||
| safeOriginLabel(route.provider.baseUrl ?? ""), | ||
| ) | ||
| : null; | ||
| let compactHostAdmissionLease: UpstreamHostAdmissionLease | null = null; | ||
| let authCtx: CodexAuthContext = { kind: "main", accountId: null }; | ||
| if (preAuthCompactHostKey) { | ||
| const admission = acquireUpstreamHostAdmission( | ||
| preAuthCompactHostKey, | ||
| config.upstreamHostCircuitThreshold, | ||
| ); | ||
| if (admission.kind === "blocked") { | ||
| return formatErrorResponse( | ||
| 503, | ||
| "upstream_host_circuit_open", | ||
| "Provider host is temporarily unavailable", | ||
| { retryAfter: String(admission.retryAfterSeconds) }, | ||
| ); | ||
| } | ||
| compactHostAdmissionLease = admission.lease; | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Extract the shared circuit eligibility predicate and the 503 builder.
This block duplicates policy that already exists in src/server/responses/core.ts:
- eligibility: lines 316-326 here restate
preAuthUpstreamHostCircuitKey(src/server/responses/core.tslines 252-261) as an inline ternary. - 503 response: lines 335-340 here restate
upstreamHostCircuitOpenResponse(src/server/responses/core.tslines 263-270), and lines 420-425 restate it a third time.
The two eligibility expressions already differ. preAuthUpstreamHostCircuitKey adds route.provider.adapter !== "openai-responses"; this one relies on the enclosing supportsNativeResponsesCompactEndpoint guard at line 312 instead. The current behavior is equivalent, but a future change to circuit eligibility must now be applied in two places written in two different shapes. A change applied to only one place would silently diverge the Responses and compact circuits for the same provider host.
Export both helpers from a shared module and call them from both handlers.
♻️ Proposed consolidation
Export the two helpers from src/server/responses/core.ts (or a shared host-circuit module):
-function preAuthUpstreamHostCircuitKey(route: RouteResult, config: OcxConfig): string | null {
+export function preAuthUpstreamHostCircuitKey(
+ route: Pick<RouteResult, "provider" | "providerName" | "codexAccountMode" | "codexAccountId">,
+ config: OcxConfig,
+ options: { requireResponsesAdapter?: boolean } = {},
+): string | null {
if (
normalizeUpstreamHostCircuitThreshold(config.upstreamHostCircuitThreshold) === 0
|| route.codexAccountMode !== "pool"
|| route.codexAccountId !== undefined
|| route.provider.authMode !== "forward"
- || route.provider.adapter !== "openai-responses"
+ || (options.requireResponsesAdapter !== false && route.provider.adapter !== "openai-responses")
) return null;
return upstreamHostHealthKey(route.providerName, safeOriginLabel(route.provider.baseUrl ?? ""));
}
-function upstreamHostCircuitOpenResponse(retryAfterSeconds: number): Response {
+export function upstreamHostCircuitOpenResponse(retryAfterSeconds: number): Response {Then in this file:
- const preAuthCompactHostKey = normalizeUpstreamHostCircuitThreshold(
- config.upstreamHostCircuitThreshold,
- ) > 0
- && route.codexAccountMode === "pool"
- && route.codexAccountId === undefined
- && route.provider.authMode === "forward"
- ? upstreamHostHealthKey(
- route.providerName,
- safeOriginLabel(route.provider.baseUrl ?? ""),
- )
- : null;
+ // The enclosing supportsNativeResponsesCompactEndpoint() guard already
+ // restricts this path to the canonical backends, so the adapter check
+ // from the Responses path is redundant here.
+ const preAuthCompactHostKey = preAuthUpstreamHostCircuitKey(route, config, {
+ requireResponsesAdapter: false,
+ });
let compactHostAdmissionLease: UpstreamHostAdmissionLease | null = null;
let authCtx: CodexAuthContext = { kind: "main", accountId: null };
if (preAuthCompactHostKey) {
const admission = acquireUpstreamHostAdmission(
preAuthCompactHostKey,
config.upstreamHostCircuitThreshold,
);
if (admission.kind === "blocked") {
- return formatErrorResponse(
- 503,
- "upstream_host_circuit_open",
- "Provider host is temporarily unavailable",
- { retryAfter: String(admission.retryAfterSeconds) },
- );
+ return upstreamHostCircuitOpenResponse(admission.retryAfterSeconds);
}
compactHostAdmissionLease = admission.lease;
}Apply the same substitution at lines 420-425.
🤖 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/compact.ts` around lines 316 - 343, Extract the shared
upstream-host circuit eligibility predicate and 503 response builder currently
defined in core.ts, export them, and replace the inline ternary and
blocked-admission response in the compact handler with those helpers. Apply the
same response-builder substitution to the later blocked-admission path around
the existing compact handler logic, while preserving the compact endpoint’s
current eligibility behavior and using the shared helpers from both handlers.
Comment on lines
+936
to
+960
| test("an opt-in regular circuit blocks before selecting another pool account", async () => { | ||
| await withPoolEnv("ocx-regular-host-circuit-", async config => { | ||
| config.upstreamHostCircuitThreshold = 1; | ||
| let sends = 0; | ||
| globalThis.fetch = (async () => { | ||
| sends += 1; | ||
| throw Object.assign(new Error("connection refused"), { code: "ECONNREFUSED" }); | ||
| }) as typeof fetch; | ||
|
|
||
| const request = () => new Request("http://localhost/v1/responses", { | ||
| method: "POST", | ||
| headers: { "content-type": "application/json" }, | ||
| body: JSON.stringify({ model: "gpt-5.6-sol", input: "hello", stream: false }), | ||
| }); | ||
| const first = await handleResponses(request(), config, { model: "", provider: "" }); | ||
| const second = await handleResponses(request(), config, { model: "", provider: "" }); | ||
|
|
||
| expect(first.status).toBe(502); | ||
| expect(second.status).toBe(503); | ||
| expect(second.headers.get("retry-after")).toBe("30"); | ||
| expect(sends).toBe(1); | ||
| expect(getCodexUpstreamHealth("pool-a")).toBeNull(); | ||
| expect(getCodexUpstreamHealth("pool-b")).toBeNull(); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
Make the no-account-selection assertion observable.
These tests prove that the blocked request sends no upstream request and creates no account-health record. They do not prove that the blocked request skips Pool account selection.
A regression could select pool-b, then return 503 before fetch. The current assertions would still pass. Add an account-selection spy or a before-and-after assertion on Pool affinity or quota state for the second request in both tests.
The supplied routing context places host admission before resolveCodexAuthContext in src/server/responses/compact.ts (Lines 258-417) and defines the eligible pre-auth route in src/server/responses/core.ts (Lines 252-261). The tests should protect that contract.
As per path instructions and the routing objective, this regression test must verify the no-account-selection behavior, not only the no-send outcome.
Also applies to: 962-988
🤖 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 `@tests/responses-compaction-routing.test.ts` around lines 936 - 960, Add an
observable assertion that the second blocked request in both circuit tests skips
Pool account selection, such as spying on the account-selection method or
comparing Pool affinity/quota state before and after that request. Keep the
existing no-send and health assertions, and ensure the test specifically
verifies host admission prevents resolveCodexAuthContext/account selection
before returning 503.
Source: Path instructions
Comment on lines
+142
to
+173
| test("a stale completion cannot mutate the generation that opened the circuit", () => { | ||
| const key = upstreamHostHealthKey("openai", "https://chatgpt.com"); | ||
| const stale = admit(key, 1, 8_000); | ||
| fail(key, 1, 8_001); | ||
| const before = getUpstreamHostHealth(key); | ||
|
|
||
| recordUpstreamHostFailure(key, { | ||
| code: "ECONNREFUSED", | ||
| now: 8_002, | ||
| threshold: 1, | ||
| lease: stale, | ||
| }); | ||
| expect(getUpstreamHostHealth(key)).toEqual(before); | ||
| }); | ||
|
|
||
| test("a later physical retry without its lease cannot close a newer circuit", () => { | ||
| const key = upstreamHostHealthKey("openai", "https://chatgpt.com"); | ||
| fail(key, 1, 9_000); | ||
| const before = getUpstreamHostHealth(key); | ||
|
|
||
| expect(resetUpstreamHostHealth(key, null, 9_001)).toBe(false); | ||
| recordUpstreamHostFailure(key, { | ||
| code: "ECONNREFUSED", | ||
| now: 9_002, | ||
| threshold: 1, | ||
| lease: null, | ||
| }); | ||
| // Unwired observational callers are also unable to mutate circuit-owned state. | ||
| recordUpstreamHostFailure(key, { code: "ECONNREFUSED", now: 9_003 }); | ||
|
|
||
| expect(getUpstreamHostHealth(key)).toEqual(before); | ||
| }); |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add a regression test for the lease key mismatch guard.
The suite covers generation fencing, explicit-null settlement, and unleased observational writes. It does not cover the key mismatch guard.
Two source paths depend on that guard:
src/codex/upstream-host-health.tsline 253:if (!entry || opts.lease.key !== key) return;src/codex/upstream-host-health.tsline 307:if (!entry || lease.key !== key) return false;
Both callers rely on it. src/server/responses/core.ts line 1817 and src/server/responses/compact.ts line 405 detect a host change after admission and return 502. If the key equality check regressed, a lease acquired for host A could settle health for host B, and the circuit state for B would become wrong while A kept a leaked lease. No current test would fail.
Add a focused case near the stale-completion tests.
💚 Proposed regression test
+ test("a lease cannot settle a different host key", () => {
+ const keyA = upstreamHostHealthKey("openai", "https://chatgpt.com");
+ const keyB = upstreamHostHealthKey("openai", "https://api.openai.com");
+ const leaseA = admit(keyA, 1, 10_000);
+
+ expect(resetUpstreamHostHealth(keyB, leaseA, 10_001)).toBe(false);
+ recordUpstreamHostFailure(keyB, {
+ code: "ECONNREFUSED",
+ now: 10_002,
+ threshold: 1,
+ lease: leaseA,
+ });
+ expect(getUpstreamHostHealth(keyB)).toBeNull();
+
+ // Lease A is still unsettled and retains authority over its own key.
+ recordUpstreamHostFailure(keyA, {
+ code: "ECONNREFUSED",
+ now: 10_003,
+ threshold: 1,
+ lease: leaseA,
+ });
+ expect(getUpstreamHostHealth(keyA)).toMatchObject({
+ consecutiveFailures: 1,
+ cooldownUntil: 10_003 + UPSTREAM_HOST_CIRCUIT_COOLDOWN_MS,
+ });
+ });As per path instructions, "A behavior change in src/ should come with a focused regression test near the existing tests for that subsystem."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| test("a stale completion cannot mutate the generation that opened the circuit", () => { | |
| const key = upstreamHostHealthKey("openai", "https://chatgpt.com"); | |
| const stale = admit(key, 1, 8_000); | |
| fail(key, 1, 8_001); | |
| const before = getUpstreamHostHealth(key); | |
| recordUpstreamHostFailure(key, { | |
| code: "ECONNREFUSED", | |
| now: 8_002, | |
| threshold: 1, | |
| lease: stale, | |
| }); | |
| expect(getUpstreamHostHealth(key)).toEqual(before); | |
| }); | |
| test("a later physical retry without its lease cannot close a newer circuit", () => { | |
| const key = upstreamHostHealthKey("openai", "https://chatgpt.com"); | |
| fail(key, 1, 9_000); | |
| const before = getUpstreamHostHealth(key); | |
| expect(resetUpstreamHostHealth(key, null, 9_001)).toBe(false); | |
| recordUpstreamHostFailure(key, { | |
| code: "ECONNREFUSED", | |
| now: 9_002, | |
| threshold: 1, | |
| lease: null, | |
| }); | |
| // Unwired observational callers are also unable to mutate circuit-owned state. | |
| recordUpstreamHostFailure(key, { code: "ECONNREFUSED", now: 9_003 }); | |
| expect(getUpstreamHostHealth(key)).toEqual(before); | |
| }); | |
| test("a stale completion cannot mutate the generation that opened the circuit", () => { | |
| const key = upstreamHostHealthKey("openai", "https://chatgpt.com"); | |
| const stale = admit(key, 1, 8_000); | |
| fail(key, 1, 8_001); | |
| const before = getUpstreamHostHealth(key); | |
| recordUpstreamHostFailure(key, { | |
| code: "ECONNREFUSED", | |
| now: 8_002, | |
| threshold: 1, | |
| lease: stale, | |
| }); | |
| expect(getUpstreamHostHealth(key)).toEqual(before); | |
| }); | |
| test("a lease cannot settle a different host key", () => { | |
| const keyA = upstreamHostHealthKey("openai", "https://chatgpt.com"); | |
| const keyB = upstreamHostHealthKey("openai", "https://api.openai.com"); | |
| const leaseA = admit(keyA, 1, 10_000); | |
| expect(resetUpstreamHostHealth(keyB, leaseA, 10_001)).toBe(false); | |
| recordUpstreamHostFailure(keyB, { | |
| code: "ECONNREFUSED", | |
| now: 10_002, | |
| threshold: 1, | |
| lease: leaseA, | |
| }); | |
| expect(getUpstreamHostHealth(keyB)).toBeNull(); | |
| // Lease A is still unsettled and retains authority over its own key. | |
| recordUpstreamHostFailure(keyA, { | |
| code: "ECONNREFUSED", | |
| now: 10_003, | |
| threshold: 1, | |
| lease: leaseA, | |
| }); | |
| expect(getUpstreamHostHealth(keyA)).toMatchObject({ | |
| consecutiveFailures: 1, | |
| cooldownUntil: 10_003 + UPSTREAM_HOST_CIRCUIT_COOLDOWN_MS, | |
| }); | |
| }); | |
| test("a later physical retry without its lease cannot close a newer circuit", () => { | |
| const key = upstreamHostHealthKey("openai", "https://chatgpt.com"); | |
| fail(key, 1, 9_000); | |
| const before = getUpstreamHostHealth(key); | |
| expect(resetUpstreamHostHealth(key, null, 9_001)).toBe(false); | |
| recordUpstreamHostFailure(key, { | |
| code: "ECONNREFUSED", | |
| now: 9_002, | |
| threshold: 1, | |
| lease: null, | |
| }); | |
| // Unwired observational callers are also unable to mutate circuit-owned state. | |
| recordUpstreamHostFailure(key, { code: "ECONNREFUSED", now: 9_003 }); | |
| expect(getUpstreamHostHealth(key)).toEqual(before); | |
| }); |
🤖 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 `@tests/upstream-host-circuit.test.ts` around lines 142 - 173, Add a focused
regression test near the existing stale-completion tests that admits a lease for
one upstream-host health key, then attempts both failure recording and reset
against a different key using that lease. Assert the mismatched-key operations
do not mutate the target host’s state, and preserve coverage for both lease
validation paths used by recordUpstreamHostFailure and resetUpstreamHostHealth.
Source: Path instructions
|
Codex Review: Didn't find any major issues. Nice work! 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". |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Status
This preserves the optional provider-host circuit work that was separated from closed PR #922. PR #1023 merged as
af100ec9while this draft was being prepared, so the branch was rebased onto the post-#1023devline (4cfdc716when opened) and now contains one follow-up commit only.The planned continuation point has therefore arrived: completion will continue from the merged #1023 baseline. Before this is marked ready, it still needs maintainer direction on the public option/response contract, the full pre-push and cross-platform suite on the then-current
dev, and resolution of any review findings.What this preserves
upstreamHostCircuitThresholdas an opt-in top-level setting. Default0leaves fix(codex): pre-connection transport failures no longer rotate pool accounts (#914) #1023's observational host ledger behavior unchanged; valid enabled values are1–20.connect_neutralfailures already proven by fix(codex): pre-connection transport failures no longer rotate pool accounts (#914) #1023's DNS/TCP reachability classifier. Timeouts, resets/TLS/unknown failures, and every HTTP response stay outside the circuit count.503+Retry-Afterbefore pool account selection/upstream send, and admits exactly one half-open logical request after cooldown.Deliberate limits
Locationremains unchanged.Validation on
19938291git diff --checkpassed.The full repository suite and public CI are intentionally completion gates for moving this preservation draft to Ready, not claims made by this draft.
Related: #914, #922, #1023.
Summary by CodeRabbit
New Features
503with aRetry-Afterheader and prevent unnecessary upstream retries.Documentation