fix(sse): stop combo's aggregated failure response from mixing fields across targets (#8486) (#8508)

handleComboChat/handleRoundRobinCombo tracked lastStatus (first-write-wins),
lastError (last-write-wins), and earliestRetryAfter (global MIN across all
targets) independently, so the final unavailableResponse() could surface a
status/message pair from two different failing targets and decorate a
config-class error (e.g. Antigravity's 422 missing_project_id, which carries
no retryAfter of its own) with an unrelated target's long reset window.

- lastStatus now overwrites on every failure (last-write-wins), matching
  lastError, so status and message always come from the same target.
- the "(reset after ...)" decoration is only applied when the surfaced
  status is itself rate-limit-class (429/503) — see the new
  open-sse/services/combo/unavailableRetryGate.ts leaf module (both
  combo.ts and chat.ts are already over their file-size baseline, so the
  gate logic lives in a new module and combo.ts only wires it in).

Co-authored-by: ikelvingo <im.kelvinwong@gmail.com>
This commit is contained in:
Diego Rodrigues de Sa e Souza
2026-07-25 04:57:29 -03:00
committed by GitHub
parent 9ced2e99df
commit 30709255c9
5 changed files with 161 additions and 13 deletions

View File

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

View File

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