From 92cd0d2ff9347ba5fc59fd3ea007c4c22aa9f48a Mon Sep 17 00:00:00 2001 From: adevwithpurpose Date: Tue, 18 Aug 2026 12:21:52 -0300 Subject: [PATCH] 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). --- src/app/api/v1/models/catalog.ts | 25 +++++++++++-------- .../10313-catalog-cache-key-hashing.test.ts | 13 ++++++---- tests/unit/models-catalog-route.test.ts | 18 ++++++++----- 3 files changed, 35 insertions(+), 21 deletions(-) diff --git a/src/app/api/v1/models/catalog.ts b/src/app/api/v1/models/catalog.ts index 31a9a14374..8f2121dec8 100644 --- a/src/app/api/v1/models/catalog.ts +++ b/src/app/api/v1/models/catalog.ts @@ -241,17 +241,22 @@ async function buildUnifiedModelsResponseCore( await yieldCatalogBuildTurn(); } }; - // #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). - const hiddenModelsByProvider = getHiddenModelsByProvider(); - const isModelHiddenBulk = (providerId: string, modelId: string): boolean => { - const hiddenSet = hiddenModelsByProvider.get(providerId); - return hiddenSet ? hiddenSet.has(modelId) : false; - }; 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 = {}; try { settings = await getSettings(); diff --git a/tests/unit/10313-catalog-cache-key-hashing.test.ts b/tests/unit/10313-catalog-cache-key-hashing.test.ts index 8fbe97ada0..265fed1150 100644 --- a/tests/unit/10313-catalog-cache-key-hashing.test.ts +++ b/tests/unit/10313-catalog-cache-key-hashing.test.ts @@ -106,17 +106,20 @@ test("cache keys embed the sha256 digest of the secret, never the raw secret (#1 const catalogKeys = probe.keys.filter(isCatalogCacheKey); assert.ok(catalogKeys.length > 0, "expected catalog cache Map.set() calls"); - const { createHash } = await import("node:crypto"); - const digestA = createHash("sha256").update("sk-10313-DIGEST-A").digest("hex"); - const digestB = createHash("sha256").update("sk-10313-DIGEST-B").digest("hex"); + // #10538 sync note: buildCatalogCacheKey() delegates to the canonical + // fingerprintCatalogAuthKey() (landed on release/v3.8.50 independently of #10313), + // which truncates the sha256 hex digest to 16 chars for a shorter Map key. Derive + // the expected fingerprint the same way rather than re-hardcoding the full digest. + const digestA = catalogCacheMod.fingerprintCatalogAuthKey("sk-10313-DIGEST-A"); + const digestB = catalogCacheMod.fingerprintCatalogAuthKey("sk-10313-DIGEST-B"); const rawA = "sk-10313-DIGEST-A"; const rawB = "sk-10313-DIGEST-B"; // The hashed fingerprint, not the raw secret, rides in the cache keys. const keysWithDigestA = catalogKeys.filter((k) => k.includes(digestA)); const keysWithDigestB = catalogKeys.filter((k) => k.includes(digestB)); - assert.ok(keysWithDigestA.length > 0, `expected a cache key embedding sha256 of A: ${catalogKeys.join(",")}`); - assert.ok(keysWithDigestB.length > 0, `expected a cache key embedding sha256 of B: ${catalogKeys.join(",")}`); + assert.ok(keysWithDigestA.length > 0, `expected a cache key embedding the fingerprint of A: ${catalogKeys.join(",")}`); + assert.ok(keysWithDigestB.length > 0, `expected a cache key embedding the fingerprint of B: ${catalogKeys.join(",")}`); // Raw secrets must never appear (issue #10313 root cause). assert.ok(!catalogKeys.some((k) => k.includes(rawA) || k.includes(rawB))); diff --git a/tests/unit/models-catalog-route.test.ts b/tests/unit/models-catalog-route.test.ts index cebb7cc89d..48ee3bbd9b 100644 --- a/tests/unit/models-catalog-route.test.ts +++ b/tests/unit/models-catalog-route.test.ts @@ -1340,18 +1340,24 @@ test("v1 models catalog returns 500 when model compatibility lookup crashes", as db.prepare = (sql) => { const statement = originalPrepare(sql); - if (String(sql) !== "SELECT value FROM key_value WHERE namespace = ? AND key = ?") { + // #9147: the catalog builder now resolves hidden models via a single bulk + // read (`getHiddenModelsByProvider()`, src/lib/db/models.ts) instead of the + // old per-provider `SELECT value FROM key_value WHERE namespace = ? AND + // key = ?` / readCompatList() lookup — intercept the bulk query's `.all()` + // call so this test still exercises "DB read for model visibility crashes + // -> catalog endpoint surfaces 500" against the current implementation. + if ( + String(sql) !== + "SELECT namespace, key, value FROM key_value WHERE namespace IN ('modelCompatOverrides', 'customModels')" + ) { return statement; } return new Proxy(statement, { get(target, prop, receiver) { - if (prop === "get") { + if (prop === "all") { return (...args) => { - if (args[0] === "modelCompatOverrides") { - throw new Error("compat lookup boom"); - } - return target.get(...args); + throw new Error("compat lookup boom"); }; } return Reflect.get(target, prop, receiver);