Skip to content

Fix/3199 llamacpp model delete - #3236

Closed
diegosouzapw wants to merge 2 commits into
release/v3.8.11from
fix/3199-llamacpp-model-delete
Closed

diegosouzapw wants to merge 2 commits into
release/v3.8.11from
fix/3199-llamacpp-model-delete

Conversation

@diegosouzapw

Copy link
Copy Markdown
Collaborator

Summary

  • Describe the user-facing or operational change.

Related Issues

  • Closes #
  • Related to #

Validation

  • npm run lint
  • npm run test:unit
  • npm run test:coverage
  • Coverage is still >= 60% for statements, lines, functions, and branches
  • SonarQube PR analysis is green or any remaining issues are explicitly documented below

Tests Added Or Updated

  • List every changed or added automated test file.
  • If no production code changed, state that here.

Coverage Notes

  • If this PR changes src/, open-sse/, electron/, or bin/, explain which tests cover the change.
  • If coverage moved down in any touched file, explain why and what follow-up task will recover it.

Reviewer Notes

  • Call out any risky areas, migrations, feature flags, or manual validation that reviewers should know about.

diegosouzapw and others added 2 commits June 5, 2026 02:50
…eep 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 <tjengbudi@users.noreply.github.com>
# Conflicts:
#	src/app/api/provider-models/route.ts
#	src/lib/db/models.ts

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request addresses issue #3199 by enabling the deletion of individual synced models and ensuring these deletions persist across auto-fetches by marking deleted models as hidden. It updates the DELETE and GET API routes, refactors removeSyncedAvailableModel to support connection-scoped deletions, and adds comprehensive regression tests. The code review feedback highlights a critical performance bottleneck where getModelIsHidden (which performs synchronous database queries and JSON parsing) is called inside .filter() loops. The reviewer recommends introducing a getHiddenModelIds helper to retrieve hidden IDs once and perform O(1) lookups, as well as wrapping database updates in a transaction for atomicity and better write performance.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread src/lib/db/models.ts
Comment on lines 719 to 721
const normalizedModels = normalizeSyncedAvailableModels(models).filter(
(m) => !getModelIsHidden(providerId, m.id)
);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Calling getModelIsHidden inside a .filter() loop is a performance bottleneck. getModelIsHidden performs synchronous database queries and parses JSON arrays on every single call. For a provider with many models, this results in O(N) database queries and O(N * M) JSON parsing operations. We can optimize this by retrieving the set of hidden model IDs once using a new helper function getHiddenModelIds and checking against the Set in O(1) time.

  const hiddenIds = getHiddenModelIds(providerId);
  const normalizedModels = normalizeSyncedAvailableModels(models).filter(
    (m) => !hiddenIds.has(m.id)
  );

Comment thread src/lib/db/models.ts
Comment on lines +769 to +832
/**
* 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;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

This refactoring addresses two key issues:

  1. Performance Optimization: Added the getHiddenModelIds helper function to fetch all hidden model IDs for a provider in a single query, avoiding O(N) database queries inside loops.
  2. Database Transaction Safety: Wrapped the database updates inside db.transaction() to ensure atomicity and significantly improve write performance when deleting a model across multiple connections.
/**
 * Get all hidden model IDs for a provider.
 * This is a highly optimized helper to avoid querying the database in a loop.
 */
export function getHiddenModelIds(providerId: string): Set<string> {
  const hidden = new Set<string>();
  const db = getDbInstance();
  const row = db
    .prepare("SELECT value FROM key_value WHERE namespace = 'customModels' AND key = ?")
    .get(providerId);
  const value = getKeyValue(row).value;
  if (value) {
    try {
      const models = JSON.parse(value);
      if (Array.isArray(models)) {
        for (const m of models) {
          if (m && typeof m === "object" && m.isHidden === true && m.id) {
            hidden.add(m.id);
          }
        }
      }
    } catch {}
  }

  const compatList = readCompatList(providerId);
  for (const co of compatList) {
    if (co.isHidden === true && co.id) {
      hidden.add(co.id);
    }
  }
  return hidden;
}

/**
 * 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;
  const runUpdate = db.transaction(() => {
    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");
  });

  runUpdate();
  return changed;
}

Comment on lines +811 to +814
// #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));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Avoid calling getModelIsHidden inside the .filter() loop as it performs synchronous database queries and JSON parsing on every iteration. Instead, load the set of hidden model IDs once using getHiddenModelIds and filter in O(1) time.

      // #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 { getHiddenModelIds } = await import("@/lib/db/models");
      const hiddenIds = getHiddenModelIds(provider);
      const visibleSynced = allSynced.filter((m) => !hiddenIds.has(m.id));

Comment on lines +939 to +942
// #3199 — keep operator-deleted (hidden) models out of the re-derived list.
freshSynced = (await getSyncedAvailableModels(provider)).filter(
(m) => !getModelIsHidden(provider, m.id)
);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Avoid calling getModelIsHidden inside the .filter() loop to prevent executing synchronous database queries and JSON parsing on every iteration. Use getHiddenModelIds to load the hidden model IDs once and filter efficiently.

        // #3199 — keep operator-deleted (hidden) models out of the re-derived list.
        const { getHiddenModelIds } = await import("@/lib/db/models");
        const hiddenIds = getHiddenModelIds(provider);
        freshSynced = (await getSyncedAvailableModels(provider)).filter(
          (m) => !hiddenIds.has(m.id)
        );

@diegosouzapw

Copy link
Copy Markdown
Collaborator Author

Closing as redundant: the synced/fetched-model delete-persistence fix (#3199) already shipped to release/v3.8.11 via commits 6fe7c6b (#3212) and 7b4bda1 (#3204). Stale fork; superseded. Deleting the branch.

@diegosouzapw
diegosouzapw deleted the fix/3199-llamacpp-model-delete branch June 5, 2026 16:13
@diegosouzapw diegosouzapw mentioned this pull request Jun 5, 2026
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant