Pr 3211

src/app/api/provider-models/route.ts

@@ -397,17 +397,22 @@ export async function DELETE(request) {
       );
     }
 
+    // #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) {
-      // #3199: mark the deleted synced model hidden so a later auto-fetch
-      // re-import (replaceSyncedAvailableModelsForConnection) does not re-add it
-      // — otherwise the model reappears and the delete looks like it did nothing.
       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(

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<ReturnType<typeof getSyncedAvailableModels>> = [];
       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 */
       }

src/lib/db/models.ts

@@ -714,8 +714,8 @@ export async function replaceSyncedAvailableModelsForConnection(
 ): Promise<SyncedAvailableModel[]> {
   const db = getDbInstance();
   const key = `${providerId}:${connectionId}`;
-  // #3199: drop ids the operator has deleted/hidden so a re-fetch does not
-  // re-import a model that was explicitly removed.
+  // #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)
   );
@@ -733,59 +733,6 @@ export async function replaceSyncedAvailableModelsForConnection(
   return getSyncedAvailableModels(providerId);
 }
 
-/**
- * Remove a single synced available model from all connections of a provider.
- * Returns true if the model was found and removed from at least one connection.
- */
-export async function removeSyncedAvailableModel(
-  providerId: string,
-  modelId: string
-): Promise<boolean> {
-  const db = getDbInstance();
-  const prefix = `${providerId}:`;
-  const rows = db
-    .prepare(
-      "SELECT key, value FROM key_value WHERE namespace = 'syncedAvailableModels' AND key LIKE ?"
-    )
-    .all(`${prefix}%`);
-
-  let removedAny = false;
-  const removeModel = db.transaction(() => {
-    for (const row of rows) {
-      const { key, value } = getKeyValue(row);
-      if (!key || value === null) continue;
-
-      let parsedModels: unknown;
-      try {
-        parsedModels = JSON.parse(value);
-      } catch (error) {
-        console.warn(`[DB] Skipping malformed syncedAvailableModels entry for key ${key}:`, error);
-        continue;
-      }
-
-      const models = normalizeSyncedAvailableModels(parsedModels);
-      const filtered = models.filter((m) => m.id !== modelId);
-      if (filtered.length !== models.length) {
-        removedAny = 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 (removedAny) backupDbFile("pre-write");
-  });
-
-  removeModel();
-  return removedAny;
-}
-
 /**
  * Delete all synced models for a specific connection.
  * Returns the remaining unioned list for the provider.
@@ -819,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<boolean> {
+  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.

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"]);
+});