From 5847c4392295bd3e6f2ad890ca40e91f9008ee8c Mon Sep 17 00:00:00 2001 From: lorenzozane Date: Thu, 17 Sep 2026 00:36:35 +0800 Subject: [PATCH] fix(resilience): name collision keys, surface header drops, retry embeds, skip far-reset pings (#13766) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Merged. Four small, independently justified changes, each with its own regression test — naming both sides of a case-insensitive key collision, surfacing the dropped-header count to the caller instead of only to the log, one retry before a memory is left unvectorized, and skipping warm pings for a window whose reset is more than 24h out. The first-seen-wins resolution and the caller-visible behaviour of everything else are unchanged. Validated as a combined board first (this PR merged with the 21 siblings of the same wave on the release tip): eslint with the frozen suppressions, typecheck:core, check:open-sse-typecheck, complexity, cognitive-complexity, changelog-integrity, i18n new-key coverage, docs-counts, docs-sync, migration-numbering, provider-consistency and a duplicate-identifier audit all green, plus 176 passing / 0 failing focused node:test cases across the 25 test files the wave touches and the dashboard test under Vitest (2/0). Then re-validated alone on the fresh tip before this merge: conflicts re-resolved, file sizes rebaselined for this PR's own growth, eslint and this PR's focused tests re-run. Thank you for keeping each of the four minimal and commented — that is what made this reviewable as one PR. --- open-sse/handlers/chatCore/responseHeaders.ts | 13 +++ src/lib/memory/embedding/index.ts | 32 ++++++- src/lib/memory/store.ts | 6 +- src/lib/modelMetadataRegistry.ts | 17 ++-- src/lib/services/quotaAutoPing.ts | 7 ++ src/shared/constants/quotaAutoPing.ts | 5 ++ tests/unit/13601-embed-retry.test.ts | 43 +++++++++ .../13601-header-drop-count-surfaced.test.ts | 47 ++++++++++ tests/unit/13601-quota-far-reset-skip.test.ts | 89 +++++++++++++++++++ ...3601-registry-collision-names-keys.test.ts | 45 ++++++++++ 10 files changed, 296 insertions(+), 8 deletions(-) create mode 100644 tests/unit/13601-embed-retry.test.ts create mode 100644 tests/unit/13601-header-drop-count-surfaced.test.ts create mode 100644 tests/unit/13601-quota-far-reset-skip.test.ts create mode 100644 tests/unit/13601-registry-collision-names-keys.test.ts diff --git a/open-sse/handlers/chatCore/responseHeaders.ts b/open-sse/handlers/chatCore/responseHeaders.ts index 8206304544..54e3ddc4e0 100644 --- a/open-sse/handlers/chatCore/responseHeaders.ts +++ b/open-sse/handlers/chatCore/responseHeaders.ts @@ -40,6 +40,14 @@ const STREAMING_RESPONSE_HEADER_DENYLIST = new Set([ */ const CODEX_TURN_STATE_RESPONSE_HEADER = "x-codex-turn-state"; +/** + * #13601: when upstream headers exceed the forwarding budget, the drop is + * surfaced to the caller with this count header instead of staying log-only. + * Diagnostic headers already win the budget via getForwardingPriority; this + * covers the remainder so no drop is ever silent to the client. + */ +export const DROPPED_UPSTREAM_HEADERS_RESPONSE_HEADER = "X-OmniRoute-Dropped-Upstream-Headers"; + const DEFAULT_FORWARDED_HEADER_BUDGET_BYTES = 768; /** @@ -272,6 +280,11 @@ export function buildStreamingResponseHeaders( const responseHeaders: Record = { ...Object.fromEntries(forwardedHeaders), + // #13601: surface the drop to the caller so it is never silent. Only + // present when at least one header was dropped; absent otherwise. + ...(droppedHeaders.length > 0 + ? { [DROPPED_UPSTREAM_HEADERS_RESPONSE_HEADER]: String(droppedHeaders.length) } + : {}), "Content-Type": "text/event-stream", "Cache-Control": "no-cache, no-transform", Connection: "keep-alive", diff --git a/src/lib/memory/embedding/index.ts b/src/lib/memory/embedding/index.ts index 272b86ad64..336e3252eb 100644 --- a/src/lib/memory/embedding/index.ts +++ b/src/lib/memory/embedding/index.ts @@ -303,6 +303,33 @@ export async function embed( return result; } +/** + * Embed with a single retry (#13601). + * + * A failed embedding write used to skip vectorization immediately — one + * transient failure (slow potion load, brief remote 5xx) left the memory + * stored but never vectorized until the next reindex sweep. The retry covers + * error results only; a thrown error still propagates to the caller's catch + * (scheduleVectorUpsert marks needs_reindex there, as before). + */ +export async function embedWithRetry( + text: string, + settings: MemorySettingsExtended, + embedFn: ( + text: string, + settings: MemorySettingsExtended + ) => Promise = embed, + maxAttempts = 2 +): Promise { + let last: EmbeddingResult | EmbeddingError | null = null; + for (let attempt = 1; attempt <= Math.max(1, maxAttempts); attempt++) { + const result = await embedFn(text, settings); + if ("vector" in result) return result; + last = result; + } + return last as EmbeddingResult | EmbeddingError; +} + /** * List providers that have embedding models, marking which ones have a configured API key. * Aggregates from EMBEDDING_PROVIDERS + local provider_nodes. @@ -381,7 +408,10 @@ export async function listEmbeddingProviders(): Promise>(); -function findInsensitive(obj: Record | null | undefined, key: string): T | undefined { +/** Test hook (#13601): exercised directly by the collision-naming unit test. */ +export function findInsensitive( + obj: Record | null | undefined, + key: string +): T | undefined { if (!obj || !key) return undefined; if (key in obj) return obj[key]; let index = lowercaseIndexCache.get(obj); if (!index) { index = new Map(); + const firstKeyByLower = new Map(); for (const [k, v] of Object.entries(obj)) { const lowerKey = k.toLowerCase(); // Warn once at index-build time (not per-lookup) if two keys collide // case-insensitively — a real data-quality signal from an upstream sync (e.g. // models.dev returning both "OpenAI" and "openai" as distinct provider keys). - // Matches the pre-fix scan's silent first-match-wins behavior, just surfaced - // instead of swallowed. - if (index.has(lowerKey)) { + // Names both keys so the operator can tell which entries clash; resolution + // stays deterministic first-seen-wins (#13601). + const firstKey = firstKeyByLower.get(lowerKey); + if (firstKey !== undefined) { console.warn( - `[modelMetadataRegistry] findInsensitive: case-insensitive key collision on "${lowerKey}" — keeping first-seen value, later one discarded` + `[modelMetadataRegistry] findInsensitive: case-insensitive key collision on "${lowerKey}" ("${firstKey}" vs "${k}") — keeping first-seen value, later one discarded` ); continue; } + firstKeyByLower.set(lowerKey, k); index.set(lowerKey, v); } lowercaseIndexCache.set(obj, index); diff --git a/src/lib/services/quotaAutoPing.ts b/src/lib/services/quotaAutoPing.ts index 1f7d2aede4..5d678a627e 100644 --- a/src/lib/services/quotaAutoPing.ts +++ b/src/lib/services/quotaAutoPing.ts @@ -34,6 +34,7 @@ import { refreshAndUpdateCredentialsWithResolver } from "@/lib/usage/providerLim import { getCircuitBreaker } from "@/shared/utils/circuitBreaker"; import { QUOTA_AUTOPING_FAILURE_COOLDOWN_MS, + QUOTA_AUTOPING_FAR_RESET_SKIP_MS, QUOTA_AUTOPING_PROVIDERS, QUOTA_AUTOPING_REFRESH_AHEAD_MS, QUOTA_AUTOPING_TICK_INTERVAL_MS, @@ -346,6 +347,12 @@ function shouldSendPing( resetKey: string, nowMs: number ): boolean { + // #13601: warming a window whose reset is days away has no benefit — the + // window cannot roll soon, so the ping can only fail (quota-hammering). + const resetAtMs = new Date(resetAt).getTime(); + if (Number.isFinite(resetAtMs) && resetAtMs - nowMs > QUOTA_AUTOPING_FAR_RESET_SKIP_MS) { + return false; + } if ( providerConfig.skipWhenBlockingQuotaExhausted && hasExhaustedBlockingQuota(quotas, providerConfig.quotaKey) diff --git a/src/shared/constants/quotaAutoPing.ts b/src/shared/constants/quotaAutoPing.ts index 4cb90254af..64f5ec9917 100644 --- a/src/shared/constants/quotaAutoPing.ts +++ b/src/shared/constants/quotaAutoPing.ts @@ -15,6 +15,11 @@ export const QUOTA_AUTOPING_FAILURE_COOLDOWN_MS = 15 * 60 * 1000; // within this window of it (Codex resetAt slides constantly while idle, so we // still poll every tick, but this bounds how eagerly we ping right after a slide). export const QUOTA_AUTOPING_REFRESH_AHEAD_MS = 5 * 60 * 1000; +// #13601: never ping to warm a window whose reset is further out than this. +// The Codex session window rolls every ~5h; a reset days away means the +// account is long-term exhausted/disabled and the ping can only fail — +// previously one such account produced 146 futile `ping failed` warns. +export const QUOTA_AUTOPING_FAR_RESET_SKIP_MS = 24 * 60 * 60 * 1000; export type QuotaAutoPingProviderConfig = { settingsKey: string; diff --git a/tests/unit/13601-embed-retry.test.ts b/tests/unit/13601-embed-retry.test.ts new file mode 100644 index 0000000000..b5e38a2653 --- /dev/null +++ b/tests/unit/13601-embed-retry.test.ts @@ -0,0 +1,43 @@ +// #13601.3: a failed memory vector embedding write must be retried once +// before it is logged and queued for reindex — a single transient failure +// (slow potion load, brief remote 5xx) should not silently skip vectorization. +import { test } from "node:test"; +import assert from "node:assert/strict"; + +const { embedWithRetry } = await import("../../src/lib/memory/embedding/index.ts"); + +type EmbedResult = { vector: number[] } | { reason: string; message: string }; + +const settings = {} as Parameters[1]; + +test("#13601.3: success on the first attempt performs exactly one embed", async () => { + let calls = 0; + const result = (await embedWithRetry("hello", settings, async () => { + calls += 1; + return { vector: [1, 2, 3] }; + })) as EmbedResult; + assert.equal(calls, 1); + assert.ok("vector" in result); +}); + +test("#13601.3: a transient first failure is retried and can still succeed", async () => { + let calls = 0; + const result = (await embedWithRetry("hello", settings, async () => { + calls += 1; + if (calls === 1) return { reason: "request_failed", message: "boom" }; + return { vector: [1, 2, 3] }; + })) as EmbedResult; + assert.equal(calls, 2); + assert.ok("vector" in result); +}); + +test("#13601.3: a persistent failure surfaces the last error after one retry", async () => { + let calls = 0; + const result = (await embedWithRetry("hello", settings, async () => { + calls += 1; + return { reason: "request_failed", message: `boom-${calls}` }; + })) as EmbedResult; + assert.equal(calls, 2); + assert.ok(!("vector" in result)); + if (!("vector" in result)) assert.equal(result.message, "boom-2"); +}); diff --git a/tests/unit/13601-header-drop-count-surfaced.test.ts b/tests/unit/13601-header-drop-count-surfaced.test.ts new file mode 100644 index 0000000000..6c03a288c2 --- /dev/null +++ b/tests/unit/13601-header-drop-count-surfaced.test.ts @@ -0,0 +1,47 @@ +// #13601.2: when upstream response headers exceed the forwarding budget, the +// drop must be counted/surfaced to the caller — and diagnostic headers +// (request IDs, retry-after, rate-limit) must win the budget. +import { test } from "node:test"; +import assert from "node:assert/strict"; + +const { buildStreamingResponseHeaders, resetDroppedHeaderWarnFingerprints } = + await import("../../open-sse/handlers/chatCore/responseHeaders.ts"); + +const meta = {} as Parameters[1]; +const silentLog = { warn: () => {}, debug: () => {} }; + +function budgetBustingHeaders(): Headers { + return new Headers({ + "x-request-id": "req-123", + "retry-after": "30", + "x-ratelimit-remaining": "10", + "x-drop-alpha": "a".repeat(600), + "x-drop-beta": "b".repeat(600), + }); +} + +test("#13601.2: a drop count header surfaces silent header drops to the caller", () => { + resetDroppedHeaderWarnFingerprints(); + const headers = buildStreamingResponseHeaders(budgetBustingHeaders(), meta, silentLog); + const countHeader = headers["X-OmniRoute-Dropped-Upstream-Headers"]; + assert.ok(countHeader !== undefined, "expected a dropped-headers count header"); + assert.ok(Number.parseInt(countHeader, 10) >= 1, `expected count >= 1, got ${countHeader}`); +}); + +test("#13601.2: diagnostic headers survive budget pressure", () => { + resetDroppedHeaderWarnFingerprints(); + const headers = buildStreamingResponseHeaders(budgetBustingHeaders(), meta, silentLog); + assert.equal(headers["x-request-id"], "req-123"); + assert.equal(headers["retry-after"], "30"); + assert.equal(headers["x-ratelimit-remaining"], "10"); +}); + +test("#13601.2: no drops means no count header", () => { + resetDroppedHeaderWarnFingerprints(); + const headers = buildStreamingResponseHeaders( + new Headers({ "x-request-id": "req-1" }), + meta, + silentLog + ); + assert.equal(headers["X-OmniRoute-Dropped-Upstream-Headers"], undefined); +}); diff --git a/tests/unit/13601-quota-far-reset-skip.test.ts b/tests/unit/13601-quota-far-reset-skip.test.ts new file mode 100644 index 0000000000..d78d766beb --- /dev/null +++ b/tests/unit/13601-quota-far-reset-skip.test.ts @@ -0,0 +1,89 @@ +// #13601.4: QuotaAutoPing must not hammer an account whose quota reset is +// days away — warming a window that cannot roll soon has no benefit, so the +// ping is skipped instead of failing every tick (146 warns for one account). +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"; + +process.env.DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-13601-farreset-")); + +const { runQuotaAutoPingTick, createQuotaAutoPingState } = + await import("../../src/lib/services/quotaAutoPing.ts"); +const { resetDbInstance } = await import("../../src/lib/db/core.ts"); + +test.after(() => { + resetDbInstance(); +}); + +const NOW_MS = new Date("2026-09-13T12:00:00.000Z").getTime(); +const FAR_RESET = new Date(NOW_MS + 30 * 24 * 3600 * 1000).toISOString(); +const FAR_RESET_SLID = new Date(new Date(FAR_RESET).getTime() + 120_000).toISOString(); +const NEAR_RESET = new Date(NOW_MS + 4 * 3600 * 1000).toISOString(); +const NEAR_RESET_SLID = new Date(new Date(NEAR_RESET).getTime() + 120_000).toISOString(); + +function farResetDeps(getCodexUsage: () => Promise) { + const calls = { executorExecute: 0, getExecutor: 0, updateProviderConnection: 0 }; + return { + calls, + deps: { + getSettings: async () => ({ codexAutoPing: { connections: { "codex-1": true } } }), + getProviderConnections: async () => [ + { id: "codex-1", provider: "codex", authType: "oauth", accessToken: "token" }, + ], + updateProviderConnection: async () => { + calls.updateProviderConnection += 1; + return null; + }, + refreshAndUpdateCredentials: async (connection: unknown) => ({ + connection: connection as never, + }), + getCodexUsage: getCodexUsage as never, + throttleQuotaFetch: async () => {}, + getExecutor: () => { + calls.getExecutor += 1; + return { + execute: async () => { + calls.executorExecute += 1; + return { response: { ok: true, text: async () => "" } }; + }, + }; + }, + canExecuteProvider: () => true, + isConnectionUnavailableToAuxiliaryActivity: async () => false, + resolvePingModel: async () => "gpt-5-codex", + }, + }; +} + +test("#13601.4: no ping when resetAt is a month away, even after a window slide", async () => { + let resetAt = FAR_RESET; + const { deps, calls } = farResetDeps(async () => ({ + quotas: { session: { used: 1, resetAt } }, + })); + const state = createQuotaAutoPingState(); + + await runQuotaAutoPingTick(deps as never, state as never, () => NOW_MS); + assert.equal(calls.getExecutor, 0); + + resetAt = FAR_RESET_SLID; + await runQuotaAutoPingTick(deps as never, state as never, () => NOW_MS); + assert.equal(calls.getExecutor, 0); + assert.equal(calls.executorExecute, 0); +}); + +test("#13601.4: a near reset still pings after a window slide", async () => { + let resetAt = NEAR_RESET; + const { deps, calls } = farResetDeps(async () => ({ + quotas: { session: { used: 1, resetAt } }, + })); + const state = createQuotaAutoPingState(); + + await runQuotaAutoPingTick(deps as never, state as never, () => NOW_MS); + assert.equal(calls.executorExecute, 0); + + resetAt = NEAR_RESET_SLID; + await runQuotaAutoPingTick(deps as never, state as never, () => NOW_MS); + assert.equal(calls.executorExecute, 1); +}); diff --git a/tests/unit/13601-registry-collision-names-keys.test.ts b/tests/unit/13601-registry-collision-names-keys.test.ts new file mode 100644 index 0000000000..632a539fac --- /dev/null +++ b/tests/unit/13601-registry-collision-names-keys.test.ts @@ -0,0 +1,45 @@ +// #13601.1: modelMetadataRegistry case-insensitive key collisions must name +// BOTH colliding keys so the operator can tell which upstream entries clash +// (e.g. `Kimi-K2.6` vs `kimi-k2.6` as route keys). First-seen-wins stays. +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"; + +process.env.DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-13601-collision-")); + +const { findInsensitive } = await import("../../src/lib/modelMetadataRegistry.ts"); + +function captureWarns(fn: () => void): string[] { + const messages: string[] = []; + const original = console.warn; + console.warn = (...args: unknown[]) => { + messages.push(args.map(String).join(" ")); + }; + try { + fn(); + } finally { + console.warn = original; + } + return messages; +} + +test("#13601.1: collision warning names both keys, not just the normalized form", () => { + const warns = captureWarns(() => { + // "KIMI-K2.6" matches no exact key, forcing the lowercase-index path. + findInsensitive({ "Kimi-K2.6": "upper", "kimi-k2.6": "lower" }, "KIMI-K2.6"); + }); + assert.equal(warns.length, 1); + assert.ok(warns[0].includes("Kimi-K2.6"), `warning must name first key: ${warns[0]}`); + assert.ok(warns[0].includes("kimi-k2.6"), `warning must name second key: ${warns[0]}`); +}); + +test("#13601.1: collision resolution stays deterministic (first-seen wins)", () => { + const obj = { "Kimi-K2.6": "upper", "kimi-k2.6": "lower" }; + const warns = captureWarns(() => { + assert.equal(findInsensitive(obj, "KIMI-K2.6"), "upper"); + assert.equal(findInsensitive(obj, "kImI-k2.6"), "upper"); + }); + assert.equal(warns.length, 1); +});