diff --git a/config/quality/eslint-suppressions.json b/config/quality/eslint-suppressions.json index 4a77db36c9..81e91243ca 100644 --- a/config/quality/eslint-suppressions.json +++ b/config/quality/eslint-suppressions.json @@ -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 diff --git a/open-sse/services/combo/comboPredicates.ts b/open-sse/services/combo/comboPredicates.ts index 0b6a939d7b..cc29f710fa 100644 --- a/open-sse/services/combo/comboPredicates.ts +++ b/open-sse/services/combo/comboPredicates.ts @@ -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. */ diff --git a/open-sse/services/combo/targetTimeoutRunner.ts b/open-sse/services/combo/targetTimeoutRunner.ts index a1479b8e07..6264cb7ff7 100644 --- a/open-sse/services/combo/targetTimeoutRunner.ts +++ b/open-sse/services/combo/targetTimeoutRunner.ts @@ -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 }); diff --git a/open-sse/services/comboConfig.ts b/open-sse/services/comboConfig.ts index 12dc440466..30fd959186 100644 --- a/open-sse/services/comboConfig.ts +++ b/open-sse/services/comboConfig.ts @@ -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). */ diff --git a/tests/unit/combo-config.test.ts b/tests/unit/combo-config.test.ts index 03888e5ec9..61fe705d0a 100644 --- a/tests/unit/combo-config.test.ts +++ b/tests/unit/combo-config.test.ts @@ -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 ); diff --git a/tests/unit/combo-target-timeout-runner.test.ts b/tests/unit/combo-target-timeout-runner.test.ts index 9b89461d42..e75fee746b 100644 --- a/tests/unit/combo-target-timeout-runner.test.ts +++ b/tests/unit/combo-target-timeout-runner.test.ts @@ -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((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((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); }); diff --git a/tests/unit/combo/combo-target-exhaustion.test.ts b/tests/unit/combo/combo-target-exhaustion.test.ts index 3e776d6d23..3a9dfb6edd 100644 --- a/tests/unit/combo/combo-target-exhaustion.test.ts +++ b/tests/unit/combo/combo-target-exhaustion.test.ts @@ -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 diff --git a/tests/unit/combo/combo-target-timeout-standards.test.ts b/tests/unit/combo/combo-target-timeout-standards.test.ts new file mode 100644 index 0000000000..52c8322519 --- /dev/null +++ b/tests/unit/combo/combo-target-timeout-standards.test.ts @@ -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; + +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 = {}) { + 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[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(), + exhaustedConnections: new Set(), + transientRateLimitedProviders: new Set(), + }; + 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(), + exhaustedConnections: new Set(), + transientRateLimitedProviders: new Set(), + }; + 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((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" + ); +});