diff --git a/changelog.d/fixes/12111-vision-bridge-model-lockout.md b/changelog.d/fixes/12111-vision-bridge-model-lockout.md new file mode 100644 index 0000000000..11e833e8d5 --- /dev/null +++ b/changelog.d/fixes/12111-vision-bridge-model-lockout.md @@ -0,0 +1 @@ +- fix(guardrails): stop Vision Bridge from re-selecting a model locked after a 404 (#12111) diff --git a/src/lib/guardrails/visionBridgeCredentials.ts b/src/lib/guardrails/visionBridgeCredentials.ts index 1e63e471e7..9d25b45219 100644 --- a/src/lib/guardrails/visionBridgeCredentials.ts +++ b/src/lib/guardrails/visionBridgeCredentials.ts @@ -7,6 +7,7 @@ import { resolveProviderId } from "@/shared/constants/providers"; import { isNoAuthProviderKey } from "@/shared/utils/noAuthProviders"; +import { SYNTHETIC_NOAUTH_CONNECTION_ID } from "@omniroute/open-sse/services/autoCombo/resilienceCandidateFilter.ts"; /** * True when a provider connection can actually authenticate upstream. @@ -119,3 +120,51 @@ export async function hasUsableCredentialsForModel(model: string): Promise { + const rawProvider = typeof model === "string" ? model.split("/")[0]?.trim() : ""; + if (!rawProvider) return null; + const provider = resolveProviderId(rawProvider); + const isNoAuth = isNoAuthProviderKey(rawProvider, provider); + try { + const { getProviderConnections } = await loadProvidersModule(); + const connections = await getProviderConnections({ provider, isActive: true }); + if (!Array.isArray(connections)) return null; + if (connections.length === 0) { + return isNoAuth ? [{ id: SYNTHETIC_NOAUTH_CONNECTION_ID }] : []; + } + const usable = isNoAuth + ? connections.filter((c: any) => !hasTerminalConnectionStatus(c)) + : connections.filter((c: any) => isProviderConnectionUsable(c)); + return usable.map((c: any) => ({ id: String(c.id) })); + } catch { + return null; + } +} diff --git a/src/lib/guardrails/visionBridgeRouter.ts b/src/lib/guardrails/visionBridgeRouter.ts index c2dbbc9121..2ad5d7620b 100644 --- a/src/lib/guardrails/visionBridgeRouter.ts +++ b/src/lib/guardrails/visionBridgeRouter.ts @@ -7,8 +7,16 @@ import { getResolvedModelCapabilities } from "@/lib/modelCapabilities"; import { getActiveSyncedCatalog } from "@/lib/db/models/activeSyncedCatalog"; import { PROVIDER_MODELS } from "@omniroute/open-sse/config/providerModels"; import { getRegisteredProviderEffortBaseModelId } from "@omniroute/open-sse/utils/registeredEffortVariants.ts"; -import { hasUsableCredentialsForModel } from "./visionBridgeCredentials"; +import { + hasUsableCredentialsForModel, + getUsableConnectionsForModel, +} from "./visionBridgeCredentials"; import { isVisionBridgeForcedModel } from "@/shared/constants/visionBridgeDefaults"; +import { resolveProviderId } from "@/shared/constants/providers"; +import { + isModelLocked, + getAllModelLockouts, +} from "@omniroute/open-sse/services/accountFallback.ts"; export interface VisionModelCandidate { modelId: string; @@ -109,6 +117,12 @@ function calculateSuccessRate(modelId: string): number { export interface VisionBridgeRouterDeps { hasUsableCredentials?: (model: string) => Promise; getActiveSyncedCatalog?: (provider: string) => Promise; + /** + * (#12111) Per-connection model-lockout check, defaulting to the real + * `accountFallback.isModelLocked`. Injectable for the same reason as + * `hasUsableCredentials`: `node:test` has no supported ESM module-mocking. + */ + isModelLocked?: (provider: string, connectionId: string, model: string) => boolean; } export interface VisionModelCatalog { @@ -150,6 +164,53 @@ function createCatalogModelPredicate( }; } +/** + * connectionIds worth probing for a `(providerAlias, modelId)` lockout check: + * the provider's DB-known usable connections, plus any connectionId that + * already has an active lockout entry for this provider (#12111) — a 404 + * lock (`accountFallback.lockModel`) can target a connectionId the DB-backed + * lookup does not surface (e.g. it predates a reconnect, or the credential + * check path a caller injected does not go through the same DB rows), and + * missing it would silently fail the exclusion open. + */ +function collectLockoutConnectionIds(providerAlias: string): string[] { + const canonicalProvider = resolveProviderId(providerAlias); + return getAllModelLockouts() + .filter((entry) => entry.provider === canonicalProvider) + .map((entry) => entry.connectionId); +} + +/** + * (#12111) True unless `modelId` is locked (a post-404 model lockout, see + * `accountFallback.lockModel`) on every connection that could actually serve + * it. `isModelLocked` is scoped per provider+connection+model, so a single + * locked connection must not exclude a model that's still reachable through + * another connection on the same provider — mirrors + * `isConnectionEligibleForModel` in + * open-sse/services/autoCombo/resilienceCandidateFilter.ts. Fails open (never + * excludes) when nothing is known about the provider's connections, matching + * `hasUsableCredentialsForModel`'s existing fail-open contract — this check + * only narrows an already-credentialed candidate, it never widens the pool. + */ +async function isModelUsableGivenLockouts( + providerAlias: string, + modelId: string, + deps: VisionBridgeRouterDeps +): Promise { + const checkLocked = deps.isModelLocked ?? isModelLocked; + const dbConnections = await getUsableConnectionsForModel(`${providerAlias}/${modelId}`); + if (dbConnections === null) return true; // indeterminate credential store — fail open + + const candidateIds = new Set(dbConnections.map((conn) => conn.id)); + for (const id of collectLockoutConnectionIds(providerAlias)) candidateIds.add(id); + if (candidateIds.size === 0) return true; // nothing known about this provider's connections + + for (const id of candidateIds) { + if (!checkLocked(providerAlias, id, modelId)) return true; + } + return false; +} + async function cachedModelRemainsAvailable( fullModelId: string, deps: VisionBridgeRouterDeps @@ -162,6 +223,8 @@ async function cachedModelRemainsAvailable( const registryModel = PROVIDER_MODELS[providerAlias]?.find((model) => model.id === modelId); if (!registryModel) return false; + if (!(await isModelUsableGivenLockouts(providerAlias, modelId, deps))) return false; + const catalog = await readActiveCatalog(providerAlias, deps); return createCatalogModelPredicate(providerAlias, catalog)(registryModel); } @@ -193,13 +256,27 @@ async function getVisionCapableModels( }); if (visionModels.length === 0) return []; - const usableModels = ( + const credentialedModels = ( await Promise.all( visionModels.map(async (model) => (await checkCreds(`${providerAlias}/${model.id}`)) === false ? null : model ) ) ).filter((model): model is (typeof visionModels)[number] => model !== null); + if (credentialedModels.length === 0) return []; + + // (#12111) A healthy provider connection does not mean every model on + // it is servable: chatCore.ts locks one specific model for 120s on a + // 404 while leaving the connection active, so the credential check + // above never sees it. Drop only the models locked on every usable + // connection for this provider. + const usableModels = ( + await Promise.all( + credentialedModels.map(async (model) => + (await isModelUsableGivenLockouts(providerAlias, model.id, deps)) ? model : null + ) + ) + ).filter((model): model is (typeof credentialedModels)[number] => model !== null); if (usableModels.length === 0) return []; const catalog = await readActiveCatalog(providerAlias, deps); diff --git a/tests/unit/guardrails/visionBridge12111Repro.test.ts b/tests/unit/guardrails/visionBridge12111Repro.test.ts new file mode 100644 index 0000000000..9854602b4c --- /dev/null +++ b/tests/unit/guardrails/visionBridge12111Repro.test.ts @@ -0,0 +1,64 @@ +/** + * TDD repro for issue #12111: Vision Bridge auto-router can select a model + * already locked after a 404. + * + * getVisionCapableModels() (src/lib/guardrails/visionBridgeRouter.ts) filters + * candidates only on the registry vision flag and hasUsableCredentialsForModel + * (connection-scoped). It never consults isModelLocked + * (open-sse/services/accountFallback.ts), which the provider layer sets on a + * 404 "model not found" (open-sse/handlers/chatCore.ts). This test locks a + * vision-capable model exactly as chatCore.ts would after a 404, then asks + * getBestVisionModel() for a pick while forcing every other vision-capable + * provider to look uncredentialed (mirroring the reporter's setup: only one + * provider connection is actually usable) -- the locked model must not win. + */ +import test from "node:test"; +import assert from "node:assert/strict"; + +const { getBestVisionModel, clearSelectionCache } = + await import("../../../src/lib/guardrails/visionBridgeRouter.ts"); +const { lockModel, clearAllModelLockouts, isModelLocked } = + await import("../../../open-sse/services/accountFallback.ts"); + +test.beforeEach(() => { + clearSelectionCache(); + clearAllModelLockouts(); +}); + +test("getBestVisionModel must not select a model locked after a 404 (#12111)", async () => { + const provider = "nvidia"; + const connectionId = "conn-nvidia-1"; + // The issue's original log line named "moonshotai/kimi-k2.6"; the registry + // has since renamed that entry to "kimi-k3" (open-sse/config/providers/ + // registry/nvidia/index.ts) but it resolves the same way: it is the first + // vision-capable nvidia model in registry order, so it is still the model + // getBestVisionModel picks first when only nvidia is credentialed. + const modelId = "moonshotai/kimi-k3"; + const fullModelId = `${provider}/${modelId}`; + + // Reproduce the exact runtime event from the issue log line: + // "[provider] Node model not found (404) for + // - locking model for 120s (connection stays active)" + lockModel(provider, connectionId, modelId, "not_found", 120_000); + assert.equal( + isModelLocked(provider, connectionId, modelId), + true, + "sanity check: accountFallback must report the model as locked" + ); + + // Only the nvidia provider looks credentialed -- mirrors the reporter's + // setup where the NVIDIA connection tests 200 all day (connection-scoped + // credential check passes) but the specific model 404s for the account. + const model = await getBestVisionModel( + {}, + { hasUsableCredentials: async (id) => id.startsWith(`${provider}/`) } + ); + + assert.notEqual( + model, + fullModelId, + "getBestVisionModel selected a model that accountFallback has locked after " + + "a 404 -- getVisionCapableModels() never consults isModelLocked " + + "(src/lib/guardrails/visionBridgeRouter.ts)" + ); +}); diff --git a/tests/unit/guardrails/visionBridgeRouter.test.ts b/tests/unit/guardrails/visionBridgeRouter.test.ts index dc685a61ff..df471588d5 100644 --- a/tests/unit/guardrails/visionBridgeRouter.test.ts +++ b/tests/unit/guardrails/visionBridgeRouter.test.ts @@ -25,6 +25,10 @@ const { getLatencyStats, } = await import("../../../src/lib/guardrails/visionBridgeRouter.ts"); const { PROVIDER_MODELS } = await import("../../../open-sse/config/providerModels.ts"); +const { lockModel, clearAllModelLockouts, isModelLocked } = + await import("../../../open-sse/services/accountFallback.ts"); +const { createProviderConnection, deleteProviderConnectionsByProvider } = + await import("../../../src/lib/db/providers.ts"); type VisionBridgeRouterDepsT = import("../../../src/lib/guardrails/visionBridgeRouter.ts").VisionBridgeRouterDeps; @@ -294,3 +298,103 @@ test("getLatencyStats — should return latency statistics", () => { assert.equal(stats["model-a"].avg, 110); assert.equal(stats["model-a"].successRate, 1); }); + +// ── model-lockout exclusion (#12111) ──────────────────────────────────────── +// getVisionCapableModels() must consult accountFallback's per-connection +// model lockout (set by chatCore.ts on a 404) in addition to the credential +// check, and drop a model only when every usable connection has it locked — +// see tests/unit/guardrails/visionBridge12111Repro.test.ts for the original +// end-to-end reproduction against the exact reporter setup. These cases +// exercise the same production code path (getBestVisionModel → +// getVisionCapableModels → isModelUsableGivenLockouts) with a synthetic +// registry entry, following the pattern in "accepts a registry model whose +// liveCatalogIds match upstream" above. + +test("getBestVisionModel — excludes a model locked on its only usable connection (#12111)", async () => { + const provider = "__vision-bridge-lockout-test-1__"; + const connectionId = "conn-1"; + const modelId = "synthetic-vision-model"; + PROVIDER_MODELS[provider] = [ + { id: modelId, name: "Synthetic Vision Model", supportsVision: true }, + ]; + clearAllModelLockouts(); + lockModel(provider, connectionId, modelId, "not_found", 120_000); + + try { + const model = await getBestVisionModel( + {}, + { hasUsableCredentials: async (id) => id.startsWith(`${provider}/`) } + ); + assert.notEqual(model, `${provider}/${modelId}`); + } finally { + delete PROVIDER_MODELS[provider]; + clearAllModelLockouts(); + } +}); + +test("getBestVisionModel — keeps a model locked on one connection while a second connection stays usable (#12111)", async () => { + const provider = "__vision-bridge-lockout-test-2__"; + const modelId = "synthetic-vision-model"; + PROVIDER_MODELS[provider] = [ + { id: modelId, name: "Synthetic Vision Model", supportsVision: true }, + ]; + clearAllModelLockouts(); + + const lockedConn = await createProviderConnection({ + provider, + authType: "apikey", + apiKey: "sk-test-locked", + }); + const openConn = await createProviderConnection({ + provider, + authType: "apikey", + apiKey: "sk-test-open", + }); + lockModel(provider, (lockedConn as { id: string }).id, modelId, "not_found", 120_000); + // Sanity: the OTHER connection must not itself be locked. + assert.equal(isModelLocked(provider, (openConn as { id: string }).id, modelId), false); + + try { + const model = await getBestVisionModel( + {}, + { hasUsableCredentials: async (id) => id.startsWith(`${provider}/`) } + ); + assert.equal( + model, + `${provider}/${modelId}`, + "a model locked on only ONE of two usable connections must stay selectable" + ); + } finally { + delete PROVIDER_MODELS[provider]; + clearAllModelLockouts(); + await deleteProviderConnectionsByProvider(provider); + } +}); + +test("getBestVisionModel — drops a cached selection once it becomes locked mid-window (#12111)", async () => { + const provider = "__vision-bridge-lockout-test-3__"; + const connectionId = "conn-1"; + const modelId = "synthetic-vision-model"; + PROVIDER_MODELS[provider] = [ + { id: modelId, name: "Synthetic Vision Model", supportsVision: true }, + ]; + clearAllModelLockouts(); + const deps = { hasUsableCredentials: async (id: string) => id.startsWith(`${provider}/`) }; + + try { + // First call populates the 60s selection cache with the only candidate. + assert.equal(await getBestVisionModel({}, deps), `${provider}/${modelId}`); + + // The model 404s and gets locked mid-cache-window, exactly like chatCore.ts. + lockModel(provider, connectionId, modelId, "not_found", 120_000); + + // A cache hit that never re-validates lockouts would keep returning the + // now-locked model for up to 60s of further failing requests (the + // reporter's complaint); it must fall through to "no usable candidate". + assert.equal(await getBestVisionModel({}, deps), null); + } finally { + delete PROVIDER_MODELS[provider]; + clearAllModelLockouts(); + clearSelectionCache(); + } +});