mirror of
https://github.com/diegosouzapw/OmniRoute.git
synced 2026-09-11 01:13:02 +03:00
Compare commits
1 Commits
fix/12783-
...
fix/12659-
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
e29123c359 |
@@ -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)
|
||||
@@ -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;
|
||||
|
||||
@@ -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<ComboSkipReason, SkippedTargetGroup>();
|
||||
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 }
|
||||
|
||||
@@ -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 };
|
||||
|
||||
@@ -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<string, unknown> | 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<string, unknown> | 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`,
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
|
||||
|
||||
19
tests/unit/combo-quality-tiny-budget-probe.test.ts
Normal file
19
tests/unit/combo-quality-tiny-budget-probe.test.ts
Normal file
@@ -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})`);
|
||||
});
|
||||
120
tests/unit/combo/combo-skipped-targets-summary.test.ts
Normal file
120
tests/unit/combo/combo-skipped-targets-summary.test.ts
Normal file
@@ -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");
|
||||
});
|
||||
Reference in New Issue
Block a user