diff --git a/changelog.d/fixes/12659-combo-skip-reasons-tiny-budget-probe.md b/changelog.d/fixes/12659-combo-skip-reasons-tiny-budget-probe.md new file mode 100644 index 000000000000..fb6fa47eda60 --- /dev/null +++ b/changelog.d/fixes/12659-combo-skip-reasons-tiny-budget-probe.md @@ -0,0 +1 @@ +- fix(sse): exempt tiny-budget reasoning probes from combo quality failure and surface persisted-cooldown skips in ALL_TARGETS_SKIPPED diagnostics (#12659) diff --git a/open-sse/services/combo/comboAttemptLoop.ts b/open-sse/services/combo/comboAttemptLoop.ts index acdcd24da564..102c27196fe9 100644 --- a/open-sse/services/combo/comboAttemptLoop.ts +++ b/open-sse/services/combo/comboAttemptLoop.ts @@ -33,7 +33,12 @@ import { waitForCooldownAwareRetry, } from "../../../src/sse/services/cooldownAwareRetry.ts"; import { toRetryAfterDisplayValue } from "./validateQuality.ts"; -import { finalizeComboTrace, finishComboTrace } from "./decisionTrace.ts"; +import { + finalizeComboTrace, + finishComboTrace, + getComboTrace, + summarizeSkippedTargets, +} from "./decisionTrace.ts"; import { isRetryAfterEligibleStatus } from "./unavailableRetryGate.ts"; import { withQuotaExhaustionClassification } from "./quotaExhaustion.ts"; import { @@ -133,6 +138,16 @@ export async function dispatchWithCooldownRetry(opts: { attemptOrder: state.comboAttemptOrder, terminalReason, recovery: buildRecoveryHint(terminalReason, retryAfterSeconds), + // #12659: surface per-target skip reasons (e.g. persisted_cooldown) + // that `excluded` above never captures — only worth the trace lookup + // on the diagnostic-heavy terminal reason. + skippedTargets: + terminalReason === "all_targets_skipped" + ? summarizeSkippedTargets(getComboTrace(deps.traceInvocationId)).map((g) => ({ + reason: g.reason, + targets: g.targets, + })) + : undefined, }); let globalResolve: ((res: Response) => void) | null = null; diff --git a/open-sse/services/combo/decisionTrace.ts b/open-sse/services/combo/decisionTrace.ts index 7660af7ea01f..1480ce699681 100644 --- a/open-sse/services/combo/decisionTrace.ts +++ b/open-sse/services/combo/decisionTrace.ts @@ -21,6 +21,7 @@ import { randomUUID } from "node:crypto"; export const COMBO_SKIP_REASONS = [ "circuit_open", "provider_cooldown", + "persisted_cooldown", "request_exhaustion", "model_lockout", "quota_cutoff", @@ -43,6 +44,12 @@ export interface ComboTraceEntry { decision: ComboDecision; reason?: ComboSkipReason; ts: number; + /** + * Safe, non-secret elaboration on `reason` (e.g. a cooldown reset ISO + * timestamp). SAFETY CONTRACT above still applies: never a credential + * fragment, header, or raw upstream error string. + */ + detail?: string; } export interface ComboTrace { @@ -121,9 +128,43 @@ export function recordComboDecision( decision: entry.decision, reason: entry.reason as ComboSkipReason | undefined, ts: Date.now(), + detail: entry.detail, }); } +/** One skip reason's targets, for the ALL_TARGETS_SKIPPED diagnostics body. */ +export interface SkippedTargetGroup { + reason: ComboSkipReason; + targets: string[]; + detail?: string; +} + +/** + * #12659: group a trace's skipped-before-dispatch decisions by reason so an + * ALL_TARGETS_SKIPPED 503 body can report WHY every target was skipped + * instead of an opaque `excluded: []`. Pure — takes a trace, returns groups; + * does not read or mutate the in-memory store. + */ +export function summarizeSkippedTargets(trace: ComboTrace | null): SkippedTargetGroup[] { + if (!trace) return []; + const byReason = new Map(); + for (const entry of trace.decisions) { + if (entry.decision !== "skipped_before_dispatch" || !entry.reason) continue; + const group = byReason.get(entry.reason); + if (group) { + group.targets.push(entry.target); + if (!group.detail && entry.detail) group.detail = entry.detail; + } else { + byReason.set(entry.reason, { + reason: entry.reason, + targets: [entry.target], + detail: entry.detail, + }); + } + } + return Array.from(byReason.values()); +} + export function finishComboTrace( invocationId: string, terminal: { status: number | null; errorClass?: string | null } diff --git a/open-sse/services/combo/executeTargetGates.ts b/open-sse/services/combo/executeTargetGates.ts index b0d5f3edf43e..7d71075ac90c 100644 --- a/open-sse/services/combo/executeTargetGates.ts +++ b/open-sse/services/combo/executeTargetGates.ts @@ -141,6 +141,15 @@ export async function evaluateExecuteTargetGates(opts: { if (persistedSkip) { // Lift-as-is: combo.ts skips without observeFailure / stopProtectedPriorityTarget. deps.log.info("COMBO", persistedSkip); + // #12659: this branch used to be untraced, so an ALL_TARGETS_SKIPPED + // caused purely by persisted cooldowns surfaced as an opaque + // `attempted=0, excluded=[]` diagnostics body. + recordComboDecision(deps.traceInvocationId, { + step: target.executionKey, + target: modelStr, + decision: "skipped_before_dispatch", + reason: "persisted_cooldown", + }); deps.clearStaleLKGP(deps.combo.name, target.executionKey, deps.combo.id, deps.log, "COMBO"); bumpFallback(); return { kind: "skip", result: null }; diff --git a/open-sse/services/combo/validateQuality.ts b/open-sse/services/combo/validateQuality.ts index 723a10b61451..d95dc59f1074 100644 --- a/open-sse/services/combo/validateQuality.ts +++ b/open-sse/services/combo/validateQuality.ts @@ -14,8 +14,24 @@ import { } from "../../utils/streamHelpers.ts"; import { evaluateResponseValidation, type ResponseValidationConfig } from "./responseValidation.ts"; import { getReasoningTokens } from "../../../src/lib/usage/tokenAccounting.ts"; +import { REASONING_BUFFER_MIN_TRIGGER } from "../reasoningTokenBuffer.ts"; import type { ComboRetryAfter } from "./types.ts"; +/** + * #12659: below this actual `completion_tokens` count, a reasoning-truncated + * response is a deliberate tiny-budget capability probe (#10281, e.g. Claude + * Code's `/model` check sending `max_tokens: 1`) rather than a genuine + * exhaustion of a real reasoning budget -- `completion_tokens` cannot exceed + * the caller's `max_tokens`, so a tiny count here proves a tiny budget was + * requested without needing to thread the request body through the combo + * dispatch call sites. Reuses #10281's own threshold constant instead of + * duplicating the magic number; every existing #3587 exhaustion regression + * case (512/1024/4096 completion_tokens) sits well above it. + */ +function isTinyBudgetTruncation(completionTokens: number): boolean { + return completionTokens > 0 && completionTokens < REASONING_BUFFER_MIN_TRIGGER; +} + /** * Detects tool_calls entries within one assistant message that repeat the * exact same function name + arguments verbatim -- always a bug (no @@ -801,19 +817,26 @@ export async function validateResponseQuality( // hasReasoningContent is already false and this branch never runs for them. const finishReason = typeof firstChoice.finish_reason === "string" ? firstChoice.finish_reason : ""; + const usage = json?.usage as Record | undefined; + const completionTokens = usage ? Number(usage.completion_tokens) || 0 : 0; if (finishReason === "length" || finishReason === "max_tokens") { + // #12659: a tiny deliberate capability probe (e.g. `max_tokens: 1` + // connectivity/`/model` pings) hits this exact shape on a reasoning + // model -- exempt it into the #10281 truncated-200 treatment (pass the + // original 200 through unmodified) instead of a genuine quality + // failure, so the caller never records a model-lockout for a probe. + if (isTinyBudgetTruncation(completionTokens)) return { valid: true }; return { valid: false, reason: `reasoning truncated at token limit (finish_reason: ${finishReason}) — no content output`, }; } - const usage = json?.usage as Record | undefined; if (usage) { - const completionTokens = Number(usage.completion_tokens) || 0; const reasoningTokens = getReasoningTokens(usage); // If reasoning consumed 90%+ of completion tokens, the model ran out of // budget before producing any content output. if (completionTokens > 0 && reasoningTokens >= completionTokens * 0.9) { + if (isTinyBudgetTruncation(completionTokens)) return { valid: true }; return { valid: false, reason: `reasoning consumed ${reasoningTokens}/${completionTokens} tokens — no content output`, diff --git a/open-sse/utils/error.ts b/open-sse/utils/error.ts index 5d7357eb1cf0..0f986694be18 100644 --- a/open-sse/utils/error.ts +++ b/open-sse/utils/error.ts @@ -387,6 +387,11 @@ export interface ComboExclusion { model?: string; reason: string; } +/** #12659: one skip reason's targets, surfaced on an ALL_TARGETS_SKIPPED body. */ +export interface ComboSkippedTargetGroup { + reason: string; + targets: string[]; +} export interface ComboDiagnostics { poolSize: number; attempted: number; @@ -395,6 +400,13 @@ export interface ComboDiagnostics { terminalReason: string; /** Optional next-step hint — populated when the dispatcher can recommend a recovery action. */ recovery?: ComboRecoveryHint; + /** + * #12659: per-target skip reasons (e.g. `persisted_cooldown`) recorded on the + * decision trace but not captured by `excluded` (which only sources from + * exhaustedProviders/exhaustedConnections). Optional — populated only when + * the caller has a decision trace to summarize. + */ + skippedTargets?: ComboSkippedTargetGroup[]; } function clampDiagStr(v: unknown, max = 128): string { @@ -482,6 +494,12 @@ export function sanitizeComboDiagnostics(d: ComboDiagnostics): ComboDiagnostics terminalReason: clampDiagStr(d?.terminalReason, 200), }; if (recovery) out.recovery = recovery; + if (Array.isArray(d?.skippedTargets) && d.skippedTargets.length > 0) { + out.skippedTargets = d.skippedTargets.slice(0, 32).map((g) => ({ + reason: clampDiagStr(g?.reason, 64), + targets: (g?.targets ?? []).slice(0, 32).map((t) => clampDiagStr(t, 96)), + })); + } return out; } diff --git a/tests/unit/combo-quality-tiny-budget-probe.test.ts b/tests/unit/combo-quality-tiny-budget-probe.test.ts new file mode 100644 index 000000000000..1241caba604b --- /dev/null +++ b/tests/unit/combo-quality-tiny-budget-probe.test.ts @@ -0,0 +1,19 @@ +import test from "node:test"; +import assert from "node:assert/strict"; +const { validateResponseQuality } = await import("../../open-sse/services/combo.ts"); +const silentLog = { warn: () => {} }; +function makeTinyProbeResponse(): Response { + // Mirrors the issue's reported shape verbatim: "reasoning consumed 10/10 tokens — no content output" + return new Response( + JSON.stringify({ + choices: [{ message: { content: null, reasoning_content: "Ok" }, finish_reason: "length" }], + usage: { completion_tokens: 10, reasoning_tokens: 10 }, + }), + { status: 200, headers: { "content-type": "application/json" } } + ); +} +test("#12659 EXPECTED: combo validator exempts a tiny-budget reasoning probe (finish_reason:length, tiny completion_tokens) instead of a genuine quality failure", async () => { + const res = makeTinyProbeResponse(); + const out = await validateResponseQuality(res, false, silentLog); + assert.equal(out.valid, true, `reproduces #12659: ... (reason: ${out.reason})`); +}); diff --git a/tests/unit/combo/combo-skipped-targets-summary.test.ts b/tests/unit/combo/combo-skipped-targets-summary.test.ts new file mode 100644 index 000000000000..8375860872d5 --- /dev/null +++ b/tests/unit/combo/combo-skipped-targets-summary.test.ts @@ -0,0 +1,120 @@ +/** + * #12659 — ALL_TARGETS_SKIPPED must carry per-target skip reasons. + * + * Before this fix, `executeTargetGates.ts`'s persisted-connection-cooldown + * skip branch never called `recordComboDecision`, `persisted_cooldown` was + * not even an allowlisted `ComboSkipReason`, and the 503 diagnostics body's + * `excluded[]` only ever sourced from exhaustedProviders/exhaustedConnections + * — so a persisted-cooldown-only failure surfaced as an opaque + * `attempted=0, excluded=[]`. + */ +import { test, beforeEach } 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-skipped-")); +process.env.DATA_DIR = TEST_DATA_DIR; +process.env.API_KEY_SECRET = process.env.API_KEY_SECRET || "combo-skipped-targets-secret"; + +const { + COMBO_SKIP_REASONS, + recordComboDecision, + resetComboTraceStore, + startComboTrace, + summarizeSkippedTargets, + getComboTrace, +} = await import("../../../open-sse/services/combo/decisionTrace.ts"); + +beforeEach(() => resetComboTraceStore()); + +test("#12659: persisted_cooldown is an allowlisted skip reason", () => { + assert.ok( + (COMBO_SKIP_REASONS as readonly string[]).includes("persisted_cooldown"), + "persisted_cooldown must be recordable — it used to have no allowlist entry at all" + ); +}); + +test("#12659: a persisted-cooldown skip is grouped into skippedTargets[] by reason", () => { + startComboTrace("combo-skip-1", { strategy: "priority", comboName: "my-combo" }); + recordComboDecision("combo-skip-1", { + step: "step-1", + target: "zai/glm-5.3", + decision: "skipped_before_dispatch", + reason: "persisted_cooldown", + }); + recordComboDecision("combo-skip-1", { + step: "step-2", + target: "openai/gpt-x", + decision: "skipped_before_dispatch", + reason: "persisted_cooldown", + }); + recordComboDecision("combo-skip-1", { + step: "step-3", + target: "anthropic/claude-y", + decision: "skipped_before_dispatch", + reason: "circuit_open", + }); + + const groups = summarizeSkippedTargets(getComboTrace("combo-skip-1")); + const persisted = groups.find((g) => g.reason === "persisted_cooldown"); + assert.ok(persisted, "expected a persisted_cooldown group in the summary"); + assert.deepEqual(persisted!.targets.sort(), ["openai/gpt-x", "zai/glm-5.3"]); + + const circuit = groups.find((g) => g.reason === "circuit_open"); + assert.ok(circuit); + assert.deepEqual(circuit!.targets, ["anthropic/claude-y"]); +}); + +test("#12659: summarizeSkippedTargets ignores dispatched/not_reached decisions", () => { + startComboTrace("combo-skip-2", { strategy: "priority", comboName: "my-combo" }); + recordComboDecision("combo-skip-2", { + step: "step-1", + target: "zai/glm-5.3", + decision: "dispatched", + }); + recordComboDecision("combo-skip-2", { + step: "step-2", + target: "openai/gpt-x", + decision: "not_reached", + }); + const groups = summarizeSkippedTargets(getComboTrace("combo-skip-2")); + assert.deepEqual(groups, []); +}); + +test("#12659: summarizeSkippedTargets is safe on a null/missing trace", () => { + assert.deepEqual(summarizeSkippedTargets(null), []); + assert.deepEqual(summarizeSkippedTargets(getComboTrace("does-not-exist")), []); +}); + +test("#12659: diagnostics body groups persisted-cooldown skips WITHOUT leaking a connection id or a stack trace", async () => { + const { errorResponseWithComboDiagnostics } = await import("../../../open-sse/utils/error.ts"); + const res = errorResponseWithComboDiagnostics( + 503, + "Service temporarily unavailable: all targets were skipped by pre-dispatch filters", + { + poolSize: 2, + attempted: 0, + excluded: [], + attemptOrder: [], + terminalReason: "all_targets_skipped", + skippedTargets: [{ reason: "persisted_cooldown", targets: ["zai/glm-5.3", "openai/gpt-x"] }], + }, + { code: "ALL_TARGETS_SKIPPED", type: "service_unavailable" } + ); + const body = (await res.json()) as { + diagnostics: { skippedTargets?: Array<{ reason: string; targets: string[] }> }; + }; + assert.ok(body.diagnostics.skippedTargets, "diagnostics.skippedTargets must be present"); + assert.deepEqual(body.diagnostics.skippedTargets![0], { + reason: "persisted_cooldown", + targets: ["zai/glm-5.3", "openai/gpt-x"], + }); + const serialized = JSON.stringify(body); + // Task notes (#12659): a skip reason must never leak an upstream stack + // trace or a credential/account-id fragment — this body was built through + // buildErrorBody()/sanitizeComboDiagnostics(), never raw err.stack. + assert.ok(!/\bat\s+\/[\w./-]+:\d+:\d+/.test(serialized), "no stack-trace frame in the body"); + assert.ok(!serialized.includes("0217fa47"), "no connection/account id leaked into the body"); +});