diff --git a/changelog.d/fixes/8486-combo-unavailable-field-mismatch.md b/changelog.d/fixes/8486-combo-unavailable-field-mismatch.md new file mode 100644 index 0000000000..e0d6826fed --- /dev/null +++ b/changelog.d/fixes/8486-combo-unavailable-field-mismatch.md @@ -0,0 +1 @@ +- fix(sse): stop combo's "all targets failed" response from attaching one target's retry-after window to an unrelated target's error message (#8486) diff --git a/open-sse/services/combo.ts b/open-sse/services/combo.ts index 1743ee0ea1..6009430177 100644 --- a/open-sse/services/combo.ts +++ b/open-sse/services/combo.ts @@ -170,6 +170,7 @@ import { import { applyComboTargetExhaustion } from "./combo/targetExhaustion.ts"; import { executeRuntimeUnitCombo } from "./combo/runtimeUnits.ts"; import { extractFusionPanelSpec, buildFusionHandleSingleModel } from "./combo/fusionPanel.ts"; +import { isRetryAfterEligibleStatus } from "./combo/unavailableRetryGate.ts"; import { isRecord } from "./combo/comboData.ts"; import { expandProviderWildcardsInCombo, @@ -1931,7 +1932,7 @@ export async function handleComboChat({ // Fix #1707: Set terminal state so the fallback doesn't emit // misleading ALL_ACCOUNTS_INACTIVE when the real issue is quality. lastError = `Upstream response failed quality validation: ${quality.reason}`; - if (!lastStatus) lastStatus = 502; + lastStatus = 502; if (i > 0) fallbackCount++; if (provider && rawModel) { const mlSettings = resolveModelLockoutSettings(settings); @@ -2369,7 +2370,7 @@ export async function handleComboChat({ status: result.status, error: errorText || String(result.status), }); - if (!lastStatus) lastStatus = result.status; + lastStatus = result.status; if (i > 0) fallbackCount++; log.warn("COMBO", `Model ${modelStr} failed with body-specific error, stopping combo`); // #4279: surface the 400 via the {ok,response} contract so the OUTER @@ -2425,7 +2426,7 @@ export async function handleComboChat({ // decision below, even though a real 429 with a short (~1min) retry-after // was just observed. Recording it here mirrors the "done retrying" path. lastError = errorText || String(result.status); - if (!lastStatus) lastStatus = result.status; + lastStatus = result.status; if (i > 0) fallbackCount++; return null; } @@ -2462,7 +2463,7 @@ export async function handleComboChat({ // Same fix as the already-locked branch above — this is the // first-failure lockout path, so lastStatus needs recording here too. lastError = errorText || String(result.status); - if (!lastStatus) lastStatus = result.status; + lastStatus = result.status; if (i > 0) fallbackCount++; return null; } @@ -2484,7 +2485,7 @@ export async function handleComboChat({ status: result.status, error: errorText || String(result.status), }); - if (!lastStatus) lastStatus = result.status; + lastStatus = result.status; if (i > 0) fallbackCount++; // Wire combo failures into the resilience dashboard (model-level lockout) // alongside the provider-level cooldown below — they govern different scopes. @@ -2708,7 +2709,7 @@ export async function handleComboChat({ : ""; const msg = (lastError || "All combo models unavailable") + comboErrorSummary; - if (earliestRetryAfter) { + if (earliestRetryAfter && isRetryAfterEligibleStatus(status)) { // Cooldown-aware retry: instead of crystallizing the 429/503, wait out // a SHORT transient cooldown and re-run the whole set loop. Guarded by // the helper (quota_exhausted/auth/not-found excluded, ceiling, @@ -3257,7 +3258,7 @@ async function handleRoundRobinCombo({ // Fix #1707: Set terminal state so the fallback doesn't emit // misleading ALL_ACCOUNTS_INACTIVE when the real issue is quality. lastError = `Upstream response failed quality validation: ${quality.reason}`; - if (!lastStatus) lastStatus = 502; + lastStatus = 502; if (offset > 0) fallbackCount++; break; // move to next model } @@ -3515,7 +3516,7 @@ async function handleRoundRobinCombo({ }); recordedAttempts++; lastError = errorText || String(result.status); - if (!lastStatus) lastStatus = result.status; + lastStatus = result.status; if (offset > 0) fallbackCount++; log.warn("COMBO-RR", `${modelStr} failed, trying next model`, { status: result.status }); @@ -3625,7 +3626,7 @@ async function handleRoundRobinCombo({ const status = lastStatus; const msg = lastError || "All round-robin combo models unavailable"; - if (earliestRetryAfter) { + if (earliestRetryAfter && isRetryAfterEligibleStatus(status)) { const retryHuman = formatRetryAfter(toRetryAfterDisplayValue(earliestRetryAfter)); log.warn("COMBO-RR", `All models failed | ${msg} (${retryHuman})`); return unavailableResponse(status, msg, earliestRetryAfter, retryHuman); diff --git a/open-sse/services/combo/unavailableRetryGate.ts b/open-sse/services/combo/unavailableRetryGate.ts new file mode 100644 index 0000000000..fe9c7670b3 --- /dev/null +++ b/open-sse/services/combo/unavailableRetryGate.ts @@ -0,0 +1,33 @@ +/** + * #8486 Part B: gate for the "all targets failed" unavailable-response + * retry-after decoration in combo.ts (handleComboChat / handleRoundRobinCombo). + * + * Root cause: the aggregation loop tracks `earliestRetryAfter` as the MINIMUM + * retry-after parsed out of ANY target's own response body across the whole + * failure loop, independent of which target ends up supplying the surfaced + * `status`/`msg` pair. When a combo mixes a genuinely rate-limited target + * (real retryAfter) with a config-class failure (e.g. Antigravity's 422 + * `missing_project_id`, which carries no retryAfter of its own), the final + * unavailableResponse() stitches the rate-limited target's reset window onto + * the unrelated target's message text. + * + * `unavailableResponse()` (open-sse/utils/error.ts) has no way to know + * `status`/`msg` and `retryAfter` came from different targets, so the gate + * has to live at the call site: only decorate the response with a + * `(reset after ...)` suffix when the surfaced `status` is itself a + * rate-limit-class code that plausibly owns a retry-after window. A + * config-class status (422, 400, 401, 403, 404, ...) must never receive a + * retry-after suffix that did not originate from its own response body. + */ + +const RETRY_AFTER_ELIGIBLE_STATUSES = new Set([429, 503]); + +/** + * Whether the final aggregated combo-failure `status` is a rate-limit-class + * code allowed to be decorated with a `(reset after ...)` retry-after + * suffix. Config-class statuses (422 missing_project_id, 400, 401, 403, 404, + * ...) are excluded — see module header for the field-mismatch this guards. + */ +export function isRetryAfterEligibleStatus(status: number | null | undefined): boolean { + return typeof status === "number" && RETRY_AFTER_ELIGIBLE_STATUSES.has(status); +} diff --git a/tests/unit/combo-antigravity-missing-project-reset-8486.test.ts b/tests/unit/combo-antigravity-missing-project-reset-8486.test.ts new file mode 100644 index 0000000000..b645d309ac --- /dev/null +++ b/tests/unit/combo-antigravity-missing-project-reset-8486.test.ts @@ -0,0 +1,113 @@ +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-8486-")); +process.env.DATA_DIR = TEST_DATA_DIR; +process.env.API_KEY_SECRET = process.env.API_KEY_SECRET || "combo-8486-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-8486", + strategy: "priority", + models: models.map((m) => ({ model: m })), + }; +} + +async function runScenario(models: string[]) { + const longRetryAfterMs = (21 * 3600 + 47 * 60 + 32) * 1000; + const longRetryAfterIso = new Date(Date.now() + longRetryAfterMs).toISOString(); + + const missingProjectBody = { + error: { + message: + "Missing Google projectId for Antigravity account. Auto-discovery via loadCodeAssist " + + "found no Cloud Code project. Please reconnect OAuth in Providers → Antigravity (and " + + "ensure the Google account has completed Gemini Code Assist onboarding).", + type: "oauth_missing_project_id", + code: "missing_project_id", + }, + }; + + const modelsCalled: string[] = []; + const handleSingleModel = async (_body: unknown, modelStr: string) => { + modelsCalled.push(modelStr); + if (modelStr.includes("account-a")) { + return new Response( + JSON.stringify({ + error: { message: "Your quota will reset after 21h47m32s." }, + retryAfter: longRetryAfterIso, + }), + { status: 429, headers: { "Content-Type": "application/json" } } + ); + } + return new Response(JSON.stringify(missingProjectBody), { + status: 422, + headers: { "Content-Type": "application/json" }, + }); + }; + + const result = await handleComboChat({ + body: { model: "test", messages: [{ role: "user", content: "hi" }] }, + combo: makeCombo(models), + handleSingleModel, + log, + settings: {}, + allCombos: [], + }); + + 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 () => { + const { result, modelsCalled } = await runScenario([ + "antigravity/account-a-model", + "antigravity/account-b-model", + ]); + + assert.ok( + modelsCalled.some((m) => m.includes("account-a")) && + modelsCalled.some((m) => m.includes("account-b")), + `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}` + ); +}); + +test("#8486 Part B (reverse order): the config-class 422 must not swallow a genuinely rate-limited sibling's message either", async () => { + const { result, modelsCalled } = await runScenario([ + "antigravity/account-b-model", + "antigravity/account-a-model", + ]); + + assert.ok( + modelsCalled.some((m) => m.includes("account-a")) && + modelsCalled.some((m) => m.includes("account-b")), + `expected both targets to be tried, got: ${JSON.stringify(modelsCalled)}` + ); + + const text = await result.clone().text(); + + // 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}` + ); +}); diff --git a/tests/unit/combo-routing-engine.test.ts b/tests/unit/combo-routing-engine.test.ts index 41cda10850..9e2893d28b 100644 --- a/tests/unit/combo-routing-engine.test.ts +++ b/tests/unit/combo-routing-engine.test.ts @@ -889,7 +889,7 @@ test("handleComboChat records per-target metrics separately when the same model assert.equal(metrics.byTarget[secondStep.id].connectionId, "conn-openai-b"); }); -test("handleComboChat preserves the first failure status but surfaces the last error message plus per-model diagnostics", async () => { +test("handleComboChat surfaces the last failing target's status AND error message together, not a cross-target mismatch (#8486)", async () => { const result = await handleComboChat({ body: {}, combo: { @@ -910,7 +910,7 @@ test("handleComboChat preserves the first failure status but surfaces the last e const payload = (await result.json()) as any; - assert.equal(result.status, 500); + 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. @@ -1671,7 +1671,7 @@ 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 final error payload when no target recovers", async () => { +test("handleComboChat round-robin falls through 400s and returns the LAST target's status+message together, not a cross-target mismatch (#8486)", async () => { const calls: any[] = []; const result = await handleComboChat({ @@ -1709,7 +1709,7 @@ test("handleComboChat round-robin falls through 400s and returns the final error }); const payload = (await result.json()) as any; - assert.equal(result.status, 400); + assert.equal(result.status, 500); // #8486: status/message from the SAME (last) failing target assert.equal(payload.error.message, "rr-final-fail"); assert.deepEqual(calls, ["model-a", "model-b"]); });