mirror of
https://github.com/diegosouzapw/OmniRoute.git
synced 2026-09-19 05:12:16 +03:00
fix(resilience): name collision keys, surface header drops, retry embeds, skip far-reset pings (#13766)
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.
This commit is contained in:
@@ -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<string, string> = {
|
||||
...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",
|
||||
|
||||
@@ -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<EmbeddingResult | EmbeddingError> = embed,
|
||||
maxAttempts = 2
|
||||
): Promise<EmbeddingResult | EmbeddingError> {
|
||||
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<EmbeddingProviderListing
|
||||
// Cheap sync pass first: which providers CAN derive an endpoint at all.
|
||||
const derivable: string[] = [];
|
||||
for (const id of Object.keys(chatRegistry)) {
|
||||
if (!getEmbeddingProvider(id) && deriveEmbeddingProviderForChatProvider(id, chatRegistry[id])) {
|
||||
if (
|
||||
!getEmbeddingProvider(id) &&
|
||||
deriveEmbeddingProviderForChatProvider(id, chatRegistry[id])
|
||||
) {
|
||||
derivable.push(id);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -7,7 +7,7 @@ import { upsertSemanticMemoryPoint, deleteSemanticMemoryPoint } from "./qdrant";
|
||||
import { Memory, MemoryType } from "./types";
|
||||
import { logger } from "../../../open-sse/utils/logger.ts";
|
||||
import { sanitizeErrorMessage } from "../../../open-sse/utils/error.ts";
|
||||
import { resolveEmbeddingSource, embed, withMeasuredDimensions } from "./embedding";
|
||||
import { resolveEmbeddingSource, embedWithRetry, withMeasuredDimensions } from "./embedding";
|
||||
import { getVectorStore } from "./vectorStore";
|
||||
import { getMemorySettings } from "./settings";
|
||||
import { markMemoryNeedsReindex } from "@/lib/db/memoryVec";
|
||||
@@ -137,7 +137,9 @@ function scheduleVectorUpsert(id: string, content: string): void {
|
||||
const resolution = resolveEmbeddingSource(settings);
|
||||
if (!resolution.source) return;
|
||||
|
||||
const embeddingResult = await embed(content, settings);
|
||||
// #13601: one retry before giving up — a single transient embed failure
|
||||
// must not skip vectorization. The warn below keeps the cause visible.
|
||||
const embeddingResult = await embedWithRetry(content, settings);
|
||||
if (!("vector" in embeddingResult)) {
|
||||
log.warn("memory.vec.embed.fail", {
|
||||
id,
|
||||
|
||||
@@ -303,25 +303,32 @@ export function getCanonicalModelMetadata(input: {
|
||||
// a rebuild instead of rebuilt per lookup.
|
||||
const lowercaseIndexCache = new WeakMap<object, Map<string, unknown>>();
|
||||
|
||||
function findInsensitive<T>(obj: Record<string, T> | null | undefined, key: string): T | undefined {
|
||||
/** Test hook (#13601): exercised directly by the collision-naming unit test. */
|
||||
export function findInsensitive<T>(
|
||||
obj: Record<string, T> | 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<string, string>();
|
||||
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);
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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;
|
||||
|
||||
43
tests/unit/13601-embed-retry.test.ts
Normal file
43
tests/unit/13601-embed-retry.test.ts
Normal file
@@ -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<typeof embedWithRetry>[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");
|
||||
});
|
||||
47
tests/unit/13601-header-drop-count-surfaced.test.ts
Normal file
47
tests/unit/13601-header-drop-count-surfaced.test.ts
Normal file
@@ -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<typeof buildStreamingResponseHeaders>[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);
|
||||
});
|
||||
89
tests/unit/13601-quota-far-reset-skip.test.ts
Normal file
89
tests/unit/13601-quota-far-reset-skip.test.ts
Normal file
@@ -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<unknown>) {
|
||||
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);
|
||||
});
|
||||
45
tests/unit/13601-registry-collision-names-keys.test.ts
Normal file
45
tests/unit/13601-registry-collision-names-keys.test.ts
Normal file
@@ -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);
|
||||
});
|
||||
Reference in New Issue
Block a user