Conversation
Greptile SummaryThis PR adds a module-level
Confidence Score: 4/5Safe to merge after addressing the failed-fetch caching issue; the P1 defect can lead to permanently missing model metadata for a session. The cache hit/miss logic and eviction are structurally sound, and the tests are well-structured. The one P1 finding — that transient fetch failures are cached as permanent empty results for the lifetime of the process (until the model digest changes) — is a real defect that could silently degrade the model-picker UX. extensions/ollama/src/provider-models.ts — specifically Prompt To Fix All With AIThis is a comment left during a code review.
Path: extensions/ollama/src/provider-models.ts
Line: 78-88
Comment:
**Unreachable `has(key)` branch**
`queryOllamaModelShowInfoCached` only calls `setOllamaModelShowCacheEntry` after confirming the key is absent (`if (cached) { return … }` covers the present-key path). The `ollamaModelShowInfoCache.has(key)` branch in this setter can therefore never be `true` in practice — it is dead code that will silently mislead future readers who assume it handles a "refresh LRU position" case.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: extensions/ollama/src/provider-models.ts
Line: 145-162
Comment:
**Failed fetches permanently cached as empty results**
`queryOllamaModelShowInfo` always resolves — it catches every error and non-OK response and returns `{}`. That settled-to-empty promise is stored in the cache before `await pending` is called. On any subsequent call for the same `name+digest` the cached empty promise is served immediately, so a model whose enrichment failed (Ollama timeout, restart, 500) will never be retried until its digest changes. Users end up with permanently missing context-window and capability metadata for that session.
To fix this, remove the cache entry when the resolved value is empty (e.g., in a `.then()` on `pending` that calls `ollamaModelShowInfoCache.delete(cacheKey)` when both `contextWindow` and `capabilities` are absent), so the next discovery pass can attempt a fresh fetch.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "Ollama: cache model show metadata" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| function setOllamaModelShowCacheEntry(key: string, value: Promise<OllamaModelShowInfo>): void { | ||
| if (ollamaModelShowInfoCache.has(key)) { | ||
| ollamaModelShowInfoCache.delete(key); | ||
| } else if (ollamaModelShowInfoCache.size >= MAX_OLLAMA_SHOW_CACHE_ENTRIES) { | ||
| const oldestKey = ollamaModelShowInfoCache.keys().next().value; | ||
| if (typeof oldestKey === "string") { | ||
| ollamaModelShowInfoCache.delete(oldestKey); | ||
| } | ||
| } | ||
| ollamaModelShowInfoCache.set(key, value); | ||
| } |
There was a problem hiding this comment.
has(key) branch
queryOllamaModelShowInfoCached only calls setOllamaModelShowCacheEntry after confirming the key is absent (if (cached) { return … } covers the present-key path). The ollamaModelShowInfoCache.has(key) branch in this setter can therefore never be true in practice — it is dead code that will silently mislead future readers who assume it handles a "refresh LRU position" case.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/ollama/src/provider-models.ts
Line: 78-88
Comment:
**Unreachable `has(key)` branch**
`queryOllamaModelShowInfoCached` only calls `setOllamaModelShowCacheEntry` after confirming the key is absent (`if (cached) { return … }` covers the present-key path). The `ollamaModelShowInfoCache.has(key)` branch in this setter can therefore never be `true` in practice — it is dead code that will silently mislead future readers who assume it handles a "refresh LRU position" case.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Cleaned up in b2380c0c99 while fixing the empty-result caching path. The setter now only handles bounded insertion/eviction, without the dead has(key) branch.
| async function queryOllamaModelShowInfoCached( | ||
| apiBase: string, | ||
| model: Pick<OllamaTagModel, "name" | "digest" | "modified_at">, | ||
| ): Promise<OllamaModelShowInfo> { | ||
| const cacheKey = buildOllamaModelShowCacheKey(apiBase, model); | ||
| if (!cacheKey) { | ||
| return await queryOllamaModelShowInfo(apiBase, model.name); | ||
| } | ||
|
|
||
| const cached = ollamaModelShowInfoCache.get(cacheKey); | ||
| if (cached) { | ||
| return await cached; | ||
| } | ||
|
|
||
| const pending = queryOllamaModelShowInfo(apiBase, model.name); | ||
| setOllamaModelShowCacheEntry(cacheKey, pending); | ||
| return await pending; | ||
| } |
There was a problem hiding this comment.
queryOllamaModelShowInfo always resolves — it catches every error and non-OK response and returns {}. That settled-to-empty promise is stored in the cache before await pending is called. On any subsequent call for the same name+digest the cached empty promise is served immediately, so a model whose enrichment failed (Ollama timeout, restart, 500) will never be retried until its digest changes. Users end up with permanently missing context-window and capability metadata for that session.
To fix this, remove the cache entry when the resolved value is empty (e.g., in a .then() on pending that calls ollamaModelShowInfoCache.delete(cacheKey) when both contextWindow and capabilities are absent), so the next discovery pass can attempt a fresh fetch.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/ollama/src/provider-models.ts
Line: 145-162
Comment:
**Failed fetches permanently cached as empty results**
`queryOllamaModelShowInfo` always resolves — it catches every error and non-OK response and returns `{}`. That settled-to-empty promise is stored in the cache before `await pending` is called. On any subsequent call for the same `name+digest` the cached empty promise is served immediately, so a model whose enrichment failed (Ollama timeout, restart, 500) will never be retried until its digest changes. Users end up with permanently missing context-window and capability metadata for that session.
To fix this, remove the cache entry when the resolved value is empty (e.g., in a `.then()` on `pending` that calls `ollamaModelShowInfoCache.delete(cacheKey)` when both `contextWindow` and `capabilities` are absent), so the next discovery pass can attempt a fresh fetch.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Addressed in b2380c0c99. The cached lookup now evicts empty {} results so transient /api/show failures are retried on the next discovery pass instead of sticking for the session.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 23c10dfd05
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const pending = queryOllamaModelShowInfo(apiBase, model.name); | ||
| setOllamaModelShowCacheEntry(cacheKey, pending); |
There was a problem hiding this comment.
This stores the in-flight /api/show result in cache before knowing whether it succeeded, so transient failures (timeouts/non-OK responses that currently resolve to {} in queryOllamaModelShowInfo) get memoized under a stable digest/modified timestamp key. In that case, later discovery/model-picker runs for the same model never retry and keep missing contextWindow/capabilities (for example, losing vision) until process restart or digest change. Cache insertion should be conditional on a successful metadata fetch, or failed/empty results should evict the key.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Adds an in-memory cache for Ollama /api/show enrichment so repeated model discovery/model-picker flows can reuse context-window/capability metadata when a model’s digest/modified timestamp is stable.
Changes:
- Introduce a bounded (256 entry) cache for
/api/showresults keyed by base URL + model + digest/modified timestamp. - Route
enrichOllamaModelsWithContext()through the cached lookup path. - Add tests asserting cache reuse on stable digest and invalidation when digest changes.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| extensions/ollama/src/provider-models.ts | Adds /api/show cache + cached query helper and wires it into enrichment. |
| extensions/ollama/src/provider-models.test.ts | Adds tests covering cache reuse and invalidation; clears cache between tests. |
| const pending = queryOllamaModelShowInfo(apiBase, model.name); | ||
| setOllamaModelShowCacheEntry(cacheKey, pending); | ||
| return await pending; |
There was a problem hiding this comment.
The cache key is built from a normalized base URL (via resolveOllamaApiBase), but the actual request is still made with the original apiBase argument. If callers ever pass a base URL with /v1 (or trailing slashes) in one call and a normalized base in another, this can cause cache poisoning (e.g., caching {} from a 404 at /v1/api/show) and then reusing it for the correct base. Normalize apiBase once in this function (or inside queryOllamaModelShowInfo) and use the same normalized value both for the cache key and the fetch URL.
There was a problem hiding this comment.
Addressed in b2380c0c99. queryOllamaModelShowInfo() now normalizes the base URL up front and uses that same normalized value for both the fetch path and the SSRF policy, so the cache key and request target stay aligned.
ImLukeF
force-pushed
the
imlukef/local-ollama-show-cache
branch
from
fe9b15a to
de56dfb
Compare
|
Merged via squash.
|
🔒 Aisle Security AnalysisWe found 2 potential security issue(s) in this PR:
1. 🟡 Cache key collision allows /api/show metadata cache poisoning in Ollama provider
DescriptionA module-level cache (
Vulnerable code: return `${resolveOllamaApiBase(apiBase)}|${model.name}|${version}`;RecommendationAvoid building cache keys via ambiguous string concatenation of untrusted components. Use a structured key that cannot collide, such as nested Maps or a stable encoding that escapes separators. Example (nested maps): const cache = new Map<string, Map<string, Map<string, Promise<OllamaModelShowInfo>>>>();
function getEntry(apiBase: string, modelName: string, version: string) {
let byBase = cache.get(apiBase);
if (!byBase) cache.set(apiBase, (byBase = new Map()));
let byName = byBase.get(modelName);
if (!byName) byBase.set(modelName, (byName = new Map()));
return { byName, key: version };
}Additionally, validate/normalize 2. 🔵 Unbounded cache key size from remote Ollama model metadata can cause memory pressure (DoS)
DescriptionThe
Vulnerable code: const version = model.digest?.trim() || model.modified_at?.trim();
return `${resolveOllamaApiBase(apiBase)}|${model.name}|${version}`;RecommendationMitigate memory pressure by ensuring cache keys are bounded in size and derived from validated inputs. Options:
import { createHash } from "node:crypto";
function buildOllamaModelShowCacheKey(apiBase: string, model: Pick<OllamaTagModel, "name"|"digest"|"modified_at">) {
const version = model.digest?.trim() || model.modified_at?.trim();
if (!version) return undefined;
const raw = `${resolveOllamaApiBase(apiBase)}|${model.name}|${version}`;
return createHash("sha256").update(raw).digest("hex");
}
if (model.name.length > 256 || version.length > 256) return undefined;Additionally, consider validating/sanitizing fields from Analyzed PR: #64753 at commit Last updated on: 2026-04-11T12:37:35Z |
Summary
/api/showenrichment results for discovery/model-picker flows when a model digest or modified timestamp is stableTesting
pnpm test extensions/ollama/src/provider-models.test.ts extensions/ollama/src/provider-models.ssrf.test.ts