From d897ea00e0eb945915eeaf35b025745fe80d409f Mon Sep 17 00:00:00 2001 From: "[._.]/ Adam Eivy" Date: Sat, 15 Aug 2026 04:46:39 +0000 Subject: [PATCH 1/2] perf: dedup Ollama provider model-refresh fan-out by daemon base URL (#4154) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Several providers commonly resolve to one Ollama daemon (the built-in `ollama` provider plus the four shipped Claude/OpenCode-over-Ollama CLI/TUI providers, all defaulting to the same local endpoint). The post-install/delete fan-out in `refreshOllamaBackedProviders` called `refreshProviderModels` once per provider, so each one independently re-fetched `/api/tags` and re-ran the whole per-model `/api/show` tool-capability probe against an identical daemon and model set. Providers are now bucketed by `ollamaRefreshGroupKey` — the daemon base URL plus the probe shape — and a bucket with more than one member is probed once via the new compute-only `fetchProviderModels`, then applied to every member with `updateProvider`. The two probe shapes stay in separate key namespaces: an `api`-type provider persists the unfiltered tag list while a CLI/TUI one persists the tool-use-capable subset, so collapsing them would cross-contaminate the lists. A provider with no group key, or the only member of its bucket, keeps the plain one-call refresh. --- .changelog/next/changed-issue-4154.md | 1 + server/lib/aiToolkit/index.js | 9 +- .../lib/aiToolkit/internal/modelFetchers.js | 51 +++++++- .../aiToolkit/internal/modelFetchers.test.js | 61 ++++++++- server/lib/aiToolkit/internal/ollamaBacked.js | 16 +++ server/lib/aiToolkit/providers.js | 51 +++++--- server/lib/aiToolkit/providers.test.js | 73 +++++++++++ server/services/localLlm.js | 67 +++++++++- server/services/localLlm.test.js | 116 ++++++++++++++++-- server/services/providers.js | 18 ++- 10 files changed, 429 insertions(+), 34 deletions(-) create mode 100644 .changelog/next/changed-issue-4154.md diff --git a/.changelog/next/changed-issue-4154.md b/.changelog/next/changed-issue-4154.md new file mode 100644 index 0000000000..662e24921b --- /dev/null +++ b/.changelog/next/changed-issue-4154.md @@ -0,0 +1 @@ +- Ollama provider model refresh now probes each daemon once and applies the result to every provider sharing it, instead of re-running the full /api/tags + per-model capability sweep per provider diff --git a/server/lib/aiToolkit/index.js b/server/lib/aiToolkit/index.js index ddeb67f8f3..be5b89ab14 100644 --- a/server/lib/aiToolkit/index.js +++ b/server/lib/aiToolkit/index.js @@ -6,7 +6,12 @@ import { fileURLToPath } from 'url'; import { dirname, join } from 'path'; -import { canRefreshModels, createProviderService, isOllamaBackedProvider } from './providers.js'; +import { + canRefreshModels, + createProviderService, + isOllamaBackedProvider, + ollamaRefreshGroupKey, +} from './providers.js'; import { createRunnerService } from './runner.js'; import { createPromptsService } from './prompts.js'; import { createProviderStatusService } from './providerStatus.js'; @@ -25,7 +30,7 @@ export * from './validation.js'; export * from './errorDetection.js'; export * from './constants.js'; export { createProviderService, createRunnerService, createPromptsService, createProviderStatusService }; -export { isOllamaBackedProvider, canRefreshModels }; +export { isOllamaBackedProvider, canRefreshModels, ollamaRefreshGroupKey }; export { createProvidersRoutes, createRunsRoutes, createPromptsRoutes, createProviderStatusRoutes }; export function createAIToolkit(config = {}) { diff --git a/server/lib/aiToolkit/internal/modelFetchers.js b/server/lib/aiToolkit/internal/modelFetchers.js index ed9ad7eeb3..9ec0f36d91 100644 --- a/server/lib/aiToolkit/internal/modelFetchers.js +++ b/server/lib/aiToolkit/internal/modelFetchers.js @@ -39,7 +39,7 @@ */ import { ANTIGRAVITY_TUI_ID, isAntigravityCommand } from './antigravity.js'; import { CURSOR_TUI_ID, isCursorCommand } from './cursor.js'; -import { isOllamaBackedProvider } from './ollamaBacked.js'; +import { isOllamaBackedProvider, ollamaBaseFromProvider } from './ollamaBacked.js'; const displayName = (provider) => String(provider?.name || '').toLowerCase(); @@ -161,3 +161,52 @@ export function withRefreshCapabilityList(providers) { if (!Array.isArray(providers)) return providers; return providers.map(withRefreshCapability); } + +/** + * Stable key grouping providers whose model refresh issues the IDENTICAL probe + * against the same Ollama daemon — or `null` when the provider's refresh isn't + * an Ollama probe at all (refresh it on its own). + * + * Why a host wants this: several providers commonly resolve to ONE daemon (the + * built-in `ollama` provider plus any number of Claude/Codex/Gemini-over-Ollama + * CLI/TUI providers, all defaulting to `http://localhost:11434`). Refreshing + * them one by one re-fetches `/api/tags` and re-runs the whole per-model + * `/api/show` capability probe once per provider for a result that cannot + * differ. Grouping on this key lets the caller fetch once and apply the answer + * to every member (`fetchProviderModels` + `updateProvider`). + * + * `null` is a real sentinel, NOT "no group": a provider without a key must + * still be refreshed individually. Never treat a nullish key as a bucket. + * + * Two probe shapes are deliberately kept in SEPARATE key namespaces because + * they return different lists for the same daemon: + * - `api:` — an `api`-type provider short-circuits in `_refreshAPIProviderModels` + * to `${endpoint}/api/tags` and persists the UNFILTERED tag list. + * - `tools:` — a `cli`/`tui` provider routes to `_fetchOllamaToolCapableModels`, + * which filters down to tool-use-capable models. + * Collapsing the two would persist a tool-filtered list onto the plain `ollama` + * provider (or an unfiltered one onto a Claude harness that then silently fails + * to edit files). + * + * @param {object|null|undefined} provider + * @param {Array} [table] + * @returns {string|null} + */ +export function ollamaRefreshGroupKey(provider, table = MODEL_FETCHERS) { + if (!provider) return null; + if (provider.type === 'api') { + // Mirror of the short-circuit condition in `_refreshAPIProviderModels`. + // `apiKey` is part of the identity, not an extra: when the `/api/tags` + // short-circuit misses, that method falls THROUGH to a generic `/models` + // fetch carrying `provider.apiKey`, so two providers on one endpoint with + // different keys can legitimately see different catalogs. Only the keyless + // shape is safe to share, and a key makes the provider ungroupable. + if (provider.apiKey) return null; + const endpoint = String(provider.endpoint || ''); + if (!endpoint.includes('ollama') && !endpoint.includes(':11434')) return null; + return `api:${endpoint.replace(/\/+$/, '')}`; + } + return resolveModelFetcher(provider, table)?.key === 'ollama' + ? `tools:${ollamaBaseFromProvider(provider)}` + : null; +} diff --git a/server/lib/aiToolkit/internal/modelFetchers.test.js b/server/lib/aiToolkit/internal/modelFetchers.test.js index bc2d39bef2..617b121cfb 100644 --- a/server/lib/aiToolkit/internal/modelFetchers.test.js +++ b/server/lib/aiToolkit/internal/modelFetchers.test.js @@ -2,7 +2,7 @@ import { describe, it, expect } from 'vitest'; import { readFileSync } from 'node:fs'; import { fileURLToPath } from 'node:url'; import { dirname, resolve } from 'node:path'; -import { MODEL_FETCHERS, canRefreshModels, resolveModelFetcher, withRefreshCapability, withRefreshCapabilityList } from './modelFetchers.js'; +import { MODEL_FETCHERS, canRefreshModels, ollamaRefreshGroupKey, resolveModelFetcher, withRefreshCapability, withRefreshCapabilityList } from './modelFetchers.js'; const __dirname = dirname(fileURLToPath(import.meta.url)); const SHIPPED = JSON.parse(readFileSync(resolve(__dirname, '../../../../data.reference/providers.json'), 'utf8')); @@ -42,6 +42,65 @@ describe('MODEL_FETCHERS — shipped catalog visibility is unchanged', () => { }); }); +describe('ollamaRefreshGroupKey — one probe per daemon, not one per provider', () => { + it('collapses every shipped CLI/TUI Ollama provider onto ONE key', () => { + // The shipped catalog ships four of them (claude-ollama, claude-ollama-tui, + // opencode-ollama, opencode-ollama-tui) all pointed at the same default + // daemon — the exact fan-out this key exists to dedup. If a future seed adds + // a fifth on the same daemon it must land in this same bucket. + const shared = Object.values(SHIPPED.providers) + .filter((p) => (p.type === 'cli' || p.type === 'tui') && p.ollamaBacked === true); + expect(shared.length).toBeGreaterThanOrEqual(4); + const keys = new Set(shared.map((p) => ollamaRefreshGroupKey(p))); + expect(keys.size).toBe(1); + expect([...keys][0]).toBe('tools:http://localhost:11434'); + }); + + it('keeps the api-type ollama provider OUT of the tool-filtered bucket', () => { + // `_refreshAPIProviderModels` persists the unfiltered tag list; the CLI/TUI + // probe persists a tool-use-only subset. Same daemon, different answers. + const api = SHIPPED.providers.ollama; + expect(api.type).toBe('api'); + expect(ollamaRefreshGroupKey(api)).toBe('api:http://localhost:11434/v1'); + expect(ollamaRefreshGroupKey(api)).not.toBe(ollamaRefreshGroupKey(SHIPPED.providers['claude-ollama'])); + }); + + it('separates providers on DIFFERENT daemons', () => { + const local = { id: 'a', type: 'cli', ollamaBacked: true, envVars: { ANTHROPIC_BASE_URL: 'http://localhost:11434' } }; + const remote = { id: 'b', type: 'cli', ollamaBacked: true, envVars: { ANTHROPIC_BASE_URL: 'http://192.0.2.10:11434' } }; + expect(ollamaRefreshGroupKey(local)).not.toBe(ollamaRefreshGroupKey(remote)); + }); + + it('normalizes trailing slashes and an OpenAI-compat /v1 to the same daemon key', () => { + const bare = { id: 'a', type: 'tui', ollamaBacked: true, envVars: { ANTHROPIC_BASE_URL: 'http://localhost:11434' } }; + const suffixed = { id: 'b', type: 'tui', ollamaBacked: true, envVars: { ANTHROPIC_BASE_URL: 'http://localhost:11434/v1/' } }; + expect(ollamaRefreshGroupKey(suffixed)).toBe(ollamaRefreshGroupKey(bare)); + }); + + it('returns null — never a shared bucket — for anything that is not an Ollama probe', () => { + // A null key means "refresh me individually". Treating it as a group would + // persist one vendor's catalog onto every other provider. + expect(ollamaRefreshGroupKey(null)).toBeNull(); + expect(ollamaRefreshGroupKey({ id: 'anthropic', type: 'api', endpoint: 'https://api.anthropic.com' })).toBeNull(); + expect(ollamaRefreshGroupKey({ id: 'cursor-cli', type: 'cli', command: 'cursor-agent' })).toBeNull(); + expect(ollamaRefreshGroupKey({ id: 'codex-tui', type: 'tui', command: 'codex' })).toBeNull(); + // An api provider with no endpoint at all has nothing to key on. + expect(ollamaRefreshGroupKey({ id: 'x', type: 'api' })).toBeNull(); + }); + + it('never groups two API providers that would attach DIFFERENT keys to the same probe', () => { + // `_refreshAPIProviderModels` falls through from its `/api/tags` + // short-circuit to a generic `/models` fetch carrying `provider.apiKey`, so + // a keyed provider is ungroupable even on an Ollama-shaped endpoint. + const paid = { id: 'x', type: 'api', endpoint: 'https://api.example.com/v1', apiKey: 'k1' }; + expect(ollamaRefreshGroupKey(paid)).toBeNull(); + const keyed = { id: 'y', type: 'api', endpoint: 'http://localhost:11434/v1', apiKey: 'k2' }; + const alsoKeyed = { id: 'z', type: 'api', endpoint: 'http://localhost:11434/v1', apiKey: 'k3' }; + expect(ollamaRefreshGroupKey(keyed)).toBeNull(); + expect(ollamaRefreshGroupKey(alsoKeyed)).toBeNull(); + }); +}); + describe('resolveModelFetcher — the ordering the old chains encoded in prose', () => { it('routes an Ollama-backed claude CLI to Ollama, not the static Anthropic list', () => { // The one load-bearing row order: ollama first. diff --git a/server/lib/aiToolkit/internal/ollamaBacked.js b/server/lib/aiToolkit/internal/ollamaBacked.js index 42ec20729c..4b4642684d 100644 --- a/server/lib/aiToolkit/internal/ollamaBacked.js +++ b/server/lib/aiToolkit/internal/ollamaBacked.js @@ -22,3 +22,19 @@ export function isOllamaBackedProvider(provider) { const base = String(provider?.envVars?.ANTHROPIC_BASE_URL || provider?.endpoint || ''); return /:11434\b/.test(base) || /ollama/i.test(base); } + +/** + * Normalize an Ollama base URL (strip trailing slash + an OpenAI-compat `/v1`) + * so two providers pointed at the same daemon through differently-spelled URLs + * resolve to the same string. + * + * Lives here beside {@link isOllamaBackedProvider} rather than in `providers.js` + * so `internal/modelFetchers.js` can build a refresh group key on it without + * importing back into `providers.js` and forming a module cycle. Re-exported + * from `providers.js` (and `server/services/providers.js`) for hosts that group + * providers by daemon. + */ +export function ollamaBaseFromProvider(provider) { + const base = String(provider?.envVars?.ANTHROPIC_BASE_URL || provider?.endpoint || 'http://localhost:11434'); + return base.replace(/\/+$/, '').replace(/\/v1$/, ''); +} diff --git a/server/lib/aiToolkit/providers.js b/server/lib/aiToolkit/providers.js index b1e1d3e0d7..fffb0de222 100644 --- a/server/lib/aiToolkit/providers.js +++ b/server/lib/aiToolkit/providers.js @@ -20,12 +20,17 @@ import { CURSOR_COMMAND, parseCursorModelList, } from './internal/cursor.js'; -import { isOllamaBackedProvider } from './internal/ollamaBacked.js'; -import { canRefreshModels, resolveModelFetcher } from './internal/modelFetchers.js'; +import { isOllamaBackedProvider, ollamaBaseFromProvider } from './internal/ollamaBacked.js'; +import { canRefreshModels, ollamaRefreshGroupKey, resolveModelFetcher } from './internal/modelFetchers.js'; // Re-exported (rather than defined here) so the model-fetcher table can key its // ollama row on the same predicate without importing back into this module. export { isOllamaBackedProvider }; +// Groups providers that share one daemon + one probe shape, so a host fanning a +// refresh across them can fetch once instead of once per provider. The base-URL +// normalizer it keys on stays internal — the group key IS the contract, and an +// exported normalizer only invites callers to re-derive the grouping rule. +export { ollamaRefreshGroupKey }; // The pure capability predicate that both refresh arms below dispatch through — // exported so the providers route can decorate its payload with it and the // client stops re-deriving refreshability from command/name string sniffing. @@ -125,12 +130,6 @@ const TOOL_USE_RE = new RegExp([ 'deepseek-v3', 'deepseek-r1', 'deepseek-v4', ].join('|'), 'i'); -/** Normalize an Ollama base URL (strip trailing slash + an OpenAI-compat `/v1`). */ -function ollamaBaseFromProvider(provider) { - const base = String(provider?.envVars?.ANTHROPIC_BASE_URL || provider?.endpoint || 'http://localhost:11434'); - return base.replace(/\/+$/, '').replace(/\/v1$/, ''); -} - /** * Whether an Ollama model supports tool use. Prefers the authoritative `tools` * capability from /api/show (a non-empty capabilities array without `tools` is @@ -725,7 +724,23 @@ export function createProviderService(config = {}) { return { success: false, error: 'Unknown provider type' }; }, - async refreshProviderModels(id) { + /** + * Probe a provider's model list WITHOUT persisting it — the compute half of + * {@link refreshProviderModels}. + * + * Split out so a host fanning a refresh across several providers backed by + * the SAME upstream (see {@link ollamaRefreshGroupKey}) can run the probe + * once and apply the answer to each of them via `updateProvider`, instead of + * re-issuing an identical `/api/tags` + per-model `/api/show` sweep per + * provider. + * + * Same contract as `refreshProviderModels` minus the write: `null` means + * ONLY "no such provider"; every other failure throws (with `.status`). + * + * @param {string} id + * @returns {Promise} + */ + async fetchProviderModels(id) { const data = await loadProviders(); const provider = data.providers[id]; @@ -799,14 +814,18 @@ export function createProviderService(config = {}) { throw unsupported; } - const updatedProvider = { - ...data.providers[id], - models - }; + return models; + }, - data.providers[id] = updatedProvider; - await saveProviders(data); - return updatedProvider; + /** + * Probe AND persist a provider's model list. Thin composition of + * {@link fetchProviderModels} + `updateProvider` — keep it that way so the + * two halves can't drift. + */ + async refreshProviderModels(id) { + const models = await this.fetchProviderModels(id); + if (models === null) return null; + return this.updateProvider(id, { models }); }, async _refreshAPIProviderModels(provider) { diff --git a/server/lib/aiToolkit/providers.test.js b/server/lib/aiToolkit/providers.test.js index 81d109ef07..ad7dcb28e2 100644 --- a/server/lib/aiToolkit/providers.test.js +++ b/server/lib/aiToolkit/providers.test.js @@ -830,6 +830,79 @@ describe('Provider Service', () => { expect(await providerService.refreshProviderModels('no-such-provider')).toBeNull(); }); + describe('fetchProviderModels — the compute half, without the write', () => { + const ollamaCli = { + name: 'Claude Ollama (local model)', + type: 'cli', + command: 'claude', + ollamaBacked: true, + models: ['stale-model'], + envVars: { ANTHROPIC_BASE_URL: 'http://localhost:11434' }, + }; + + it('returns the probed list and leaves the stored one untouched', async () => { + stubOllama(['qwen2.5:7b', 'gemma2:9b'], { + 'qwen2.5:7b': ['completion', 'tools'], + 'gemma2:9b': ['completion', 'vision'], + }); + const p = await providerService.createProvider(ollamaCli); + + expect(await providerService.fetchProviderModels(p.id)).toEqual(['qwen2.5:7b']); + // The whole point of the split: a caller can fan ONE probe out to + // several providers itself, so this call must not have persisted. + expect((await providerService.getProviderById(p.id)).models).toEqual(['stale-model']); + }); + + it('is what refreshProviderModels persists — the two halves cannot drift', async () => { + stubOllama(['qwen2.5:7b', 'gemma2:9b'], { + 'qwen2.5:7b': ['completion', 'tools'], + 'gemma2:9b': ['completion', 'vision'], + }); + const p = await providerService.createProvider(ollamaCli); + + const fetched = await providerService.fetchProviderModels(p.id); + const persisted = await providerService.refreshProviderModels(p.id); + expect(persisted.models).toEqual(fetched); + expect((await providerService.getProviderById(p.id)).models).toEqual(fetched); + }); + + it('applying a fetched list to a SIBLING provider matches refreshing it directly', async () => { + // The dedup path in localLlm.js: probe once through one member of a + // group, then `updateProvider(id, { models })` every member. + stubOllama(['qwen2.5:7b', 'gemma2:9b'], { + 'qwen2.5:7b': ['completion', 'tools'], + 'gemma2:9b': ['completion', 'vision'], + }); + const lead = await providerService.createProvider({ ...ollamaCli, name: 'Lead Ollama' }); + const sibling = await providerService.createProvider({ ...ollamaCli, name: 'Sibling Ollama', type: 'tui' }); + + const models = await providerService.fetchProviderModels(lead.id); + const applied = await providerService.updateProvider(sibling.id, { models }); + const directly = await providerService.refreshProviderModels(sibling.id); + + expect(applied.models).toEqual(directly.models); + // Every other field survives the apply — `updateProvider` spreads. + expect(applied.ollamaBacked).toBe(true); + expect(applied.id).toBe(sibling.id); + }); + + it('returns null only for a provider that does not exist', async () => { + expect(await providerService.fetchProviderModels('no-such-provider')).toBeNull(); + }); + + it('throws (does not return null) when the probe fails', async () => { + const p = await providerService.createProvider(ollamaCli); + vi.stubGlobal('fetch', vi.fn(async () => ({ ok: false, status: 503 }))); + const errSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + const err = await providerService.fetchProviderModels(p.id).catch(e => e); + errSpy.mockRestore(); + + expect(err).toBeInstanceOf(Error); + expect(err.status).toBe(502); + expect((await providerService.getProviderById(p.id)).models).toEqual(['stale-model']); + }); + }); + // Pins the `provider.id === ANTIGRAVITY_TUI_ID` half of that arm's OR — the // same gap that was open on CURSOR_TUI_ID. Every other antigravity-TUI test // matches on the `agy` command, so deleting the id clause left the suite diff --git a/server/services/localLlm.js b/server/services/localLlm.js index db20394734..a3eb5b35df 100644 --- a/server/services/localLlm.js +++ b/server/services/localLlm.js @@ -36,7 +36,7 @@ import { recommendEditorialModel, isVisionModel, isVisionCapableCliProvider, isT import { commandExists } from '../lib/commandExists.js' import * as ollamaManager from './ollamaManager.js' import * as lmStudioManager from './lmStudioManager.js' -import { getProviderById, getAllProviders, updateProvider, refreshProviderModels, isOllamaBackedProvider } from './providers.js' +import { getProviderById, getAllProviders, updateProvider, refreshProviderModels, fetchProviderModels, isOllamaBackedProvider, ollamaRefreshGroupKey } from './providers.js' const execFileAsync = promisify(execFile) const ENV_PATH = join(PATHS.root, '.env') @@ -861,6 +861,36 @@ async function listVisionCliModels() { // ---- install / delete -------------------------------------------------------- +/** + * Fetch one Ollama probe for a group of providers that share a daemon + probe + * shape, then persist that single answer onto each of them. + * + * The probe is issued through the group's first member; every member (that one + * included) is then written via `updateProvider`, so no provider is special- + * cased. Writes are sequential because `saveProviders` is a whole-file + * read-modify-write — firing them concurrently would have members clobber each + * other's models. + * + * A failed probe skips the whole group with ONE log line: every member would + * have failed identically against the same unreachable daemon, so N copies of + * the same error is noise, not information. + */ +async function refreshOllamaProviderGroup(group) { + const [lead] = group + const models = await fetchProviderModels(lead.id).catch((err) => { + console.error(`⚠️ Failed to refresh models for ${group.length} Ollama-backed provider(s) via ${lead.id}: ${err.message}`) + return null + }) + // `null` = probe failed or the provider vanished; `[]` = a real, empty catalog + // (the user just deleted their last model) and MUST still be persisted. + if (models === null) return + for (const p of group) { + await updateProvider(p.id, { models }).catch((err) => { + console.error(`⚠️ Failed to save refreshed models for provider ${p.id}: ${err.message}`) + }) + } +} + /** * Push a live model-list refresh to every provider backed by the Ollama daemon, * so an install/delete on the Local LLMs tab is immediately reflected in every @@ -870,13 +900,42 @@ async function listVisionCliModels() { * wait on an extra round-trip to Ollama per matching provider. Best-effort per * provider — one failing refresh (e.g. Ollama briefly unreachable mid-pull) * must not block the others. + * + * Providers are DEDUPED by `ollamaRefreshGroupKey` before the fan-out. Several + * providers (the built-in `ollama` one plus any Claude/Codex/Gemini-over-Ollama + * CLI/TUI providers) normally resolve to the same `http://localhost:11434` + * daemon; refreshing each in turn re-ran the full `/api/tags` + per-model + * `/api/show` capability sweep once per provider for an answer that cannot + * differ. One probe per (daemon, probe shape) now serves all of them. */ function refreshOllamaBackedProviders() { getAllProviders().then(({ providers }) => { const targets = (providers || []).filter(isOllamaBackedProvider) - return Promise.all(targets.map((p) => refreshProviderModels(p.id).catch((err) => { - console.error(`⚠️ Failed to refresh models for provider ${p.id} after Ollama model change: ${err.message}`) - }))) + const groups = new Map() + const singles = [] + for (const p of targets) { + // A null key is "not an Ollama probe", NOT a bucket — those providers + // still get their own individual refresh. + const key = ollamaRefreshGroupKey(p) + if (!key) { singles.push(p); continue } + const existing = groups.get(key) + if (existing) existing.push(p) + else groups.set(key, [p]) + } + // Only a group with something to SHARE takes the split fetch/apply path; a + // lone member keeps the plain one-call refresh, so the common single-daemon + // single-provider install behaves exactly as it did before. + const shared = [] + for (const group of groups.values()) { + if (group.length === 1) singles.push(group[0]) + else shared.push(group) + } + return Promise.all([ + ...singles.map((p) => refreshProviderModels(p.id).catch((err) => { + console.error(`⚠️ Failed to refresh models for provider ${p.id} after Ollama model change: ${err.message}`) + })), + ...shared.map(refreshOllamaProviderGroup), + ]) }).catch((err) => { console.error(`⚠️ Failed to list providers for post-install Ollama refresh: ${err.message}`) }) diff --git a/server/services/localLlm.test.js b/server/services/localLlm.test.js index 68634209c6..3d31bf525a 100644 --- a/server/services/localLlm.test.js +++ b/server/services/localLlm.test.js @@ -49,19 +49,23 @@ const mocks = vi.hoisted(() => ({ getAllProviders: vi.fn(async () => ({ providers: [] })), updateProvider: vi.fn(async () => ({})), refreshProviderModels: vi.fn(async (id) => ({ id, models: [] })), - // Mirrors the real aiToolkit/providers.js predicate (not a spy — tests - // assert against refreshProviderModels calls, not this classification). - isOllamaBackedProvider: (provider) => { - if (provider?.id === 'ollama') return true; - if (provider?.ollamaBacked === true) return true; - const base = String(provider?.envVars?.ANTHROPIC_BASE_URL || provider?.endpoint || ''); - return /:11434\b/.test(base) || /ollama/i.test(base); - } + fetchProviderModels: vi.fn(async () => ['qwen2.5:7b']) } })); vi.mock('./ollamaManager.js', () => mocks.ollama); vi.mock('./lmStudioManager.js', () => mocks.lmstudio); -vi.mock('./providers.js', () => mocks.providers); +// The two classification helpers come from the REAL toolkit module rather than +// being re-implemented here: they decide which providers get refreshed and which +// share one probe, so a hand-mirrored copy would let the service and the suite +// drift together and assert nothing. Everything with a side effect stays a spy. +vi.mock('./providers.js', async () => { + const real = await import('../lib/aiToolkit/providers.js'); + return { + ...mocks.providers, + isOllamaBackedProvider: real.isOllamaBackedProvider, + ollamaRefreshGroupKey: real.ollamaRefreshGroupKey + }; +}); // child_process is mocked so the install/upgrade paths (spawn-based streaming + // execFile-based presence checks) are drivable. Defaults are benign for the rest @@ -109,6 +113,12 @@ const flushMicrotasks = () => new Promise((resolve) => setTimeout(resolve, 0)); let svc; beforeEach(async () => { vi.clearAllMocks(); // clears calls, keeps the default impls defined above + // `clearAllMocks` clears recorded CALLS but not queued `…Once` values, and the + // provider fan-out is fire-and-forget: a test where it correctly never runs + // (a failed pull) leaves its `getAllProviders` answer queued and the NEXT test + // silently consumes it. `mockReset` drops the queue and restores the default + // implementation each spy was declared with. + for (const fn of Object.values(mocks.providers)) fn.mockReset(); cp.spawn = cp.defaults.spawn; // reset child_process drivers to benign defaults cp.execFile = cp.defaults.execFile; delete process.env.LLM_BACKEND; @@ -240,6 +250,94 @@ describe('localLlm', () => { await flushMicrotasks(); expect(mocks.providers.refreshProviderModels).not.toHaveBeenCalled(); }); + it('probes the daemon ONCE for providers that share it, then applies that answer to each', async () => { + // Four CLI/TUI providers on the same default daemon — the shipped catalog + // ships exactly this shape. Before the dedup each one re-ran /api/tags plus + // the whole per-model /api/show capability sweep for an identical answer. + mocks.providers.getAllProviders.mockResolvedValueOnce({ + providers: [ + { id: 'claude-ollama', type: 'cli', ollamaBacked: true, envVars: { ANTHROPIC_BASE_URL: 'http://localhost:11434' } }, + { id: 'claude-ollama-tui', type: 'tui', ollamaBacked: true, envVars: { ANTHROPIC_BASE_URL: 'http://localhost:11434/v1' } }, + { id: 'opencode-ollama', type: 'cli', ollamaBacked: true }, + { id: 'opencode-ollama-tui', type: 'tui', ollamaBacked: true } + ] + }); + await svc.installModel('ollama', 'llama3.2'); + await flushMicrotasks(); + + expect(mocks.providers.fetchProviderModels).toHaveBeenCalledTimes(1); + expect(mocks.providers.refreshProviderModels).not.toHaveBeenCalled(); + // …and every member still ends up with the models, including the one the + // probe was issued through. + expect(mocks.providers.updateProvider.mock.calls.map(([id]) => id)).toEqual([ + 'claude-ollama', 'claude-ollama-tui', 'opencode-ollama', 'opencode-ollama-tui' + ]); + for (const [, updates] of mocks.providers.updateProvider.mock.calls) { + expect(updates).toEqual({ models: ['qwen2.5:7b'] }); + } + }); + + it('keeps providers on different daemons (and different probe shapes) apart', async () => { + mocks.providers.getAllProviders.mockResolvedValueOnce({ + providers: [ + // Same daemon, but an api-type provider persists the UNFILTERED tag + // list — a different answer, so it must not join the tools bucket. + { id: 'ollama', type: 'api', endpoint: 'http://localhost:11434/v1' }, + { id: 'local-a', type: 'cli', ollamaBacked: true, envVars: { ANTHROPIC_BASE_URL: 'http://localhost:11434' } }, + { id: 'local-b', type: 'tui', ollamaBacked: true, envVars: { ANTHROPIC_BASE_URL: 'http://localhost:11434' } }, + { id: 'remote', type: 'cli', ollamaBacked: true, envVars: { ANTHROPIC_BASE_URL: 'http://192.0.2.10:11434' } } + ] + }); + await svc.installModel('ollama', 'llama3.2'); + await flushMicrotasks(); + + // The api provider and the lone remote daemon each keep a plain one-call + // refresh; only the two-member local group takes the split fetch/apply path. + expect(mocks.providers.refreshProviderModels.mock.calls.map(([id]) => id).sort()).toEqual(['ollama', 'remote']); + expect(mocks.providers.fetchProviderModels).toHaveBeenCalledTimes(1); + expect(mocks.providers.updateProvider.mock.calls.map(([id]) => id)).toEqual(['local-a', 'local-b']); + }); + + it('skips a whole group with one log line when its shared probe fails', async () => { + mocks.providers.getAllProviders.mockResolvedValueOnce({ + providers: [ + { id: 'local-a', type: 'cli', ollamaBacked: true }, + { id: 'local-b', type: 'tui', ollamaBacked: true } + ] + }); + mocks.providers.fetchProviderModels.mockRejectedValueOnce(new Error('unreachable')); + const errSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + + const result = await svc.installModel('ollama', 'llama3.2'); + await flushMicrotasks(); + + expect(result.success).toBe(true); + // No half-write: a failed probe must not persist anything. + expect(mocks.providers.updateProvider).not.toHaveBeenCalled(); + expect(errSpy).toHaveBeenCalledTimes(1); + errSpy.mockRestore(); + }); + + it('persists a legitimately EMPTY catalog across the group', async () => { + // `[]` (the user just deleted their last model) is a real answer and must + // be written; only `null` — probe failed — is the skip signal. + mocks.providers.getAllProviders.mockResolvedValueOnce({ + providers: [ + { id: 'local-a', type: 'cli', ollamaBacked: true }, + { id: 'local-b', type: 'tui', ollamaBacked: true } + ] + }); + mocks.providers.fetchProviderModels.mockResolvedValueOnce([]); + + await svc.deleteModel('ollama', 'llama3.2'); + await flushMicrotasks(); + + expect(mocks.providers.updateProvider.mock.calls).toEqual([ + ['local-a', { models: [] }], + ['local-b', { models: [] }] + ]); + }); + it('does not fail install when a provider refresh throws', async () => { mocks.providers.getAllProviders.mockResolvedValueOnce({ providers: [{ id: 'ollama' }] }); mocks.providers.refreshProviderModels.mockRejectedValueOnce(new Error('unreachable')); diff --git a/server/services/providers.js b/server/services/providers.js index 30fcd6debb..60fb30469b 100644 --- a/server/services/providers.js +++ b/server/services/providers.js @@ -15,7 +15,14 @@ export const setAIToolkit = setAIToolkitInstance; // a provider shape without an initialized toolkit instance. // `canRefreshModels` is the model-refresh capability predicate the providers // routes decorate their payloads with; it is derived on read and never stored. -export { isOllamaBackedProvider, canRefreshModels } from '../lib/aiToolkit/providers.js'; +// `ollamaRefreshGroupKey` buckets providers whose refresh hits the same Ollama +// daemon with the same probe, so a fan-out can fetch once per daemon instead of +// once per provider (see `refreshOllamaBackedProviders` in localLlm.js). +export { + isOllamaBackedProvider, + canRefreshModels, + ollamaRefreshGroupKey, +} from '../lib/aiToolkit/providers.js'; export async function getAllProviders() { return requireToolkit().services.providers.getAllProviders(); @@ -52,3 +59,12 @@ export async function testProvider(id) { export async function refreshProviderModels(id) { return requireToolkit().services.providers.refreshProviderModels(id); } + +/** + * Probe a provider's model list without persisting it. Pair with + * `updateProvider(id, { models })` to apply one probe's result to several + * providers that share an upstream. + */ +export async function fetchProviderModels(id) { + return requireToolkit().services.providers.fetchProviderModels(id); +} From 198b90f64cc882b3606469a7d31fa9c06306c41d Mon Sep 17 00:00:00 2001 From: "[._.]/ Adam Eivy" Date: Sat, 15 Aug 2026 04:50:25 +0000 Subject: [PATCH 2/2] address review (antigravity): serialize the refresh fan-out and name the vanished-lead skip (#4154) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three findings from the review pass: - Every refresh ends in `saveProviders`, a whole-file read-modify-write of providers.json, so the fan-out no longer runs under `Promise.all` — concurrent members interleaved and clobbered each other's model arrays. The chain is fire-and-forget background work, so nothing waits on the wall clock. - A group whose lead provider was deleted between the listing and the probe got `null` back from `fetchProviderModels` and was dropped in silence. Probe failure now returns a distinct sentinel so the vanished-lead case logs. - Corrected the `ollamaBaseFromProvider` docstring, which still claimed a re-export that was deliberately dropped. A fourth finding — normalize `/v1` away for api-type providers too — is declined and pinned with a test: that arm probes `${endpoint}/api/tags` and `${endpoint}/models` verbatim, so `…:11434` and `…:11434/v1` are different requests and only the `/v1` spelling answers `/models`. --- .../lib/aiToolkit/internal/modelFetchers.js | 6 +++ .../aiToolkit/internal/modelFetchers.test.js | 16 +++++++ server/lib/aiToolkit/internal/ollamaBacked.js | 7 +-- server/services/localLlm.js | 43 +++++++++++++------ server/services/localLlm.test.js | 22 ++++++++++ 5 files changed, 77 insertions(+), 17 deletions(-) diff --git a/server/lib/aiToolkit/internal/modelFetchers.js b/server/lib/aiToolkit/internal/modelFetchers.js index 9ec0f36d91..2511809ce4 100644 --- a/server/lib/aiToolkit/internal/modelFetchers.js +++ b/server/lib/aiToolkit/internal/modelFetchers.js @@ -204,6 +204,12 @@ export function ollamaRefreshGroupKey(provider, table = MODEL_FETCHERS) { if (provider.apiKey) return null; const endpoint = String(provider.endpoint || ''); if (!endpoint.includes('ollama') && !endpoint.includes(':11434')) return null; + // Only a trailing slash is normalized away here — NOT an OpenAI-compat + // `/v1`, the way `ollamaBaseFromProvider` does for the tools namespace. + // The api arm probes `${endpoint}/api/tags` and then `${endpoint}/models` + // verbatim, so `…:11434` and `…:11434/v1` are two DIFFERENT requests (only + // the `/v1` spelling answers `/models`). Folding them together would share + // one provider's catalog onto another whose own probe would have 404'd. return `api:${endpoint.replace(/\/+$/, '')}`; } return resolveModelFetcher(provider, table)?.key === 'ollama' diff --git a/server/lib/aiToolkit/internal/modelFetchers.test.js b/server/lib/aiToolkit/internal/modelFetchers.test.js index 617b121cfb..a8ac7814a7 100644 --- a/server/lib/aiToolkit/internal/modelFetchers.test.js +++ b/server/lib/aiToolkit/internal/modelFetchers.test.js @@ -72,11 +72,27 @@ describe('ollamaRefreshGroupKey — one probe per daemon, not one per provider', }); it('normalizes trailing slashes and an OpenAI-compat /v1 to the same daemon key', () => { + // Safe here because `_fetchOllamaToolCapableModels` itself resolves the base + // through `ollamaBaseFromProvider` — both spellings issue the SAME request. const bare = { id: 'a', type: 'tui', ollamaBacked: true, envVars: { ANTHROPIC_BASE_URL: 'http://localhost:11434' } }; const suffixed = { id: 'b', type: 'tui', ollamaBacked: true, envVars: { ANTHROPIC_BASE_URL: 'http://localhost:11434/v1/' } }; expect(ollamaRefreshGroupKey(suffixed)).toBe(ollamaRefreshGroupKey(bare)); }); + it('does NOT fold /v1 away for an api provider — that arm probes the endpoint verbatim', () => { + // `_refreshAPIProviderModels` requests `${endpoint}/api/tags` then + // `${endpoint}/models` with no normalization, so `…:11434` and + // `…:11434/v1` are genuinely different probes (only the `/v1` spelling + // answers `/models`). Sharing one answer between them would persist a + // catalog onto a provider whose own refresh would have failed. + const bare = { id: 'a', type: 'api', endpoint: 'http://localhost:11434' }; + const v1 = { id: 'b', type: 'api', endpoint: 'http://localhost:11434/v1' }; + expect(ollamaRefreshGroupKey(bare)).not.toBe(ollamaRefreshGroupKey(v1)); + // A bare trailing slash IS just a spelling of the same URL, so it folds. + expect(ollamaRefreshGroupKey({ id: 'c', type: 'api', endpoint: 'http://localhost:11434/v1/' })) + .toBe(ollamaRefreshGroupKey(v1)); + }); + it('returns null — never a shared bucket — for anything that is not an Ollama probe', () => { // A null key means "refresh me individually". Treating it as a group would // persist one vendor's catalog onto every other provider. diff --git a/server/lib/aiToolkit/internal/ollamaBacked.js b/server/lib/aiToolkit/internal/ollamaBacked.js index 4b4642684d..0a6216e767 100644 --- a/server/lib/aiToolkit/internal/ollamaBacked.js +++ b/server/lib/aiToolkit/internal/ollamaBacked.js @@ -30,9 +30,10 @@ export function isOllamaBackedProvider(provider) { * * Lives here beside {@link isOllamaBackedProvider} rather than in `providers.js` * so `internal/modelFetchers.js` can build a refresh group key on it without - * importing back into `providers.js` and forming a module cycle. Re-exported - * from `providers.js` (and `server/services/providers.js`) for hosts that group - * providers by daemon. + * importing back into `providers.js` and forming a module cycle. Deliberately + * NOT re-exported from `providers.js`: `ollamaRefreshGroupKey` is the contract + * hosts group on, and exporting the normalizer alongside it only invites a + * caller to re-derive the grouping rule and drift from the real dispatch. */ export function ollamaBaseFromProvider(provider) { const base = String(provider?.envVars?.ANTHROPIC_BASE_URL || provider?.endpoint || 'http://localhost:11434'); diff --git a/server/services/localLlm.js b/server/services/localLlm.js index a3eb5b35df..3b7f5a7ba0 100644 --- a/server/services/localLlm.js +++ b/server/services/localLlm.js @@ -861,15 +861,18 @@ async function listVisionCliModels() { // ---- install / delete -------------------------------------------------------- +// Probe-failure sentinel, distinct from `fetchProviderModels`' own `null` ("no +// such provider") and from a legitimately empty `[]` catalog. Three outcomes, +// three values — collapsing any two of them loses a real distinction. +const PROBE_FAILED = Symbol('ollama-probe-failed') + /** * Fetch one Ollama probe for a group of providers that share a daemon + probe * shape, then persist that single answer onto each of them. * * The probe is issued through the group's first member; every member (that one * included) is then written via `updateProvider`, so no provider is special- - * cased. Writes are sequential because `saveProviders` is a whole-file - * read-modify-write — firing them concurrently would have members clobber each - * other's models. + * cased. * * A failed probe skips the whole group with ONE log line: every member would * have failed identically against the same unreachable daemon, so N copies of @@ -879,11 +882,18 @@ async function refreshOllamaProviderGroup(group) { const [lead] = group const models = await fetchProviderModels(lead.id).catch((err) => { console.error(`⚠️ Failed to refresh models for ${group.length} Ollama-backed provider(s) via ${lead.id}: ${err.message}`) - return null + return PROBE_FAILED }) - // `null` = probe failed or the provider vanished; `[]` = a real, empty catalog - // (the user just deleted their last model) and MUST still be persisted. - if (models === null) return + if (models === PROBE_FAILED) return + if (models === null) { + // The lead was deleted between listing the providers and probing it. Say so + // — silently dropping the whole group would leave its siblings stale with no + // trace of why. + console.error(`⚠️ Skipped refreshing ${group.length} Ollama-backed provider(s): lead provider ${lead.id} no longer exists`) + return + } + // `[]` is a real, empty catalog (the user just deleted their last model) and + // MUST still be persisted — only the two cases above are skips. for (const p of group) { await updateProvider(p.id, { models }).catch((err) => { console.error(`⚠️ Failed to save refreshed models for provider ${p.id}: ${err.message}`) @@ -925,17 +935,22 @@ function refreshOllamaBackedProviders() { // Only a group with something to SHARE takes the split fetch/apply path; a // lone member keeps the plain one-call refresh, so the common single-daemon // single-provider install behaves exactly as it did before. - const shared = [] + const work = [] for (const group of groups.values()) { if (group.length === 1) singles.push(group[0]) - else shared.push(group) + else work.push(() => refreshOllamaProviderGroup(group)) } - return Promise.all([ - ...singles.map((p) => refreshProviderModels(p.id).catch((err) => { + for (const p of singles) { + work.push(() => refreshProviderModels(p.id).catch((err) => { console.error(`⚠️ Failed to refresh models for provider ${p.id} after Ollama model change: ${err.message}`) - })), - ...shared.map(refreshOllamaProviderGroup), - ]) + })) + } + // Sequential, not `Promise.all`: every one of these ends in `saveProviders`, + // a whole-file read-modify-write of providers.json. Run concurrently they + // interleave and clobber each other's model arrays. This whole chain is + // fire-and-forget background work behind an install/delete, so nothing is + // waiting on the wall-clock saving. + return work.reduce((tail, run) => tail.then(run), Promise.resolve()) }).catch((err) => { console.error(`⚠️ Failed to list providers for post-install Ollama refresh: ${err.message}`) }) diff --git a/server/services/localLlm.test.js b/server/services/localLlm.test.js index 3d31bf525a..8c6ebfc181 100644 --- a/server/services/localLlm.test.js +++ b/server/services/localLlm.test.js @@ -318,6 +318,28 @@ describe('localLlm', () => { errSpy.mockRestore(); }); + it('logs rather than silently dropping a group whose lead provider vanished', async () => { + // `fetchProviderModels` answers null (not a throw) for a provider deleted + // between the listing and the probe — a third outcome that must not be + // confused with a failed probe or an empty catalog. + mocks.providers.getAllProviders.mockResolvedValueOnce({ + providers: [ + { id: 'local-a', type: 'cli', ollamaBacked: true }, + { id: 'local-b', type: 'tui', ollamaBacked: true } + ] + }); + mocks.providers.fetchProviderModels.mockResolvedValueOnce(null); + const errSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + + await svc.installModel('ollama', 'llama3.2'); + await flushMicrotasks(); + + expect(mocks.providers.updateProvider).not.toHaveBeenCalled(); + expect(errSpy).toHaveBeenCalledTimes(1); + expect(String(errSpy.mock.calls[0][0])).toMatch(/local-a no longer exists/); + errSpy.mockRestore(); + }); + it('persists a legitimately EMPTY catalog across the group', async () => { // `[]` (the user just deleted their last model) is a real answer and must // be written; only `null` — probe failed — is the skip signal.