From 2230fbbe9353062c5c502b0a1f0b1fb53c0b87f4 Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza Date: Tue, 18 Aug 2026 10:51:16 -0300 Subject: [PATCH] fix(resilience): keep combo quality and auth reasons separate and redact connection labels in terminal errors (#10314) (#10501) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(resilience): keep combo quality and auth reasons separate and redact connection labels in terminal errors (#10314) * fix(resilience): sanitize identifiers in error text, add explicit terminal-status policy, fix classifier ordering (#10314) Four gaps in the prior combo-error-aggregation fix: - formatComboOutcomes() only redacted connection identifiers in the model label, never in the raw upstream error TEXT — a proxy echoing a connection/account id back in its error body leaked it into the client-facing terminal message. Redact both. - The terminal HTTP status was still `lastStatus` — whichever target happened to fail last, independent of the other targets' reasons. Add resolveComboTerminalStatus(): preserve a 4xx only when every eligible target's failure is genuinely "the request is invalid" (model-class); a heterogeneous mix (e.g. a quality failure + a sibling's 401) now normalizes to a 5xx-class status reflecting an infra/provider problem, never a misleading client error borrowed from an unrelated target. - classifyComboOutcome()'s ordering had `status === 408 || status >= 499` checked before `status >= 500`, making the provider branch permanently unreachable — every real 5xx (500/502/503/504) was silently mislabeled as "timeout". Fixed to an exact match (408/499) and gave 429 its own explicit `rate_limit` kind instead of falling into the generic "model" (request-invalid) bucket by accident. - Added an integration-level regression driving the real handleComboChat wiring end-to-end (quality failure + sibling 401, and a success-after- quality-failure case), not just the pure aggregation helpers. Updated three pre-existing tests whose assertions encoded the OLD last-writer-wins contract this fix intentionally supersedes (#8486 Part B antigravity retryAfter tests, two combo-routing-engine status/message tests) to the new, more precise contract; verified the underlying #8486 concern (wrong target's retryAfter header) is still honored under the new status policy. --------- Co-authored-by: adevwithpurpose --- .../fixes/10314-combo-error-aggregation.md | 1 + open-sse/services/combo.ts | 82 +++++--- .../services/combo/comboErrorAggregation.ts | 179 ++++++++++++++++++ ...gravity-missing-project-reset-8486.test.ts | 50 +++-- tests/unit/combo-error-aggregation.test.ts | 165 ++++++++++++++++ ...mbo-quota-exhaustion-only-fallback.test.ts | 21 +- tests/unit/combo-routing-engine.test.ts | 36 +++- ...combo-terminal-status-policy-10501.test.ts | 123 ++++++++++++ 8 files changed, 605 insertions(+), 52 deletions(-) create mode 100644 changelog.d/fixes/10314-combo-error-aggregation.md create mode 100644 open-sse/services/combo/comboErrorAggregation.ts create mode 100644 tests/unit/combo-error-aggregation.test.ts create mode 100644 tests/unit/combo-terminal-status-policy-10501.test.ts diff --git a/changelog.d/fixes/10314-combo-error-aggregation.md b/changelog.d/fixes/10314-combo-error-aggregation.md new file mode 100644 index 0000000000..7dd3ef6a60 --- /dev/null +++ b/changelog.d/fixes/10314-combo-error-aggregation.md @@ -0,0 +1 @@ +- fix(resilience): keep combo quality and auth failure reasons separate and redact connection labels in terminal errors (#10314) diff --git a/open-sse/services/combo.ts b/open-sse/services/combo.ts index f8b2333f46..a1c15e5022 100644 --- a/open-sse/services/combo.ts +++ b/open-sse/services/combo.ts @@ -115,6 +115,14 @@ import { expandPromptCacheAffinityTargetsFromConnections, resolvePromptCacheAffinityKey, } from "./combo/promptCacheAffinity.ts"; +import { + classifyComboOutcome, + formatComboOutcomes, + redactConnectionLabel, + buildRedactedSummary, + resolveComboTerminalStatus, +} from "./combo/comboErrorAggregation.ts"; +import type { ComboErrorEntry } from "./combo/comboErrorAggregation.ts"; import type { CompressionMode } from "./compression/types.ts"; import { getCachedProviderConnections } from "../../src/lib/db/readCache"; import { isProviderInCooldown, recordProviderCooldown } from "./providerCooldownTracker.ts"; @@ -875,7 +883,7 @@ export async function handleComboChat({ let comboExpired = false; // Accumulator for per-model error details across targets in the current set try. // Reset at the start of each set retry (same lifecycle as lastError/recordedAttempts). - let comboErrors: Array<{ model: string; status: number; error: string }> = []; + let comboErrors: Array = []; // Quota trust spans set retries and recursive cooldown re-dispatches. Once any // failure is non-quota, a nested caller must never treat this dispatch as quota-only. let observedFailure = false; @@ -1387,6 +1395,15 @@ export async function handleComboChat({ // misleading ALL_ACCOUNTS_INACTIVE when the real issue is quality. lastError = `Upstream response failed quality validation: ${quality.reason}`; lastStatus = 502; + // #10314: record quality failures as a FIRST-CLASS per-target outcome + // so a quality reason is never silently dropped from the aggregated + // terminal message when a later sibling overwrites lastError. + comboErrors.push({ + model: modelStr, + status: 502, + error: quality.reason || "upstream response failed quality validation", + kind: "quality", + }); if (i > 0) fallbackCount++; if (provider && rawModel) { const mlSettings = resolveModelLockoutSettings(settings); @@ -1894,6 +1911,7 @@ export async function handleComboChat({ model: modelStr, status: result.status, error: errorText || String(result.status), + kind: classifyComboOutcome(result.status, errorText), }); lastStatus = result.status; if (i > 0) fallbackCount++; @@ -2087,6 +2105,7 @@ export async function handleComboChat({ model: modelStr, status: result.status, error: errorText || String(result.status), + kind: classifyComboOutcome(result.status, errorText), }); lastStatus = result.status; if (i > 0) fallbackCount++; @@ -2241,15 +2260,10 @@ export async function handleComboChat({ // Global combo timeout: return aggregated error immediately, skipping set retries. if (comboExpired) { - const summary = comboErrors - .slice(0, 5) - .map((e) => `${e.model} (${e.status})`) - .join(", "); + const summary = buildRedactedSummary(comboErrors); const msg = `Combo global timeout (${comboTimeoutMs}ms) after ${recordedAttempts}/${orderedTargets.length} targets` + - (comboErrors.length > 0 - ? ` | tried: ${summary}${comboErrors.length > 5 ? `... (+${comboErrors.length - 5})` : ""}` - : ""); + (comboErrors.length > 0 ? ` | tried: ${summary}` : ""); const latencyMs = Date.now() - startTime; if (recordedAttempts === 0) { recordComboRequest(combo.name, null, { @@ -2319,19 +2333,20 @@ export async function handleComboChat({ ); } - const status = lastStatus; - // Build aggregated error message with per-model failure details for diagnostics. - const comboErrorSummary = - comboErrors.length > 0 - ? " [" + - comboErrors - .slice(0, 5) - .map((e) => `${e.model} (${e.status})`) - .join(", ") + - (comboErrors.length > 5 ? `... (+${comboErrors.length - 5})` : "") + - "]" - : ""; - const msg = (lastError || "All combo models unavailable") + comboErrorSummary; + // #10501: derive the terminal HTTP status from the structured per-target + // outcomes instead of `lastStatus` (whichever target happened to fail + // LAST). A 4xx is preserved only when the request itself is genuinely + // invalid across every eligible target; a heterogeneous mix of failure + // classes (e.g. a quality failure + a sibling's 401) normalizes to a + // 5xx-class status reflecting an infra/provider problem, not a client + // error. See comboErrorAggregation.ts::resolveComboTerminalStatus. + const status = resolveComboTerminalStatus(comboErrors, lastStatus); + // #10314: build the terminal message from the structured per-target + // outcomes (each distinct class+reason listed separately) instead of + // mashing a single lastError with raw `[model (status)]` markers. Connection + // identifiers are redacted. Falls back to lastError when no target recorded + // a structured outcome. + const msg = formatComboOutcomes(comboErrors) || lastError || "All combo models unavailable"; // Cooldown-aware retry: instead of crystallizing a transient failure, wait // out a SHORT cooldown and re-run the whole set loop. Guarded by the helper @@ -2759,6 +2774,10 @@ async function handleRoundRobinCombo({ let globalAttempts = 0; let fallbackCount = 0; let recordedAttempts = 0; + // #10314: per-target outcome accumulator for the round-robin twin so the + // terminal message lists each distinct reason separately (see the quality path + // and the "Done with this model" path below), mirroring handleComboChat. + const rrOutcomes: Array = []; // #1731: Per-request in-memory set of providers whose quota is fully exhausted. // When a target returns a quota-exhausted 429, remaining targets from the same @@ -2974,6 +2993,12 @@ async function handleRoundRobinCombo({ // misleading ALL_ACCOUNTS_INACTIVE when the real issue is quality. lastError = `Upstream response failed quality validation: ${quality.reason}`; lastStatus = 502; + rrOutcomes.push({ + model: modelStr, + status: 502, + error: quality.reason || "upstream response failed quality validation", + kind: "quality", + }); if (offset > 0) fallbackCount++; break; // move to next model } @@ -3280,6 +3305,12 @@ async function handleRoundRobinCombo({ recordedAttempts++; lastError = errorText || String(result.status); lastStatus = result.status; + rrOutcomes.push({ + model: modelStr, + status: result.status, + error: errorText || String(result.status), + kind: classifyComboOutcome(result.status, errorText), + }); if (offset > 0) fallbackCount++; log.warn("COMBO-RR", `${modelStr} failed, trying next model`, { status: result.status }); @@ -3399,8 +3430,13 @@ async function handleRoundRobinCombo({ ); } - const status = lastStatus; - const msg = lastError || "All round-robin combo models unavailable"; + // #10501: same terminal-status policy as handleComboChat — see + // comboErrorAggregation.ts::resolveComboTerminalStatus. + const status = resolveComboTerminalStatus(rrOutcomes, lastStatus); + // #10314: same structured per-target aggregation as handleComboChat — list each + // distinct reason separately (redacted), fall back to lastError when no outcome. + const msg = + formatComboOutcomes(rrOutcomes) || lastError || "All round-robin combo models unavailable"; if (earliestRetryAfter && isRetryAfterEligibleStatus(status)) { const retryHuman = formatRetryAfter(toRetryAfterDisplayValue(earliestRetryAfter)); diff --git a/open-sse/services/combo/comboErrorAggregation.ts b/open-sse/services/combo/comboErrorAggregation.ts new file mode 100644 index 0000000000..c7b80fdad4 --- /dev/null +++ b/open-sse/services/combo/comboErrorAggregation.ts @@ -0,0 +1,179 @@ +/** + * Shared combo terminal-error aggregation. + * + * #10314 — combo error aggregation mixes quality and auth. Prior to this module + * the combo terminal message was built as a single `lastError` string (last + * writer wins — it can only ever represent ONE target's reason) concatenated + * with a raw `[model (status)]` suffix. A quality-failure reason from one + * target and a sibling's 401 were collapsed into one client-facing sentence + * (`invalid_api_key [openai/proxy-account-b (401)]`) and a quality reason that + * was not the final failing target was dropped entirely. + * + * This module gives each per-target failure a structured {model, status, error, + * kind} entry, so the terminal message can list every distinct reason + * separately (and classification-labelled) instead of mashing them, and it + * redacts connection/account identifiers that, on openai-compatible proxy + * connections, used to surface verbatim in client-visible and shared-warn + * strings (ops/PII leak). + */ + +export type ComboOutcomeKind = + | "quality" + | "auth" + | "rate_limit" + | "model" + | "provider" + | "timeout" + | "skipped" + | "upstream"; + +export interface ComboErrorEntry { + model: string; + status: number; + error: string; + kind: ComboOutcomeKind; +} + +const KIND_LABELS: Record = { + quality: "quality validation", + auth: "auth", + rate_limit: "rate limit", + model: "model", + provider: "provider", + timeout: "timeout", + skipped: "skipped", + upstream: "upstream", +}; + +/** + * Classify a single target's terminal outcome for the client-facing message. + * Auth-class errors (401/403 or auth-sounding text) are kept distinct from + * model-class (400/422) and provider-class (5xx) so a sibling's 401 is never + * presented as "quality failed". Fall through to `model` for everything else. + * + * #10501: the ordering below is deliberate and load-bearing — the timeout + * check MUST use an exact match (408 / 499), never `status >= 499`. A `>=` + * comparison there swallows every 5xx status too (500 >= 499), which made the + * `status >= 500` branch permanently unreachable and silently mislabeled every + * real provider outage (500/502/503/504) as a client-side "timeout". 429 is + * also given its own explicit branch: a rate-limit/quota signal is neither a + * "the client's request is invalid" (`model`) nor a hard provider outage, and + * lumping it into `model` would make `resolveComboTerminalStatus` treat a + * heterogeneous 429 mix as a genuinely-invalid-request case by accident. + */ +export function classifyComboOutcome(status: number, errorText: string): ComboOutcomeKind { + const text = typeof errorText === "string" ? errorText : ""; + if ( + status === 401 || + status === 403 || + /(invalid.?api.?key|unauthorized|not.?authorized|auth(entication|orization)?)/i.test(text) + ) { + return "auth"; + } + if (status === 429) return "rate_limit"; + if (status === 408 || status === 499) return "timeout"; + if (status >= 500) return "provider"; + return "model"; +} + +/** + * Redact connection/account identifiers that can ride inside a proxy target's + * model string (openai-compatible proxy model names often carry a connection + * label). UUIDs and long hex hashes are truncated to a short `conn:` prefix. + * Provider/model names operators need for debugging are left intact. + */ +export function redactConnectionLabel(modelStr: string | null | undefined): string { + const label = typeof modelStr === "string" && modelStr ? modelStr : "unknown"; + return label + .replace( + /\b[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}\b/g, + (m) => `conn:${m.slice(0, 8)}` + ) + .replace(/\b[0-9a-fA-F]{16,}\b/g, (m) => `conn:${m.slice(0, 8)}`); +} + +/** Build the redacted, collision-free `model (status)` summary used by the + * global-combo-timeout diagnostics path. */ +export function buildRedactedSummary( + entries: Array<{ model: string; status: number }> | ReadonlyArray<{ model: string; status: number }> +): string { + const slice = entries.slice(0, 5); + const parts = slice.map((e) => `${redactConnectionLabel(e.model)} (${e.status})`).join(", "); + return entries.length > 5 ? `${parts}... (+${entries.length - 5})` : parts; +} + +/** + * Format per-target terminal outcomes into one client-facing sentence that keeps + * every distinct reason separate (and classification-labelled) instead of + * mashing a single `lastError` with raw status markers. Always redacts + * connection identifiers unless `{ redact: false }` is explicitly passed. + */ +export function formatComboOutcomes( + entries: ReadonlyArray<{ model: string; status: number; error: string; kind?: ComboOutcomeKind }>, + opts?: { redact?: boolean } +): string { + if (!entries.length) return ""; + const redact = opts?.redact !== false; + const slice = entries.slice(0, 5); + const parts = slice.map((e) => { + const label = redact ? redactConnectionLabel(e.model) : e.model; + const kind = e.kind ? KIND_LABELS[e.kind] ?? e.kind : null; + // #10501: the raw upstream error TEXT can itself carry a connection/account + // identifier (some openai-compatible proxies echo it back in the error body, + // e.g. "invalid key for connection ") — redact it here too, not just + // the model label above, or the identifier leaks into the client-facing + // terminal message regardless of the label redaction. + const rawReason = e.error || `HTTP ${e.status}`; + const reason = redact ? redactConnectionLabel(rawReason) : rawReason; + const statusTxt = ` (HTTP ${e.status})`; + return kind ? `${label}: ${kind} — ${reason}${statusTxt}` : `${label}: ${reason}${statusTxt}`; + }); + return entries.length > 5 + ? `${parts.join("; ")}... (+${entries.length - 5} more)` + : parts.join("; "); +} + +/** + * #10501: explicit terminal-status policy for heterogeneous combo target + * exhaustion. Prior behavior returned `lastStatus` — whichever target + * happened to fail LAST, independent of what the other targets failed with. + * That let an unrelated target's config-class 4xx (or a target's own auth + * failure) masquerade as the combo's overall verdict, and vice versa. + * + * Policy: + * - No structured entries: keep the caller's fallback status unchanged. + * - Every entry is `model`-class AND a genuine 4xx (the request itself is + * invalid on EVERY eligible target, homogeneous or not): preserve that + * 4xx — this is a real client-request error, not an infra problem. + * - All entries share the SAME kind (any kind, e.g. every target failed + * with `auth`, or every target was `rate_limit`): preserve that shared + * class's own status — a uniform reason across all targets is still a + * single, well-defined verdict. + * - Otherwise (a genuine MIX of different failure classes — e.g. a quality + * failure on one target and a 401 on a sibling): this is heterogeneous by + * definition, so it is normalized to a 5xx-class infra/provider status + * instead of surfacing whichever target's status happened to be recorded + * last. `timeout` present anywhere in the mix maps to 504 (Gateway + * Timeout); otherwise 502 (Bad Gateway) — combo routing itself is the + * "gateway" that could not complete the request via any target. + */ +export function resolveComboTerminalStatus( + entries: ReadonlyArray, + fallbackStatus: number +): number { + if (!entries.length) return fallbackStatus; + + const allGenuinelyInvalidRequest = entries.every( + (e) => e.kind === "model" && e.status >= 400 && e.status < 500 + ); + if (allGenuinelyInvalidRequest) { + return entries[entries.length - 1].status; + } + + const distinctKinds = new Set(entries.map((e) => e.kind)); + if (distinctKinds.size === 1) { + return entries[entries.length - 1].status; + } + + return entries.some((e) => e.kind === "timeout") ? 504 : 502; +} \ No newline at end of file diff --git a/tests/unit/combo-antigravity-missing-project-reset-8486.test.ts b/tests/unit/combo-antigravity-missing-project-reset-8486.test.ts index b645d309ac..f3fcf9ca97 100644 --- a/tests/unit/combo-antigravity-missing-project-reset-8486.test.ts +++ b/tests/unit/combo-antigravity-missing-project-reset-8486.test.ts @@ -66,7 +66,18 @@ async function runScenario(models: string[]) { return { result, modelsCalled }; } -test("#8486 Part B: combo unavailableResponse must not attach an unrelated target's long retryAfter to the antigravity missing-projectId 422", async () => { +// #10314/#10501 superseded the original "one target's message wins, silently +// drops the sibling's reason" contract these two tests pinned: combo terminal +// aggregation now DELIBERATELY lists every distinct per-target reason (#10314) +// and normalizes a heterogeneous failure mix to a 5xx-class status instead of a +// bare `lastStatus` (#10501 — see comboErrorAggregation.ts::resolveComboTerminalStatus). +// The underlying #8486 concern — a config-class error getting the WRONG target's +// long retry-after window stitched onto it — is still the thing under test, just +// verified against the new contract: the response's `Retry-After` HEADER (the +// actual out-of-band decoration #8486 was about) must never carry the unrelated +// 21h47m window, in EITHER attempt order, regardless of which reasons appear in +// the (now intentionally multi-reason) message body. +test("#8486 Part B: heterogeneous rate_limit+config-class antigravity failure never attaches the unrelated 21h47m retryAfter as a response header", async () => { const { result, modelsCalled } = await runScenario([ "antigravity/account-a-model", "antigravity/account-b-model", @@ -78,17 +89,24 @@ test("#8486 Part B: combo unavailableResponse must not attach an unrelated targe `expected both targets to be tried, got: ${JSON.stringify(modelsCalled)}` ); - const text = await result.clone().text(); - - assert.ok( - !/reset after/i.test(text) || !/missing google projectid/i.test(text), - "a config-class antigravity error (missing_project_id, no retryAfter of its own) " + - "must not be decorated with an unrelated target's long retry-after window — " + - `got body: ${text}` + // #10501: neither target's failure alone proves the CLIENT's request was + // invalid (one is a rate limit, the other a config/auth problem) — the + // heterogeneous mix must normalize to a 5xx infra/provider status. + assert.equal(result.status, 502); + assert.equal( + result.headers.get("Retry-After"), + null, + "the config-class 422 (no retryAfter of its own) must never end up decorated " + + "with account-a's unrelated 21h47m retry-after header" ); + + // #10314: both distinct reasons are now surfaced (never silently dropped). + const text = await result.clone().text(); + assert.match(text, /reset after 21h47m32s/i); + assert.match(text, /missing google projectid/i); }); -test("#8486 Part B (reverse order): the config-class 422 must not swallow a genuinely rate-limited sibling's message either", async () => { +test("#8486 Part B (reverse order): same result independent of which target failed first — both reasons present, no bogus Retry-After header", async () => { const { result, modelsCalled } = await runScenario([ "antigravity/account-b-model", "antigravity/account-a-model", @@ -100,14 +118,10 @@ test("#8486 Part B (reverse order): the config-class 422 must not swallow a genu `expected both targets to be tried, got: ${JSON.stringify(modelsCalled)}` ); - const text = await result.clone().text(); + assert.equal(result.status, 502, "attempt order must not change the terminal status"); + assert.equal(result.headers.get("Retry-After"), null); - // The surfaced status/message pair must always originate from the SAME - // (last-attempted) target: here that's account-a (429, real retryAfter), - // so the response must carry ITS message and MAY carry its own retry-after - // — but must never resurrect the unrelated account-b 422 text alongside it. - assert.ok( - !/missing google projectid/i.test(text), - `expected the last target's (account-a, 429) own message, not the unrelated account-b 422 text — got body: ${text}` - ); + const text = await result.clone().text(); + assert.match(text, /reset after 21h47m32s/i); + assert.match(text, /missing google projectid/i); }); diff --git a/tests/unit/combo-error-aggregation.test.ts b/tests/unit/combo-error-aggregation.test.ts new file mode 100644 index 0000000000..c7f10fe8e9 --- /dev/null +++ b/tests/unit/combo-error-aggregation.test.ts @@ -0,0 +1,165 @@ +import test from "node:test"; +import assert from "node:assert/strict"; +import { + classifyComboOutcome, + formatComboOutcomes, + redactConnectionLabel, + buildRedactedSummary, + resolveComboTerminalStatus, +} from "../../open-sse/services/combo/comboErrorAggregation.ts"; + +// #10314 — combo error aggregation mixes quality and auth. +// Regression guard for the pure aggregation helpers: a quality-failure reason from one +// target and a sibling's 401 must be presented as SEPARATE classified outcomes (never +// mashed into a single lastError), and account/connection identifiers must be redacted +// from client-visible and shared-warn strings. + +test("#10314: classifyComboOutcome keeps auth distinct from quality/model", () => { + assert.equal(classifyComboOutcome(401, "invalid_api_key"), "auth"); + assert.equal(classifyComboOutcome(403, "not authorized"), "auth"); + assert.equal(classifyComboOutcome(408, "timeout"), "timeout"); + assert.equal(classifyComboOutcome(400, "bad request"), "model"); +}); + +// #10501: the classifier's ordering used to have `status === 408 || status >= 499` +// checked BEFORE `status >= 500` — since 500 >= 499, that made the `provider` +// branch unreachable for every real 5xx status (500/502/503/504), silently +// mislabeling every provider outage as a client-side "timeout". These cases +// pin the corrected, intentional mapping for the exact statuses call out in +// the fix: 499 (client-abort convention), 408 (request timeout), 429 (rate +// limit/quota — its own class, not lumped into `model`), and the 5xx family. +test("#10501: classifyComboOutcome — 499/408 are timeout, 429 is rate_limit, 5xx is provider (not timeout)", () => { + assert.equal(classifyComboOutcome(499, "client closed request"), "timeout"); + assert.equal(classifyComboOutcome(408, "request timeout"), "timeout"); + assert.equal(classifyComboOutcome(429, "rate limited"), "rate_limit"); + assert.equal(classifyComboOutcome(500, "internal server error"), "provider"); + assert.equal(classifyComboOutcome(502, "bad gateway"), "provider"); + assert.equal(classifyComboOutcome(503, "upstream unavailable"), "provider"); + assert.equal(classifyComboOutcome(504, "gateway timeout"), "provider"); +}); + +test("#10314: formatComboOutcomes lists quality and auth reasons SEPARATELY (both visible)", () => { + const msg = formatComboOutcomes([ + { model: "openai/model-quality", status: 502, error: "response failed quality validation", kind: "quality" }, + { model: "openai/proxy-account-b", status: 401, error: "invalid_api_key", kind: "auth" }, + ]); + assert.match(msg, /quality validation/); + assert.match(msg, /invalid_api_key/); + assert.match(msg, /auth/); + assert.ok(msg.indexOf("quality validation") < msg.indexOf("invalid_api_key")); +}); + +test("#10314: redactConnectionLabel masks connection/account identifiers", () => { + assert.equal( + redactConnectionLabel("openai/proxy-account-b"), + "openai/proxy-account-b" + ); + const withUuid = redactConnectionLabel("openai/8a4f0c6e-3b27-4c51-9d88-1f2a3b4c5d6e"); + assert.equal(withUuid, "openai/conn:8a4f0c6e"); + const withHex = redactConnectionLabel("openai/0f1e2d3c4b5a69788796170a1b2c3d4e5f607182"); + assert.equal(withHex, "openai/conn:0f1e2d3c"); +}); + +test("#10314: buildRedactedSummary is redacted and truncates past 5 entries", () => { + const s = buildRedactedSummary( + Array.from({ length: 6 }, (_, i) => ({ model: `openai/8a4f0c6e-3b27-4c51-9d88-1f2a3b4c5d6e-${i}`, status: 401 + i })) + ); + assert.ok(!s.includes("8a4f0c6e-3b27"), "summary must not leak a full UUID"); + assert.match(s, /conn:8a4f0c6e/); + assert.match(s, /\(\+1\)/); +}); + +// #10501: identifiers can ride inside the raw upstream ERROR TEXT too (some +// openai-compatible proxies echo the connection/account id back in the error +// body), not just the model label. formatComboOutcomes must redact BOTH. +test("#10501: formatComboOutcomes redacts a UUID embedded in the error TEXT, not just the model label", () => { + const msg = formatComboOutcomes([ + { + model: "openai/proxy-account-b", + status: 401, + error: "invalid key for connection 8a4f0c6e-3b27-4c51-9d88-1f2a3b4c5d6e", + kind: "auth", + }, + ]); + assert.ok(!msg.includes("8a4f0c6e-3b27-4c51-9d88-1f2a3b4c5d6e"), "must not leak the full UUID"); + assert.match(msg, /conn:8a4f0c6e/, "must redact the UUID inside the error reason text"); +}); + +test("#10501: formatComboOutcomes({redact:false}) intentionally leaves identifiers intact (internal/debug callers only)", () => { + const msg = formatComboOutcomes( + [ + { + model: "openai/proxy-account-b", + status: 401, + error: "invalid key for connection 8a4f0c6e-3b27-4c51-9d88-1f2a3b4c5d6e", + kind: "auth", + }, + ], + { redact: false } + ); + assert.ok(msg.includes("8a4f0c6e-3b27-4c51-9d88-1f2a3b4c5d6e")); +}); + +// #10501: explicit terminal-status policy for heterogeneous combo target +// exhaustion — see comboErrorAggregation.ts::resolveComboTerminalStatus header. +test("#10501: resolveComboTerminalStatus preserves 4xx only when EVERY target is a genuine request-invalid (model) failure", () => { + assert.equal( + resolveComboTerminalStatus( + [ + { model: "a", status: 400, error: "bad request", kind: "model" }, + { model: "b", status: 422, error: "unprocessable", kind: "model" }, + ], + 500 + ), + 422, + "all-model-class 4xx across every target must be preserved (the request really is invalid)" + ); +}); + +test("#10501: resolveComboTerminalStatus preserves a homogeneous non-model status (every target failed the SAME way)", () => { + assert.equal( + resolveComboTerminalStatus( + [ + { model: "a", status: 401, error: "invalid_api_key", kind: "auth" }, + { model: "b", status: 401, error: "invalid_api_key", kind: "auth" }, + ], + 500 + ), + 401, + "every target failing with the identical auth reason is still a well-defined single verdict" + ); +}); + +test("#10501: resolveComboTerminalStatus normalizes a heterogeneous mix (quality + auth) to 5xx, never a bare lastStatus 401", () => { + const status = resolveComboTerminalStatus( + [ + { + model: "a", + status: 502, + error: "response failed quality validation", + kind: "quality", + }, + { model: "b", status: 401, error: "invalid_api_key", kind: "auth" }, + ], + 401 // lastStatus — the OLD behavior would have surfaced this bare 401 + ); + assert.ok( + status >= 500, + `heterogeneous quality+auth exhaustion must surface an infra/provider 5xx, got ${status}` + ); +}); + +test("#10501: resolveComboTerminalStatus maps a heterogeneous mix containing a timeout to 504", () => { + const status = resolveComboTerminalStatus( + [ + { model: "a", status: 408, error: "request timeout", kind: "timeout" }, + { model: "b", status: 400, error: "bad request", kind: "model" }, + ], + 400 + ); + assert.equal(status, 504); +}); + +test("#10501: resolveComboTerminalStatus falls back to the caller's status when there are no structured entries", () => { + assert.equal(resolveComboTerminalStatus([], 503), 503); +}); \ No newline at end of file diff --git a/tests/unit/combo-quota-exhaustion-only-fallback.test.ts b/tests/unit/combo-quota-exhaustion-only-fallback.test.ts index eb4a7baaff..2a90d7e3fb 100644 --- a/tests/unit/combo-quota-exhaustion-only-fallback.test.ts +++ b/tests/unit/combo-quota-exhaustion-only-fallback.test.ts @@ -458,6 +458,15 @@ test("opted-in combo ref remains a black box and only quota exhaustion advances } }); +// #10501: the child combo's terminal status is now derived from +// resolveComboTerminalStatus instead of a bare `lastStatus`. A quality failure +// (openai/quality-invalid) mixed with a genuine quota-exhaustion 429 +// (anthropic/quota) is a heterogeneous, non-"the request itself is invalid" +// mix, so it normalizes to a 5xx — the `fallbackOnlyOnQuotaExhaustion` STOP +// decision itself is untouched (it is driven by internal quota-observation +// tracking, not by re-reading the final HTTP status — asserted below via +// `calls`, which must still show the parent stopping instead of falling back +// to paid/backup). test("protected parent combo-ref stops after normal child quality rejection then quota exhaustion", async () => { const calls: string[] = []; const child = { @@ -496,8 +505,16 @@ test("protected parent combo-ref stops after normal child quality rejection then : ok(modelStr); }, }); - assert.equal(result.status, 429); - assert.deepEqual(calls, ["openai/quality-invalid", "anthropic/quota"]); + assert.ok( + result.status >= 500, + `heterogeneous quality+quota-exhaustion mix must normalize to a 5xx, got ${result.status}` + ); + assert.deepEqual( + calls, + ["openai/quality-invalid", "anthropic/quota"], + "the parent must still STOP at the child (not fall back to paid/backup) — the " + + "fallbackOnlyOnQuotaExhaustion decision is unaffected by the status-code change" + ); }); test("protected parent combo-ref stops when a child has mixed quota and non-quota failures", async () => { diff --git a/tests/unit/combo-routing-engine.test.ts b/tests/unit/combo-routing-engine.test.ts index 5997fc83a5..bcf8f5dc0d 100644 --- a/tests/unit/combo-routing-engine.test.ts +++ b/tests/unit/combo-routing-engine.test.ts @@ -897,7 +897,15 @@ test("handleComboChat records per-target metrics separately when the same model assert.equal(metrics.byTarget[secondStep.id].connectionId, "conn-openai-b"); }); -test("handleComboChat surfaces the last failing target's status AND error message together, not a cross-target mismatch (#8486)", async () => { +// #10314/#10501: superseded the original "last writer wins" contract (a single +// `lastError` + raw `[model (status), ...]` suffix). Combo terminal aggregation +// now lists every distinct per-target reason separately (comboErrorAggregation.ts +// ::formatComboOutcomes) and derives the terminal status from an explicit policy +// instead of whichever target happened to fail LAST — a provider 500 mixed with a +// rate_limit 429 is a heterogeneous, non-client-fault outcome, so it normalizes to +// a 5xx (::resolveComboTerminalStatus), never a bare 429 that would misrepresent +// model-a's real 500 as "the client should retry the rate limit". +test("handleComboChat surfaces EVERY failing target's reason (never drops one) and normalizes a heterogeneous 500+429 mix to 5xx (#8486/#10314/#10501)", async () => { const result = await handleComboChat({ body: {}, combo: { @@ -918,11 +926,12 @@ test("handleComboChat surfaces the last failing target's status AND error messag const payload = (await result.json()) as any; - assert.equal(result.status, 429); // #8486: status/message from the SAME (last) failing target - // The last error message is preserved and now carries an aggregated - // per-model diagnostics suffix (status codes for every target attempted - // in this set try), added alongside the global comboTimeoutMs feature. - assert.equal(payload.error.message, "fail:model-b [model-a (500), model-b (429)]"); + assert.ok( + result.status >= 500, + `heterogeneous provider(500)+rate_limit(429) must normalize to a 5xx status, got ${result.status}` + ); + assert.match(payload.error.message, /model-a.*fail:model-a.*HTTP 500/); + assert.match(payload.error.message, /model-b.*fail:model-b.*HTTP 429/); }); interface ComboErrorPayload { @@ -1679,7 +1688,12 @@ test("handleComboChat round-robin falls through generic 400s when a later model assert.deepEqual(calls, ["model-a", "model-b"]); }); -test("handleComboChat round-robin falls through 400s and returns the LAST target's status+message together, not a cross-target mismatch (#8486)", async () => { +// #10314/#10501: same policy update as the priority-strategy test above, applied +// to the round-robin twin. model-a's 400 is a genuine request-shape/model-class +// error, but model-b's 500 is an infra/provider failure — since NOT every target +// failed with a "model" (request-is-invalid) reason, this is a heterogeneous mix +// and must normalize to a 5xx, never a bare "trust the last target's status" 500. +test("handleComboChat round-robin surfaces EVERY target's reason and normalizes a heterogeneous 400+500 mix to 5xx (#8486/#10314/#10501)", async () => { const calls: any[] = []; const result = await handleComboChat({ @@ -1717,8 +1731,12 @@ test("handleComboChat round-robin falls through 400s and returns the LAST target }); const payload = (await result.json()) as any; - assert.equal(result.status, 500); // #8486: status/message from the SAME (last) failing target - assert.equal(payload.error.message, "rr-final-fail"); + assert.ok( + result.status >= 500, + `heterogeneous model(400)+provider(500) mix must normalize to a 5xx status, got ${result.status}` + ); + assert.match(payload.error.message, /model-a.*unsupported message role.*HTTP 400/); + assert.match(payload.error.message, /model-b.*rr-final-fail.*HTTP 500/); assert.deepEqual(calls, ["model-a", "model-b"]); }); diff --git a/tests/unit/combo-terminal-status-policy-10501.test.ts b/tests/unit/combo-terminal-status-policy-10501.test.ts new file mode 100644 index 0000000000..f4b8076bc5 --- /dev/null +++ b/tests/unit/combo-terminal-status-policy-10501.test.ts @@ -0,0 +1,123 @@ +/** + * #10314 / #10501 — integration-level regression for the combo terminal-error + * aggregation + status policy. Drives the REAL `handleComboChat` wiring (via + * an injected `handleSingleModel`, the same seam `combo-body-specific-400- + * stop-4279.test.ts` uses) end-to-end, asserting the actual HTTP `Response` + * the client receives — not just the pure `comboErrorAggregation.ts` helpers + * in isolation (those are covered by `combo-error-aggregation.test.ts`). + * + * Scenario: a priority combo where target #1 returns a 200 that FAILS + * response-quality validation (empty body — see validateQuality.ts) and + * target #2 returns a real 401 (auth). Before #10314/#10501 this surfaced a + * bare 401 ("last writer wins") and dropped the quality reason entirely; now + * it must list BOTH reasons and surface a 5xx-class infra status instead of + * the sibling's 401 (resolveComboTerminalStatus — the request itself was + * never proven invalid on every target). + */ +import test from "node:test"; +import assert from "node:assert/strict"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; + +const TEST_DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-combo-10501-")); +process.env.DATA_DIR = TEST_DATA_DIR; +process.env.API_KEY_SECRET = process.env.API_KEY_SECRET || "combo-10501-test-secret"; + +const { handleComboChat } = await import("../../open-sse/services/combo.ts"); + +const noop = () => {}; +const log = { info: noop, warn: noop, debug: noop, error: noop }; + +function makeCombo(models: string[]) { + return { + name: "test-combo-10501", + strategy: "priority", + models: models.map((m) => ({ model: m })), + }; +} + +// Empty body + application/json content-type trips validateResponseQuality's +// "empty response body" case for a non-streaming response. +function qualityFailingResponse() { + return new Response("", { status: 200, headers: { "Content-Type": "application/json" } }); +} + +function authFailureResponse() { + return new Response(JSON.stringify({ error: { message: "invalid_api_key" } }), { + status: 401, + headers: { "Content-Type": "application/json" }, + }); +} + +function successResponse() { + return new Response( + JSON.stringify({ + id: "chatcmpl-1", + choices: [{ index: 0, message: { role: "assistant", content: "hi there" }, finish_reason: "stop" }], + }), + { status: 200, headers: { "Content-Type": "application/json" } } + ); +} + +test("#10314/#10501: quality failure on target 1 + auth 401 on target 2 → 5xx terminal status, BOTH reasons listed", async () => { + const modelsCalled: string[] = []; + const handleSingleModel = async (_body: unknown, modelStr: string) => { + modelsCalled.push(modelStr); + if (modelStr.includes("model-a")) return qualityFailingResponse(); + return authFailureResponse(); + }; + + const result = await handleComboChat({ + body: { model: "test", messages: [{ role: "user", content: "hi" }], stream: false }, + combo: makeCombo(["openai/model-a", "openai/model-b"]), + handleSingleModel, + log, + settings: {}, + allCombos: [], + }); + + assert.deepEqual(modelsCalled, ["openai/model-a", "openai/model-b"]); + + // #10501: a heterogeneous quality+auth mix must NOT surface the sibling's + // bare 401 (the old `lastStatus` behavior) — it is an infra/provider-class + // outcome (neither target proved the CLIENT's request itself was invalid). + assert.ok( + result.status >= 500, + `expected a 5xx terminal status for a heterogeneous quality+auth mix, got ${result.status}` + ); + assert.notEqual(result.status, 401, "must not regress to surfacing the sibling target's bare 401"); + + const body = (await result.json()) as { error?: { message?: string } }; + const message = body.error?.message ?? ""; + // #10314: both distinct reasons must be listed — neither silently dropped. + assert.match(message, /quality/i, "quality-validation reason must be present in the message"); + assert.match(message, /invalid_api_key|auth/i, "auth reason must be present in the message"); +}); + +test("#10314: success on target 2 after a quality failure on target 1 returns the real success response", async () => { + const modelsCalled: string[] = []; + const handleSingleModel = async (_body: unknown, modelStr: string) => { + modelsCalled.push(modelStr); + if (modelStr.includes("model-a")) return qualityFailingResponse(); + return successResponse(); + }; + + const result = await handleComboChat({ + body: { model: "test", messages: [{ role: "user", content: "hi" }], stream: false }, + combo: makeCombo(["openai/model-a", "openai/model-b"]), + handleSingleModel, + log, + settings: {}, + allCombos: [], + }); + + assert.deepEqual( + modelsCalled, + ["openai/model-a", "openai/model-b"], + "must fail over from the quality-rejected target 1 to target 2" + ); + assert.equal(result.status, 200, "combo must return the successful target's response"); + const body = (await result.json()) as { choices?: Array<{ message?: { content?: string } }> }; + assert.equal(body.choices?.[0]?.message?.content, "hi there"); +});