diff --git a/.changelog/next/changed-issue-4155.md b/.changelog/next/changed-issue-4155.md new file mode 100644 index 0000000000..a859c42227 --- /dev/null +++ b/.changelog/next/changed-issue-4155.md @@ -0,0 +1 @@ +- Provider model refreshes now batch into a single providers.json write — an Ollama install/delete no longer rewrites the whole provider file once per matching provider diff --git a/server/lib/aiToolkit/CLAUDE.md b/server/lib/aiToolkit/CLAUDE.md index 199601a3c3..eb3efaa517 100644 --- a/server/lib/aiToolkit/CLAUDE.md +++ b/server/lib/aiToolkit/CLAUDE.md @@ -9,6 +9,7 @@ The AI provider/runner/prompt toolkit is vendored in-tree here. (It was previous - PortOS extends toolkit routes in `server/routes/providers.js` for vision testing and provider status (status routes live in PortOS, not the toolkit, because they call PortOS-side socket helpers) - When adding new provider fields (e.g., `fallbackProvider`, `lightModel`), update `createProvider()` in `providers.js` - `updateProvider()` uses spread so existing providers preserve custom fields, but `createProvider()` has an explicit field list +- **Refreshing more than one provider goes through `refreshProviderModelsBatch(ids)`, never a loop over `refreshProviderModels(id)`.** Every single-provider refresh ends in `saveProviders`, which invalidates the read cache and rewrites all of `providers.json`; an N-provider fan-out that way is N full-file writes, each superseded by the next. The batch form groups by `ollamaRefreshGroupKey` (one probe per daemon + probe shape), probes each group's lead without persisting, then applies every result and saves **exactly once**. It never throws for a per-provider failure — each group carries its own `status` (`updated` / `failed` / `missing`) so the host logs one line per group rather than one per member. PortOS's post-install fan-out (`refreshOllamaBackedProviders` in `server/services/localLlm.js`) is the worked example. **Runner extension points.** The runner exposes a small declared override surface (in `runner.js`) so the host (PortOS) supplies its own runners without reaching into private internals: diff --git a/server/lib/aiToolkit/providers.batch.test.js b/server/lib/aiToolkit/providers.batch.test.js new file mode 100644 index 0000000000..a816ec583e --- /dev/null +++ b/server/lib/aiToolkit/providers.batch.test.js @@ -0,0 +1,262 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { mkdtemp, rm } from 'fs/promises'; +import { tmpdir } from 'os'; +import { join } from 'path'; +import { atomicWrite } from './internal/atomicWrite.js'; +import { createProviderService } from './providers.js'; + +// The whole point of `refreshProviderModelsBatch` is HOW MANY TIMES the file is +// written, and `saveProviders` is private. `atomicWrite` is the one observable +// it funnels through, so wrap it in a counting spy that still does the real +// write — a stub would make every "and the value landed" assertion vacuous. +vi.mock('./internal/atomicWrite.js', async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, atomicWrite: vi.fn(actual.atomicWrite) }; +}); + +// Temp dir, NOT a cwd-rooted one — see providerStatus.test.js (#3823). +let TEST_DATA_DIR; + +/** + * Fake Ollama daemon. Counts `/api/tags` hits PER base URL so a test can assert + * "one probe per daemon", and answers `/api/show` with tool capability so every + * listed model survives the tool-use filter. + */ +const stubOllama = (modelsByBase) => { + const tagHits = new Map(); + vi.stubGlobal('fetch', vi.fn(async (url) => { + const href = String(url); + if (href.endsWith('/api/tags')) { + const base = href.slice(0, -'/api/tags'.length); + tagHits.set(base, (tagHits.get(base) || 0) + 1); + const names = modelsByBase[base]; + if (!names) return { ok: false, status: 404 }; + return { ok: true, json: async () => ({ models: names.map((n) => ({ name: n })) }) }; + } + if (href.endsWith('/api/show')) { + return { ok: true, json: async () => ({ capabilities: ['completion', 'tools'] }) }; + } + return { ok: false, status: 404 }; + })); + return tagHits; +}; + +const LOCAL = 'http://localhost:11434'; +const REMOTE = 'http://192.0.2.10:11434'; + +const ollamaProvider = (name, { type = 'cli', base = LOCAL, models = ['stale-model'] } = {}) => ({ + name, + type, + command: 'claude', + ollamaBacked: true, + models, + envVars: { ANTHROPIC_BASE_URL: base }, +}); + +describe('refreshProviderModelsBatch — one providers.json write per fan-out', () => { + let providerService; + + beforeEach(async () => { + TEST_DATA_DIR = await mkdtemp(join(tmpdir(), 'portos-providers-batch-')); + providerService = createProviderService({ + dataDir: TEST_DATA_DIR, + providersFile: 'providers.json', + }); + }); + + afterEach(async () => { + vi.unstubAllGlobals(); + if (TEST_DATA_DIR) await rm(TEST_DATA_DIR, { recursive: true, force: true }); + }); + + it('saves ONCE for a multi-provider, multi-group batch', async () => { + const tagHits = stubOllama({ [LOCAL]: ['qwen2.5:7b'], [REMOTE]: ['gemma2:9b'] }); + const local1 = await providerService.createProvider(ollamaProvider('Local One')); + const local2 = await providerService.createProvider(ollamaProvider('Local Two', { type: 'tui' })); + const remote = await providerService.createProvider(ollamaProvider('Remote', { base: REMOTE })); + + // Ignore the three creates — only the batch's own writes are under test. + atomicWrite.mockClear(); + + const groups = await providerService.refreshProviderModelsBatch([local1.id, local2.id, remote.id]); + + // Three providers, two daemons — one write total, not one per provider and + // not one per group. + expect(atomicWrite).toHaveBeenCalledTimes(1); + // …and one probe per daemon, not per provider. + expect(tagHits.get(LOCAL)).toBe(1); + expect(tagHits.get(REMOTE)).toBe(1); + + expect(groups.map((g) => g.status)).toEqual(['updated', 'updated']); + expect(groups.map((g) => g.ids)).toEqual([[local1.id, local2.id], [remote.id]]); + + // Every member got the answer, and it survived the single write to disk. + const { providers } = await providerService.getAllProviders(); + const byId = Object.fromEntries(providers.map((p) => [p.id, p])); + expect(byId[local1.id].models).toEqual(['qwen2.5:7b']); + expect(byId[local2.id].models).toEqual(['qwen2.5:7b']); + expect(byId[remote.id].models).toEqual(['gemma2:9b']); + // Unrelated fields survive the batch apply. + expect(byId[local1.id].ollamaBacked).toBe(true); + expect(byId[local2.id].type).toBe('tui'); + }); + + it('persists a legitimately EMPTY catalog rather than skipping it', async () => { + // `[]` is a real answer (the user just deleted their last model) — only a + // failed or missing probe is a skip. + stubOllama({ [LOCAL]: [] }); + const a = await providerService.createProvider(ollamaProvider('Local One')); + const b = await providerService.createProvider(ollamaProvider('Local Two', { type: 'tui' })); + atomicWrite.mockClear(); + + const groups = await providerService.refreshProviderModelsBatch([a.id, b.id]); + + expect(groups).toHaveLength(1); + expect(groups[0].status).toBe('updated'); + expect(atomicWrite).toHaveBeenCalledTimes(1); + expect((await providerService.getProviderById(a.id)).models).toEqual([]); + expect((await providerService.getProviderById(b.id)).models).toEqual([]); + }); + + it('writes NOTHING when the shared probe fails, and reports the group once', async () => { + stubOllama({}); // every /api/tags 404s + const a = await providerService.createProvider(ollamaProvider('Local One')); + const b = await providerService.createProvider(ollamaProvider('Local Two', { type: 'tui' })); + atomicWrite.mockClear(); + const errSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + + const groups = await providerService.refreshProviderModelsBatch([a.id, b.id]); + errSpy.mockRestore(); + + expect(groups).toHaveLength(1); + expect(groups[0].status).toBe('failed'); + expect(groups[0].ids).toEqual([a.id, b.id]); + expect(groups[0].error).toBeInstanceOf(Error); + // No half-write: the stored lists are untouched. + expect(atomicWrite).not.toHaveBeenCalled(); + expect((await providerService.getProviderById(a.id)).models).toEqual(['stale-model']); + expect((await providerService.getProviderById(b.id)).models).toEqual(['stale-model']); + }); + + it('one failing group does not cost the healthy groups their update', async () => { + // The remote daemon is unreachable; the local one answers. A single write + // still lands, carrying only the group that succeeded. + const tagHits = stubOllama({ [LOCAL]: ['qwen2.5:7b'] }); + const local = await providerService.createProvider(ollamaProvider('Local One')); + const remote = await providerService.createProvider(ollamaProvider('Remote', { base: REMOTE })); + atomicWrite.mockClear(); + const errSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + + const groups = await providerService.refreshProviderModelsBatch([local.id, remote.id]); + errSpy.mockRestore(); + + expect(groups.map((g) => g.status)).toEqual(['updated', 'failed']); + expect(atomicWrite).toHaveBeenCalledTimes(1); + expect(tagHits.get(REMOTE)).toBe(1); + expect((await providerService.getProviderById(local.id)).models).toEqual(['qwen2.5:7b']); + expect((await providerService.getProviderById(remote.id)).models).toEqual(['stale-model']); + }); + + it('reports an unknown id as missing without probing or blocking the rest', async () => { + const tagHits = stubOllama({ [LOCAL]: ['qwen2.5:7b'] }); + const real = await providerService.createProvider(ollamaProvider('Local One')); + atomicWrite.mockClear(); + + const groups = await providerService.refreshProviderModelsBatch(['no-such-provider', real.id]); + + expect(groups[0]).toEqual({ ids: ['no-such-provider'], leadId: 'no-such-provider', status: 'missing' }); + expect(groups[1].status).toBe('updated'); + expect(tagHits.get(LOCAL)).toBe(1); + expect(atomicWrite).toHaveBeenCalledTimes(1); + expect((await providerService.getProviderById(real.id)).models).toEqual(['qwen2.5:7b']); + }); + + it('keeps a provider that has no shared probe in its own group', async () => { + // An `api`-type Ollama provider persists the UNFILTERED tag list, so it must + // never share a bucket with the tool-filtered CLI/TUI probe of the SAME + // daemon — but it still rides the same single write. + stubOllama({ [LOCAL]: ['qwen2.5:7b', 'embed-only:1b'] }); + const cli = await providerService.createProvider(ollamaProvider('Local CLI')); + const api = await providerService.createProvider({ + name: 'Ollama API', type: 'api', endpoint: LOCAL, models: ['stale-model'], + }); + atomicWrite.mockClear(); + + const groups = await providerService.refreshProviderModelsBatch([cli.id, api.id]); + + expect(groups.map((g) => g.ids)).toEqual([[cli.id], [api.id]]); + expect(atomicWrite).toHaveBeenCalledTimes(1); + // The api arm keeps every tag; the tools arm filters — proof they did not + // share one probe's answer. + expect((await providerService.getProviderById(api.id)).models).toEqual(['qwen2.5:7b', 'embed-only:1b']); + expect((await providerService.getProviderById(cli.id)).models).toEqual(['qwen2.5:7b', 'embed-only:1b']); + }); + + it('lands the same stored state as refreshing each provider one by one', async () => { + // The parity that makes the batch a drop-in for the loop it replaces: same + // models on every provider, at a fraction of the writes. + stubOllama({ [LOCAL]: ['qwen2.5:7b'], [REMOTE]: ['gemma2:9b'] }); + const a = await providerService.createProvider(ollamaProvider('Local One')); + const b = await providerService.createProvider(ollamaProvider('Local Two', { type: 'tui' })); + const c = await providerService.createProvider(ollamaProvider('Remote', { base: REMOTE })); + + atomicWrite.mockClear(); + for (const id of [a.id, b.id, c.id]) await providerService.refreshProviderModels(id); + const oneByOneWrites = atomicWrite.mock.calls.length; + const oneByOne = (await providerService.getAllProviders()).providers; + + // Reset the stored lists and do it again as a batch. + for (const id of [a.id, b.id, c.id]) await providerService.updateProvider(id, { models: ['stale-model'] }); + atomicWrite.mockClear(); + await providerService.refreshProviderModelsBatch([a.id, b.id, c.id]); + const batched = (await providerService.getAllProviders()).providers; + + expect(batched).toEqual(oneByOne); + expect(oneByOneWrites).toBe(3); + expect(atomicWrite).toHaveBeenCalledTimes(1); + }); + + it('treats a probe that answers with no array as missing, not as an update', async () => { + // `fetchProviderModels` promises an array or a throw, so this is a guard + // against a future fetcher rather than a reachable path today — but the + // failure mode it prevents is the whole batch dying on `[...undefined]`, + // taking the healthy groups' write down with it. + stubOllama({ [LOCAL]: ['qwen2.5:7b'] }); + const broken = await providerService.createProvider(ollamaProvider('Broken', { base: REMOTE })); + const healthy = await providerService.createProvider(ollamaProvider('Local One')); + vi.spyOn(providerService, 'fetchProviderModels').mockImplementation(async (id) => ( + id === broken.id ? undefined : ['qwen2.5:7b'] + )); + atomicWrite.mockClear(); + + const groups = await providerService.refreshProviderModelsBatch([broken.id, healthy.id]); + + expect(groups.map((g) => g.status)).toEqual(['missing', 'updated']); + expect(atomicWrite).toHaveBeenCalledTimes(1); + expect((await providerService.getProviderById(broken.id)).models).toEqual(['stale-model']); + expect((await providerService.getProviderById(healthy.id)).models).toEqual(['qwen2.5:7b']); + }); + + it('is a no-op for an empty id list', async () => { + stubOllama({ [LOCAL]: ['qwen2.5:7b'] }); + await providerService.createProvider(ollamaProvider('Local One')); + atomicWrite.mockClear(); + + expect(await providerService.refreshProviderModelsBatch([])).toEqual([]); + expect(await providerService.refreshProviderModelsBatch(undefined)).toEqual([]); + expect(atomicWrite).not.toHaveBeenCalled(); + }); + + it('probes a duplicated id once and writes it once', async () => { + const tagHits = stubOllama({ [LOCAL]: ['qwen2.5:7b'] }); + const p = await providerService.createProvider(ollamaProvider('Local One')); + atomicWrite.mockClear(); + + const groups = await providerService.refreshProviderModelsBatch([p.id, p.id, p.id]); + + expect(groups).toHaveLength(1); + expect(groups[0].ids).toEqual([p.id]); + expect(tagHits.get(LOCAL)).toBe(1); + expect(atomicWrite).toHaveBeenCalledTimes(1); + }); +}); diff --git a/server/lib/aiToolkit/providers.js b/server/lib/aiToolkit/providers.js index fffb0de222..4e06c99283 100644 --- a/server/lib/aiToolkit/providers.js +++ b/server/lib/aiToolkit/providers.js @@ -828,6 +828,124 @@ export function createProviderService(config = {}) { return this.updateProvider(id, { models }); }, + /** + * Refresh MANY providers with ONE `providers.json` write. + * + * `refreshProviderModels` is a per-provider `loadProviders` → mutate → + * `saveProviders` round-trip, and `saveProviders` invalidates the cache and + * rewrites the whole file. A host fanning a refresh across every provider + * backed by one local daemon (PortOS does this after an Ollama install or + * delete) therefore paid N full-file writes — each superseded by the next — + * plus N cache invalidate/repopulate cycles for any concurrent reader. This + * does the same work as three phases: group, probe, then a single write. + * + * 1. **Group** by {@link ollamaRefreshGroupKey}, so providers sharing a + * daemon AND a probe shape are probed once rather than once each. A + * `null` key is the "not a shared Ollama probe" sentinel, NOT a bucket — + * those providers each become a group of one and keep their own probe. + * 2. **Probe** one lead per group, sequentially. Nothing is persisted here, + * so a probe that fails or answers late cannot leave a half-written file. + * 3. **Apply + save once.** The providers map is re-read after the probes + * (they are network-bound and outlive the read cache's TTL), every + * probed list is applied in one pass, and `saveProviders` runs exactly + * once — or not at all when nothing was probed successfully. + * + * Never throws for a per-provider failure: each group carries its own + * outcome so the host can log group-level context (one line per group, not + * one per member) and decide what a failure means. + * + * - `updated` — probed successfully; `models` was applied to every member + * still present at save time. `[]` is a real answer (the user deleted + * their last model) and IS persisted; only the two statuses below skip. + * - `failed` — the probe threw; `error` carries it. The stored lists are + * left untouched. + * - `missing` — no such provider, or the lead was deleted between the + * grouping read and its probe. + * + * @param {string[]} ids + * @returns {Promise>} + */ + async refreshProviderModelsBatch(ids) { + const requested = [...new Set(Array.isArray(ids) ? ids : [])]; + if (requested.length === 0) return []; + + const data = await loadProviders(); + const groups = []; + const byKey = new Map(); + + for (const id of requested) { + const provider = data.providers[id]; + if (!provider) { + groups.push({ ids: [id], leadId: id, status: 'missing' }); + continue; + } + const key = ollamaRefreshGroupKey(provider); + const existing = key ? byKey.get(key) : null; + if (existing) { + existing.ids.push(id); + continue; + } + // `missing` is the starting status for EVERY group, not just the ones + // whose id is already unknown: the probe below either upgrades it or + // leaves it, which is exactly the answer for a lead that vanished + // between this read and its probe. + const group = { ids: [id], leadId: id, status: 'missing' }; + if (key) byKey.set(key, group); + groups.push(group); + } + + // Sequential, not `Promise.all`: several groups commonly hit the same + // local daemon, and this whole call is background work behind an + // install/delete, so nothing is waiting on the wall clock. + for (const group of groups) { + if (!data.providers[group.leadId]) continue; + const probed = await this.fetchProviderModels(group.leadId).then( + (models) => ({ models }), + (error) => ({ error }) + ); + if (probed.error) { + group.status = 'failed'; + group.error = probed.error; + continue; + } + // `null` here means the lead vanished mid-probe — the same `missing` + // the group already carries, so leave the status alone. Validated as an + // ARRAY rather than compared to `null`: `fetchProviderModels` promises + // an array or a throw, but a future fetcher that leaks `undefined` must + // land on `missing` too, not be persisted as an "updated" catalog (and + // then crash the whole batch on the spread below). `[]` is an array, so + // a legitimately empty catalog still passes. + if (!Array.isArray(probed.models)) continue; + group.status = 'updated'; + group.models = probed.models; + } + + const updated = groups.filter((g) => g.status === 'updated'); + if (updated.length === 0) return groups; + + // Re-read rather than reusing the pre-probe snapshot: the probes above are + // network-bound and outlast the read cache's TTL, so `data` may no longer + // be the freshest view. Writing our stale copy would drop anything saved + // while we were probing. + const fresh = await loadProviders(); + let changed = false; + for (const group of updated) { + for (const id of group.ids) { + const provider = fresh.providers[id]; + // Deleted between the grouping read and now — nothing to write, and + // re-adding it here would resurrect a provider the user removed. + if (!provider) continue; + // Copy the list per provider so members of one group don't end up + // sharing (and later mutating) a single array instance. + fresh.providers[id] = { ...provider, models: [...group.models], id }; + changed = true; + } + } + if (changed) await saveProviders(fresh); + + return groups; + }, + async _refreshAPIProviderModels(provider) { if (provider.endpoint?.includes('ollama') || provider.endpoint?.includes(':11434')) { const ollamaUrl = `${provider.endpoint}/api/tags`; diff --git a/server/services/localLlm.js b/server/services/localLlm.js index 3b7f5a7ba0..d0def66d74 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, fetchProviderModels, isOllamaBackedProvider, ollamaRefreshGroupKey } from './providers.js' +import { getProviderById, getAllProviders, updateProvider, refreshProviderModelsBatch, isOllamaBackedProvider } from './providers.js' const execFileAsync = promisify(execFile) const ENV_PATH = join(PATHS.root, '.env') @@ -861,46 +861,6 @@ 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. - * - * 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 PROBE_FAILED - }) - 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}`) - }) - } -} - /** * 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 @@ -911,46 +871,31 @@ async function refreshOllamaProviderGroup(group) { * 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. + * The grouping, probing and persistence all live in the toolkit's + * `refreshProviderModelsBatch`: it dedupes providers that share a daemon AND a + * probe shape so `/api/tags` + the per-model `/api/show` capability sweep runs + * once rather than once per provider, and it collapses the whole fan-out into a + * SINGLE `providers.json` write instead of one full-file save per provider. + * All this function adds is the host-side log line — one per group, because + * every member of a group failed identically against the same daemon and N + * copies of one error is noise, not information. */ function refreshOllamaBackedProviders() { getAllProviders().then(({ providers }) => { const targets = (providers || []).filter(isOllamaBackedProvider) - 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 work = [] - for (const group of groups.values()) { - if (group.length === 1) singles.push(group[0]) - else work.push(() => refreshOllamaProviderGroup(group)) - } - 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}`) - })) - } - // 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()) + if (targets.length === 0) return null + return refreshProviderModelsBatch(targets.map((p) => p.id)).then((groups) => { + for (const group of groups) { + if (group.status === 'failed') { + console.error(`⚠️ Failed to refresh models for ${group.ids.length} Ollama-backed provider(s) via ${group.leadId}: ${group.error?.message}`) + } else if (group.status === 'missing') { + // The lead was deleted between listing the providers and probing it. + // Say so — silently dropping the group would leave its siblings stale + // with no trace of why. + console.error(`⚠️ Skipped refreshing ${group.ids.length} Ollama-backed provider(s): lead provider ${group.leadId} no longer exists`) + } + } + }) }).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 8c6ebfc181..835f402e02 100644 --- a/server/services/localLlm.test.js +++ b/server/services/localLlm.test.js @@ -48,22 +48,24 @@ const mocks = vi.hoisted(() => ({ getProviderById: vi.fn(async () => ({ id: 'ollama', enabled: false })), getAllProviders: vi.fn(async () => ({ providers: [] })), updateProvider: vi.fn(async () => ({})), - refreshProviderModels: vi.fn(async (id) => ({ id, models: [] })), - fetchProviderModels: vi.fn(async () => ['qwen2.5:7b']) + // Default: every requested provider lands in one happy group. Tests that + // care about a skip queue their own group shapes. + refreshProviderModelsBatch: vi.fn(async (ids) => [ + { ids: [...ids], leadId: ids[0], status: 'updated', models: ['qwen2.5:7b'] } + ]) } })); vi.mock('./ollamaManager.js', () => mocks.ollama); vi.mock('./lmStudioManager.js', () => mocks.lmstudio); -// 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. +// The classification predicate comes from the REAL toolkit module rather than +// being re-implemented here: it decides which providers get refreshed at all, 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 + isOllamaBackedProvider: real.isOllamaBackedProvider }; }); @@ -233,139 +235,104 @@ describe('localLlm', () => { // The refresh is fire-and-forget (doesn't block the install response) — // flush the microtask queue so the async chain it kicks off has settled. await flushMicrotasks(); - expect(mocks.providers.refreshProviderModels).toHaveBeenCalledWith('ollama'); - expect(mocks.providers.refreshProviderModels).toHaveBeenCalledWith('claude-ollama'); - expect(mocks.providers.refreshProviderModels).not.toHaveBeenCalledWith('anthropic'); + // ONE batch call carrying every ollama-backed id — not a per-provider + // loop, which is what cost a full providers.json write per provider. The + // grouping/probing/writing inside it is the toolkit's contract and is + // covered by lib/aiToolkit/providers.batch.test.js. + expect(mocks.providers.refreshProviderModelsBatch).toHaveBeenCalledTimes(1); + expect(mocks.providers.refreshProviderModelsBatch).toHaveBeenCalledWith(['ollama', 'claude-ollama']); }); it('refreshes Ollama-backed providers after a successful delete', async () => { mocks.providers.getAllProviders.mockResolvedValueOnce({ providers: [{ id: 'ollama' }] }); await svc.deleteModel('ollama', 'llama3.2'); await flushMicrotasks(); - expect(mocks.providers.refreshProviderModels).toHaveBeenCalledWith('ollama'); + expect(mocks.providers.refreshProviderModelsBatch).toHaveBeenCalledWith(['ollama']); }); it('does not refresh providers when the Ollama pull fails', async () => { mocks.ollama.pullModel.mockResolvedValueOnce({ success: false, error: 'boom' }); mocks.providers.getAllProviders.mockResolvedValueOnce({ providers: [{ id: 'ollama' }] }); await svc.installModel('ollama', 'llama3.2'); await flushMicrotasks(); - expect(mocks.providers.refreshProviderModels).not.toHaveBeenCalled(); + expect(mocks.providers.refreshProviderModelsBatch).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. + it('does not call the batch at all when nothing is Ollama-backed', async () => { 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' } } - ] + providers: [{ id: 'anthropic', type: 'api', endpoint: 'https://api.anthropic.com' }] }); 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']); + expect(mocks.providers.refreshProviderModelsBatch).not.toHaveBeenCalled(); }); - it('skips a whole group with one log line when its shared probe fails', async () => { + it('logs ONE line per failed group, not one per member', 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')); + mocks.providers.refreshProviderModelsBatch.mockResolvedValueOnce([ + { ids: ['local-a', 'local-b'], leadId: 'local-a', status: 'failed', error: 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); + expect(String(errSpy.mock.calls[0][0])).toMatch(/2 Ollama-backed provider\(s\) via local-a: unreachable/); 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. + // `missing` is a third outcome that must not be confused with a failed + // probe or an empty catalog — silently dropping it would leave the + // group's siblings stale with no trace of why. mocks.providers.getAllProviders.mockResolvedValueOnce({ providers: [ { id: 'local-a', type: 'cli', ollamaBacked: true }, { id: 'local-b', type: 'tui', ollamaBacked: true } ] }); - mocks.providers.fetchProviderModels.mockResolvedValueOnce(null); + mocks.providers.refreshProviderModelsBatch.mockResolvedValueOnce([ + { ids: ['local-a', 'local-b'], leadId: 'local-a', status: 'missing' } + ]); 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. + it('says nothing about the groups that succeeded', async () => { mocks.providers.getAllProviders.mockResolvedValueOnce({ - providers: [ - { id: 'local-a', type: 'cli', ollamaBacked: true }, - { id: 'local-b', type: 'tui', ollamaBacked: true } - ] + providers: [{ id: 'local-a', type: 'cli', ollamaBacked: true }] }); - mocks.providers.fetchProviderModels.mockResolvedValueOnce([]); + mocks.providers.refreshProviderModelsBatch.mockResolvedValueOnce([ + // `[]` is a real, empty catalog — an update, not a skip, so no log line. + { ids: ['local-a'], leadId: 'local-a', status: 'updated', models: [] } + ]); + const errSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); await svc.deleteModel('ollama', 'llama3.2'); await flushMicrotasks(); - expect(mocks.providers.updateProvider.mock.calls).toEqual([ - ['local-a', { models: [] }], - ['local-b', { models: [] }] - ]); + expect(errSpy).not.toHaveBeenCalled(); + errSpy.mockRestore(); }); - it('does not fail install when a provider refresh throws', async () => { + it('does not fail install when the batch refresh throws', async () => { mocks.providers.getAllProviders.mockResolvedValueOnce({ providers: [{ id: 'ollama' }] }); - mocks.providers.refreshProviderModels.mockRejectedValueOnce(new Error('unreachable')); + mocks.providers.refreshProviderModelsBatch.mockRejectedValueOnce(new Error('unreachable')); + const errSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); const result = await svc.installModel('ollama', 'llama3.2'); expect(result.success).toBe(true); await flushMicrotasks(); // let the rejection be caught internally, not surface as unhandled + errSpy.mockRestore(); }); }); diff --git a/server/services/providers.js b/server/services/providers.js index 60fb30469b..9a7288608e 100644 --- a/server/services/providers.js +++ b/server/services/providers.js @@ -17,7 +17,9 @@ export const setAIToolkit = setAIToolkitInstance; // routes decorate their payloads with; it is derived on read and never stored. // `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). +// once per provider. `refreshProviderModelsBatch` below already applies it — +// this export is for a caller that needs to reason about the grouping without +// running a refresh. export { isOllamaBackedProvider, canRefreshModels, @@ -68,3 +70,13 @@ export async function refreshProviderModels(id) { export async function fetchProviderModels(id) { return requireToolkit().services.providers.fetchProviderModels(id); } + +/** + * Refresh a whole set of providers with ONE providers.json write: the toolkit + * groups them by shared Ollama daemon + probe shape, probes one lead per group, + * then applies every result in a single save. Returns one result per group so + * the caller logs group-level context instead of one line per member. + */ +export async function refreshProviderModelsBatch(ids) { + return requireToolkit().services.providers.refreshProviderModelsBatch(ids); +}