Dedup Ollama provider model-refresh fan-out by daemon base URL - #4263
Merged
Conversation
…4154) 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.
…the vanished-lead skip (#4154) 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`.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
refreshOllamaBackedProviders(post-install / post-delete on the Local LLMs tab) fanned out with no dedup: onerefreshProviderModels(id)per Ollama-backed provider. Several providers commonly resolve to the same daemon — the built-inollamaprovider plus the four shipped CLI/TUI providers (claude-ollama,claude-ollama-tui,opencode-ollama,opencode-ollama-tui), all defaulting to the same local endpoint — so each independently re-fetched/api/tagsand re-ran the entire per-model/api/showtool-capability probe for an answer that cannot differ.Providers are now bucketed by a new
ollamaRefreshGroupKey(provider)before the fan-out:fetchProviderModels(id)is the probe half ofrefreshProviderModels(id)with the write removed;refreshProviderModelsis now a thin composition of it plusupdateProvider(id, { models }), so the two halves cannot drift. No new persist helper was needed — the existingupdateProviderspread already has exactly the right semantics.updateProvider. Writes are sequential becausesaveProvidersis a whole-file read-modify-write.api-type provider short-circuits to${endpoint}/api/tagsand persists the unfiltered tag list; acli/tuiprovider routes to_fetchOllamaToolCapableModelsand persists the tool-use-capable subset. Collapsing them would persist a tool-filtered list onto the plainollamaprovider — or an unfiltered one onto a Claude harness that then silently fails to edit files.apiprovider carrying anapiKeyis deliberately ungroupable: when the/api/tagsshort-circuit misses,_refreshAPIProviderModelsfalls through to a generic/modelsfetch that sends the key, so two providers on one endpoint with different keys can legitimately see different catalogs.[](a legitimately empty catalog after the user deleted their last model) is still persisted; onlynullis the skip signal.The group key lives in
internal/modelFetchers.jsand reusesresolveModelFetcher— the same resolver the real dispatch uses — so the grouping rule can't drift from the probe it is predicting.ollamaBaseFromProvidermoved tointernal/ollamaBacked.js(besideisOllamaBackedProvider) to avoid a module cycle, and stays internal: the group key is the public contract, not the normalizer.The toolkit stays self-contained — no imports out to other PortOS modules.
Scope note: this is the dedup half only. The sibling issue about batching provider-refresh writes (one
providers.jsonsave per provider) shares the same compute/persist split as a prerequisite but is intentionally left alone here so the two changes don't collide.Test plan
server/lib/aiToolkit/internal/modelFetchers.test.js— newollamaRefreshGroupKeyblock: all four shipped CLI/TUI Ollama providers collapse onto one key (driven offdata.reference/providers.json, so a future seed addition on the same daemon is caught); the api-typeollamaprovider stays out of the tool-filtered bucket; different daemons stay apart; trailing-slash and/v1spellings normalize together; non-Ollama and keyed-api shapes returnnullrather than a shared bucket.server/lib/aiToolkit/providers.test.js— newfetchProviderModelsblock: returns the probed list without persisting; agrees with whatrefreshProviderModelspersists; applying a fetched list to a sibling provider matches refreshing that sibling directly (and preserves its other fields); returnsnullonly for a missing provider; throws502on a failed probe and leaves the stored list untouched.server/services/localLlm.test.js— new fan-out cases: four providers sharing a daemon produce exactly onefetchProviderModelscall and fourupdateProviderwrites; providers on different daemons / different probe shapes stay on the individual path; a failed shared probe writes nothing and logs once; an empty catalog is still persisted across the group. The suite's provider mocks now use the realisOllamaBackedProvider/ollamaRefreshGroupKeyinstead of hand-mirrored copies, so the grouping assertions aren't vacuous.vi.clearAllMocks()clears recorded calls but not queued…Oncevalues, and the fire-and-forget fan-out meant a test where it correctly never ran left itsgetAllProvidersanswer queued for the next test to consume.cd server && NODE_ENV=test npm test→ 1395 files passed, 29243 tests passed, 26 files / 252 tests skipped (DB-backed suites, correctly gated off the non-test database).Closes #4154