From 09e9ecef975d017c9ed8fad7f645488f983fad8d Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza Date: Fri, 24 Jul 2026 20:37:31 -0300 Subject: [PATCH] fix(resilience): treat an unreachable-proxy ECONNREFUSED as a circuit-breaker event so combo fails over instead of hitting the 503 max-retry limit (#8376) (#8403) --- .../fixes/8376-econnrefused-breaker.md | 1 + open-sse/handlers/chatCore.ts | 24 +++-- open-sse/services/combo.ts | 12 +-- open-sse/services/combo/comboPredicates.ts | 12 ++- open-sse/utils/proxyFetch.ts | 43 ++++++++- tests/unit/8376-econnrefused-breaker.test.ts | 96 +++++++++++++++++++ 6 files changed, 172 insertions(+), 16 deletions(-) create mode 100644 changelog.d/fixes/8376-econnrefused-breaker.md create mode 100644 tests/unit/8376-econnrefused-breaker.test.ts diff --git a/changelog.d/fixes/8376-econnrefused-breaker.md b/changelog.d/fixes/8376-econnrefused-breaker.md new file mode 100644 index 0000000000..4e9756c203 --- /dev/null +++ b/changelog.d/fixes/8376-econnrefused-breaker.md @@ -0,0 +1 @@ +- fix(resilience): treat an unreachable-proxy ECONNREFUSED as a circuit-breaker event so combo fails over instead of hitting the 503 max-retry limit (#8376) diff --git a/open-sse/handlers/chatCore.ts b/open-sse/handlers/chatCore.ts index 5d7f3d5dbc..2cbb8eb569 100644 --- a/open-sse/handlers/chatCore.ts +++ b/open-sse/handlers/chatCore.ts @@ -3214,17 +3214,29 @@ export async function handleChatCore({ // via isLocalStreamLifecycleError so those map to 499 instead of falling // through to the 502 provider-failure default. const isRequestAborted = isLocalStreamLifecycleError(error); + // #8376: an unreachable upstream proxy (ECONNREFUSED/ECONNRESET/...) is tagged by + // proxyFetch.ts (tagProxyUnreachable) with `.errorCode = "proxy_unreachable"` before + // it reaches this catch. Classify it explicitly to 502 instead of falling through + // the generic `error.status` branch (a raw connect-refused error has no `.status` at + // all, so it used to collapse into an ordinary 502/504 the provider-breaker predicate + // can't tell apart from a per-model 5xx). + const isProxyUnreachableFailure = + !isRequestAborted && (error as { errorCode?: unknown })?.errorCode === "proxy_unreachable"; const failureStatus = isRequestAborted ? 499 - : error.name === "TimeoutError" || error.name === "BodyTimeoutError" - ? HTTP_STATUS.GATEWAY_TIMEOUT - : error.status && typeof error.status === "number" - ? error.status - : HTTP_STATUS.BAD_GATEWAY; + : isProxyUnreachableFailure + ? HTTP_STATUS.BAD_GATEWAY + : error.name === "TimeoutError" || error.name === "BodyTimeoutError" + ? HTTP_STATUS.GATEWAY_TIMEOUT + : error.status && typeof error.status === "number" + ? error.status + : HTTP_STATUS.BAD_GATEWAY; const failureMessage = isRequestAborted ? "Request aborted" : formatProviderError(error, provider, model, failureStatus); - const upstreamErrorCode = getUpstreamErrorIdentifier(error); + const upstreamErrorCode = isProxyUnreachableFailure + ? "proxy_unreachable" + : getUpstreamErrorIdentifier(error); // Tag our own deadline timeouts (fetch-start TimeoutError / body BodyTimeoutError, // both surfaced as a 504) as "upstream_timeout" so the cooldown layer can tell a // slow-but-not-failed request apart from a real provider 5xx. (Antigravity already diff --git a/open-sse/services/combo.ts b/open-sse/services/combo.ts index 5441ebf8f5..1743ee0ea1 100644 --- a/open-sse/services/combo.ts +++ b/open-sse/services/combo.ts @@ -2381,12 +2381,11 @@ export async function handleComboChat({ return { ok: false, response: result }; } - // Trigger shared provider circuit breaker for 5xx errors and connection failures. - // If the next target in the combo is on the same provider, don't mark the provider - // as failed — different models on the same provider may still succeed. - // G-02: when fallbackResult.skipProviderBreaker is set (embedded service supervisor - // outage signalled via X-Omni-Fallback-Hint: connection_cooldown) apply connection - // cooldown only — do NOT trip the whole-provider breaker. + // Trigger shared provider circuit breaker for 5xx errors and connection failures. If the + // next target is on the same provider, don't mark it failed (a different model may still + // succeed) — #8376: EXCEPT a proxy-unreachable failure, which poisons every model alike. + // G-02: when fallbackResult.skipProviderBreaker is set (embedded service supervisor outage + // signalled via X-Omni-Fallback-Hint: connection_cooldown) apply cooldown only — never trip. const nextTarget = orderedTargets[i + 1]; const sameProviderNext = typeof nextTarget?.provider === "string" && nextTarget.provider === provider; @@ -2398,6 +2397,7 @@ export async function handleComboChat({ skipProviderBreaker: fallbackResult.skipProviderBreaker, requestScopedFailure, error: errorText, + isProxyUnreachable: structuredError?.code === "proxy_unreachable", }) ) { recordProviderFailure(provider, log, targetWithConnection.connectionId, profile); diff --git a/open-sse/services/combo/comboPredicates.ts b/open-sse/services/combo/comboPredicates.ts index 315a49a5a2..f7f0f11321 100644 --- a/open-sse/services/combo/comboPredicates.ts +++ b/open-sse/services/combo/comboPredicates.ts @@ -140,7 +140,12 @@ const PROVIDER_BREAKER_FAILURE_STATUSES = new Set([408, 500, 502, 503, 504]); * this intentionally differs from `isProviderFailureCode` (accountFallback.ts), which * INCLUDES 429 for connection-cooldown purposes and must not be changed here. * - When the next combo target is on the SAME provider, don't trip the provider breaker: - * a different model on that provider may still succeed. + * a different model on that provider may still succeed. #8376: EXCEPT when the failure + * itself is a transport-level "proxy unreachable" event (`isProxyUnreachable`) — a dead + * upstream proxy poisons every account on that provider identically, so a different + * model on the same provider will fail the exact same way. Without this override a + * homogeneous same-provider combo pool never trips the breaker and instead burns every + * attempt against the same dead proxy until it hits the 503 max-retry limit. * - G-02 / #2743: when the fallback result carries `skipProviderBreaker` (an embedded * service supervisor outage signalled via `X-Omni-Fallback-Hint: connection_cooldown`) * apply connection cooldown ONLY — never trip the whole-provider breaker. @@ -161,11 +166,14 @@ export function shouldRecordProviderBreakerFailure(args: { skipProviderBreaker?: boolean; requestScopedFailure?: boolean; error?: unknown; + /** #8376: transport-level "proxy unreachable" signal — overrides the `sameProviderNext` + * exemption only; every other AND-term still gates the trip. */ + isProxyUnreachable?: boolean; }): boolean { return ( !args.isStreamReadinessFailure && PROVIDER_BREAKER_FAILURE_STATUSES.has(args.status) && - !args.sameProviderNext && + (!args.sameProviderNext || args.isProxyUnreachable === true) && !args.skipProviderBreaker && !args.requestScopedFailure && !isLocalStreamLifecycleError(args.error) diff --git a/open-sse/utils/proxyFetch.ts b/open-sse/utils/proxyFetch.ts index bddfa207bf..8eea0d2774 100644 --- a/open-sse/utils/proxyFetch.ts +++ b/open-sse/utils/proxyFetch.ts @@ -22,6 +22,45 @@ function isTlsFingerprintEnabled() { return process.env.ENABLE_TLS_FINGERPRINT === "true"; } +// #8376: transport-level connect-failure codes that mean "the configured upstream +// proxy (or the target itself, for direct egress) is unreachable" — as opposed to an +// ordinary upstream HTTP error. Read `.code` first (stable across undici/node +// versions); native fetch wraps the real socket error in `.cause`, so fall back to +// `.cause.code` when the top-level error is a bare "fetch failed" TypeError. +const PROXY_UNREACHABLE_ERROR_CODES = new Set([ + "ECONNREFUSED", + "ECONNRESET", + "ETIMEDOUT", + "ENETUNREACH", + "EHOSTUNREACH", + "EPIPE", + "UND_ERR_CONNECT_TIMEOUT", +]); + +function isProxyUnreachableError(err: unknown): boolean { + if (!err || typeof err !== "object") return false; + const code = (err as { code?: unknown }).code; + if (typeof code === "string" && PROXY_UNREACHABLE_ERROR_CODES.has(code)) return true; + const causeCode = (err as { cause?: { code?: unknown } }).cause?.code; + return typeof causeCode === "string" && PROXY_UNREACHABLE_ERROR_CODES.has(causeCode); +} + +/** + * #8376: tag a connect-failure error with a stable `.code`/`.errorCode` BEFORE it is + * rethrown, so chatCore's catch block (and, through the response body, the combo + * provider-breaker predicate) can classify it as "proxy unreachable" instead of + * falling through to a generic 502 that never trips the whole-provider breaker on a + * homogeneous same-provider combo pool. No-op when the error isn't connect-shaped. + */ +function tagProxyUnreachable(err: T): T { + if (isProxyUnreachableError(err)) { + const e = err as Error & { code?: string; errorCode?: string }; + e.code = e.code || "PROXY_UNREACHABLE"; + e.errorCode = "proxy_unreachable"; + } + return err; +} + /** Per-request tracking of whether TLS fingerprint was used */ type TlsFingerprintStore = { used: boolean }; const tlsFingerprintContext = new AsyncLocalStorage(); @@ -530,7 +569,7 @@ async function patchedFetch( if (dispatcherError instanceof Error) { (dispatcherError as Error & { proxyFetchDetail?: string }).proxyFetchDetail = detail; } - throw dispatcherError; + throw tagProxyUnreachable(dispatcherError); } // All attempts exhausted — try proxy fallback before native fetch @@ -573,7 +612,7 @@ async function patchedFetch( if (nativeError instanceof Error) { (nativeError as Error & { proxyFetchDetail?: string }).proxyFetchDetail = detail; } - throw nativeError; + throw tagProxyUnreachable(nativeError); } } throw dispatcherError; diff --git a/tests/unit/8376-econnrefused-breaker.test.ts b/tests/unit/8376-econnrefused-breaker.test.ts new file mode 100644 index 0000000000..6d99ae4e39 --- /dev/null +++ b/tests/unit/8376-econnrefused-breaker.test.ts @@ -0,0 +1,96 @@ +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { shouldRecordProviderBreakerFailure } from "../../open-sse/services/combo/comboPredicates.ts"; + +// #8376 — an unreachable upstream proxy (ECONNREFUSED) on a homogeneous same-provider +// combo pool must still trip the whole-provider circuit breaker so combo routing fails +// over to a different provider, instead of burning MAX_GLOBAL_ATTEMPTS against the same +// dead proxy and returning 503 "Maximum combo retry limit". + +test("#8376: proxy-unreachable failure on a homogeneous same-provider combo trips the breaker via isProxyUnreachable override", () => { + const result = shouldRecordProviderBreakerFailure({ + isStreamReadinessFailure: false, + status: 502, + sameProviderNext: true, + skipProviderBreaker: false, + requestScopedFailure: false, + error: "connect ECONNREFUSED 127.0.0.1:8787", + isProxyUnreachable: true, + }); + assert.equal(result, true); +}); + +test("#8376 control: without the override, the SAME same-provider failure still does not trip (proves the override is additive, not a blanket bypass)", () => { + const result = shouldRecordProviderBreakerFailure({ + isStreamReadinessFailure: false, + status: 502, + sameProviderNext: true, + skipProviderBreaker: false, + requestScopedFailure: false, + error: "connect ECONNREFUSED 127.0.0.1:8787", + isProxyUnreachable: false, + }); + assert.equal(result, false); +}); + +test("#8376: the override never bypasses the other AND-terms — a stream-readiness failure still does not trip even when isProxyUnreachable is true", () => { + const result = shouldRecordProviderBreakerFailure({ + isStreamReadinessFailure: true, + status: 502, + sameProviderNext: true, + skipProviderBreaker: false, + requestScopedFailure: false, + error: "connect ECONNREFUSED 127.0.0.1:8787", + isProxyUnreachable: true, + }); + assert.equal(result, false); +}); + +test("#8376: the override never bypasses skipProviderBreaker (embedded-service connection-cooldown-only hint) even when isProxyUnreachable is true", () => { + const result = shouldRecordProviderBreakerFailure({ + isStreamReadinessFailure: false, + status: 502, + sameProviderNext: true, + skipProviderBreaker: true, + requestScopedFailure: false, + error: "connect ECONNREFUSED 127.0.0.1:8787", + isProxyUnreachable: true, + }); + assert.equal(result, false); +}); + +test("#8376: a genuine same-provider 5xx (not proxy-unreachable) still does NOT trip the breaker — no over-widening", () => { + const result = shouldRecordProviderBreakerFailure({ + isStreamReadinessFailure: false, + status: 502, + sameProviderNext: true, + skipProviderBreaker: false, + requestScopedFailure: false, + error: "upstream returned 502", + }); + assert.equal(result, false); +}); + +test("#8376: a normal 200-derived non-breaker-status failure is unaffected by isProxyUnreachable being true (status gate still applies)", () => { + const result = shouldRecordProviderBreakerFailure({ + isStreamReadinessFailure: false, + status: 200, + sameProviderNext: true, + skipProviderBreaker: false, + requestScopedFailure: false, + isProxyUnreachable: true, + }); + assert.equal(result, false); +}); + +test("#8376: a normal 429 (rate limit) is unaffected by isProxyUnreachable being true (429 intentionally excluded from breaker statuses)", () => { + const result = shouldRecordProviderBreakerFailure({ + isStreamReadinessFailure: false, + status: 429, + sameProviderNext: true, + skipProviderBreaker: false, + requestScopedFailure: false, + isProxyUnreachable: true, + }); + assert.equal(result, false); +});