From 80b517edeaaf778daa44fb6213f369e2948e5a4e Mon Sep 17 00:00:00 2001 From: Nguyen Thanh Dat Date: Fri, 21 Aug 2026 01:19:41 +0700 Subject: [PATCH] fix(providers): treat a degraded cached catalog as a failed model sync (#10862) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Obrigado — root cause preciso: a rota sync-models só reconhecia a degradação para local_catalog, não para o fallback de cache com warning, então uma chave expirada (401) virava silenciosamente "Nenhum modelo novo foi adicionado" em vez de um erro visível. Validação (worktree combinado a partir de origin/release/v3.8.50, 0 conflitos): - typecheck:core limpo, complexity/cognitive-complexity dentro do baseline - tests/unit/sync-models-degraded-cached-catalog-9683.test.ts — 6/6 passando (payloads reais do models/route.ts) - Suítes model-sync/provider-models/sync-models/siliconflow — 181/181 passando, incluindo as 3 asserções pré-existentes #5460/#5465 --- ...862-sync-models-degraded-cached-catalog.md | 1 + .../[id]/sync-models/degradedLocalCatalog.ts | 41 ++++++ .../api/providers/[id]/sync-models/route.ts | 6 +- ...odels-degraded-cached-catalog-9683.test.ts | 121 ++++++++++++++++++ 4 files changed, 166 insertions(+), 3 deletions(-) create mode 100644 changelog.d/fixes/10862-sync-models-degraded-cached-catalog.md create mode 100644 tests/unit/sync-models-degraded-cached-catalog-9683.test.ts diff --git a/changelog.d/fixes/10862-sync-models-degraded-cached-catalog.md b/changelog.d/fixes/10862-sync-models-degraded-cached-catalog.md new file mode 100644 index 0000000000..fd6f7d3f04 --- /dev/null +++ b/changelog.d/fixes/10862-sync-models-degraded-cached-catalog.md @@ -0,0 +1 @@ +- **fix(providers):** importing models with an expired API key now surfaces the credential error instead of reporting "No new models were added". The Import button posts to `/api/providers/{id}/sync-models`, which self-fetches the models route; that route does not fail on an upstream 401 but degrades to a catalog it already has, preferring the cache and using the local catalog only when there is no cache. A provider that imported successfully once therefore has a cache, so an expired key produced `{ source: "cache", warning: "Models probe failed (401) — using cached catalog" }` with HTTP 200 — and the #5460/#5465 degradation guard only recognised the `local_catalog` branch, so model-sync accepted it as a successful discovery, found every cached model already imported, and returned the empty-diff result. Retest does not go through this path, which is why it failed correctly and made the import look like a genuine "nothing to do". The existing rule — a degraded discovery must not be persisted as the synced catalog — is now applied to the branch it missed rather than special-casing 401/403, discriminating on the warning the fallback builder always attaches (an ordinary non-refresh cache hit attaches none, and model-sync always requests `refresh=true`). `isDegradedLocalCatalog` keeps its exact meaning and its existing tests diff --git a/src/app/api/providers/[id]/sync-models/degradedLocalCatalog.ts b/src/app/api/providers/[id]/sync-models/degradedLocalCatalog.ts index 6e521e81ce..dc2d8966aa 100644 --- a/src/app/api/providers/[id]/sync-models/degradedLocalCatalog.ts +++ b/src/app/api/providers/[id]/sync-models/degradedLocalCatalog.ts @@ -19,3 +19,44 @@ export function isDegradedLocalCatalog(modelsData: { typeof modelsData?.source === "string" ? modelsData.source.trim().toLowerCase() : ""; return source === "local_catalog" && modelsData?.intentional !== true; } + +/** + * #9683 — the same degradation, one branch further up. + * + * When remote discovery fails, the models route falls back to the CACHED + * catalog if it has one and only falls back to the local catalog when it does + * not (`buildDiscoveryFallbackResponse`). A provider that was imported + * successfully once therefore has a cache, so an expired key produced + * `source: "cache"` + a warning and HTTP 200 — model-sync treated that as a + * successful discovery, found every cached model already imported, and reported + * "No new models were added" instead of the credential error. Retest, which + * does not go through this path, failed correctly, which is what made the + * import look like a real "nothing to do". + * + * The discriminator is the warning: the fallback builder always attaches one, + * while the ordinary cache hit (`maybeReturnCachedDiscovery`, a non-refresh + * read) attaches none. Model-sync always requests `refresh=true`, so the one + * warning-carrying cache response it can observe is a degraded one. + */ +export function isDegradedCachedCatalog(modelsData: { + source?: unknown; + warning?: unknown; +}): boolean { + const source = + typeof modelsData?.source === "string" ? modelsData.source.trim().toLowerCase() : ""; + if (source !== "cache") return false; + return typeof modelsData?.warning === "string" && modelsData.warning.trim().length > 0; +} + +/** + * Either degraded shape. Model-sync must refuse to treat these as a successful + * discovery: persisting them would silently pin a stale catalog and hide the + * real failure from the operator. + */ +export function isDegradedDiscovery(modelsData: { + source?: unknown; + intentional?: unknown; + warning?: unknown; +}): boolean { + return isDegradedLocalCatalog(modelsData) || isDegradedCachedCatalog(modelsData); +} diff --git a/src/app/api/providers/[id]/sync-models/route.ts b/src/app/api/providers/[id]/sync-models/route.ts index 95986e15a9..1d46c269f0 100644 --- a/src/app/api/providers/[id]/sync-models/route.ts +++ b/src/app/api/providers/[id]/sync-models/route.ts @@ -22,7 +22,7 @@ import { autoSyncCodexProfilesFromLiveCatalog } from "@/lib/cli-helper/codexProf import { autoSyncClaudeProfilesFromLiveCatalog } from "@/lib/cli-helper/claudeProfileAutoSync"; import { providerUsesCuratedModelsOnly } from "@/lib/providers/modelListingCapability"; import { GET as getProviderModels } from "../models/route"; -import { isDegradedLocalCatalog } from "./degradedLocalCatalog"; +import { isDegradedDiscovery } from "./degradedLocalCatalog"; import { sanitizeErrorMessage } from "@omniroute/open-sse/utils/error"; type JsonRecord = Record; @@ -465,9 +465,9 @@ export async function POST(request: Request, { params }: { params: Promise<{ id: const modelSource = toNonEmptyString(modelsData.source)?.toLowerCase() || "unknown"; const modelWarning = toNonEmptyString(modelsData.warning); - if (isDegradedLocalCatalog(modelsData)) { + if (isDegradedDiscovery(modelsData)) { const responseError = - modelWarning || "Remote model discovery failed; local catalog fallback not synced"; + modelWarning || "Remote model discovery failed; catalog fallback not synced"; await saveCallLog({ method: "GET", path: `/api/providers/${id}/models`, diff --git a/tests/unit/sync-models-degraded-cached-catalog-9683.test.ts b/tests/unit/sync-models-degraded-cached-catalog-9683.test.ts new file mode 100644 index 0000000000..beb0fe76c0 --- /dev/null +++ b/tests/unit/sync-models-degraded-cached-catalog-9683.test.ts @@ -0,0 +1,121 @@ +/** + * #9683 — "Importing Models" reported success with an expired API key. + * + * The import button posts to `/api/providers/{id}/sync-models`, which self-fetches + * `/api/providers/{id}/models?refresh=true`. On an upstream 401 that route does not + * fail: it falls back to a catalog it already has. It prefers the CACHE and only uses + * the local catalog when there is no cache, so a provider that imported successfully + * once takes the cache branch — `{ source: "cache", warning: "…(401)…" }` with HTTP + * 200. `isDegradedLocalCatalog` only recognised `local_catalog`, so model-sync treated + * that as a successful discovery, found every cached model already imported and + * answered "No new models were added" instead of surfacing the credential error. + * + * The warning is the discriminator: `buildDiscoveryFallbackResponse` always attaches + * one, while the ordinary non-refresh cache hit attaches none. + */ +import test from "node:test"; +import assert from "node:assert/strict"; + +const { isDegradedLocalCatalog, isDegradedCachedCatalog, isDegradedDiscovery } = + await import("../../src/app/api/providers/[id]/sync-models/degradedLocalCatalog.ts"); + +// The exact payload shapes `src/app/api/providers/[id]/models/route.ts` emits from +// `buildCachedDiscoveryResponse(cacheWarning)` on a failed probe. +const AUTH_FAILED_CACHE = { + source: "cache", + warning: "Models probe failed (401) — using cached catalog", +}; +const BEDROCK_AUTH_FAILED_CACHE = { + source: "cache", + warning: "Auth failed (403) — using cached catalog", +}; +const UNAVAILABLE_CACHE = { + source: "cache", + warning: "API unavailable — using cached catalog", +}; +// `maybeReturnCachedDiscovery()` — an ordinary cache hit, built with no warning. +const HEALTHY_CACHE = { source: "cache" }; + +test("#9683: a 401 that degraded to the cached catalog is a failed discovery", () => { + assert.equal( + isDegradedCachedCatalog(AUTH_FAILED_CACHE), + true, + "an expired key must not be reported to the operator as a successful import" + ); + assert.equal(isDegradedCachedCatalog(BEDROCK_AUTH_FAILED_CACHE), true); + assert.equal(isDegradedDiscovery(AUTH_FAILED_CACHE), true); +}); + +test("#9683: any warning-carrying cache fallback is degraded, not only auth", () => { + // The rule model-sync already applies to `local_catalog` is not auth-specific: + // a degraded discovery must not be persisted as the synced catalog. + assert.equal(isDegradedCachedCatalog(UNAVAILABLE_CACHE), true); + assert.equal(isDegradedDiscovery(UNAVAILABLE_CACHE), true); +}); + +test("#9683: an ordinary cache hit stays a success", () => { + assert.equal(isDegradedCachedCatalog(HEALTHY_CACHE), false); + assert.equal(isDegradedDiscovery(HEALTHY_CACHE), false); + for (const blank of ["", " "]) { + assert.equal( + isDegradedCachedCatalog({ source: "cache", warning: blank }), + false, + "a blank warning is not a degradation signal" + ); + } + assert.equal(isDegradedCachedCatalog({ source: "cache", warning: 42 }), false); +}); + +test("#9683: only the cache source is judged by this predicate", () => { + assert.equal(isDegradedCachedCatalog({ source: "api", warning: "anything" }), false); + assert.equal(isDegradedCachedCatalog({ source: "local_catalog", warning: "x" }), false); + assert.equal(isDegradedCachedCatalog({}), false); + assert.equal(isDegradedCachedCatalog({ source: "" }), false); + assert.equal(isDegradedCachedCatalog({ source: " CACHE ", warning: "x" }), true); +}); + +// ── #5460/#5465 must be unchanged ───────────────────────────────────────── + +test("#9683: the local-catalog rule is untouched", () => { + assert.equal(isDegradedLocalCatalog({ source: "local_catalog", intentional: true }), false); + assert.equal(isDegradedLocalCatalog({ source: "local_catalog", intentional: false }), true); + assert.equal(isDegradedLocalCatalog({ source: "cache", intentional: false }), false); + + assert.equal( + isDegradedDiscovery({ source: "local_catalog", intentional: true }), + false, + "a provider whose local catalog is its only discovery source still syncs" + ); + assert.equal( + isDegradedDiscovery({ + source: "local_catalog", + intentional: true, + warning: "reka has no remote /models endpoint", + }), + false, + "an intentional local catalog is not degraded just because it carries a warning" + ); + assert.equal(isDegradedDiscovery({ source: "local_catalog", intentional: false }), true); +}); + +// ── Wiring ──────────────────────────────────────────────────────────────── +// The predicate is only half the fix: the route has to consult the combined one. +// This assertion fails on the pre-fix tree, where the guard read +// `isDegradedLocalCatalog(modelsData)` and so never saw the cache fallback. + +test("#9683: the sync-models route gates on the combined predicate", async () => { + const { readFileSync } = await import("node:fs"); + const source = readFileSync( + new URL("../../src/app/api/providers/[id]/sync-models/route.ts", import.meta.url), + "utf8" + ); + assert.match( + source, + /if\s*\(\s*isDegradedDiscovery\(modelsData\)\s*\)/, + "model-sync must refuse both degraded shapes, not just local_catalog" + ); + assert.ok( + !/isDegradedLocalCatalog\(modelsData\)/.test(source), + "the narrow predicate must no longer be the route's only gate" + ); +});