pjandro/OmniRoute
1
0
Fork 0
mirror of https://github.com/diegosouzapw/OmniRoute.git synced 2026-08-18 21:22:28 +03:00
Code Issues Packages Projects Releases Wiki Activity

fix(resilience): keep combo quality and auth reasons separate and redact connection labels in terminal errors (#10314) (#10501)

Browse Source 
* 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 <adevwithpurpose@users.noreply.github.com>
This commit is contained in:
Diego Rodrigues de Sa e Souza
2026-08-18 10:51:16 -03:00
committed by GitHub
parent da42ed6d2e
commit 2230fbbe93
8 changed files with 605 additions and 52 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
changelog.d/fixes/10314-combo-error-aggregation.md Normal file
View File

@@ -0,0 +1 @@
- fix(resilience): keep combo quality and auth failure reasons separate and redact connection labels in terminal errors (#10314)

82
open-sse/services/combo.ts
View File

@@ -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<ComboErrorEntry> = [];
// 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<ComboErrorEntry> = [];
// #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));

179
open-sse/services/combo/comboErrorAggregation.ts Normal file
View File

@@ -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<ComboOutcomeKind, string> = {
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 <uuid>") — 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<ComboErrorEntry>,
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;
}

50
tests/unit/combo-antigravity-missing-project-reset-8486.test.ts
View File

@@ -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);
});

165
tests/unit/combo-error-aggregation.test.ts Normal file
View File

@@ -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);
});

21
tests/unit/combo-quota-exhaustion-only-fallback.test.ts
View File

@@ -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 () => {

36
tests/unit/combo-routing-engine.test.ts
View File

@@ -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"]);
});

123
tests/unit/combo-terminal-status-policy-10501.test.ts Normal file
View File

@@ -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");
});