mirror of
https://github.com/diegosouzapw/OmniRoute.git
synced 2026-08-20 22:22:57 +03:00
* fix(catalog): hash API key in buildCatalogCacheKey so raw credentials never live in the key string (#10313) * fix(api): yield event loop and bulk-load override tables in catalog build (#9147) * fix(api): keep bulk hidden-model load inside catalog builder's error boundary Post-sync-merge fixup for #9147/#10313 against release/v3.8.50: - Resolve the catalog.ts/catalogCache.ts merge conflicts against several catalog PRs merged since this branch was cut: keep isModelHiddenBulk() (this PR's perf fix) alongside isExcludedByProviderConnections() (a concurrently landed feature), and adopt the already-merged canonical fingerprintCatalogAuthKey() helper for the cache-key hashing instead of the now-duplicate inline sha256 computation. - getHiddenModelsByProvider() was hoisted above buildUnifiedModelsResponseCore's try/catch, so a read failure there rejected the builder promise instead of being caught and turned into a sanitized 500 like every other failure in this function. Combined with the pre-existing promise.finally() dangling chain in catalogCache.ts's in-flight coalescing, that produced a genuine unhandled rejection. Move the bulk-load call back inside the try block. - Align tests/unit/models-catalog-route.test.ts and tests/unit/10313-catalog-cache-key-hashing.test.ts with the current implementation (bulk query text/method, truncated fingerprint format). * perf(api): memoize getConnectionsForProvider in catalog builder Combining this PR's own bulk hidden-model optimization with the already-merged isExcludedByProviderConnections() check (from a different PR) reintroduced an O(connections) scan per model inside the catalog builder's hot loop, regressing the exact single-stretch event-loop budget tests/unit/9147-catalog-eventloop-yield.test.ts enforces (was passing on this PR's own commit before the merge). Memoizing getConnectionsForProvider() by its (unordered) key-set substantially reduces the redundant per-model connection scans (measured ~497ms -> ~210-300ms worst single stretch across repeated runs), but does NOT fully close the gap to the 150ms budget — still red. Committing this as a real, safe improvement; flagging for further investigation (likely getConnectionsForProvider's first-call cost per provider, or hasEligibleConnectionForModel) before this PR merges. NOT deciding to relax the test threshold myself. --------- Co-authored-by: adevwithpurpose <adevwithpurpose@users.noreply.github.com>
This commit is contained in:
committed by
GitHub
parent
ec4802d09c
commit
d5e4c0fd97
1
changelog.d/fixes/10313-catalog-cache-key-hash.md
Normal file
1
changelog.d/fixes/10313-catalog-cache-key-hash.md
Normal file
@@ -0,0 +1 @@
|
||||
- fix(api): hash the API key before using it as the model-catalog cache Map key (no raw credentials in process heap) (#10313)
|
||||
1
changelog.d/fixes/9147-catalog-eventloop-yield.md
Normal file
1
changelog.d/fixes/9147-catalog-eventloop-yield.md
Normal file
@@ -0,0 +1 @@
|
||||
- fix(api): yield the event loop during catalog builds and bulk-load override/hidden-model tables (#9147)
|
||||
@@ -6,9 +6,9 @@ import {
|
||||
getAllCustomModels,
|
||||
getSettings,
|
||||
getCachedProviderNodes,
|
||||
getModelIsHidden,
|
||||
getModelAliases,
|
||||
getDatabaseSettings,
|
||||
getHiddenModelsByProvider,
|
||||
} from "@/lib/localDb";
|
||||
import { createLazyConnectionView } from "@/lib/db/providers/lazyConnectionView";
|
||||
import { extractAliasBackedModels } from "./aliasBackedModels";
|
||||
@@ -133,7 +133,7 @@ export {
|
||||
} from "./catalogCache";
|
||||
export type { CachedCatalog } from "./catalogCache";
|
||||
|
||||
const BUILTIN_AUTO_YIELD_INTERVAL = 8;
|
||||
const BUILTIN_AUTO_YIELD_INTERVAL = 2;
|
||||
|
||||
function yieldCatalogBuildTurn(): Promise<void> {
|
||||
return new Promise((resolve) => setImmediate(resolve));
|
||||
@@ -157,6 +157,8 @@ export async function getUnifiedModelsResponse(
|
||||
try {
|
||||
settingsForAuth = await getSettings();
|
||||
} catch {}
|
||||
// #9147: yield before auth check to allow event loop tick
|
||||
await yieldCatalogBuildTurn();
|
||||
const authRejection = await getModelCatalogAuthRejection(request, settingsForAuth, {
|
||||
...corsHeaders,
|
||||
...diagnosticHeaders,
|
||||
@@ -227,7 +229,34 @@ async function buildUnifiedModelsResponseCore(
|
||||
corsHeaders: Record<string, string> = {}
|
||||
) {
|
||||
const diagnosticHeaders = getCatalogDiagnosticsHeaders({ request });
|
||||
// #9147: this builder walks connections + model registries at catalog scale with no
|
||||
// event-loop yield, so a large deployment pins the single Node.js thread for the
|
||||
// whole build (reporter: 183 connections / 2000+ models → 10.1s stall that blocks the
|
||||
// dashboard WS heartbeat). Yield every `catYIELD_EVERY` items across the hot loops.
|
||||
const catYIELD_EVERY = 20;
|
||||
let catYieldCount = 0;
|
||||
const maybeYieldCatalogBuild = async (): Promise<void> => {
|
||||
catYieldCount++;
|
||||
if (catYieldCount % catYIELD_EVERY === 0) {
|
||||
await yieldCatalogBuildTurn();
|
||||
}
|
||||
};
|
||||
try {
|
||||
// #9147: `getModelIsHidden()` is a SQLite read per call (custom row + compat list)
|
||||
// and the build consults it ~16× per entry. Bulk-load the hidden-model map once
|
||||
// (one query — `getHiddenModelsByProvider`) and resolve from memory for the whole
|
||||
// build. A provider absent from the map has no hidden models at all — `false`,
|
||||
// no on-demand fallback (that would reintroduce the per-call SQLite reads).
|
||||
// Deliberately kept INSIDE this try block (not hoisted above it): the builder's
|
||||
// own catch below is what converts a build-time failure into a sanitized 500
|
||||
// Response instead of a rejected promise — hoisting this bulk read above the
|
||||
// try would let a crash here propagate as an unhandled rejection instead
|
||||
// (catalogCache.ts's in-flight coalescing does not fully consume rejections).
|
||||
const hiddenModelsByProvider = getHiddenModelsByProvider();
|
||||
const isModelHiddenBulk = (providerId: string, modelId: string): boolean => {
|
||||
const hiddenSet = hiddenModelsByProvider.get(providerId);
|
||||
return hiddenSet ? hiddenSet.has(modelId) : false;
|
||||
};
|
||||
let settings: Record<string, any> = {};
|
||||
try {
|
||||
settings = await getSettings();
|
||||
@@ -238,6 +267,10 @@ async function buildUnifiedModelsResponseCore(
|
||||
...diagnosticHeaders,
|
||||
});
|
||||
if (authRejection) return authRejection;
|
||||
|
||||
// #9147: yield after auth check before DB initialization prologue
|
||||
await yieldCatalogBuildTurn();
|
||||
|
||||
const { aliasToProviderId, providerIdToAlias } = buildAliasMaps();
|
||||
const _qp = new URL(request.url).searchParams.get("prefix");
|
||||
const prefixMode =
|
||||
@@ -322,6 +355,7 @@ async function buildUnifiedModelsResponseCore(
|
||||
|
||||
// Get combos
|
||||
let combos = [];
|
||||
await yieldCatalogBuildTurn();
|
||||
try {
|
||||
combos = await getCombos();
|
||||
} catch (e) {
|
||||
@@ -355,7 +389,16 @@ async function buildUnifiedModelsResponseCore(
|
||||
if ("alias" in p && typeof p.alias === "string") activeAliases.add(p.alias);
|
||||
}
|
||||
|
||||
// #9147 follow-up: this is called ~1-3x per model at catalog scale (providerSupportsModel,
|
||||
// isExcludedByProviderConnections). Connections do not change mid-build, so memoize per
|
||||
// unique (unordered) key-set instead of rescanning connectionsByProvider on every call —
|
||||
// otherwise the O(models) hot loop regains an O(connections) cost per model and blows the
|
||||
// single-stretch event-loop budget this file's own yield mechanism is meant to protect.
|
||||
const connectionsForProviderCache = new Map<string, typeof connections>();
|
||||
const getConnectionsForProvider = (...keys: Array<string | null | undefined>) => {
|
||||
const cacheKey = keys.filter((k): k is string => Boolean(k)).sort().join(" | ||||