fix(combo): classify local target timeouts as gateway timeouts

Return a typed HTTP 504 for OmniRoute's per-target timer, keep fallback active, and classify the local timeout as request-scoped so it cannot degrade provider connection health.
This commit is contained in:
Xiangzhe
2026-08-03 07:57:22 +08:00
committed by diegosouzapw
parent 2cb7567d66
commit ce764bc6f3
8 changed files with 383 additions and 25 deletions

View File

@@ -1189,11 +1189,6 @@
"count": 17
}
},
"tests/unit/combo-target-timeout-runner.test.ts": {
"@typescript-eslint/no-explicit-any": {
"count": 4
}
},
"tests/unit/combo-test-route.test.ts": {
"@typescript-eslint/no-explicit-any": {
"count": 10

View File

@@ -194,6 +194,8 @@ const REQUEST_SCOPED_UPSTREAM_ERROR_CODES = new Set([
"context_length_exceeded",
"upstream_empty_response",
"upstream_response_failed",
// Local combo per-target timer (targetTimeoutRunner) — not a connection health signal.
"combo_target_timeout",
]);
/** Request/model-specific failures must not poison provider-wide resilience state. */

View File

@@ -1,17 +1,20 @@
/**
* Wrap a single-model dispatch with a per-target timeout that aborts and falls back.
*
* Verbatim extraction of handleComboChat's `handleSingleModelWithTimeout` closure
* (combo.ts). Behavior is byte-identical; the only change is that the closed-over locals
* (`handleSingleModel`, `comboTargetTimeoutMs`, `log`) became explicit factory params.
* Extracted from handleComboChat's `handleSingleModelWithTimeout` closure (combo.ts).
* A locally expired timer aborts that target and returns a typed 504 response so the Combo
* can fall back without treating OmniRoute's own deadline as a provider-connection failure.
* The per-model abort signal still comes from the target (`target.modelAbortSignal`), so
* the outer request signal is intentionally NOT a dependency here.
*
* See _tasks/superpowers/plans/2026-07-03-blocoJ-combo-hotpath-decomposition.md (Task 1).
*/
import { errorResponse } from "../../utils/error.ts";
import { buildErrorBody, errorResponse, sanitizeErrorMessage } from "../../utils/error.ts";
import type { HandleSingleModel, SingleModelTarget, ComboLogger } from "./types.ts";
/** Stable internal classification for OmniRoute's own combo per-target timer. */
export const COMBO_TARGET_TIMEOUT_CODE = "combo_target_timeout";
export function buildTargetTimeoutRunner(deps: {
handleSingleModel: HandleSingleModel;
comboTargetTimeoutMs: number;
@@ -44,11 +47,23 @@ export function buildTargetTimeoutRunner(deps: {
`Model ${modelStr} exceeded ${comboTargetTimeoutMs}ms timeout — falling back`
);
timeoutController.abort(new Error("combo-per-model-timeout"));
// HTTP 504 (not proprietary 524): this is OmniRoute's own per-target timer.
// Typed as combo_target_timeout so request-scoped classification can keep the
// connection eligible for fallback instead of treating it like Cloudflare 524
// or a genuine upstream gateway timeout.
resolve(
new Response(JSON.stringify({ error: { message: `Model ${modelStr} timed out` } }), {
status: 524,
headers: { "Content-Type": "application/json" },
})
new Response(
JSON.stringify(
buildErrorBody(504, sanitizeErrorMessage(`Model ${modelStr} timed out`), undefined, {
type: COMBO_TARGET_TIMEOUT_CODE,
code: COMBO_TARGET_TIMEOUT_CODE,
})
),
{
status: 504,
headers: { "Content-Type": "application/json" },
}
)
);
}, comboTargetTimeoutMs);
});
@@ -72,7 +87,7 @@ export function buildTargetTimeoutRunner(deps: {
return await Promise.race([
handleSingleModel(b, modelStr, targetWithSignal).catch((err) => {
if (timedOut) {
// Inner call rejected because we aborted it. The synthetic 524 from
// Inner call rejected because we aborted it. The synthetic 504 from
// timeoutPromise already wins the race; return an empty response so
// the loser branch resolves cleanly without leaking err.message.
return new Response(null, { status: 599 });

View File

@@ -61,8 +61,8 @@ export function isComboCooldownWaitEligible(
* When the combo is wait-eligible (see isComboCooldownWaitEligible), a single target's
* dispatch can legitimately wait out cooldowns for up to `comboCooldownWait.budgetMs`
* before it resolves — so the per-target timeout must never be shorter than that budget,
* or the wait gets cut off mid-retry and the target times out with a synthetic 524
* (open-sse/services/combo/targetTimeoutRunner.ts) instead of completing the wait. This
* or the wait gets cut off mid-retry and the target times out with a synthetic 504
* (`combo_target_timeout`, open-sse/services/combo/targetTimeoutRunner.ts) instead of completing the wait. This
* only raises the *default* floor; an operator's explicit `targetTimeoutMs` on the combo
* still wins (see resolveComboTargetTimeoutMs).
*/

View File

@@ -326,7 +326,7 @@ test("resolveComboTargetTimeoutMs falls back to the saner combo default when uns
// #7360 / #7301: any strategy with comboCooldownWait enabled waits out cooldowns for up
// to comboCooldownWait.budgetMs, so the per-target timeout floor must cover that budget
// (DEFAULT_COMBO_TARGET_TIMEOUT_MS alone would cut a long wait short into a 524).
// (DEFAULT_COMBO_TARGET_TIMEOUT_MS alone would cut a long wait short into a 504 combo_target_timeout).
test("isComboCooldownWaitEligible engages for every strategy when the feature is enabled", () => {
for (const strategy of ALL_COMBO_STRATEGIES) {
assert.equal(isComboCooldownWaitEligible(strategy, { enabled: true }), true);
@@ -359,7 +359,12 @@ test("resolveComboTargetTimeoutMsForCombo raises the floor to cover the cooldown
// Explicit per-combo targetTimeoutMs still wins over the derived floor.
assert.equal(
resolveComboTargetTimeoutMsForCombo({ targetTimeoutMs: 45000 }, 600000, "auto", comboCooldownWait),
resolveComboTargetTimeoutMsForCombo(
{ targetTimeoutMs: 45000 },
600000,
"auto",
comboCooldownWait
),
45000
);

View File

@@ -1,8 +1,9 @@
import { test } from "node:test";
import assert from "node:assert/strict";
import { buildTargetTimeoutRunner } from "../../open-sse/services/combo/targetTimeoutRunner.ts";
import type { ComboLogger, SingleModelTarget } from "../../open-sse/services/combo/types.ts";
const noopLog = { warn() {}, info() {}, error() {}, debug() {} } as any;
const noopLog: ComboLogger = { warn() {}, info() {}, error() {}, debug() {} };
test("timeout<=0: passthrough direto (sem timer)", async () => {
let called = false;
@@ -31,21 +32,28 @@ test("timeout<=0: erro do upstream vira errorResponse 502", async () => {
assert.equal(res.status, 502);
});
test("excede o limite: aborta e retorna 524 timed out", async () => {
test("excede o limite: aborta e retorna 504 combo_target_timeout", async () => {
let aborted = false;
const runner = buildTargetTimeoutRunner({
handleSingleModel: (_b, _m, target) =>
new Promise<Response>((resolve) => {
// resolve só se abortado (simula um upstream que respeita o signal)
const sig = (target as any)?.modelAbortSignal as AbortSignal | undefined;
sig?.addEventListener("abort", () => resolve(new Response(null, { status: 599 })));
const sig = target?.modelAbortSignal ?? undefined;
sig?.addEventListener("abort", () => {
aborted = true;
resolve(new Response(null, { status: 599 }));
});
}),
comboTargetTimeoutMs: 20,
log: noopLog,
});
const res = await runner({}, "slow-model");
assert.equal(res.status, 524);
assert.equal(res.status, 504);
assert.equal(aborted, true, "per-target timeout must abort the in-flight target");
const body = await res.json();
assert.match(JSON.stringify(body), /timed out/i);
assert.equal(body?.error?.code, "combo_target_timeout");
assert.equal(body?.error?.type, "combo_target_timeout");
});
test("sucesso rápido vence a corrida do timeout", async () => {
@@ -66,13 +74,14 @@ test("hedge do parent já abortado propaga o abort ao filho", async () => {
const runner = buildTargetTimeoutRunner({
handleSingleModel: (_b, _m, target) =>
new Promise<Response>((resolve) => {
const sig = (target as any)?.modelAbortSignal as AbortSignal | undefined;
const sig = target?.modelAbortSignal ?? undefined;
if (sig?.aborted) sawAbort = true;
resolve(new Response("ok"));
}),
comboTargetTimeoutMs: 1000,
log: noopLog,
});
await runner({}, "m", { modelAbortSignal: parent.signal } as any);
const parentTarget: SingleModelTarget = { modelAbortSignal: parent.signal };
await runner({}, "m", parentTarget);
assert.equal(sawAbort, true);
});

View File

@@ -383,6 +383,46 @@ test("gemini 524 DOES exhaust connection (cloudflare timeout)", () => {
assert.equal(s.exhaustedConnections.has("gemini:gemini-key-abc"), true);
});
test("generic upstream 504 without combo_target_timeout still exhausts the connection", () => {
const s = sets();
applyComboTargetExhaustion(target(), {
...baseOpts,
result: { status: 504, headers: null },
fallbackResult: {},
errorText: "Gateway Timeout",
structuredError: { code: "gateway_timeout", type: "server_error" },
sets: s,
});
assert.ok(
s.exhaustedConnections.has("test-dedup-provider:conn-1"),
"genuine upstream 504 must retain connection-level exhaustion"
);
assert.equal(s.exhaustedProviders.size, 0);
});
test("OmniRoute combo_target_timeout 504 does NOT exhaust connection or provider", () => {
const s = sets();
const exhausted = applyComboTargetExhaustion(target(), {
...baseOpts,
result: { status: 504, headers: null },
fallbackResult: {},
errorText: "Model slow-model timed out",
structuredError: { code: "combo_target_timeout", type: "combo_target_timeout" },
sets: s,
});
assert.equal(exhausted, false);
assert.equal(
s.exhaustedConnections.size,
0,
"local per-target timeout must not poison exhaustedConnections"
);
assert.equal(
s.exhaustedProviders.size,
0,
"local per-target timeout must not poison exhaustedProviders"
);
});
// #8133/#8137: auth-level failures (401/403) mean THAT connection's credentials are bad.
// When the target carries a connectionId, only that connection is marked exhausted — sibling
// connections on the same provider must stay eligible (#8137: whole-provider exhaustion wrongly

View File

@@ -0,0 +1,292 @@
/**
* Behavioral evidence for Combo per-target timeout standards:
* - local timer returns typed 504 `combo_target_timeout` and fails over
* - that local timer must NOT record a provider circuit-breaker failure
* - a genuine upstream 504 still records breaker failure / connection exhaustion
*
* Decision seam for the breaker is the same composition handleComboChat uses:
* isComboRequestScopedFailure → shouldRecordProviderBreakerFailure(requestScopedFailure)
* Exhaustion uses applyComboTargetExhaustion with the same structuredError path.
* Orchestration uses public handleComboChat + injected handleSingleModel (not private mocks).
*/
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-target-timeout-std-"));
process.env.DATA_DIR = TEST_DATA_DIR;
process.env.API_KEY_SECRET = process.env.API_KEY_SECRET || "combo-target-timeout-std-secret";
const { handleComboChat } = await import("../../../open-sse/services/combo.ts");
const { isComboRequestScopedFailure, shouldRecordProviderBreakerFailure } =
await import("../../../open-sse/services/combo/comboPredicates.ts");
const { applyComboTargetExhaustion } =
await import("../../../open-sse/services/combo/targetExhaustion.ts");
const { getProviderBreakerState } = await import("../../../open-sse/services/accountFallback.ts");
const { resetAllCircuitBreakers } = await import("../../../src/shared/utils/circuitBreaker.ts");
const noop = () => {};
const log = { info: noop, warn: noop, debug: noop, error: noop };
type Body = Record<string, unknown>;
function okResponse(content: string) {
return new Response(JSON.stringify({ choices: [{ message: { role: "assistant", content } }] }), {
status: 200,
headers: { "Content-Type": "application/json" },
});
}
function upstreamGatewayTimeoutResponse() {
return new Response(
JSON.stringify({
error: {
message: "Gateway Timeout",
type: "server_error",
code: "gateway_timeout",
},
}),
{ status: 504, headers: { "Content-Type": "application/json" } }
);
}
/** Compose the exact breaker decision seam used by handleComboChat's failure branch. */
function decideProviderBreakerRecord(args: {
status: number;
errorText: string;
structuredError?: { code?: string; type?: string };
sameProviderNext?: boolean;
}) {
const requestScopedFailure = isComboRequestScopedFailure(
args.status,
args.errorText,
args.structuredError
);
return {
requestScopedFailure,
shouldRecord: shouldRecordProviderBreakerFailure({
isStreamReadinessFailure: false,
status: args.status,
sameProviderNext: args.sameProviderNext === true,
requestScopedFailure,
error: args.errorText,
}),
};
}
function resolvedTarget(overrides: Record<string, unknown> = {}) {
return {
kind: "model" as const,
modelStr: "openai/gpt-4o-mini",
provider: "openai",
providerId: null,
connectionId: "conn-1",
executionKey: "k",
stepId: "s",
weight: 1,
label: null,
...overrides,
} as Parameters<typeof applyComboTargetExhaustion>[0];
}
test.beforeEach(() => {
resetAllCircuitBreakers();
});
// ── Decision seam: breaker + request-scoped classification ──────────────────
test("decision seam: typed combo_target_timeout 504 is request-scoped and does not record breaker failure", () => {
const decision = decideProviderBreakerRecord({
status: 504,
errorText: "Model openai/slow timed out",
structuredError: { code: "combo_target_timeout", type: "combo_target_timeout" },
sameProviderNext: false,
});
assert.equal(decision.requestScopedFailure, true);
assert.equal(
decision.shouldRecord,
false,
"local per-target timer must not trip the provider circuit breaker"
);
});
test("decision seam: generic upstream 504 is NOT request-scoped and still records breaker failure", () => {
const decision = decideProviderBreakerRecord({
status: 504,
errorText: "Gateway Timeout",
structuredError: { code: "gateway_timeout", type: "server_error" },
sameProviderNext: false,
});
assert.equal(decision.requestScopedFailure, false);
assert.equal(
decision.shouldRecord,
true,
"genuine upstream 504 must retain connection-level breaker recording"
);
});
test("decision seam: genuine Cloudflare 524 is not request-scoped (exhaustion, not breaker status set)", () => {
// Breaker status set is 408/500/502/503/504 (not 524). 524 remains a connection-
// exhaustion signal only — it does not go through request-scoped classification.
const decision = decideProviderBreakerRecord({
status: 524,
errorText: "A Timeout Occurred",
structuredError: undefined,
sameProviderNext: false,
});
assert.equal(decision.requestScopedFailure, false);
assert.equal(
decision.shouldRecord,
false,
"524 is outside PROVIDER_BREAKER_FAILURE_STATUSES (exhaustion-only signal)"
);
});
test("exhaustion: typed combo_target_timeout 504 does not poison connection; generic 504 does", () => {
const base = {
fallbackResult: {},
isTokenLimitBreach: false,
allAccountsRateLimited: false,
log,
tag: "COMBO",
exhaustedLogLevel: "info" as const,
};
const localSets = {
exhaustedProviders: new Set<string>(),
exhaustedConnections: new Set<string>(),
transientRateLimitedProviders: new Set<string>(),
};
applyComboTargetExhaustion(resolvedTarget(), {
...base,
result: { status: 504, headers: null },
errorText: "Model openai/slow timed out",
rawModel: "gpt-4o-mini",
structuredError: { code: "combo_target_timeout", type: "combo_target_timeout" },
sets: localSets,
});
assert.equal(localSets.exhaustedConnections.size, 0);
assert.equal(localSets.exhaustedProviders.size, 0);
const upstreamSets = {
exhaustedProviders: new Set<string>(),
exhaustedConnections: new Set<string>(),
transientRateLimitedProviders: new Set<string>(),
};
applyComboTargetExhaustion(resolvedTarget(), {
...base,
result: { status: 504, headers: null },
errorText: "Gateway Timeout",
rawModel: "gpt-4o-mini",
structuredError: { code: "gateway_timeout", type: "server_error" },
sets: upstreamSets,
});
assert.ok(upstreamSets.exhaustedConnections.has("openai:conn-1"));
});
// ── Orchestration: public handleComboChat ───────────────────────────────────
test("handleComboChat: local per-target timeout aborts first target, fails over, succeeds, no breaker record", async () => {
const calls: string[] = [];
let firstAborted = false;
const res = await handleComboChat({
body: { messages: [{ role: "user", content: "ping" }] },
combo: {
name: "timeout-failover-std",
strategy: "priority",
models: ["openai/slow-model", "claude/backup-model"],
config: {
maxRetries: 0,
retryDelayMs: 0,
fallbackDelayMs: 0,
targetTimeoutMs: 40,
},
},
handleSingleModel: async (_b: Body, modelStr: string, target) => {
calls.push(modelStr);
if (modelStr === "openai/slow-model") {
return await new Promise<Response>((resolve) => {
const sig = target?.modelAbortSignal;
const onAbort = () => {
firstAborted = true;
// Loser branch; timeoutPromise already supplies the typed 504.
resolve(new Response(null, { status: 599 }));
};
if (sig?.aborted) {
onAbort();
return;
}
sig?.addEventListener("abort", onAbort, { once: true });
});
}
return okResponse("recovered-after-local-timeout");
},
isModelAvailable: async () => true,
log,
settings: null,
allCombos: null,
});
assert.equal(res.status, 200, "combo must succeed on the second target after local timeout");
assert.deepEqual(calls, ["openai/slow-model", "claude/backup-model"]);
assert.equal(firstAborted, true, "first target must be aborted by the per-target timer");
const body = (await res.json()) as {
choices: Array<{ message: { content: string } }>;
};
assert.equal(body.choices[0].message.content, "recovered-after-local-timeout");
const breaker = getProviderBreakerState("openai");
assert.equal(
breaker?.failureCount ?? 0,
0,
"local combo_target_timeout must not record a provider circuit-breaker failure"
);
});
test("handleComboChat: generic upstream 504 fails over but still records provider breaker failure", async () => {
const calls: string[] = [];
const res = await handleComboChat({
body: { messages: [{ role: "user", content: "ping" }] },
combo: {
name: "upstream-504-failover-std",
strategy: "priority",
models: ["openai/primary", "claude/backup"],
config: {
maxRetries: 0,
retryDelayMs: 0,
fallbackDelayMs: 0,
// Keep timeout high so this path is pure upstream 504, not the local timer.
targetTimeoutMs: 60_000,
},
},
handleSingleModel: async (_b: Body, modelStr: string) => {
calls.push(modelStr);
if (modelStr === "openai/primary") {
return upstreamGatewayTimeoutResponse();
}
return okResponse("recovered-after-upstream-504");
},
isModelAvailable: async () => true,
log,
settings: null,
allCombos: null,
});
assert.equal(res.status, 200);
assert.deepEqual(calls, ["openai/primary", "claude/backup"]);
const body = (await res.json()) as {
choices: Array<{ message: { content: string } }>;
};
assert.equal(body.choices[0].message.content, "recovered-after-upstream-504");
const breaker = getProviderBreakerState("openai");
assert.ok(
(breaker?.failureCount ?? 0) >= 1,
"genuine upstream 504 must record at least one provider breaker failure"
);
});