From aaa0d87abeef142338c64bcceb8cf37fa4729574 Mon Sep 17 00:00:00 2001 From: diegosouzapw Date: Fri, 5 Jun 2026 02:50:07 -0300 Subject: [PATCH] fix(provider-models): allow deleting an individual synced model and keep it deleted (#3199) Synced (fetched) models were authoritative with no per-model delete, so individual llama-cpp models couldn't be removed (or re-appeared on re-fetch). Add removeSyncedAvailableModel + mark the id hidden so re-import skips it; the DELETE route now removes from both custom and synced sets. Co-authored-by: tjengbudi --- src/app/api/provider-models/route.ts | 18 ++- src/app/api/providers/[id]/models/route.ts | 13 ++- src/lib/db/models.ts | 71 +++++++++++- src/lib/localDb.ts | 1 + tests/unit/synced-model-delete-3199.test.ts | 116 ++++++++++++++++++++ 5 files changed, 213 insertions(+), 6 deletions(-) create mode 100644 tests/unit/synced-model-delete-3199.test.ts diff --git a/src/app/api/provider-models/route.ts b/src/app/api/provider-models/route.ts index 46997bc24a4b..928deba7c824 100644 --- a/src/app/api/provider-models/route.ts +++ b/src/app/api/provider-models/route.ts @@ -5,6 +5,7 @@ import { removeCustomModel, replaceCustomModels, deleteSyncedAvailableModelsForProvider, + removeSyncedAvailableModel, updateCustomModel, getModelCompatOverrides, mergeModelCompatOverride, @@ -396,9 +397,22 @@ export async function DELETE(request) { ); } - const removed = await removeCustomModel(provider, modelId); + // #3199 — a single model may live in the custom-model namespace, the synced + // (fetched/authoritative, #3148) namespace, or both. Remove from both, and + // mark it hidden so a later auto-fetch does not re-import the deleted model + // (the sync write path skips hidden ids). + const removedCustom = await removeCustomModel(provider, modelId); + const removedSynced = await removeSyncedAvailableModel(provider, modelId); + if (removedSynced) { + mergeModelCompatOverride(provider, modelId, { isHidden: true }); + } + const removed = removedCustom || removedSynced; const removedAliases = await deleteManagedAvailableModelAliases(provider, [modelId]); - return Response.json({ removed, aliasChanges: { removed: removedAliases, assigned: [] } }); + return Response.json({ + removed, + removedSynced, + aliasChanges: { removed: removedAliases, assigned: [] }, + }); } catch (error) { console.error("Error removing provider model:", error); return Response.json( diff --git a/src/app/api/providers/[id]/models/route.ts b/src/app/api/providers/[id]/models/route.ts index f2d4bcb2dce7..d13dee7fd384 100755 --- a/src/app/api/providers/[id]/models/route.ts +++ b/src/app/api/providers/[id]/models/route.ts @@ -808,8 +808,12 @@ export async function GET( }> | null = null; try { const allSynced = await getSyncedAvailableModels(provider); - if (Array.isArray(allSynced) && allSynced.length > 0) { - providerSyncedModels = allSynced.map((m) => ({ + // #3199 — a model the operator deleted is flagged hidden; drop it from the + // authoritative synced list so the deletion sticks across re-fetch even when + // excludeHidden is not requested by the caller. + const visibleSynced = allSynced.filter((m) => !getModelIsHidden(provider, m.id)); + if (Array.isArray(visibleSynced) && visibleSynced.length > 0) { + providerSyncedModels = visibleSynced.map((m) => ({ id: m.id, name: m.name || m.id, ...(m.apiFormat ? { apiFormat: m.apiFormat } : {}), @@ -932,7 +936,10 @@ export async function GET( // its other connections) or the static catalog when none remain. let freshSynced: Awaited> = []; try { - freshSynced = await getSyncedAvailableModels(provider); + // #3199 — keep operator-deleted (hidden) models out of the re-derived list. + freshSynced = (await getSyncedAvailableModels(provider)).filter( + (m) => !getModelIsHidden(provider, m.id) + ); } catch { /* DB unavailable — fall through to static catalog */ } diff --git a/src/lib/db/models.ts b/src/lib/db/models.ts index f8206565fdda..afbc214121fb 100644 --- a/src/lib/db/models.ts +++ b/src/lib/db/models.ts @@ -714,7 +714,11 @@ export async function replaceSyncedAvailableModelsForConnection( ): Promise { const db = getDbInstance(); const key = `${providerId}:${connectionId}`; - const normalizedModels = normalizeSyncedAvailableModels(models); + // #3199 — a model the operator explicitly deleted is marked hidden; skip those + // here so an auto-fetch re-import cannot re-add a deleted/synced model. + const normalizedModels = normalizeSyncedAvailableModels(models).filter( + (m) => !getModelIsHidden(providerId, m.id) + ); if (normalizedModels.length === 0) { db.prepare("DELETE FROM key_value WHERE namespace = 'syncedAvailableModels' AND key = ?").run( key @@ -762,6 +766,71 @@ export async function deleteSyncedAvailableModelsForProvider(providerId: string) return Number(result.changes || 0); } +/** + * Remove a single model from the synced available-model set for a provider (#3199). + * + * Without a `connectionId` the model is removed from every connection list that + * advertises it (the lists are unioned on read). With a `connectionId` only that + * connection's list is touched. Returns `true` if at least one list changed. + * + * Note: this only clears the stored copy. To make the deletion survive a later + * auto-fetch the caller must also mark the model hidden via + * `mergeModelCompatOverride(providerId, modelId, { isHidden: true })` — the sync + * write path (`replaceSyncedAvailableModelsForConnection`) skips hidden ids. + */ +export async function removeSyncedAvailableModel( + providerId: string, + modelId: string, + connectionId?: string +): Promise { + const db = getDbInstance(); + const targetId = toNonEmptyString(modelId); + if (!targetId) return false; + + const rows: Array<{ key: string; value: string }> = []; + if (connectionId) { + const key = `${providerId}:${connectionId}`; + const row = db + .prepare("SELECT key, value FROM key_value WHERE namespace = 'syncedAvailableModels' AND key = ?") + .get(key); + const { key: k, value } = getKeyValue(row); + if (k && value !== null) rows.push({ key: k, value }); + } else { + const found = db + .prepare("SELECT key, value FROM key_value WHERE namespace = 'syncedAvailableModels' AND key LIKE ?") + .all(`${providerId}:%`); + for (const r of found) { + const { key, value } = getKeyValue(r); + if (key && value !== null) rows.push({ key, value }); + } + } + + let changed = false; + for (const { key, value } of rows) { + let models: SyncedAvailableModel[]; + try { + models = normalizeSyncedAvailableModels(JSON.parse(value)); + } catch { + continue; + } + const filtered = models.filter((m) => m.id !== targetId); + if (filtered.length === models.length) continue; + changed = true; + if (filtered.length === 0) { + db.prepare("DELETE FROM key_value WHERE namespace = 'syncedAvailableModels' AND key = ?").run( + key + ); + } else { + db.prepare( + "UPDATE key_value SET value = ? WHERE namespace = 'syncedAvailableModels' AND key = ?" + ).run(JSON.stringify(filtered), key); + } + } + + if (changed) backupDbFile("pre-write"); + return changed; +} + /** * Prune stale synced available models for a provider, keeping only the specified allowed connection IDs. * Returns the number of keys deleted. diff --git a/src/lib/localDb.ts b/src/lib/localDb.ts index 56d1082fa64b..1a91121c2e9f 100755 --- a/src/lib/localDb.ts +++ b/src/lib/localDb.ts @@ -66,6 +66,7 @@ export { replaceSyncedAvailableModelsForConnection, deleteSyncedAvailableModelsForConnection, deleteSyncedAvailableModelsForProvider, + removeSyncedAvailableModel, } from "./db/models"; export type { ModelCompatPerProtocol, ModelCompatPatch, SyncedAvailableModel } from "./db/models"; diff --git a/tests/unit/synced-model-delete-3199.test.ts b/tests/unit/synced-model-delete-3199.test.ts new file mode 100644 index 000000000000..efd61b7eec3e --- /dev/null +++ b/tests/unit/synced-model-delete-3199.test.ts @@ -0,0 +1,116 @@ +// Regression guard for #3199 — a user could not delete an individual model from a +// llama-cpp (local/catalog) provider. Fetched models are stored as synced +// (authoritative, #3148) and there was no per-model synced delete, so deleting a +// model either failed or re-appeared on the next auto-fetch. +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"; + +const TEST_DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-synced-delete-")); +process.env.DATA_DIR = TEST_DATA_DIR; + +const core = await import("../../src/lib/db/core.ts"); +const modelsDb = await import("../../src/lib/db/models.ts"); +const discovery = await import("../../src/lib/providerModels/modelDiscovery.ts"); + +async function resetStorage() { + core.resetDbInstance(); + for (let attempt = 0; attempt < 10; attempt++) { + try { + if (fs.existsSync(TEST_DATA_DIR)) { + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true }); + } + break; + } catch (error: any) { + if ((error?.code === "EBUSY" || error?.code === "EPERM") && attempt < 9) { + await new Promise((resolve) => setTimeout(resolve, 50 * (attempt + 1))); + } else { + throw error; + } + } + } + fs.mkdirSync(TEST_DATA_DIR, { recursive: true }); +} + +test.beforeEach(async () => { + await resetStorage(); +}); + +test.after(async () => { + core.resetDbInstance(); + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true }); +}); + +const PROVIDER = "llamacpp"; +const CONN = "conn-1"; + +test("removeSyncedAvailableModel removes a single model from the synced list", async () => { + await modelsDb.replaceSyncedAvailableModelsForConnection(PROVIDER, CONN, [ + { id: "model-a", name: "Model A" }, + { id: "model-b", name: "Model B" }, + { id: "model-c", name: "Model C" }, + ]); + + const removed = await modelsDb.removeSyncedAvailableModel(PROVIDER, "model-b"); + assert.equal(removed, true); + + const ids = (await modelsDb.getSyncedAvailableModels(PROVIDER)).map((m) => m.id).sort(); + assert.deepEqual(ids, ["model-a", "model-c"]); +}); + +test("removeSyncedAvailableModel returns false for a missing model id", async () => { + await modelsDb.replaceSyncedAvailableModelsForConnection(PROVIDER, CONN, [ + { id: "model-a", name: "Model A" }, + ]); + const removed = await modelsDb.removeSyncedAvailableModel(PROVIDER, "does-not-exist"); + assert.equal(removed, false); +}); + +test("deleted synced model stays deleted across a re-fetch (hidden marker)", async () => { + // Seed via the same discovery path the provider /models route uses. + await discovery.persistDiscoveredModels(PROVIDER, CONN, [ + { id: "model-a", name: "Model A" }, + { id: "model-b", name: "Model B" }, + ]); + + // Delete model-b and mark it hidden (the persistent "deleted" marker). + await modelsDb.removeSyncedAvailableModel(PROVIDER, "model-b"); + modelsDb.mergeModelCompatOverride(PROVIDER, "model-b", { isHidden: true }); + + assert.equal(modelsDb.getModelIsHidden(PROVIDER, "model-b"), true); + + // Auto-fetch fires again and the upstream still advertises model-b. + await discovery.persistDiscoveredModels(PROVIDER, CONN, [ + { id: "model-a", name: "Model A" }, + { id: "model-b", name: "Model B" }, + ]); + + // model-b must NOT come back: the merge/sync path skips hidden ids. + const ids = (await modelsDb.getSyncedAvailableModels(PROVIDER)).map((m) => m.id).sort(); + assert.deepEqual(ids, ["model-a"]); +}); + +test("removeSyncedAvailableModel scoped to a single connection only", async () => { + await modelsDb.replaceSyncedAvailableModelsForConnection(PROVIDER, "conn-1", [ + { id: "model-a", name: "Model A" }, + { id: "shared", name: "Shared" }, + ]); + await modelsDb.replaceSyncedAvailableModelsForConnection(PROVIDER, "conn-2", [ + { id: "shared", name: "Shared" }, + ]); + + const removed = await modelsDb.removeSyncedAvailableModel(PROVIDER, "shared", "conn-1"); + assert.equal(removed, true); + + // conn-2 still has it, so the union still contains "shared". + const ids = (await modelsDb.getSyncedAvailableModels(PROVIDER)).map((m) => m.id).sort(); + assert.deepEqual(ids, ["model-a", "shared"]); + + // conn-1 no longer has it. + const conn1Ids = (await modelsDb.getSyncedAvailableModelsForConnection(PROVIDER, "conn-1")) + .map((m) => m.id) + .sort(); + assert.deepEqual(conn1Ids, ["model-a"]); +});