From defa0f07b2e79ff201cb9a61cd5e84f8f503398a Mon Sep 17 00:00:00 2001 From: Dizzle <112548150+maxmad64bis@users.noreply.github.com> Date: Tue, 15 Sep 2026 20:52:33 +0200 Subject: [PATCH] fix(routing): await stale provider-pin clears and gate swallowed errors plus fire-and-forget async (#13614) Stale provider-pin (`clearStaleLKGP`) clears are no longer silent: the fire-and-forget promise carries a `.catch` that warns with combo, comboId and executionKey, and a `check:routing-error-guard` npm script keeps the inventory of swallowed catches in the routing hot path from growing. Maintainer rework before merge (kept the idea, no default behavior change): - The awaited DB writes in the fallback loop were reverted (they added latency and SQLite lock exposure on every skip); the clear stays non-blocking. - The guard keys its allowlist by file + normalized catch body instead of line numbers (the PR's version broke on any edit) and is wired as an npm script only, not in CI; the unused stats counters were dropped. Validated first on the combined board of all 38 PRs of this batch (10 merged as-is, 28 after the maintainer rework) on top of release/v3.8.51 c0f92ec: typecheck:core, check:open-sse-typecheck and check:dashboard-typecheck clean; ESLint clean on every changed file; file-size (rebaselined for the combined growth), complexity, cognitive-complexity, changelog-integrity, docs-counts, docs-sync, migration-numbering and i18n new-key gates green; 735 focused node:test cases with the only batch-caused failure (a flag-count assertion) fixed. Then re-validated alone on the fresh release tip right before this merge: ESLint on the changed files, typecheck:core, check:open-sse-typecheck, the file-size/complexity/changelog gates and this PR's own tests. Thanks @maxmad64bis! --- .../fixes/13614-routing-error-guard.md | 1 + open-sse/services/combo.ts | 29 +- open-sse/services/combo/staleLkgpClear.ts | 44 +++ package.json | 1 + .../allowlist-routing-swallowed-catch.json | 372 ++++++++++++++++++ scripts/check/allowlist-void-async.json | 15 + scripts/check/check-routing-error-guard.mjs | 274 +++++++++++++ tests/unit/check-routing-error-guard.test.ts | 133 +++++++ .../unit/combo/stale-lkgp-clear-13614.test.ts | 100 +++++ 9 files changed, 943 insertions(+), 26 deletions(-) create mode 100644 changelog.d/fixes/13614-routing-error-guard.md create mode 100644 open-sse/services/combo/staleLkgpClear.ts create mode 100644 scripts/check/allowlist-routing-swallowed-catch.json create mode 100644 scripts/check/allowlist-void-async.json create mode 100644 scripts/check/check-routing-error-guard.mjs create mode 100644 tests/unit/check-routing-error-guard.test.ts create mode 100644 tests/unit/combo/stale-lkgp-clear-13614.test.ts diff --git a/changelog.d/fixes/13614-routing-error-guard.md b/changelog.d/fixes/13614-routing-error-guard.md new file mode 100644 index 0000000000..5a8818ea01 --- /dev/null +++ b/changelog.d/fixes/13614-routing-error-guard.md @@ -0,0 +1 @@ +- **fix(routing):** a failed stale-pin (LKGP) clear on the combo fallback path now logs the combo and execution key while staying non-blocking, and a new opt-in `npm run check:routing-error-guard` script (not wired into CI) flags new swallowed catches and unanchored fire-and-forget async on routing paths, with frozen entries keyed by file and catch body instead of line numbers ([#13614](https://github.com/diegosouzapw/OmniRoute/pull/13614)) — thanks @maxmad64bis diff --git a/open-sse/services/combo.ts b/open-sse/services/combo.ts index 89ab50a142..8b8da3a0e2 100644 --- a/open-sse/services/combo.ts +++ b/open-sse/services/combo.ts @@ -129,6 +129,7 @@ import { dispatchWithCooldownRetry } from "./combo/comboAttemptLoop.ts"; import { evaluateExecuteTargetGates } from "./combo/executeTargetGates.ts"; import { executeTargetAttempt } from "./combo/executeTargetAttempt.ts"; import type { AttemptLoopDeps, AttemptLoopState } from "./combo/attemptLoopTypes.ts"; +import { clearStaleLKGP } from "./combo/staleLkgpClear.ts"; export { RESET_WINDOW_NAMES, QUOTA_SOFT_DEPRIORITIZE_FACTOR, setCandidateQuotaSoftPenalty }; export { scoreAutoTargets, expandAutoComboCandidatePool }; @@ -175,32 +176,8 @@ export function releaseStickyPinOnFailure( clearStickyBinding(messageHash); } -/** - * Clear persisted LKGP pins when a target fails or is skipped due to - * exhaustion, cooldown, or unavailability (#11911 #919). - */ -export function clearStaleLKGP( - comboName: string, - executionKey?: string | null, - comboId?: string | null, - log?: { warn?: (tag: string, msg: string, data?: unknown) => void } | null, - tag: string = "COMBO" -): void { - void (async () => { - try { - const { clearLKGP } = await import("@/lib/db/settings"); - const promises: Promise[] = [clearLKGP(comboName, comboId || comboName)]; - if (executionKey) { - promises.push(clearLKGP(comboName, executionKey)); - } - await Promise.all(promises); - } catch (err) { - log?.warn?.(tag, "Failed to clear Last Known Good Provider. This is non-fatal.", { - err, - }); - } - })(); -} +// #11911 #919: non-blocking stale-pin clear whose failures log with combo context. +export { clearStaleLKGP }; const DEFAULT_MODEL_P95_MS: Record = { "grok-4-fast-non-reasoning": 1143, diff --git a/open-sse/services/combo/staleLkgpClear.ts b/open-sse/services/combo/staleLkgpClear.ts new file mode 100644 index 0000000000..5d9824b4a9 --- /dev/null +++ b/open-sse/services/combo/staleLkgpClear.ts @@ -0,0 +1,44 @@ +/** + * Clear persisted LKGP pins when a combo target fails or is skipped for exhaustion, + * cooldown or unavailability (#11911 #919). + * + * Non-blocking by design: the fallback loop never waits on these SQLite writes. A + * failed clear is not silent — it logs a warning carrying the combo and the + * execution key. The returned promise never rejects: routing callers ignore it, + * tests await it. + * + * @internal — re-exported by combo.ts as `clearStaleLKGP`. + */ + +type WarnLogger = { warn?: (tag: string, msg: string, data?: unknown) => void } | null; +type ClearLkgp = (comboName: string, modelKey: string) => Promise; + +async function clearPins( + comboName: string, + executionKey: string | null | undefined, + comboId: string | null | undefined, + clearLKGP: ClearLkgp | undefined +): Promise { + const clear = clearLKGP ?? (await import("@/lib/db/settings")).clearLKGP; + const keys = [comboId || comboName, ...(executionKey ? [executionKey] : [])]; + await Promise.all(keys.map((key) => clear(comboName, key))); +} + +export function clearStaleLKGP( + comboName: string, + executionKey?: string | null, + comboId?: string | null, + log?: WarnLogger, + tag: string = "COMBO", + /** Test seam; the routing path always resolves clearLKGP from @/lib/db/settings. */ + clearLKGP?: ClearLkgp +): Promise { + return clearPins(comboName, executionKey, comboId, clearLKGP).catch((err: unknown) => { + log?.warn?.(tag, "Failed to clear Last Known Good Provider. This is non-fatal.", { + combo: comboName, + comboId: comboId ?? null, + executionKey: executionKey ?? null, + err, + }); + }); +} diff --git a/package.json b/package.json index 5230e1710e..a42bf3a0cf 100644 --- a/package.json +++ b/package.json @@ -201,6 +201,7 @@ "check:agent-skills-sync": "node --import tsx/esm scripts/skills/generate-agent-skills.mjs", "check:build-scope": "node scripts/check/check-build-scope.mjs", "check:error-helper": "node scripts/check/check-error-helper.mjs", + "check:routing-error-guard": "node scripts/check/check-routing-error-guard.mjs", "check:migration-numbering": "node scripts/check/check-migration-numbering.mjs", "check:public-creds": "node scripts/check/check-public-creds.mjs", "check:db-rules": "node scripts/check/check-db-rules.mjs", diff --git a/scripts/check/allowlist-routing-swallowed-catch.json b/scripts/check/allowlist-routing-swallowed-catch.json new file mode 100644 index 0000000000..5aa8719efe --- /dev/null +++ b/scripts/check/allowlist-routing-swallowed-catch.json @@ -0,0 +1,372 @@ +{ + "$schema": "allowlist-routing-swallowed-catch", + "_comment": "Frozen swallowed catches on routing paths for scripts/check/check-routing-error-guard.mjs. Keyed by file + normalized catch-body snippet (not line numbers); count = identical bodies in that file. Do NOT add entries without a justification; shrink or remove an entry when its catch is fixed.", + "entries": [ + { + "file": "open-sse/services/combo.ts", + "snippet": "// keep empty stats — auto-combo will use runtime + bootstrap signals", + "count": 1, + "reason": "stats fallback to defaults, auto path uses runtime signals" + }, + { + "file": "open-sse/services/combo.ts", + "snippet": "connectionPoolCounts.set(provider, 0); connectionsByProvider.set(provider, []);", + "count": 1, + "reason": "pool counts fallback to empty lists" + }, + { + "file": "open-sse/services/combo.ts", + "snippet": "// keep default cost", + "count": 1, + "reason": "cost fallback to default pricing" + }, + { + "file": "open-sse/services/combo.ts", + "snippet": "log?.debug?.( \"COMBO\", `resolveTargetTimeoutMsForTarget connection lookup failed: ${ err instanceof Error ? err.message ", + "count": 1, + "reason": "logged at debug, undefined fallback" + }, + { + "file": "open-sse/services/combo/applyStrategyOrdering.ts", + "snippet": "log.warn(\"COMBO\", \"Failed to retrieve Last Known Good Provider. This is non-fatal.\", { err });", + "count": 1, + "reason": "logged, best-effort provider read fallback" + }, + { + "file": "open-sse/services/combo/applyStrategyOrdering.ts", + "snippet": "log.warn({ err }, \"manifest routing failed, falling back to standard strategy\");", + "count": 1, + "reason": "logged, manifest routing fallback" + }, + { + "file": "open-sse/services/combo/autoStrategy.ts", + "snippet": "log.warn?.( \"COMBO\", `Tag routing failed to load connections for provider=${providerId}: ${error instanceof Error ? erro", + "count": 1, + "reason": "logged, tag routing connections fallback" + }, + { + "file": "open-sse/services/combo/autoStrategy.ts", + "snippet": "// Best-effort candidate expansion only: if loading active connections or // provider models fails, fall back to the exp", + "count": 1, + "reason": "expanded targets fallback, abort-safe" + }, + { + "file": "open-sse/services/combo/autoStrategy.ts", + "snippet": "return null;", + "count": 1, + "reason": "null fallback, best-effort expansion" + }, + { + "file": "open-sse/services/combo/comboPredicates.ts", + "snippet": "// A DB read failure must never block dispatch — fall through to the upstream call. return null;", + "count": 1, + "reason": "null fallback, DB read failure" + }, + { + "file": "open-sse/services/combo/concurrencyCaps.ts", + "snippet": "return null; // fail-open: never block routing on a lookup error", + "count": 1, + "reason": "null fallback, fail-open routing" + }, + { + "file": "open-sse/services/combo/connectionAwareExpansion.ts", + "snippet": "// Fail-open (spec section 3.1): expansion is a best-effort pre-filter, never a // hard dependency. Auth-layer gates rem", + "count": 1, + "reason": "logged, fail-open expansion" + }, + { + "file": "open-sse/services/combo/dispatchPrelude.ts", + "snippet": "return false;", + "count": 1, + "reason": "false fallback, pinned dispatch check" + }, + { + "file": "open-sse/services/combo/dispatchPrelude.ts", + "snippet": "pinnedClone = pinnedResult;", + "count": 1, + "reason": "pinned clone fallback, release on failure" + }, + { + "file": "open-sse/services/combo/dispatchPrelude.ts", + "snippet": "log.warn( \"COMBO\", `Pinned model ${pinnedModel} threw error: ${pinErr instanceof Error ? pinErr.message : String(pinErr)", + "count": 1, + "reason": "logged, pinned model fallthrough" + }, + { + "file": "open-sse/services/combo/executeTargetAttempt.ts", + "snippet": "qualityClone = result;", + "count": 1, + "reason": "clone fallback to original response" + }, + { + "file": "open-sse/services/combo/executeTargetAttempt.ts", + "snippet": "deps.log.warn( \"COMBO\", \"Failed to record Last Known Good Provider. This is non-fatal.\", { err, } );", + "count": 1, + "reason": "logged at warn, success-path best-effort LKGP persist (failure only loses a routing optimization)" + }, + { + "file": "open-sse/services/combo/executeTargetAttempt.ts", + "snippet": "/* Clone parse failed */", + "count": 1, + "reason": "nested clone-parse fallback, error text preserved" + }, + { + "file": "open-sse/services/combo/executeTargetAttempt.ts", + "snippet": "/* Clone failed */", + "count": 1, + "reason": "clone fallback, error parse skipped" + }, + { + "file": "open-sse/services/combo/executeTargetAttempt.ts", + "snippet": "errorText = String(errorText);", + "count": 1, + "reason": "stringify fallback to String()" + }, + { + "file": "open-sse/services/combo/failureTracker.ts", + "snippet": "// Best effort — the counter still records the streak, future clears will // retry on the next threshold-cross.", + "count": 1, + "reason": "counter kept, retry on next threshold" + }, + { + "file": "open-sse/services/combo/failureTracker.ts", + "snippet": "return { count: 0, pinClearedNow: false };", + "count": 1, + "reason": "zeroed streak fallback" + }, + { + "file": "open-sse/services/combo/failureTracker.ts", + "snippet": "/* fail-open */", + "count": 1, + "reason": "fail-open tracker state fallback" + }, + { + "file": "open-sse/services/combo/failureTracker.ts", + "snippet": "return 0;", + "count": 1, + "reason": "zero fallback, fail-open counter" + }, + { + "file": "open-sse/services/combo/nativeCodexTurnPin.ts", + "snippet": "return undefined;", + "count": 1, + "reason": "undefined fallback, best-effort pin" + }, + { + "file": "open-sse/services/combo/promptCacheAffinity.ts", + "snippet": "return \"\";", + "count": 1, + "reason": "empty-string fallback" + }, + { + "file": "open-sse/services/combo/promptCacheAffinity.ts", + "snippet": "connectionsByProvider.set(provider, []);", + "count": 1, + "reason": "connections fallback to empty list" + }, + { + "file": "open-sse/services/combo/providerWildcard.ts", + "snippet": "return modelIds;", + "count": 1, + "reason": "model list fallback" + }, + { + "file": "open-sse/services/combo/quotaExhaustion.ts", + "snippet": "try { text = await response.clone().text(); } catch { // The status and trusted in-process classification remain availab", + "count": 1, + "reason": "status preserved, cloned text fallback" + }, + { + "file": "open-sse/services/combo/quotaExhaustionCutoff.ts", + "snippet": "connection = undefined;", + "count": 1, + "reason": "undefined connection fallback" + }, + { + "file": "open-sse/services/combo/quotaExhaustionCutoff.ts", + "snippet": "// Fail-open: never block routing because the preflight fetch itself errored. return { blocked: false };", + "count": 1, + "reason": "fail-open, blocked false" + }, + { + "file": "open-sse/services/combo/quotaShareConcurrency.ts", + "snippet": "// Fail-open: a saturated queue / timeout must never worsen availability — // proceed without a slot rather than reject ", + "count": 1, + "reason": "fail-open, proceed without a slot" + }, + { + "file": "open-sse/services/combo/quotaStrategies.ts", + "snippet": "log.warn?.(\"COMBO\", \"Reset-aware failed to load quota-aware connections.\", { comboName, err: error, operation: \"getProvi", + "count": 1, + "reason": "logged, quota-aware connections fallback" + }, + { + "file": "open-sse/services/combo/quotaStrategies.ts", + "snippet": "log.warn?.(\"COMBO\", \"Reset-aware quota fetch failed.\", { comboName, connectionId, err: error, operation: \"quotaFetch\", p", + "count": 1, + "reason": "logged, reset-aware quota fetch fallback" + }, + { + "file": "open-sse/services/combo/quotaStrategies.ts", + "snippet": "log.warn?.( { err: (err as Error)?.message, comboName }, \"headroom ordering failed — keeping target order\" ); return tar", + "count": 1, + "reason": "logged, headroom ordering kept" + }, + { + "file": "open-sse/services/combo/resolveAutoStrategy.ts", + "snippet": "log.warn(\"COMBO\", \"Failed to retrieve Last Known Good Provider. This is non-fatal.\", { err });", + "count": 1, + "reason": "logged, provider read best-effort" + }, + { + "file": "open-sse/services/combo/resolveAutoStrategy.ts", + "snippet": "log.warn( \"COMBO\", `Auto strategy '${routingStrategy}' failed (${err?.message || \"unknown\"}), falling back to rules` );", + "count": 1, + "reason": "logged, auto strategy rules fallback" + }, + { + "file": "open-sse/services/combo/roundRobinCombo.ts", + "snippet": "return undefined;", + "count": 1, + "reason": "undefined fallback, quota path unaffected" + }, + { + "file": "open-sse/services/combo/roundRobinCombo.ts", + "snippet": "// best-effort only", + "count": 1, + "reason": "best-effort quota reserve only" + }, + { + "file": "open-sse/services/combo/roundRobinCombo.ts", + "snippet": "rrClone = result;", + "count": 1, + "reason": "clone fallback to original" + }, + { + "file": "open-sse/services/combo/roundRobinCombo.ts", + "snippet": "log.warn( \"COMBO-RR\", \"Failed to record Last Known Good Provider. This is non-fatal.\", { err, } );", + "count": 1, + "reason": "logged at warn, success-path best-effort LKGP persist (failure only loses a routing optimization)" + }, + { + "file": "open-sse/services/combo/roundRobinCombo.ts", + "snippet": "/* Clone parse failed */", + "count": 1, + "reason": "clone-parse fallback, error text preserved" + }, + { + "file": "open-sse/services/combo/roundRobinCombo.ts", + "snippet": "/* Clone failed */", + "count": 1, + "reason": "clone fallback, error parse skipped" + }, + { + "file": "open-sse/services/combo/roundRobinCombo.ts", + "snippet": "errorText = String(errorText);", + "count": 1, + "reason": "stringify fallback to String()" + }, + { + "file": "open-sse/services/combo/roundRobinCombo.ts", + "snippet": "// G4: unexpected exception in the round-robin loop must never crash the // request silently — surface a 500 instead of ", + "count": 1, + "reason": "logged at error, 500 response surfaced" + }, + { + "file": "open-sse/services/combo/runtimeUnits.ts", + "snippet": "unitClone = response;", + "count": 1, + "reason": "clone fallback to original response" + }, + { + "file": "open-sse/services/combo/sessionStickiness.ts", + "snippet": "return undefined;", + "count": 2, + "reason": "undefined fallback, cooldown read" + }, + { + "file": "open-sse/services/combo/sessionStickiness.ts", + "snippet": "return false;", + "count": 1, + "reason": "false fallback, sticky write best-effort" + }, + { + "file": "open-sse/services/combo/sessionStickiness.ts", + "snippet": "// Completely unexpected error — fail-open return noOp;", + "count": 1, + "reason": "no-op fallback, fail-open stickiness" + }, + { + "file": "open-sse/services/combo/shadowRouting.ts", + "snippet": "// Shadow draining is best-effort and must never affect the production response.", + "count": 1, + "reason": "best-effort shadow drain only" + }, + { + "file": "open-sse/services/combo/shadowRouting.ts", + "snippet": "log.warn(\"COMBO\", \"Shadow routing skipped: failed to clone request body\", { error: error instanceof Error ? error.messag", + "count": 1, + "reason": "logged, shadow body clone skipped" + }, + { + "file": "open-sse/services/combo/shadowRouting.ts", + "snippet": "recordComboShadowRequest(combo.name, target.modelStr, { success: false, latencyMs: Date.now() - startedAt, target: toRec", + "count": 1, + "reason": "combo shadow request recorded as failed" + }, + { + "file": "open-sse/services/combo/targetResolution.ts", + "snippet": "logPipelineFallthrough(pipelineErr, log); return null;", + "count": 1, + "reason": "logged, pipeline fallthrough to null" + }, + { + "file": "open-sse/services/combo/targetSorters.ts", + "snippet": "return { modelStr, cost: Infinity };", + "count": 1, + "reason": "infinite-cost fallback" + }, + { + "file": "open-sse/services/combo/targetSorters.ts", + "snippet": "// If pricing lookup fails entirely, return original order return models;", + "count": 1, + "reason": "original order fallback" + }, + { + "file": "open-sse/services/combo/targetTimeoutRunner.ts", + "snippet": "// Diagnostic logging failed — never let this break the process.", + "count": 1, + "reason": "diagnostic logging failed" + }, + { + "file": "open-sse/services/combo/validateQuality.ts", + "snippet": "return null;", + "count": 1, + "reason": "null fallback, quality check skipped" + }, + { + "file": "open-sse/services/combo/validateQuality.ts", + "snippet": "controller.close();", + "count": 1, + "reason": "controller closed, stream cleanup" + }, + { + "file": "open-sse/services/combo/validateQuality.ts", + "snippet": "// If reading the stream fails due to a locked stream or pipe error, // the content cannot be verified — mark as invalid", + "count": 1, + "reason": "invalid fallback, unverifiable stream" + }, + { + "file": "open-sse/services/combo/validateQuality.ts", + "snippet": "return { valid: true };", + "count": 2, + "reason": "valid fallback, teardown race" + }, + { + "file": "open-sse/services/combo/validateQuality.ts", + "snippet": "// An SSE stream body is expected for streamed upstreams. Besides `data:` and // `event:` frames, the SSE spec also allo", + "count": 1, + "reason": "comment-line SSE frame skipped" + } + ] +} diff --git a/scripts/check/allowlist-void-async.json b/scripts/check/allowlist-void-async.json new file mode 100644 index 0000000000..ded2cd9f4b --- /dev/null +++ b/scripts/check/allowlist-void-async.json @@ -0,0 +1,15 @@ +{ + "$schema": "allowlist-void-async", + "entries": [ + { + "file": "open-sse/services/combo/executeTargetAttempt.ts", + "anchor": "Failed to record Last Known Good Provider", + "reason": "success-path best-effort persist; failure only loses an optimization and is logged" + }, + { + "file": "open-sse/services/combo/roundRobinCombo.ts", + "anchor": "Failed to record Last Known Good Provider", + "reason": "same as above, round-robin success path" + } + ] +} diff --git a/scripts/check/check-routing-error-guard.mjs b/scripts/check/check-routing-error-guard.mjs new file mode 100644 index 0000000000..b8769231e3 --- /dev/null +++ b/scripts/check/check-routing-error-guard.mjs @@ -0,0 +1,274 @@ +#!/usr/bin/env node +// scripts/check/check-routing-error-guard.mjs +// Gate: swallowed `catch` blocks and fire-and-forget `void (async ...)` on routing +// paths (open-sse/services/combo.ts + open-sse/services/combo/). +// +// Run with `npm run check:routing-error-guard`. It is NOT wired into CI; run it when +// touching routing error handling. +// +// Rule A (swallowed-catch): a `catch` block with no `throw` and no inline +// `// no-effect: ` marker is a violation unless frozen in +// scripts/check/allowlist-routing-swallowed-catch.json. Entries are keyed by file + +// the normalized catch-body snippet (never by line number, so unrelated edits that +// shift lines do not break the gate) with a `count` for identical bodies in one file. +// More live catches than the frozen count → violation; fewer → stale entry (anti-rot: +// lower the count or remove the entry). Chained `.catch(...)` promise handlers are +// ignored by construction. +// +// Rule B (void-async): `void (async` is a violation unless an entry in +// scripts/check/allowlist-void-async.json names the file and an `anchor` substring +// found within the next VOID_ASYNC_ANCHOR_WINDOW lines of that site; a `reason` is +// mandatory and entries matching no site are stale. +// +// Output mirrors scripts/check/check-error-helper.mjs: `file:line :: rule :: hint`. +import fs from "node:fs"; +import path from "node:path"; +import { pathToFileURL } from "node:url"; + +const cwd = process.cwd(); + +const SCOPE_FILES = [path.join(cwd, "open-sse/services/combo.ts")]; +const SCOPE_DIRS = [path.join(cwd, "open-sse/services/combo")]; +const VOID_ASYNC_ALLOWLIST_PATH = path.join(cwd, "scripts/check/allowlist-void-async.json"); +const SWALLOWED_CATCH_ALLOWLIST_PATH = path.join( + cwd, + "scripts/check/allowlist-routing-swallowed-catch.json" +); + +const NO_EFFECT_MARKER = /\/\/\s*no-effect\s*:/; +const THROW_PATTERN = /\bthrow\b/; +const VOID_ASYNC_PATTERN = /\bvoid\s*\(\s*async\b/; +export const SNIPPET_MAX_LENGTH = 120; +export const VOID_ASYNC_ANCHOR_WINDOW = 25; + +function stripStringsAndComments(source) { + // Length-preserving mask: every string/comment char becomes a space (newlines + // kept) so offsets and line numbers survive. Keyword scans use the masked copy; + // marker reads and snippets use the raw slice at the same offsets. + const chars = source.split(""); + const blank = (from, to) => { + for (let i = from; i < to; i++) if (chars[i] !== "\n") chars[i] = " "; + }; + let i = 0; + while (i < chars.length) { + const c = chars[i]; + const next = chars[i + 1]; + if (c === "/" && next === "/") { + let j = i; + while (j < chars.length && chars[j] !== "\n") j++; + blank(i, j); + i = j; + } else if (c === "/" && next === "*") { + const end = source.indexOf("*/", i + 2); + const j = end === -1 ? chars.length : end + 2; + blank(i, j); + i = j; + } else if (c === '"' || c === "'" || c === "`") { + let j = i + 1; + while (j < chars.length && (chars[j] !== c || chars[j - 1] === "\\") && chars[j] !== "\n") + j++; + blank(i, Math.min(j + 1, chars.length)); + i = Math.min(j + 1, chars.length); + } else { + i++; + } + } + return chars.join(""); +} + +function skipBalanced(masked, i, open, close) { + let depth = 0; + while (i < masked.length) { + if (masked[i] === open) depth++; + else if (masked[i] === close) { + depth--; + if (depth === 0) return i; + } + i++; + } + return -1; +} + +function findCatchBlocks(source) { + const masked = stripStringsAndComments(source); + const blocks = []; + const catchKeyword = /\bcatch\b/g; + let match; + while ((match = catchKeyword.exec(masked)) !== null) { + if (match.index > 0 && masked[match.index - 1] === ".") continue; + let i = match.index + 5; + while (i < masked.length && /\s/.test(masked[i])) i++; + if (masked[i] === "(") { + const closeParen = skipBalanced(masked, i, "(", ")"); + if (closeParen === -1) continue; + i = closeParen + 1; + } + while (i < masked.length && /\s/.test(masked[i])) i++; + if (masked[i] !== "{") continue; + const end = skipBalanced(masked, i, "{", "}"); + if (end === -1) continue; + blocks.push({ + line: source.slice(0, match.index).split("\n").length, + body: source.slice(i + 1, end), + maskedBody: masked.slice(i + 1, end), + }); + catchKeyword.lastIndex = end + 1; + } + return blocks; +} + +/** Line-independent identity of a catch body: whitespace-collapsed raw text, truncated. */ +export function catchSnippet(body) { + return body.replace(/\s+/g, " ").trim().slice(0, SNIPPET_MAX_LENGTH); +} + +/** Every catch that neither rethrows nor carries a `// no-effect:` marker. */ +export function collectSwallowedCatches(files) { + const swallowed = []; + for (const { path: rel, source } of files) { + for (const block of findCatchBlocks(source)) { + if (THROW_PATTERN.test(block.maskedBody)) continue; + if (NO_EFFECT_MARKER.test(block.body)) continue; + swallowed.push({ file: rel, line: block.line, snippet: catchSnippet(block.body) }); + } + } + return swallowed; +} + +const entryKey = (file, snippet) => `${file} :: ${snippet}`; + +/** + * Compare live swallowed catches against the frozen allowlist. + * @returns {{ violations: string[], stale: string[] }} + */ +export function evaluateSwallowedCatches(files, frozenEntries = []) { + const allowed = new Map(); + for (const entry of frozenEntries) { + allowed.set(entryKey(entry.file, entry.snippet), entry); + } + const live = new Map(); + for (const hit of collectSwallowedCatches(files)) { + const key = entryKey(hit.file, hit.snippet); + if (!live.has(key)) live.set(key, []); + live.get(key).push(hit); + } + + const violations = []; + for (const [key, hits] of live) { + const entry = allowed.get(key); + const frozenCount = entry ? Number(entry.count ?? 1) : 0; + if (entry && !String(entry.reason ?? "").trim()) { + violations.push(`${hits[0].file}:${hits[0].line} :: swallowed-catch :: entry needs a reason`); + } + for (const hit of hits.slice(frozenCount)) { + violations.push( + `${hit.file}:${hit.line} :: swallowed-catch :: add 'throw' or '// no-effect: '` + + (hit.snippet ? ` (body: ${hit.snippet})` : " (empty body)") + ); + } + } + + const stale = []; + for (const [key, entry] of allowed) { + const liveCount = live.get(key)?.length ?? 0; + const frozenCount = Number(entry.count ?? 1); + if (liveCount < frozenCount) { + stale.push(`${key} (frozen ${frozenCount}, live ${liveCount})`); + } + } + return { violations, stale }; +} + +/** + * Rule B. An allowlist entry covers a `void (async` site only when its anchor appears + * within VOID_ASYNC_ANCHOR_WINDOW lines of that site in the same file. + * @returns {{ violations: string[], stale: string[] }} + */ +export function evaluateVoidAsyncSites(files, allowlist = []) { + const violations = []; + const used = new Set(); + for (const { path: rel, source } of files) { + const lines = source.split("\n"); + for (let i = 0; i < lines.length; i++) { + if (!VOID_ASYNC_PATTERN.test(lines[i])) continue; + const window = lines.slice(i, i + VOID_ASYNC_ANCHOR_WINDOW).join("\n"); + const entry = allowlist.find( + (candidate) => candidate.file === rel && window.includes(candidate.anchor) + ); + if (!entry) { + violations.push( + `${rel}:${i + 1} :: void-async :: await the async work, attach a .catch, or add an allowlist entry` + ); + continue; + } + used.add(entry); + if (!String(entry.reason ?? "").trim()) { + violations.push(`${rel}:${i + 1} :: void-async :: allowlist entry needs a reason`); + } + } + } + const stale = allowlist + .filter((entry) => !used.has(entry)) + .map((entry) => `${entry.file} :: ${entry.anchor}`); + return { violations, stale }; +} + +function loadEntries(allowlistPath) { + const raw = JSON.parse(fs.readFileSync(allowlistPath, "utf8")); + return raw.entries ?? raw; +} + +function collectFiles() { + const files = []; + const push = (p) => { + files.push({ + path: path.relative(cwd, p).replace(/\\/g, "/"), + source: fs.readFileSync(p, "utf8"), + }); + }; + for (const file of SCOPE_FILES) { + if (fs.existsSync(file)) push(file); + } + const walk = (dir) => { + if (!fs.existsSync(dir)) return; + for (const entry of fs.readdirSync(dir, { withFileTypes: true })) { + const p = path.join(dir, entry.name); + if (entry.isDirectory()) walk(p); + else if (/\.tsx?$/.test(entry.name) && !/\.test\.tsx?$/.test(entry.name)) push(p); + } + }; + for (const dir of SCOPE_DIRS) walk(dir); + return files; +} + +function main() { + const files = collectFiles(); + const catchEntries = loadEntries(SWALLOWED_CATCH_ALLOWLIST_PATH); + const voidEntries = loadEntries(VOID_ASYNC_ALLOWLIST_PATH); + const catches = evaluateSwallowedCatches(files, catchEntries); + const voids = evaluateVoidAsyncSites(files, voidEntries); + + const violations = [...catches.violations, ...voids.violations]; + const stale = [...catches.stale, ...voids.stale]; + if (violations.length) { + console.error( + `[check-routing-error-guard] ${violations.length} violation(s) on routing paths:\n` + + violations.map((v) => ` ✗ ${v}`).join("\n") + ); + } + if (stale.length) { + console.error( + `[check-routing-error-guard] ${stale.length} stale allowlist entr(y/ies) — the site was fixed or changed; shrink or remove the entry:\n` + + stale.map((s) => ` ✗ ${s}`).join("\n") + ); + } + if (violations.length || stale.length) { + process.exitCode = 1; + return; + } + console.log( + `[check-routing-error-guard] OK (${files.length} files scanned, ${catchEntries.length} frozen catch entries, ${voidEntries.length} void-async entries)` + ); +} + +if (import.meta.url === pathToFileURL(process.argv[1] || "").href) main(); diff --git a/tests/unit/check-routing-error-guard.test.ts b/tests/unit/check-routing-error-guard.test.ts new file mode 100644 index 0000000000..dd5cc11fec --- /dev/null +++ b/tests/unit/check-routing-error-guard.test.ts @@ -0,0 +1,133 @@ +/** + * #13614 — scripts/check/check-routing-error-guard.mjs (npm run check:routing-error-guard). + * Frozen swallowed catches are keyed by file + body snippet, so line shifts never break + * the gate; void-async allowlist anchors must sit next to the site they cover. + */ +import test from "node:test"; +import assert from "node:assert/strict"; +import { readFileSync } from "node:fs"; + +const { catchSnippet, collectSwallowedCatches, evaluateSwallowedCatches, evaluateVoidAsyncSites } = + await import("../../scripts/check/check-routing-error-guard.mjs"); + +const FILE = "open-sse/services/combo/example.ts"; +const file = (source: string, path = FILE) => ({ path, source }); + +const SWALLOW = "try {\n await work();\n} catch {\n pending = fallback;\n}\n"; + +test("a bare swallowed catch is a violation when not frozen", () => { + const { violations, stale } = evaluateSwallowedCatches([file(SWALLOW)], []); + assert.equal(violations.length, 1); + assert.match(violations[0], /example\.ts:3 :: swallowed-catch ::/); + assert.deepEqual(stale, []); +}); + +test("rethrowing catches, no-effect markers and chained .catch() are not swallows", () => { + const sources = [ + "try {\n await work();\n} catch (err) {\n log.warn(err);\n throw err;\n}\n", + "try {\n clone = r.clone();\n} catch {\n // no-effect: clone fallback\n clone = r;\n}\n", + "const quota = await fetchQuota(id).catch(() => null);\n", + ]; + assert.deepEqual(collectSwallowedCatches(sources.map((s) => file(s))), []); +}); + +test("a frozen entry survives line shifts (keyed by snippet, not line number)", () => { + const frozen = [ + { + file: FILE, + snippet: catchSnippet("\n pending = fallback;\n"), + count: 1, + reason: "fallback", + }, + ]; + const shifted = "// a new line\n// another\n\n" + SWALLOW; + assert.deepEqual(evaluateSwallowedCatches([file(SWALLOW)], frozen), { + violations: [], + stale: [], + }); + assert.deepEqual(evaluateSwallowedCatches([file(shifted)], frozen), { + violations: [], + stale: [], + }); +}); + +test("counts: a second identical swallow is new, a removed one makes the entry stale", () => { + const frozen = [{ file: FILE, snippet: "pending = fallback;", count: 1, reason: "fallback" }]; + const twice = evaluateSwallowedCatches([file(SWALLOW + SWALLOW)], frozen); + assert.equal(twice.violations.length, 1); + assert.match(twice.violations[0], /example\.ts:8 ::/); + + const gone = evaluateSwallowedCatches([file("const ok = 1;\n")], frozen); + assert.deepEqual(gone.violations, []); + assert.equal(gone.stale.length, 1); + assert.match(gone.stale[0], /frozen 1, live 0/); +}); + +test("editing a frozen catch body re-flags it (the snippet no longer matches)", () => { + const frozen = [{ file: FILE, snippet: "pending = fallback;", count: 1, reason: "fallback" }]; + const edited = SWALLOW.replace("pending = fallback;", "pending = otherFallback;"); + const result = evaluateSwallowedCatches([file(edited)], frozen); + assert.equal(result.violations.length, 1); + assert.equal(result.stale.length, 1); +}); + +test("a frozen entry without a reason is rejected", () => { + const frozen = [{ file: FILE, snippet: "pending = fallback;", count: 1, reason: " " }]; + const { violations } = evaluateSwallowedCatches([file(SWALLOW)], frozen); + assert.equal(violations.length, 1); + assert.match(violations[0], /entry needs a reason/); +}); + +const VOID_SITE = + "void (async () => {\n try {\n await persist();\n } catch (err) {\n log.warn('Failed to record Last Known Good Provider', err);\n }\n})();\n"; + +test("void async: an anchored allowlist entry covers the site", () => { + const allow = [ + { file: FILE, anchor: "Failed to record Last Known Good Provider", reason: "persist" }, + ]; + assert.deepEqual(evaluateVoidAsyncSites([file(VOID_SITE)], allow), { violations: [], stale: [] }); +}); + +test("void async: an unlisted site, a reasonless entry and an orphan entry all fail", () => { + const unlisted = evaluateVoidAsyncSites( + [file("void (async () => {\n await work();\n})();\n")], + [] + ); + assert.equal(unlisted.violations.length, 1); + assert.match(unlisted.violations[0], /example\.ts:1 :: void-async ::/); + + const reasonless = evaluateVoidAsyncSites( + [file(VOID_SITE)], + [{ file: FILE, anchor: "Failed to record Last Known Good Provider" }] + ); + assert.match(reasonless.violations[0], /needs a reason/); + + const orphan = evaluateVoidAsyncSites( + [file("const x = 1;\n")], + [{ file: "open-sse/services/combo/removed.ts", anchor: "gone", reason: "left over" }] + ); + assert.deepEqual(orphan.violations, []); + assert.deepEqual(orphan.stale, ["open-sse/services/combo/removed.ts :: gone"]); +}); + +test("void async: an anchor elsewhere in the file does not cover an unrelated site", () => { + const source = + "void (async () => {\n await work();\n})();\n" + + "\n".repeat(40) + + "// Failed to record Last Known Good Provider\n"; + const { violations } = evaluateVoidAsyncSites( + [file(source)], + [{ file: FILE, anchor: "Failed to record Last Known Good Provider", reason: "persist" }] + ); + assert.equal(violations.length, 1); +}); + +test("wired as the check:routing-error-guard npm script (not a CI job)", () => { + const pkg = JSON.parse(readFileSync("package.json", "utf8")) as { + scripts: Record; + }; + assert.equal( + pkg.scripts["check:routing-error-guard"], + "node scripts/check/check-routing-error-guard.mjs" + ); +}); diff --git a/tests/unit/combo/stale-lkgp-clear-13614.test.ts b/tests/unit/combo/stale-lkgp-clear-13614.test.ts new file mode 100644 index 0000000000..6cb9858e84 --- /dev/null +++ b/tests/unit/combo/stale-lkgp-clear-13614.test.ts @@ -0,0 +1,100 @@ +/** + * #13614 — stale LKGP pin clears on the combo fallback path stay non-blocking, and a + * failed clear is logged with the combo and execution key instead of a bare error. + */ +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-stale-lkgp-13614-")); +const ORIGINAL_DATA_DIR = process.env.DATA_DIR; +process.env.DATA_DIR = TEST_DATA_DIR; + +const { clearStaleLKGP } = await import("../../../open-sse/services/combo/staleLkgpClear.ts"); +const combo = await import("../../../open-sse/services/combo.ts"); +const { setLKGP, getLKGP } = await import("../../../src/lib/db/settings.ts"); +const dbCore = await import("../../../src/lib/db/core.ts"); + +test.after(() => { + try { + dbCore.resetDbInstance(); + } catch { + /* ignore */ + } + if (ORIGINAL_DATA_DIR === undefined) delete process.env.DATA_DIR; + else process.env.DATA_DIR = ORIGINAL_DATA_DIR; + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true, maxRetries: 5, retryDelay: 100 }); +}); + +function captureWarn() { + const warnings: Array<{ tag: string; msg: string; data: Record }> = []; + return { + warnings, + log: { + warn: (tag: string, msg: string, data?: unknown) => + warnings.push({ tag, msg, data: (data ?? {}) as Record }), + }, + }; +} + +test("combo.ts re-exports the non-blocking clear (single implementation)", () => { + assert.equal(combo.clearStaleLKGP, clearStaleLKGP); +}); + +test("a failed clear resolves and warns with the combo and execution key", async () => { + const { warnings, log } = captureWarn(); + const failure = new Error("database is locked"); + const pending = clearStaleLKGP("combo-a", "ek-7", "combo-id-a", log, "COMBO-RR", async () => { + throw failure; + }); + await assert.doesNotReject(pending); + assert.equal(warnings.length, 1); + assert.equal(warnings[0].tag, "COMBO-RR"); + assert.match(warnings[0].msg, /Failed to clear Last Known Good Provider/); + assert.equal(warnings[0].data.combo, "combo-a"); + assert.equal(warnings[0].data.comboId, "combo-id-a"); + assert.equal(warnings[0].data.executionKey, "ek-7"); + assert.equal(warnings[0].data.err, failure); +}); + +test("a synchronous throw from the writer is caught the same way", async () => { + const { warnings, log } = captureWarn(); + await clearStaleLKGP("combo-b", null, null, log, "COMBO", (() => { + throw new Error("sync boom"); + }) as unknown as (c: string, k: string) => Promise); + assert.equal(warnings.length, 1); + assert.equal(warnings[0].data.executionKey, null); +}); + +test("the call returns before the writes settle (the fallback loop never waits)", async () => { + let release!: () => void; + const gate = new Promise((resolve) => { + release = resolve; + }); + const cleared: string[] = []; + let settled = false; + const pending = clearStaleLKGP("combo-c", "ek-c", "id-c", null, "COMBO", async (_c, key) => { + await gate; + cleared.push(key); + }).then(() => { + settled = true; + }); + await new Promise((resolve) => setImmediate(resolve)); + assert.equal(settled, false, "clear must still be pending while the caller moves on"); + release(); + await pending; + assert.deepEqual(cleared.sort(), ["ek-c", "id-c"]); +}); + +test("default writer clears both persisted pins in the real DB, no warning", async () => { + await setLKGP("combo-db", "combo-db-id", "openai", "conn-1"); + await setLKGP("combo-db", "ek-db", "openai", "conn-1"); + assert.ok(await getLKGP("combo-db", "combo-db-id")); + const { warnings, log } = captureWarn(); + await clearStaleLKGP("combo-db", "ek-db", "combo-db-id", log, "COMBO"); + assert.equal(await getLKGP("combo-db", "combo-db-id"), null); + assert.equal(await getLKGP("combo-db", "ek-db"), null); + assert.deepEqual(warnings, []); +});