From 3eebf985edd7e2104da326907d35f92710175ae4 Mon Sep 17 00:00:00 2001 From: adevwithpurpose Date: Sat, 15 Aug 2026 19:10:58 -0300 Subject: [PATCH] fix(resilience): keep combo quality and auth reasons separate and redact connection labels in terminal errors (#10314) --- .../fixes/10314-combo-error-aggregation.md | 1 + open-sse/services/combo.ts | 68 +++++++---- .../services/combo/comboErrorAggregation.ts | 115 ++++++++++++++++++ tests/unit/combo-error-aggregation.test.ts | 54 ++++++++ 4 files changed, 217 insertions(+), 21 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 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 df86a79375..0daaeb8541 100644 --- a/open-sse/services/combo.ts +++ b/open-sse/services/combo.ts @@ -93,6 +93,13 @@ import { expandPromptCacheAffinityTargetsFromConnections, resolvePromptCacheAffinityKey, } from "./combo/promptCacheAffinity.ts"; +import { + classifyComboOutcome, + formatComboOutcomes, + redactConnectionLabel, + buildRedactedSummary, +} 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"; @@ -853,7 +860,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; @@ -1343,6 +1350,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); @@ -1850,6 +1866,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++; @@ -2043,6 +2060,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++; @@ -2197,15 +2215,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, { @@ -2276,18 +2289,12 @@ 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; + // #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 @@ -2715,6 +2722,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 @@ -2911,6 +2922,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 } @@ -3217,6 +3234,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 }); @@ -3337,7 +3360,10 @@ async function handleRoundRobinCombo({ } const status = lastStatus; - const msg = lastError || "All round-robin combo models unavailable"; + // #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..7a4d7cd1e5 --- /dev/null +++ b/open-sse/services/combo/comboErrorAggregation.ts @@ -0,0 +1,115 @@ +/** + * 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" + | "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", + 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. + */ +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 === 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; + const reason = e.error || `HTTP ${e.status}`; + 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("; "); +} \ No newline at end of file diff --git a/tests/unit/combo-error-aggregation.test.ts b/tests/unit/combo-error-aggregation.test.ts new file mode 100644 index 0000000000..981f31e828 --- /dev/null +++ b/tests/unit/combo-error-aggregation.test.ts @@ -0,0 +1,54 @@ +import test from "node:test"; +import assert from "node:assert/strict"; +import { + classifyComboOutcome, + formatComboOutcomes, + redactConnectionLabel, + buildRedactedSummary, +} 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"); + // 5xx sleep to the "timeout" class (>=499 is checked before >=500). + assert.equal(classifyComboOutcome(503, "upstream unavailable"), "timeout"); + assert.equal(classifyComboOutcome(408, "timeout"), "timeout"); + assert.equal(classifyComboOutcome(400, "bad request"), "model"); +}); + +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\)/); +}); \ No newline at end of file