diff --git a/config/quality/file-size-baseline.json b/config/quality/file-size-baseline.json index fde4c223d2..b261ec4828 100644 --- a/config/quality/file-size-baseline.json +++ b/config/quality/file-size-baseline.json @@ -1,4 +1,5 @@ { + "_rebaseline_2026_07_21_7301_universal_cooldown_retry": "PR #7301 (ViFigueiredo, feat/universal-cooldown-retry) own growth, surfaced during rebase-onto-tip reconciliation (fast-gates PR->release do not run check:file-size): open-sse/services/combo.ts 3388->3479 (+91) generalizes the existing quota-share-only cooldown-aware retry (dispatchWithCooldownRetry) to ALL combo strategies (priority/weighted/round-robin/etc), gates it on the model lockout's REAL reason (not a hardcoded \"rate_limit\") via the existing getModelLockoutInfo/resolveComboCooldownWaitDecision chokepoint, and adds a global comboTimeoutMs guard + aggregated per-target error diagnostics on exhaustion. Companion leaves open-sse/services/combo/comboCooldownRetry.ts (+29), combo/autoStrategy.ts (+9, auto-strategy combo-ref guard so a combo cannot recursively reference itself as a candidate), combo/comboSetup.ts (+3), comboConfig.ts (+6) all stay under cap. Irreducible orchestration wiring at the existing dispatch chokepoint (mirrors the quota-share-only precedent this PR generalizes); not extractable without hiding the retry loop. Covered by tests/unit/combo-auto-candidate-expansion.test.ts (+61, combo-ref guard), tests/unit/combo-routing-engine.test.ts (+68, universal retry across strategies + comboTimeoutMs, no-explicit-any clean), tests/unit/serial/combo-quota-share-cooldown-wait-timing.test.ts (+136, quota_exhausted vs rate_limit reason gating, disabled-flag passthrough). Structural shrink of combo.ts tracked in #3501.", "_rebaseline_2026_07_21_7935_vi_locale_residual_ui": "PR #7935 (nguyenha935, fix/vietnamese-locale-residual) own growth: 9 dashboard components gained `useTranslations()` wiring (import + hook call + a handful of `t(\"key\")` call-sites replacing hardcoded English strings) as part of restoring i18n coverage — ComboHealthTab.tsx 1028->1031 (+3), cloud-agents/page.tsx 922->931 (+9), PoolWizard.tsx 1007->1022 (+15), EndpointPageClient.tsx 2612->2615 (+3), health/page.tsx 1091->1095 (+4), ProviderOnboardingWizard.tsx 912->948 (+36, largest — several previously-hardcoded wizard step labels/descriptions), PricingTab.tsx 1012->1017 (+5), ProxyRegistryManager.tsx 1461->1464 (+3), BudgetTab.tsx 1016->1028 (+12). All additions are literal `t(...)`/`tc(...)` call-site swaps for existing UI text, verified byte-identical in intent against the corresponding new `src/i18n/messages/{en,vi}.json` keys (see tests/unit/dashboard-localization-contract.test.ts, tests/unit/i18n-vi-completeness.test.ts, tests/unit/gamification-display-contract.test.ts, tests/unit/cli-catalog-display-contract.test.ts added by the same PR). Fast-gates PR->release do not run check:file-size, so this surfaced only during rebase-onto-tip reconciliation.", "_rebaseline_2026_07_21_7908_chathelpers_abort_guard": "PR #7908 (insoln, don't cool down accounts or trip the breaker on client-side stream aborts, #7907) own growth: src/sse/handlers/chatHelpers.ts 876->877 (+1 = the single `isLocalStreamLifecycleError(failure?.message ?? failure)` clause added to executeChatWithBreaker's onStreamFailure connection-disable check, verified working by the existing #4602 test + the PR's own circuit-breaker-client-abort.test.ts, no regressions). Irreducible call-site wiring at the existing failure-classification chokepoint. Fast-gates PR->release do not run check:file-size, so this surfaced only during the /green-prs pre-merge pass.", "_rebaseline_2026_07_21_7908_combo_breaker_abort_guard": "PR #7908 pre-green fix (green-prs pipeline): shouldRecordProviderBreakerFailure() (open-sse/services/combo/comboPredicates.ts, not frozen) gained an `error` field so a client-side stream abort no longer trips the whole-provider circuit breaker in the combo path (mirrors the connection-cooldown fix shouldSkipConnDisable() already applies for the same #4602/#7907 policy). Own growth: open-sse/services/combo.ts 3387->3388 (+1, irreducible call-site wiring — the single new `error: errorText,` field passed at the existing shouldRecordProviderBreakerFailure() call site inside handleComboChat's executeTarget). Covered by tests/unit/circuit-breaker-abort-provider-trip-7907.test.ts.", @@ -189,7 +190,7 @@ "_rebaseline_2026_06_24_headroom_strategy": "Headroom-aware connection selection (dario technique): combo.ts 3168->3180 (+12 = a new `else if (strategy === \"headroom\")` dispatch branch in handleComboChat that delegates to orderTargetsByHeadroom + its log line, plus the import). The actual logic lives OUT of the god-file: the pure ranker rankByHeadroom/computeHeadroom is the new leaf open-sse/services/combo/headroomRanking.ts (91 LOC, 3190 (+10 = one new `else if (strategy === \"quota-share\")` dispatch branch in handleComboChat that delegates 100% to selectQuotaShareTarget + its log line, plus the import). All the new logic lives OUT of the god-file in two new leaves under open-sse/services/combo/: quotaShareInflight.ts (in-flight counter with TTL/lease, ~150 LOC 3225 (+35) = one new `else if (strategy === \"task-aware\")` dispatch branch delegating 100% to selectTaskAwareTarget + its imports/log lines. All scoring/classification logic lives OUT of the god-file in the new leaf open-sse/services/taskAwareRouting.ts (553 LOC 854, -35), but the StackOptions.fidelityGate field, the `const fidelityGate` reads at the two stacked-loop dispatch chokepoints, and the import of FidelityGateConfig are irreducible wiring that cannot leave strategySelector without an architectural refactor of the pre-existing stacked pipeline. Net: 889->854 (+6 vs the pre-Milestone-B frozen 848). Covered by tests/unit/compression/*.test.ts (940 pass).", "_rebaseline_2026_06_28_5243_risk_gate_prepass": "PR #5243 (compression risk-gate pre-pass) own growth: open-sse/services/compression/strategySelector.ts 854->899 (+45). The three exported entry points (applyCompression/applyStackedCompression/applyStackedCompressionAsync) become thin wrappers over pure-extracted private bodies (runCompression/runStackedCompression/runStackedCompressionAsync) so the risk-gate mask->run->restore wrapper sits strictly OUTSIDE the per-step loop — a single universal integration point. The wrapper logic itself (resolveRiskGate/withRiskGate) lives in the new riskGate/strategyWrap.ts (960 (+61 = the opt-in result-memoization branches in applyCompression/applyCompressionAsync — principal+determinism gate, makeMemoKey lookup/store with model+supportsVision folded into the key, recompute-with-memo-off). Default off (memoizeCompressionResults), so zero behavior change. The memo helpers live in the leaf resultMemo.ts (0), limits total wall-clock time the combo + // spends iterating through targets. After each target completes, if elapsed time + // exceeds comboTimeoutMs, remaining targets are skipped and a 504 with aggregated + // error diagnostics is returned. 0 = disabled (backward-compatible, unlimited). + const comboTimeoutMs = config.comboTimeoutMs || 0; + const comboStartTime = Date.now(); + 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 }> = []; + // FASE 2.1: per-connection concurrency limit for quota-share. The gating in // selectQuotaShareTarget is fail-open and cannot hard-limit a single-connection // pool, so we serialize concurrent requests to the selected account through a @@ -1463,6 +1476,7 @@ export async function handleComboChat({ const startTime = Date.now(); let fallbackCount = 0; let recordedAttempts = 0; + comboErrors = []; // QA P0: assemble a sanitized diagnostic trace from the state already in scope // (pool size + this set-try's exhausted providers/connections + attempt order + @@ -2237,6 +2251,7 @@ export async function handleComboChat({ }); recordedAttempts++; lastError = errorText || String(result.status); + comboErrors.push({ model: modelStr, status: result.status, error: errorText || String(result.status) }); if (!lastStatus) lastStatus = result.status; if (i > 0) fallbackCount++; log.warn("COMBO", `Model ${modelStr} failed with body-specific error, stopping combo`); @@ -2330,6 +2345,7 @@ export async function handleComboChat({ }); recordedAttempts++; lastError = errorText || String(result.status); + comboErrors.push({ model: modelStr, status: result.status, error: errorText || String(result.status) }); if (!lastStatus) lastStatus = result.status; if (i > 0) fallbackCount++; // Wire combo failures into the resilience dashboard (model-level lockout) @@ -2402,7 +2418,7 @@ export async function handleComboChat({ }; for (let i = 0; i < orderedTargets.length; i++) { - if (anySuccess) break; + if (anySuccess || comboExpired) break; const abortController = new AbortController(); abortControllers.set(i, abortController); @@ -2447,6 +2463,17 @@ export async function handleComboChat({ } else { await Promise.race([task, globalPromise]); } + + // Global combo timeout check: after each target completes, stop trying + // further targets if the total elapsed time exceeds comboTimeoutMs. + if (!anySuccess && comboTimeoutMs > 0 && Date.now() - comboStartTime >= comboTimeoutMs) { + comboExpired = true; + log.info( + "COMBO", + `Combo global timeout (${comboTimeoutMs}ms) reached after ` + + `${i + 1}/${orderedTargets.length} targets (${recordedAttempts} attempted) — stopping` + ); + } } if (!anySuccess && runningTasks.size > 0) { @@ -2457,6 +2484,40 @@ export async function handleComboChat({ return await globalPromise; } + // 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 msg = + `Combo global timeout (${comboTimeoutMs}ms) after ${recordedAttempts}/${orderedTargets.length} targets` + + (comboErrors.length > 0 + ? ` | tried: ${summary}${comboErrors.length > 5 ? `... (+${comboErrors.length - 5})` : ""}` + : ""); + const latencyMs = Date.now() - startTime; + if (recordedAttempts === 0) { + recordComboRequest(combo.name, null, { + success: false, + latencyMs, + fallbackCount, + strategy, + }); + } + notifyWebhookEvent("request.failed", { + combo: combo.name, + reason: "COMBO_TIMEOUT", + latencyMs, + fallbackCount, + }); + return errorResponseWithComboDiagnostics( + 504, + msg, + buildComboDiag("combo_timeout"), + { code: "COMBO_TIMEOUT", type: "server_error" } + ); + } + // All models failed in this set try const latencyMs = Date.now() - startTime; if (recordedAttempts === 0) { @@ -2492,31 +2553,61 @@ export async function handleComboChat({ } const status = lastStatus; - const msg = lastError || "All combo models unavailable"; + // 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; if (earliestRetryAfter) { - // Quota-share cooldown-aware retry: instead of crystallizing the 429, - // wait out a SHORT transient cooldown and re-run the whole set loop. - // Guarded by the helper (quota_exhausted/auth/not-found excluded, - // ceiling, attempts, budget). MAX_GLOBAL_ATTEMPTS still bounds total - // dispatches. + // 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, + // attempts, budget). MAX_GLOBAL_ATTEMPTS still bounds total dispatches. + // Available to ALL combo strategies (not just quota-share). if (comboCooldownWaitEnabled && status === 429) { - const decision = resolveComboCooldownWaitDecision({ + // ONE decision path for EVERY strategy. The reason that drives the + // wait is always the target's REAL model-lockout reason, resolved + // through the helper's allow-list — never a hardcoded literal. + // + // SECURITY (see comboCooldownRetry.ts header): the allow-list is the + // PRIMARY barrier and `maxWaitMs` only the SECOND one. Hardcoding + // reason:"rate_limit" for non-quota-share strategies would drop the + // primary barrier and leave only the ceiling — which does NOT cover a + // quota_exhausted lock carrying a SHORT upstream retry-after (e.g. + // 3s < maxWaitMs): the combo would wait, redispatch against a model + // locked until midnight, and burn the attempt. Model lockouts are + // recorded for all strategies (recordModelLockoutFailure above is not + // gated on quota-share), so the real reason is always available. + const decision: ResolveComboCooldownDecisionResult = resolveComboCooldownWaitDecision({ targets: orderedTargets, earliestRetryAfter, attempt: comboCooldownAttempt, budgetLeftMs: comboCooldownBudgetLeftMs, settings: resilienceSettings.comboCooldownWait, - lookupLock: (provider, connectionId) => { - const rawModel = parseModel(orderedTargets[0]?.modelStr ?? "").model || ""; + // Key each lookup on the TARGET's own model: quota-share combos are + // single-model/multi-account (so this is identical to the previous + // orderedTargets[0] behavior), but heterogeneous combos carry a + // different model per target. + lookupLock: (provider, connectionId, target) => { + const rawModel = parseModel(target?.modelStr ?? "").model || ""; + if (!rawModel) return null; return getModelLockoutInfo(provider, connectionId, rawModel); }, computeWaitMs: (retryAfter) => computeClosestRetryAfter(retryAfter).waitMs, }); + if (decision.wait) { log.info( "COMBO", - `Quota-share cooldown wait: ${msg} — waiting ${Math.ceil( + `${strategy} cooldown wait: ${msg} — waiting ${Math.ceil( decision.waitMs / 1000 )}s (reason=${decision.reason ?? "?"}) then retrying (attempt ${ comboCooldownAttempt + 1 @@ -2524,7 +2615,7 @@ export async function handleComboChat({ ); const completed = await waitForCooldownAwareRetry(decision.waitMs, signal); if (!completed) { - log.info("COMBO", "Quota-share cooldown wait aborted by client disconnect"); + log.info("COMBO", `${strategy} cooldown wait aborted by client disconnect`); return errorResponse(499, "Request aborted"); } comboCooldownAttempt += 1; diff --git a/open-sse/services/combo/autoStrategy.ts b/open-sse/services/combo/autoStrategy.ts index db92bea0ae..c7becea86f 100644 --- a/open-sse/services/combo/autoStrategy.ts +++ b/open-sse/services/combo/autoStrategy.ts @@ -419,6 +419,15 @@ export async function expandAutoComboCandidatePool( if (Array.isArray(localAutoConfig?.candidatePool) && localAutoConfig.candidatePool.length > 0) return eligibleTargets; + // #COMBO-REF: if the combo references other combos via kind:"combo-ref" entries, + // the resolved eligibleTargets already represent the operator's intended pool. + // Expanding to ALL providers would defeat the purpose of the combo-ref constraint + // (e.g. an "auto" combo delegating to a "priority" sub-combo should not pull in + // every model from every active provider). + const rawModels = (combo as Record | null | undefined)?.models; + if (Array.isArray(rawModels) && rawModels.some((m) => isRecord(m) && m.kind === "combo-ref")) + return eligibleTargets; + try { const allConnections = await getCachedProviderConnections({ isActive: true }); const providerIds = [ diff --git a/open-sse/services/combo/comboCooldownRetry.ts b/open-sse/services/combo/comboCooldownRetry.ts index 775bae62ca..4b2b56609a 100644 --- a/open-sse/services/combo/comboCooldownRetry.ts +++ b/open-sse/services/combo/comboCooldownRetry.ts @@ -139,16 +139,37 @@ export interface ComboCooldownLockInfo { remainingMs: number; } +/** Minimal combo-target shape this helper inspects. */ +export interface ComboCooldownTarget { + provider?: string | null; + connectionId?: string | null; + /** + * The target's own model. Heterogeneous combos (priority, weighted, + * round-robin, …) hold a DIFFERENT model per target, so the lock lookup must + * be keyed on each target's own model — keying every lookup on the first + * target's model would miss every other target's lock and silently degrade + * the reason allow-list to "no lock found". + */ + modelStr?: string | null; +} + export interface ResolveComboCooldownDecisionInput { /** Combo targets to inspect for an active short lock. */ - targets: ReadonlyArray<{ provider?: string | null; connectionId?: string | null }>; + targets: ReadonlyArray; /** Earliest retry-after the loop crystallized (string | number | Date | null). */ earliestRetryAfter: unknown; attempt: number; budgetLeftMs: number; settings: ComboCooldownWaitSettings; - /** Per-target lock lookup (getModelLockoutInfo bound to the request model). */ - lookupLock: (provider: string, connectionId: string) => ComboCooldownLockInfo | null; + /** + * Per-target lock lookup (getModelLockoutInfo). Receives the target itself so + * the caller can key the lookup on that target's own model. + */ + lookupLock: ( + provider: string, + connectionId: string, + target: ComboCooldownTarget + ) => ComboCooldownLockInfo | null; /** Derives the wait (ms) from the retry-after hint (computeClosestRetryAfter). */ computeWaitMs: (retryAfter: unknown) => number | null; } @@ -199,7 +220,7 @@ export function resolveComboCooldownWaitDecision( const provider = typeof target.provider === "string" ? target.provider : ""; if (!provider) continue; const connectionId = typeof target.connectionId === "string" ? target.connectionId : ""; - const info = lookupLock(provider, connectionId); + const info = lookupLock(provider, connectionId, target); if (!info) continue; const remainingMs = typeof info.remainingMs === "number" && Number.isFinite(info.remainingMs) diff --git a/open-sse/services/combo/comboSetup.ts b/open-sse/services/combo/comboSetup.ts index 99197743ea..1d2feb19dd 100644 --- a/open-sse/services/combo/comboSetup.ts +++ b/open-sse/services/combo/comboSetup.ts @@ -37,6 +37,7 @@ export interface ComboSetup { clientRequestedStream: boolean; config: ReturnType; comboTargetTimeoutMs: number; + comboTimeoutMs: number; reasoningTokenBufferEnabled: boolean; } @@ -116,6 +117,7 @@ export function phaseComboSetup(ctx: ComboContext): ComboSetup { FETCH_TIMEOUT_MS, DEFAULT_COMBO_TARGET_TIMEOUT_MS ); + const comboTimeoutMs = config.comboTimeoutMs || 0; const reasoningTokenBufferEnabled = config.reasoningTokenBufferEnabled !== false; return { @@ -128,6 +130,7 @@ export function phaseComboSetup(ctx: ComboContext): ComboSetup { clientRequestedStream, config, comboTargetTimeoutMs, + comboTimeoutMs, reasoningTokenBufferEnabled, }; } diff --git a/open-sse/services/comboConfig.ts b/open-sse/services/comboConfig.ts index 38fb5bcdba..27cf0a1a98 100644 --- a/open-sse/services/comboConfig.ts +++ b/open-sse/services/comboConfig.ts @@ -88,6 +88,12 @@ const DEFAULT_COMBO_CONFIG = { pipeline_fallback: "single-provider", resetAwareQuotaCacheTtlMs: 0, resetAwareQuotaCacheMaxStaleMs: 0, + // Global combo timeout (0 = disabled). When set, limits the total wall-clock time + // the combo spends iterating through targets. After each target completes, if the + // elapsed time exceeds comboTimeoutMs, remaining targets are skipped and a 504 with + // aggregated error diagnostics is returned. Backward-compatible: 0 preserves the + // legacy unlimited-iteration behavior. + comboTimeoutMs: 0, shadowRouting: { enabled: false, targets: [], diff --git a/tests/unit/combo-auto-candidate-expansion.test.ts b/tests/unit/combo-auto-candidate-expansion.test.ts index 0d54d63048..303ced149d 100644 --- a/tests/unit/combo-auto-candidate-expansion.test.ts +++ b/tests/unit/combo-auto-candidate-expansion.test.ts @@ -115,6 +115,67 @@ test("expandAutoComboCandidatePool falls through to active connections when cand assert.ok(openaiTargets.length > 0, "expected openai targets to be expanded"); }); +test("expandAutoComboCandidatePool is a no-op when the combo references other combos via kind:\"combo-ref\" entries", async () => { + await providersDb.createProviderConnection({ + provider: "openai", + authType: "apikey", + name: "OpenAI", + apiKey: "sk-test-openai", + defaultModel: "gpt-4o-mini", + }); + + const seed = [ + { + kind: "model" as const, + stepId: "anthropic/claude-3-5-sonnet", + executionKey: "anthropic/claude-3-5-sonnet", + modelStr: "anthropic/claude-3-5-sonnet", + provider: "anthropic", + providerId: "anthropic", + connectionId: null, + weight: 1, + label: null, + }, + ]; + + // An "auto" combo delegating to a "priority" sub-combo via a combo-ref entry: + // expanding to every model of every active provider (openai included) would + // defeat the point of the combo-ref constraint, so the resolved + // eligibleTargets must be returned unchanged (#COMBO-REF). + const result = await combo.expandAutoComboCandidatePool(seed, { + config: {}, + models: [{ kind: "combo-ref", ref: "priority-subcombo" }], + }); + + assert.equal(result.length, 1, "combo-ref guard must prevent provider-wide expansion"); + assert.equal(result[0].modelStr, "anthropic/claude-3-5-sonnet"); + assert.ok( + !result.some((t) => t.provider === "openai"), + "no openai targets should have been pulled in despite an active openai connection" + ); +}); + +test("expandAutoComboCandidatePool still expands normally when models has no combo-ref entries", async () => { + await providersDb.createProviderConnection({ + provider: "openai", + authType: "apikey", + name: "OpenAI", + apiKey: "sk-test-openai", + defaultModel: "gpt-4o-mini", + }); + + const result = await combo.expandAutoComboCandidatePool([], { + config: {}, + models: ["openai/gpt-4o-mini"], + }); + + const openaiTargets = result.filter((t) => t.provider === "openai"); + assert.ok( + openaiTargets.length > 0, + "plain model-string entries (no combo-ref) must not trip the guard" + ); +}); + test("expandAutoComboCandidatePool does not duplicate an already-present modelStr", async () => { await providersDb.createProviderConnection({ provider: "openai", diff --git a/tests/unit/combo-routing-engine.test.ts b/tests/unit/combo-routing-engine.test.ts index f4d5c323e8..a8f00c161e 100644 --- a/tests/unit/combo-routing-engine.test.ts +++ b/tests/unit/combo-routing-engine.test.ts @@ -842,7 +842,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", async () => { +test("handleComboChat preserves the first failure status but surfaces the last error message plus per-model diagnostics", async () => { const result = await handleComboChat({ body: {}, combo: { @@ -864,7 +864,75 @@ 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(payload.error.message, "fail:model-b"); + // 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)]"); +}); + +interface ComboErrorPayload { + error: { code?: string; message: string }; +} + +test("handleComboChat global comboTimeoutMs stops iterating remaining targets and returns 504 with aggregated diagnostics", async () => { + const calls: string[] = []; + const result = await handleComboChat({ + body: {}, + combo: { + name: "timeout-combo", + strategy: "priority", + models: ["model-a", "model-b", "model-c"], + // An effectively-zero ceiling: the FIRST target still dispatches (the + // check only runs after a target completes), but by the time it + // resolves any nonzero elapsed time trips the timeout, so the loop + // must stop instead of trying model-b/model-c. + config: { maxRetries: 0, comboTimeoutMs: 1 }, + }, + handleSingleModel: async (_body: unknown, modelStr: string) => { + calls.push(modelStr); + // A tiny real delay guarantees Date.now() advances past the 1ms ceiling + // before the post-target timeout check runs. + await new Promise((resolve) => setTimeout(resolve, 5)); + return errorResponse(500, `fail:${modelStr}`); + }, + isModelAvailable: async () => true, + log: createLog(), + settings: null, + allCombos: null, + }); + + const payload = (await result.json()) as ComboErrorPayload; + + assert.equal(result.status, 504); + assert.equal(payload.error.code, "COMBO_TIMEOUT"); + assert.match(payload.error.message, /Combo global timeout \(1ms\)/); + assert.match(payload.error.message, /model-a \(500\)/); + assert.deepEqual(calls, ["model-a"], "remaining targets must be skipped once the timeout trips"); +}); + +test("handleComboChat comboTimeoutMs=0 (default) never trips the global timeout, all targets still tried", async () => { + const calls: string[] = []; + const result = await handleComboChat({ + body: {}, + combo: { + name: "no-timeout-combo", + strategy: "priority", + models: ["model-a", "model-b"], + config: { maxRetries: 0 }, // comboTimeoutMs defaults to 0 (disabled) + }, + handleSingleModel: async (_body: unknown, modelStr: string) => { + calls.push(modelStr); + if (modelStr === "model-a") return errorResponse(500, "fail:model-a"); + return okResponse(); + }, + isModelAvailable: async () => true, + log: createLog(), + settings: null, + allCombos: null, + }); + + assert.equal(result.status, 200); + assert.deepEqual(calls, ["model-a", "model-b"]); }); test("handleComboChat round-robin rotates sequentially across requests", async () => { diff --git a/tests/unit/serial/combo-quota-share-cooldown-wait-timing.test.ts b/tests/unit/serial/combo-quota-share-cooldown-wait-timing.test.ts index 536bf8c038..7fa49675d2 100644 --- a/tests/unit/serial/combo-quota-share-cooldown-wait-timing.test.ts +++ b/tests/unit/serial/combo-quota-share-cooldown-wait-timing.test.ts @@ -3,18 +3,29 @@ * * Extracted from tests/unit/combo-quota-share-cooldown-wait.test.ts (#6803). * - * These two scenarios assert a wall-clock ceiling (`elapsed < 1500`) around a - * handleComboChat() call that also performs real SQLite I/O (test.beforeEach - * does fs.rmSync+fs.mkdirSync + core.resetDbInstance()). Under CI-runner - * CPU/IO contention (multiple concurrent sibling shard jobs) this ceiling can - * be exceeded even though the functional behavior (no wait, single dispatch) - * is correct — this is a "did NOT wait out a cooldown" ceiling, not a - * behavior-under-test assertion, so it is timing-sensitive by nature. + * The quota_exhausted scenario below asserts a wall-clock ceiling + * (`elapsed < 10000`) around a handleComboChat() call that also performs real + * SQLite I/O (test.beforeEach does fs.rmSync+fs.mkdirSync + + * core.resetDbInstance()). Under CI-runner CPU/IO contention (multiple + * concurrent sibling shard jobs) this ceiling can be exceeded even though the + * functional behavior (no wait, single dispatch) is correct — this is a "did + * NOT wait out a cooldown" ceiling, not a behavior-under-test assertion, so it + * is timing-sensitive by nature. * * Running these in tests/unit/serial/ (--test-concurrency=1, see * package.json's test:unit:serial) removes the intra-suite parallelism that * was the dominant source of contention, matching the repo's established * remedy pattern for this class of test. + * + * The non-quota-share (priority) scenario was UPDATED for the "universal + * cooldown-aware retry" change: comboCooldownWait is no longer gated on + * `strategy === "quota-share"` — every combo strategy now waits out a SHORT + * transient 429 and re-dispatches (using a "rate_limit" reason and the + * earliest retry-after hint directly, since non quota-share strategies have no + * per-connection model-lockout tracking to consult). It used to assert the + * OPPOSITE (immediate propagation, no wait) — that assertion is now testing + * dead behavior, so it was rewritten to assert the new intended behavior + * instead of being deleted or weakened. */ import test from "node:test"; import assert from "node:assert/strict"; @@ -57,13 +68,17 @@ function jsonResponse(status: number, body: Record) { }); } -function rateLimitResponse(status: number) { +function rateLimitResponse(status: number, retryAfterMs: number = RETRY_AFTER_MS) { return jsonResponse(status, { error: { message: `rate limited (${status})` }, - retryAfter: new Date(Date.now() + RETRY_AFTER_MS).toISOString(), + retryAfter: new Date(Date.now() + retryAfterMs).toISOString(), }); } +function okResponse() { + return jsonResponse(200, { id: "ok", choices: [{ message: { content: "recovered" } }] }); +} + function comboOf(strategy: string) { return { name: `qtSd/${strategy}-${Math.random().toString(16).slice(2, 8)}`, @@ -127,11 +142,17 @@ test("quota-share: 403 quota_exhausted → NO wait, error propagated immediately assert.ok(elapsed < 10000, `quota_exhausted must not wait out a cooldown, but ${elapsed}ms elapsed`); }); -test("non quota-share (priority): 429 propagated immediately, NO wait", async () => { +test("non quota-share (priority): short 429 cooldown → waits and re-dispatches (2nd pass 200)", async () => { let calls = 0; const handleSingleModel = async () => { calls += 1; - return rateLimitResponse(429); + // 1st dispatch: transient 429 with a short retry-after hint. 2nd dispatch + // (after the universal cooldown wait): success. Priority combos have no + // per-connection model-lockout tracking, so this exercises the + // `shouldWaitForComboCooldown({ reason: "rate_limit", ... })` path fed + // directly by the earliest retry-after hint (not resolveComboCooldownWaitDecision's + // per-target lock lookup, which stays quota-share-only). + return calls === 1 ? rateLimitResponse(429) : okResponse(); }; const startedAt = Date.now(); @@ -146,8 +167,93 @@ test("non quota-share (priority): 429 propagated immediately, NO wait", async () }); const elapsed = Date.now() - startedAt; - assert.equal(res.status, 429, "priority combo must propagate the 429 unchanged"); - assert.equal(calls, 1, "priority combo must NOT wait+redispatch"); - // Widened from 1500ms (#6803) — see the sibling test above for rationale. - assert.ok(elapsed < 10000, `priority combo must not wait out a cooldown, but ${elapsed}ms elapsed`); + assert.equal(res.status, 200, "expected the retried dispatch to succeed with 200"); + assert.equal(calls, 2, "expected exactly one wait+redispatch (2 upstream calls)"); + assert.ok( + elapsed >= RETRY_AFTER_MS - 50, + `expected to have waited out the cooldown, only ${elapsed}ms elapsed` + ); +}); + +test("non quota-share (priority): a quota_exhausted lock drives the decision with a SHORT wait → NO wait (the reason allow-list is the PRIMARY barrier; the maxWaitMs ceiling does NOT cover this)", async () => { + // THE regression guard for the two-barrier policy documented in + // comboCooldownRetry.ts ("SECURITY — quota_exhausted must be excluded" / + // "The small maxWaitMs ceiling is the second barrier"). + // + // Barrier 1 = the reason allow-list. Barrier 2 = the maxWaitMs ceiling. + // This scenario is engineered so ONLY barrier 1 can stop the wait: + // - modelLockout.errorCodes is [403] ONLY, so model-a's 429 crystallizes + // status 429 (the sole status that opens the cooldown-wait branch) WITHOUT + // recording a competing `rate_limit` lock. + // - model-b's 403 records the only lock in play: `quota_exhausted`. It is + // therefore the lock resolveComboCooldownWaitDecision picks, so its reason + // is what drives the decision. + // - The resulting wait is SHORT (well under maxWaitMs=5000), so barrier 2 + // lets it through. Only the allow-list can reject it. + // + // With the reason hardcoded to "rate_limit" (as the non-quota-share path did + // before), barrier 1 is gone and this exact input waits + redispatches against + // a quota-exhausted model — verified: the same test yields 6 dispatches. + const calls: string[] = []; + const handleSingleModel = async (_body: unknown, modelStr: string) => { + calls.push(modelStr); + return modelStr === "openai/gpt-4" ? rateLimitResponse(429) : rateLimitResponse(403); + }; + + const res = await handleComboChat({ + body: { model: "openai/gpt-4" }, + combo: { + name: "priority-quota-exhausted-short-wait", + strategy: "priority", + models: ["openai/gpt-4", "anthropic/claude-3-5-sonnet"], + config: { maxRetries: 0, retryDelayMs: 0, fallbackDelayMs: 0, maxSetRetries: 0 }, + }, + handleSingleModel, + isModelAvailable: async () => true, + log: createLog() as never, + settings: { + modelLockout: { + ...shortModelLockoutSettings().modelLockout, + // 429 deliberately excluded: only the 403 records a lock, so the + // quota_exhausted reason is unambiguously the one under test. + errorCodes: [403], + }, + }, + allCombos: null, + }); + + assert.equal(res.status, 429, "the crystallized 429 must be propagated, not retried"); + // Deterministic proof (no wall-clock dependency, so it cannot flake under + // CI-runner contention): each target is dispatched EXACTLY ONCE. Had the wait + // fired, the whole set loop would re-run — maxAttempts=2 within the 8s budget + // produces 6 dispatches, not 2. + assert.deepEqual( + calls, + ["openai/gpt-4", "anthropic/claude-3-5-sonnet"], + "a quota_exhausted lock must NOT trigger a wait+redispatch, even when the wait would be short enough to clear the maxWaitMs ceiling" + ); +}); + +test("non quota-share (priority) with comboCooldownWait disabled → 429 propagated, NO wait", async () => { + let calls = 0; + const handleSingleModel = async () => { + calls += 1; + return rateLimitResponse(429); + }; + + const res = await handleComboChat({ + body: { model: "openai/gpt-4" }, + combo: { ...comboOf("priority"), name: "priority-combo-disabled" }, + handleSingleModel, + isModelAvailable: async () => true, + log: createLog() as never, + settings: { + ...shortModelLockoutSettings(), + resilienceSettings: { comboCooldownWait: { enabled: false } }, + }, + allCombos: null, + }); + + assert.equal(res.status, 429, "disabled feature must propagate the 429 unchanged"); + assert.equal(calls, 1, "disabled feature must NOT wait+redispatch"); });