mirror of
https://github.com/diegosouzapw/OmniRoute.git
synced 2026-08-11 17:52:31 +03:00
fix(combo): network errors must not trip provider circuit breaker (#9342)
* fix(combo): keep queue/network timeouts out of the provider breaker A single-model network error (ECONNREFUSED / proxy_unreachable) means we never reached the provider — the provider may be healthy while only the network path is broken. OmniRoute's own rate-limit queue timeouts are backpressure we applied, not an upstream failure. Neither should trip the whole-provider breaker. - chatPredicates: the single-model path excludes proxy_unreachable and RATE_LIMIT_QUEUE_* from the provider-breaker trip. - accountFallback.recordProviderFailure: isQueueTimeout short-circuits before the breaker ever counts (combo.ts already flags it from errorText). - chat.ts: the queue/network guard on the allRateLimited _onFailure trip. Deliberately leaves the combo same-provider dead-proxy leg (#8376) intact: there a proxy_unreachable on the next same-provider target must still be able to open the breaker, or a dead proxy burns every attempt until the 503 max-retry limit. Signed-off-by: Minxi Hou <houminxi@gmail.com> * fix(resilience): dedup same-provider network errors per event Same-provider combo targets can all fail the same single network event (a VPN blip) within one request. Without a dedup each target counts once toward the provider breaker, so one transient blip opens the whole-provider breaker while the provider is healthy — the antigravity outage this branch originally chased. recordProviderFailure now keeps a short per-provider window (10s) for proxy_unreachable failures: the first network error in a window counts, the rest of that window are the same event and return. A genuinely dead proxy keeps failing across requests (past the window) and still accumulates to its threshold, so the #8376 dead-proxy protection is not weakened. Covered by tests/unit/breaker-network-error-guard.test.ts: same-window errors dedup to one, cross-window errors still open the breaker. Signed-off-by: Minxi Hou <houminxi@gmail.com> --------- Signed-off-by: Minxi Hou <houminxi@gmail.com>
This commit is contained in:
Bob.Hou
@@ -8,7 +8,7 @@
|
||||
"_rebaseline_2026_08_08_v3850_base_drift_batch_9757": "Base drift on release/v3.8.50, not own growth: the 08-06..08-08 merge batches grew 12 already-frozen (or newly-landed) files without carrying their rebaselines — the dedicated rebaseline PR #9616 was closed as 'superseded' but its file-size entries never actually reached the base, and later merges (#8894 combos page, #9539 EditConnectionModal, #8895 models route, #9294/#9293 catalog, #9541 db/core, #8970 tokenHealthCheck, #8925 mcp schemas+server, #8890 accountFallback, #9467 chat.ts, #8931 openai-to-kiro, ProxyRegistryManager) kept growing them. All 12 values re-measured on THIS branch's tree (= pure tip + this PR's 1-line chat.ts fix, which adds zero lines). This PR's own source changes (chat.ts identifier restore, stream.ts format carve-out) do not grow any frozen file past these values.",
|
||||
"_rebaseline_2026_08_08_migration_135_collision": "fix(db): resolve migration version 135 numbering collision — #9449's 135_connection_runtime_state.sql and #8908's 135_migrate_model_capability_max_token.sql both claimed version 135 (#9449 branched before #8908 merged and never got renumbered before landing on release/v3.8.50), which threw 'Migration version collision detected' the moment ANY code touched the database — a fresh install/deploy from this tip cannot even boot. Renumbered the later-landing file to 140 (next free slot) and added the matching isSchemaAlreadyApplied('140') retroactive guard, matching the established pattern already used for the prior 135/136 -> 137/138 renumber in the same file. Own growth: src/lib/db/migrationRunner.ts 1084->1094 (+10, the new case block) — irreducible, matches the existing per-case guard pattern exactly. Covered by tests/unit/migration-135-numbering-collision.test.ts (2/2), confirmed failing (reproducing the exact live crash) against the pre-fix colliding filenames, passing after.",
|
||||
"_rebaseline_2026_08_08_9183_reasoning_cache_index_sync": "Extracted fix(responses-api): sync reasoning-cache write index with the fixed read side (from the originally-authored #9183) — chatCore.ts's write side cached every response under a hardcoded messageIndex:0, and translator/index.ts's plain-turn (non-tool-call) cache-key lookup ALSO still hardcoded messageIndex 0 at its call site (a second, previously-undiscovered instance of the same hardcoding bug, found while re-verifying this fix against the current upstream tip — the two never agreed once a conversation went past its first assistant turn, so DeepSeek/Xiaomi-mimo plain-turn reasoning replay silently missed the cache). Own growth: open-sse/handlers/chatCore.ts 5034->5042 (+8, computing messageIndex from the incoming request's message count at both the streaming and non-streaming cache-write call sites) — irreducible call-site wiring. Covered by tests/unit/reasoning-cache.test.ts (new end-to-end write/read regression test, rebaselined below) and tests/unit/translator-helper-branches.test.ts fixture updates. Other #9183 sub-fixes (output_index collision prevention, reasoning-content-alias generalization) were originally assumed already superseded by upstream's own independent fix — a live incident 2026-08-08 disproved that for the message-vs-tool-call collision case specifically (fixed separately in #9822); not re-extracted here since this PR's own scope is the narrower messageIndex sync only.",
|
||||
"_rebaseline_2026_08_02_9259_rolling_rpm": "PR #9259 (issue #8733) own growth: open-sse/services/rateLimitManager.ts baseline 1060->1167 (+107; final source 1153). The existing withRateLimit chokepoint now composes process-local rolling RPM leases with Bottleneck admission, releases pre-dispatch leases on queue timeout/abort/connection disable, preserves caller abort reasons, and wires 429/header state into the extracted rollingRpmGate.ts. The remaining growth is irreducible lifecycle wiring at the dispatch boundary plus the real watchdog test hooks needed to verify queued-wedge recovery; moving it further would obscure lease ownership and Bottleneck cleanup. Covered by the focused rate-limit manager/sliding-window suite (33/33); distributed multi-instance coordination remains explicitly out of scope.",
|
||||
"_rebaseline_2026_08_09_9342_network_error_guard": "PR #9342 own growth during the 2026-08-09 rebase: open-sse/services/accountFallback.ts 1978->2008 (+30 = the isQueueTimeout short-circuit plus a per-provider network-error dedup window in recordProviderFailure, keeping one VPN blip from the same provider's combo targets counting once per target). Covered by tests/unit/breaker-network-error-guard.test.ts. (chat.ts stays base-red: upstream tip is already 1918 > frozen 1904, this PR only adds +12 on top; not re-bumped per the no-inherit-ratchet rule.)", "_rebaseline_2026_08_09_9296_adobe_media_capabilities": "PR #9296 (artickc, fix/adobe-firefly-model-capabilities) own growth: src/app/api/v1/models/catalog.ts 1590->1597 (+7). The image and video catalog serializers now expose the already-normalized Adobe Firefly discovery capability data (media_capabilities, plus the existing video modality/size fields) at their only response-emission chokepoints. The discovery parser and capability normalization remain in open-sse/services/adobeFireflyModels.ts; extracting these seven serialization fields would obscure the catalog contract. Covered by tests/unit/adobe-firefly.test.ts and tests/unit/image-upscale.test.ts.", "_rebaseline_2026_08_02_9259_rolling_rpm": "PR #9259 (issue #8733) own growth: open-sse/services/rateLimitManager.ts baseline 1060->1167 (+107; final source 1153). The existing withRateLimit chokepoint now composes process-local rolling RPM leases with Bottleneck admission, releases pre-dispatch leases on queue timeout/abort/connection disable, preserves caller abort reasons, and wires 429/header state into the extracted rollingRpmGate.ts. The remaining growth is irreducible lifecycle wiring at the dispatch boundary plus the real watchdog test hooks needed to verify queued-wedge recovery; moving it further would obscure lease ownership and Bottleneck cleanup. Covered by the focused rate-limit manager/sliding-window suite (33/33); distributed multi-instance coordination remains explicitly out of scope.",
|
||||
"_rebaseline_2026_07_24_8470_hyperagent_sticky_thread": "PR #8470 (artickc, fix/hyperagent-tool-loop-thread-sticky) own growth: open-sse/executors/hyperagent.ts 936->1025 (wc -l; check-file-size.mjs counts via split(\"\\n\").length so the gate sees 937->1026, +89, crosses the 1000 cap). Fixes a real bug where a reverse-conversion proxy (text-Intent/JSON to Claude Code native tool_calls) rewrites assistant messages between agentic tool-loop turns, breaking HyperAgent\u2019s conversation-prefix fingerprint and cold-starting the thread mid tool-loop. Adds Anthropic tool_use/tool_result flattening to extractMessageText() plus a new rootUserFingerprint()/root-key lookup tier in resolveHyperAgentThreadBinding()/storeHyperAgentThreadAfterTurn() so the thread stays sticky across the tool loop. Cohesive additions inside the existing single-file executor; not extractable without splitting the executor mid-request-flow. Covered by tests/unit/executor-hyperagent.test.ts (19/19, +5 new cases for tool_result/tool_use flattening + root-key stickiness). Pre-merge review flagged a cross-conversation root-key collision risk (tracked in the PR\u2019s own mandatory pre-merge checklist, not yet addressed) \u2014 unrelated to this file-size ratchet, tracked separately by /fix-prs.",
|
||||
"_rebaseline_2026_07_25_8494_capability_filter_fail_closed": "PR #8494 (fix/capability-filters-fail-closed, #8488) own growth: open-sse/services/combo.ts 3640->3693 (+53) adds a fail-closed guard after filterTargetsByRequestCompatibility() \u2014 when every eligible target is excluded by request-capability filtering (vision/tools/etc) instead of quota/health, the combo now returns an explicit `capability_mismatch` 400 (describeCapabilityFilterExhaustion, imported from combo/comboStructure.ts) rather than silently falling through to a generic no-targets error, plus a `compatFilterFailOpen` escape hatch (combo config OR settings) mirrored at both the main/auto and round-robin call sites for symmetry. combo/comboStructure.ts (previously under cap, un-frozen) grows 794->918 (+124) \u2014 new home for describeCapabilityFilterExhaustion + providerSupportsEmulatedToolCalling (#5240 emulated tool-calling exemption so fail-closed does not regress prompt-emulation-only combos like all-chatgpt-web). Irreducible orchestration wiring at the existing filter chokepoint (same precedent as #7301's universal-cooldown-retry generalization). Companion test tests/unit/combo-routing-engine.test.ts 3409->3449 (+40, fail-closed/fail-open coverage across both call sites) also rebaselined. Covered by tests/unit/8488-capability-filter-fail-closed.test.ts (new) + 95/95 passing across both files. Structural shrink of combo.ts tracked in #3501.",
|
||||
"_rebaseline_2026_07_25_8499_ts7_result_union_predicates": "PR #8499 (backryun, chore/ts7-types-executor-scattered) own growth: muse-spark-web.ts 1396->1405 (+9, irreducible). Under this workspace's `strictNullChecks: false`, the boolean-literal discriminant on `GraphqlResult` (`{ ok: true } | { ok: false; error: string }`) narrows the positive `.ok===true` branch but leaves `!result.ok` at the full union under TS7, making `.error` unreachable to the checker at the two call sites (warmup, mode-switch). Fixed by adding a single `isGraphqlFailure()` type-predicate helper (doc comment + 3-line body) reused at both call sites instead of duplicating the predicate inline \u2014 not extractable to a shared module without splitting a single-file executor's local narrowing helper out of its own file. Covered by the existing muse-spark-web executor test suite (no behavior change, pure narrowing fix).",
|
||||
|
||||
@@ -126,6 +126,15 @@ const CONNECTION_FAILURE_DEDUP_MS = 5000;
|
||||
const MAX_CONNECTION_FAILURE_DEDUP_ENTRIES = 10_000;
|
||||
const lastConnectionFailure = new Map<string, number>();
|
||||
|
||||
// Per-provider network-error dedup: several combo targets on the SAME provider can
|
||||
// fail the same single network event (a VPN blip) in the same request. Without this,
|
||||
// each target counts once and one transient blip opens the whole-provider breaker
|
||||
// while the provider is healthy. A genuinely dead proxy persists ACROSS requests
|
||||
// (past the window) and still accumulates to its threshold.
|
||||
const NETWORK_ERROR_DEDUP_MS = 10_000;
|
||||
const MAX_NETWORK_ERROR_DEDUP_ENTRIES = 1000;
|
||||
const lastNetworkErrorByProvider = new Map<string, number>();
|
||||
|
||||
function pruneConnectionFailureDedupeEntries(): void {
|
||||
while (lastConnectionFailure.size > MAX_CONNECTION_FAILURE_DEDUP_ENTRIES) {
|
||||
const oldestKey = lastConnectionFailure.keys().next().value;
|
||||
@@ -974,9 +983,30 @@ export function recordProviderFailure(
|
||||
provider: string | null | undefined,
|
||||
log?: { warn?: (...args: unknown[]) => void },
|
||||
connectionId?: string | null,
|
||||
profile?: ProviderBreakerProfile | null
|
||||
profile?: ProviderBreakerProfile | null,
|
||||
opts?: { isQueueTimeout?: boolean; isNetworkError?: boolean }
|
||||
): void {
|
||||
if (!provider) return;
|
||||
// OmniRoute's own rate-limit queue timeout is backpressure we applied, not a
|
||||
// provider failure — the provider never saw the request, so it must not count
|
||||
// toward the provider breaker.
|
||||
if (opts?.isQueueTimeout) return;
|
||||
|
||||
// Network-layer errors (proxy_unreachable) get a separate SAME-PROVIDER dedup, so a
|
||||
// single transient network event is not counted once per combo target (see the
|
||||
// declaration). A dead proxy persists across requests and still accumulates.
|
||||
if (opts?.isNetworkError) {
|
||||
const now = Date.now();
|
||||
const last = lastNetworkErrorByProvider.get(provider);
|
||||
if (last && now - last < NETWORK_ERROR_DEDUP_MS) return;
|
||||
lastNetworkErrorByProvider.delete(provider);
|
||||
lastNetworkErrorByProvider.set(provider, now);
|
||||
while (lastNetworkErrorByProvider.size > MAX_NETWORK_ERROR_DEDUP_ENTRIES) {
|
||||
const oldestKey = lastNetworkErrorByProvider.keys().next().value;
|
||||
if (typeof oldestKey !== "string") break;
|
||||
lastNetworkErrorByProvider.delete(oldestKey);
|
||||
}
|
||||
}
|
||||
|
||||
// Deduplicate rapid-fire failures from the same connection
|
||||
if (connectionId) {
|
||||
|
||||
@@ -1865,7 +1865,13 @@ export async function handleComboChat({
|
||||
isProxyUnreachable: structuredError?.code === "proxy_unreachable",
|
||||
})
|
||||
) {
|
||||
recordProviderFailure(provider, log, targetWithConnection.connectionId, profile);
|
||||
const isQueueTimeout =
|
||||
errorText.includes("RATE_LIMIT_QUEUE_TIMEOUT") ||
|
||||
errorText.includes("RATE_LIMIT_QUEUE_WEDGED");
|
||||
recordProviderFailure(provider, log, targetWithConnection.connectionId, profile, {
|
||||
isQueueTimeout,
|
||||
isNetworkError: structuredError?.code === "proxy_unreachable",
|
||||
});
|
||||
}
|
||||
|
||||
const quotaExhausted = await isQuotaExhaustionResponse(
|
||||
|
||||
@@ -1386,10 +1386,22 @@ async function handleSingleModelChat(
|
||||
}
|
||||
|
||||
const breakerFailureStatus = Number(lastStatus ?? credentials?.lastErrorCode);
|
||||
// lastError is a string here — check for the proxy_unreachable tag embedded by
|
||||
// tagProxyUnreachable (proxyFetch.ts) and OmniRoute's own queue timeouts. Both mean
|
||||
// we never reached the provider, so they must not trip the provider breaker.
|
||||
const isNetworkError =
|
||||
typeof lastError === "string" &&
|
||||
(lastError.includes("proxy_unreachable") || lastError.includes("PROXY_UNREACHABLE"));
|
||||
const isQueueTimeout =
|
||||
typeof lastError === "string" &&
|
||||
(lastError.includes("RATE_LIMIT_QUEUE_TIMEOUT") ||
|
||||
lastError.includes("RATE_LIMIT_QUEUE_WEDGED"));
|
||||
if (
|
||||
!forceLiveComboTest &&
|
||||
credentials?.allRateLimited &&
|
||||
isProviderBreakerFailureStatus(breakerFailureStatus)
|
||||
isProviderBreakerFailureStatus(breakerFailureStatus) &&
|
||||
!isNetworkError &&
|
||||
!isQueueTimeout
|
||||
) {
|
||||
breaker._onFailure();
|
||||
}
|
||||
|
||||
@@ -29,6 +29,12 @@ export function shouldTripProviderBreakerForResult(
|
||||
!isRequestScopedUpstreamFailure({ code: result.errorCode, type: result.errorType }) &&
|
||||
!(result.response && getTrustedLocalRateLimitResponse(result.response)) &&
|
||||
!isLocalStreamLifecycleError(result.error) &&
|
||||
// Network-layer errors (ECONNREFUSED, ETIMEDOUT) never reached the provider —
|
||||
// the provider may be healthy, only the network path is broken. OmniRoute's own
|
||||
// rate-limit queue timeouts are backpressure we applied, not a provider failure.
|
||||
result.errorCode !== "proxy_unreachable" &&
|
||||
result.errorCode !== "RATE_LIMIT_QUEUE_TIMEOUT" &&
|
||||
result.errorCode !== "RATE_LIMIT_QUEUE_WEDGED" &&
|
||||
PROVIDER_BREAKER_FAILURE_STATUSES.has(Number(result.status))
|
||||
);
|
||||
}
|
||||
|
||||
139
tests/unit/breaker-network-error-guard.test.ts
Normal file
139
tests/unit/breaker-network-error-guard.test.ts
Normal file
@@ -0,0 +1,139 @@
|
||||
import { test } from "node:test";
|
||||
import assert from "node:assert/strict";
|
||||
import { shouldTripProviderBreakerForResult } from "../../src/sse/handlers/chatPredicates.ts";
|
||||
import {
|
||||
recordProviderFailure,
|
||||
clearProviderFailure,
|
||||
isProviderInCooldown,
|
||||
} from "../../open-sse/services/accountFallback.ts";
|
||||
import { PROVIDER_PROFILES } from "../../open-sse/config/constants.ts";
|
||||
|
||||
// Network-layer errors and OmniRoute's own queue timeouts must NOT trip the
|
||||
// provider circuit breaker. These are not provider failures — the provider never
|
||||
// saw the request, so it may be perfectly healthy while only the network path is
|
||||
// broken (single-model path; the combo same-provider dead-proxy case is #8376's
|
||||
// contract and stays untouched).
|
||||
test("proxy_unreachable errorCode does NOT trip provider breaker", () => {
|
||||
const result = shouldTripProviderBreakerForResult(
|
||||
{ status: 502, errorCode: "proxy_unreachable", errorType: null, error: "ECONNREFUSED" },
|
||||
false,
|
||||
false
|
||||
);
|
||||
assert.equal(result, false);
|
||||
});
|
||||
test("RATE_LIMIT_QUEUE_TIMEOUT errorCode does NOT trip provider breaker", () => {
|
||||
const result = shouldTripProviderBreakerForResult(
|
||||
{ status: 503, errorCode: "RATE_LIMIT_QUEUE_TIMEOUT", errorType: null, error: "queue expired" },
|
||||
false,
|
||||
false
|
||||
);
|
||||
assert.equal(result, false);
|
||||
});
|
||||
test("RATE_LIMIT_QUEUE_WEDGED errorCode does NOT trip provider breaker", () => {
|
||||
const result = shouldTripProviderBreakerForResult(
|
||||
{ status: 503, errorCode: "RATE_LIMIT_QUEUE_WEDGED", errorType: null, error: "limiter wedged" },
|
||||
false,
|
||||
false
|
||||
);
|
||||
assert.equal(result, false);
|
||||
});
|
||||
test("genuine 502 without proxy_unreachable DOES trip provider breaker", () => {
|
||||
const result = shouldTripProviderBreakerForResult(
|
||||
{ status: 502, errorCode: null, errorType: null, error: "upstream error" },
|
||||
false,
|
||||
false
|
||||
);
|
||||
assert.equal(result, true);
|
||||
});
|
||||
test("genuine 503 without queue timeout DOES trip provider breaker", () => {
|
||||
const result = shouldTripProviderBreakerForResult(
|
||||
{ status: 503, errorCode: null, errorType: null, error: "service unavailable" },
|
||||
false,
|
||||
false
|
||||
);
|
||||
assert.equal(result, true);
|
||||
});
|
||||
test("isCombo=true prevents breaker trip regardless of error", () => {
|
||||
const result = shouldTripProviderBreakerForResult(
|
||||
{ status: 502, errorCode: null, errorType: null, error: "upstream error" },
|
||||
true,
|
||||
false
|
||||
);
|
||||
assert.equal(result, false);
|
||||
});
|
||||
test("forceLiveComboTest=true prevents breaker trip (combo will try next target)", () => {
|
||||
const result = shouldTripProviderBreakerForResult(
|
||||
{ status: 502, errorCode: null, errorType: null, error: "upstream error" },
|
||||
false,
|
||||
true
|
||||
);
|
||||
assert.equal(result, false);
|
||||
});
|
||||
|
||||
test("queue-timeout recordProviderFailure never opens the provider breaker", () => {
|
||||
// Control first: that many real failures WOULD open the breaker — proving the
|
||||
// isQueueTimeout flag, not an inert provider, is what keeps it closed.
|
||||
const control = "test-qt-control-provider";
|
||||
clearProviderFailure(control);
|
||||
const threshold = PROVIDER_PROFILES.apikey.circuitBreakerThreshold;
|
||||
for (let i = 0; i < threshold; i++) {
|
||||
recordProviderFailure(control, undefined, undefined, null, {});
|
||||
}
|
||||
assert.equal(isProviderInCooldown(control), true, "sanity: real failures open the breaker");
|
||||
|
||||
// The queue-timeout path must never reach the breaker, no matter how many fire.
|
||||
const provider = "test-qt-provider";
|
||||
clearProviderFailure(provider);
|
||||
for (let i = 0; i < threshold; i++) {
|
||||
recordProviderFailure(provider, undefined, undefined, null, { isQueueTimeout: true });
|
||||
}
|
||||
assert.equal(isProviderInCooldown(provider), false, "queue timeouts must not open the breaker");
|
||||
});
|
||||
|
||||
test("same-provider network errors in one window dedup to a single failure", () => {
|
||||
// Several combo targets on the same provider failing one network event (a VPN blip)
|
||||
// must count once, not per target — otherwise one blip opens the provider breaker.
|
||||
const originalNow = Date.now;
|
||||
let now = 1_700_000_000_000;
|
||||
Date.now = () => now;
|
||||
try {
|
||||
const provider = "test-net-dedup-provider";
|
||||
clearProviderFailure(provider);
|
||||
const threshold = PROVIDER_PROFILES.apikey.circuitBreakerThreshold;
|
||||
for (let i = 0; i < threshold; i++) {
|
||||
recordProviderFailure(provider, undefined, undefined, null, { isNetworkError: true });
|
||||
now += 500; // every call inside the same 10s window
|
||||
}
|
||||
assert.equal(
|
||||
isProviderInCooldown(provider),
|
||||
false,
|
||||
"one transient network event must not open the breaker"
|
||||
);
|
||||
} finally {
|
||||
Date.now = originalNow;
|
||||
}
|
||||
});
|
||||
|
||||
test("persistent dead proxy across windows still opens the breaker", () => {
|
||||
// A genuinely dead proxy keeps failing across requests (past the dedup window), so it
|
||||
// must still accumulate to the breaker threshold — the dedup must not shield real pain.
|
||||
const originalNow = Date.now;
|
||||
let now = 1_700_000_000_000;
|
||||
Date.now = () => now;
|
||||
try {
|
||||
const provider = "test-net-deadproxy-provider";
|
||||
clearProviderFailure(provider);
|
||||
const threshold = PROVIDER_PROFILES.apikey.circuitBreakerThreshold;
|
||||
for (let i = 0; i < threshold; i++) {
|
||||
recordProviderFailure(provider, undefined, undefined, null, { isNetworkError: true });
|
||||
now += 11_000; // past each 10s window
|
||||
}
|
||||
assert.equal(
|
||||
isProviderInCooldown(provider),
|
||||
true,
|
||||
"a persistent network failure must still accumulate to the threshold"
|
||||
);
|
||||
} finally {
|
||||
Date.now = originalNow;
|
||||
}
|
||||
});
|
||||
Reference in New Issue
Block a user