fix: LiteLLM cache key collision and silent fallback to non-existent default model#647
fix: LiteLLM cache key collision and silent fallback to non-existent default model#647awschmeder wants to merge 9 commits into
Conversation
awschmeder
requested review from
JamesRobert20,
edelauna,
hannesrudolph,
navedmerchant and
taltas
as code owners
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughFixes two LiteLLM provider bugs: (1) cache key collisions across URL-scoped and key-scoped providers are resolved by incorporating ChangesLiteLLM Cache Collision and Model Desync Fix
Sequence DiagramsequenceDiagram
participant ApiOptions as ApiOptions.tsx
participant WebviewMsg as webviewMessageHandler
participant ModelCache as modelCache
participant LiteLLMUI as LiteLLM.tsx
participant ReactQuery as React Query<br/>["routerModels"]
ApiOptions->>WebviewMsg: requestRouterModels<br/>values: {litellmApiKey, baseUrl}
WebviewMsg->>WebviewMsg: resolve credentials<br/>message.values ?? apiConfiguration
WebviewMsg->>ModelCache: refreshModels<br/>provider, baseUrl, apiKey
Note over ModelCache: Compute compound key:<br/>provider + baseUrl + pbkdf2(apiKey)
ModelCache-->>WebviewMsg: updated model list
WebviewMsg-->>LiteLLMUI: routerModels (status: success)
LiteLLMUI->>ReactQuery: invalidateQueries(["routerModels"])
ReactQuery-->>LiteLLMUI: useSelectedModel re-executes<br/>with fresh cached list
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/api/providers/router-provider.ts (1)
85-86:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve the configured model ID on the final fallback.
Line 64 already computes
idfromthis.modelId ?? this.defaultModelId, but the last resort returnsdefaultModelIdagain. If the model list/cache is empty, this still silently switches a configured LiteLLM model to the hardcoded default instead of only falling back the metadata.Suggested fix
// Last resort: return default model - return { id: this.defaultModelId, info: this.defaultModelInfo } + return { id, info: this.defaultModelInfo } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/providers/router-provider.ts` around lines 85 - 86, The last resort fallback return statement is using this.defaultModelId directly instead of preserving the configured model ID. Change the return statement on line 86 to use the same fallback logic as line 64, which computes `this.modelId ?? this.defaultModelId` for the id field. This ensures that if a model is explicitly configured via this.modelId, it is preserved even when the model list or cache is empty, rather than being silently switched to the hardcoded default.
🧹 Nitpick comments (2)
webview-ui/src/components/settings/providers/LiteLLM.tsx (1)
60-63: ⚡ Quick winAdd a local test for cache invalidation on LiteLLM refresh success.
This introduces a key behavior change (
invalidateQueries({ queryKey: ["routerModels"] })) in response to webview messages. A focused component test should assert it runs on success and is skipped on provider-specific error.As per coding guidelines,
webview-ui/src/**/*.{ts,tsx}requires preferring local webview-ui tests for React/webview behavior and adding/updating Vitest coverage underwebview-ui/src/**/__tests__.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webview-ui/src/components/settings/providers/LiteLLM.tsx` around lines 60 - 63, Add a focused Vitest component test for the LiteLLM.tsx component that verifies the cache invalidation behavior when the refresh succeeds. The test should assert that queryClient.invalidateQueries is called with the queryKey ["routerModels"] upon successful refresh, and separately verify that the cache invalidation is skipped when a provider-specific error occurs. Place this test in webview-ui/src/__tests__/ following the coding guidelines for React/webview component testing.Source: Coding guidelines
webview-ui/src/components/settings/ApiOptions.tsx (1)
225-233: ⚡ Quick winAdd a local test for LiteLLM credential forwarding in the debounced request.
This branch changes webview message wiring (
requestRouterModelspayload shape) but there’s no adjacent Vitest coverage for the debounce path and payload precedence. A focused test here would lock in the regression fix.As per coding guidelines,
webview-ui/src/**/*.{ts,tsx}requires preferring local webview-ui tests for React/webview behavior and adding/updating Vitest coverage underwebview-ui/src/**/__tests__.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webview-ui/src/components/settings/ApiOptions.tsx` around lines 225 - 233, Add a Vitest test file under `webview-ui/src/components/settings/__tests__` that covers the LiteLLM credential forwarding behavior in the debounced request handler within ApiOptions component. The test should verify that when selectedProvider is "litellm", the requestRouterModels message payload correctly includes litellmApiKey and litellmBaseUrl from the apiConfiguration, and should validate both the debounce timing and payload precedence to ensure the regression fix is locked in.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/api/providers/fetchers/modelCache.ts`:
- Around line 106-107: The cacheKeyToFilename function uses simple character
replacement which can cause multiple distinct cache keys to map to the same
filename, resulting in stale cache collisions after restarts. Replace the
current regex-based replacement approach with a hash-based solution that hashes
the full cacheKey string to guarantee uniqueness, while preserving a safe
provider prefix at the beginning of the filename for readability purposes.
- Around line 376-379: The getModelsFromCache function allows auth-scoped
providers like zoo-gateway to read from cache files, which can violate auth
isolation if stale cache files exist from previous versions. Add a check at the
beginning of getModelsFromCache that blocks auth-scoped providers from reading
cache by returning undefined early, similar to the blocking logic already
implemented in getModels and refreshModels. Extract the provider name from the
options parameter and verify it is not an auth-scoped provider before proceeding
with cache retrieval.
- Around line 88-99: The issue is that the key hash generation for
KEY_SCOPED_PROVIDERS is nested inside the isUrlScoped && options.baseUrl
conditional block, causing calls with different API keys to the default server
to incorrectly collapse to the same cache key. Refactor the logic by building
the URL and key components independently: first check if isUrlScoped and
options.baseUrl exist to include the normalizedUrl, then separately check if
isKeyScoped and options.apiKey exist to compute and include the keyHash.
Construct the final cache key by combining the provider with whichever optional
components are applicable, rather than nesting the key hash computation inside
the URL scope branch.
In `@src/core/webview/webviewMessageHandler.ts`:
- Around line 990-995: The case clause for requestRouterModels contains multiple
const declarations (litellmApiKey and litellmBaseUrl) directly within the switch
statement, which violates the Biome noSwitchDeclarations rule. Wrap all the
content of the requestRouterModels case clause (starting from where the const
declarations begin) in curly braces to create a proper block scope that
satisfies the linting rule and ensures correct variable scoping.
---
Outside diff comments:
In `@src/api/providers/router-provider.ts`:
- Around line 85-86: The last resort fallback return statement is using
this.defaultModelId directly instead of preserving the configured model ID.
Change the return statement on line 86 to use the same fallback logic as line
64, which computes `this.modelId ?? this.defaultModelId` for the id field. This
ensures that if a model is explicitly configured via this.modelId, it is
preserved even when the model list or cache is empty, rather than being silently
switched to the hardcoded default.
---
Nitpick comments:
In `@webview-ui/src/components/settings/ApiOptions.tsx`:
- Around line 225-233: Add a Vitest test file under
`webview-ui/src/components/settings/__tests__` that covers the LiteLLM
credential forwarding behavior in the debounced request handler within
ApiOptions component. The test should verify that when selectedProvider is
"litellm", the requestRouterModels message payload correctly includes
litellmApiKey and litellmBaseUrl from the apiConfiguration, and should validate
both the debounce timing and payload precedence to ensure the regression fix is
locked in.
In `@webview-ui/src/components/settings/providers/LiteLLM.tsx`:
- Around line 60-63: Add a focused Vitest component test for the LiteLLM.tsx
component that verifies the cache invalidation behavior when the refresh
succeeds. The test should assert that queryClient.invalidateQueries is called
with the queryKey ["routerModels"] upon successful refresh, and separately
verify that the cache invalidation is skipped when a provider-specific error
occurs. Place this test in webview-ui/src/__tests__/ following the coding
guidelines for React/webview component testing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 308935c5-a61a-45fe-b09e-cea31b5613c9
📒 Files selected for processing (8)
.changeset/fix-litellm-model-desync.mdsrc/api/providers/fetchers/modelCache.tssrc/api/providers/router-provider.tssrc/core/webview/webviewMessageHandler.tswebview-ui/src/components/settings/ApiOptions.tsxwebview-ui/src/components/settings/providers/LiteLLM.tsxwebview-ui/src/components/ui/hooks/__tests__/useSelectedModel.spec.tswebview-ui/src/components/ui/hooks/useSelectedModel.ts
Replaced with new implementation to avoid the advanced security flagging. |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
…gation - Invalidate React Query router-models cache on successful sync in LiteLLM.tsx so useSelectedModel picks up the refreshed list (mirrors Poe.tsx fix) - Preserve configured litellm model ID when model list is empty/loading in useSelectedModel.ts instead of silently falling back to hardcoded default - Pass litellm credentials in the debounced requestRouterModels message in ApiOptions.tsx to prevent fetches with stale config from clearing the model list
- Refactor getCacheKey to compute URL and key components independently, preventing key-scoped providers on the default server from collapsing to the same cache entry - Replace character-substitution in cacheKeyToFilename with SHA-256 hash for collision-free filesystem-safe filenames - Block AUTH_SCOPED_PROVIDERS (zoo-gateway) in getModelsFromCache to prevent stale user-specific model lists from leaking across sessions - Wrap requestRouterModels switch case body in braces to satisfy Biome noSwitchDeclarations lint rule - Preserve configured modelId in RouterProvider.getModel() last-resort fallback instead of silently defaulting to hardcoded defaultModelId - Add codeql[js/insufficient-password-hash] suppressions on SHA-256 cache key discriminator calls (false positive: not password hashing)
The inline // codeql[js/insufficient-password-hash] comments did not suppress the alert because CodeQL tracks taint flow from source variables in other files. Retaining the explanatory prose comments for human reviewers; the false positive requires maintainer dismissal in the GitHub Security tab.
…rtion The test "prefers config values over message values for LiteLLM" was asserting the old || behavior. The handler now uses ?? so message.values takes precedence over saved config, matching the DeepSeek pattern and allowing unsaved settings UI state to be used during Sync Models.
… hashing Replace the SHA-256 hash of the API key in the LiteLLM/key-scoped cache key with a memoized, truncated PBKDF2-derived 32-bit discriminator. This resolves the CodeQL js/insufficient-password-hash alert at the source (Node crypto.pbkdf2Sync is not modeled as a weak-hash sink) rather than dismissing it as a false positive, and the heavy truncation makes the value written to the on-disk cache filename non-reversible to the API key. Add provider-agnostic tests covering per-key separation, determinism, and the non-identifying discriminator shape.
awschmeder
force-pushed
the
fix/litellm-model-desync
branch
from
86470d6 to
a28a834
Compare
|
Resolved the CodeQL The key-scoped cache discriminator no longer hashes the API key with SHA-256. It now uses a memoized PBKDF2 derivation truncated to 32 bits (
Added provider-agnostic tests covering per-key separation, determinism, and the non-identifying discriminator shape. |
… CodeQL taint The API-key discriminator is embedded in the compound cache key, so CodeQL taint tracking treats the cache key as password-derived and flagged the remaining createHash(sha256) in cacheKeyToFilename. Route the filename digest through the same truncated PBKDF2 helper (deriveCacheDigest) and remove createHash entirely, eliminating the last weak-hash sink the tainted value can reach. Filename width is preserved (64-bit/16-hex).
|
@coderabbitai review |
✅ Action performedReview finished.
|
edelauna
left a comment
edelauna
left a comment
There was a problem hiding this comment.
Nice! Thanks for adding caching
Just a note on poe and lm-studio providers:
Two providers listed as fixed still have broken cache lookups after this PR.
poe.ts:41 and lm-studio.ts:173 both call getModelsFromCache with a bare string ("poe" and "lmstudio"). Since both are now declared in URL_SCOPED_PROVIDERS (and Poe also in KEY_SCOPED_PROVIDERS), their cache entries are written under compound keys like poe:<url>:<discriminator> — but the bare-string lookup in getModelsFromCache bypasses getCacheKey and will always miss. These getModel() implementations silently fall back to hardcoded defaults on every call.
| */ | ||
| export function getModelsFromCache(provider: ProviderName): ModelRecord | undefined { | ||
| export function getModelsFromCache( | ||
| options: GetModelsOptions | ProviderName, |
There was a problem hiding this comment.
The | ProviderName string branch sets cacheKey = options verbatim, bypassing getCacheKey. For any URL/key-scoped provider (poe, lmstudio) this produces a bare key that never matches what getModels/refreshModels wrote. The existing callers in poe.ts:41 and lm-studio.ts:173 both pass a bare string today, so those providers always miss the cache despite being listed as fixed in the changeset. Could this overload be dropped and those two call sites updated to pass GetModelsOptions?
| // Track the in-flight request (auth-scoped providers are excluded; see above). | ||
| if (!shouldSkipCache) { | ||
| inFlightRefresh.set(provider, refreshPromise) | ||
| inFlightRefresh.set(cacheKey, refreshPromise) |
There was a problem hiding this comment.
The IIFE runs synchronously up to its first await. If fetchModelsFromProvider resolves via a microtask before this line executes, the finally at line 359 deletes cacheKey from the map before it has ever been inserted — then this line inserts an already-resolved promise that is never evicted. Could inFlightRefresh.set(cacheKey, refreshPromise) be placed just before the IIFE is invoked rather than after?
| // only when none is configured) so an as-yet-unfetched model isn't silently | ||
| // swapped for the hardcoded default. info still comes from defaults since we | ||
| // have no fetched or cached metadata for the configured model at this point. | ||
| return { id, info: this.defaultModelInfo } |
There was a problem hiding this comment.
The previous code returned { id: this.defaultModelId, info: this.defaultModelInfo } here, guaranteeing a non-empty ID. Now it returns the computed id from line 65, which is this.modelId ?? this.defaultModelId. If this.modelId is "" (empty string), ?? doesn't fall through — id stays "" and gets forwarded to the API. Should line 65 use || instead of ?? to also guard against an empty string?
| // nothing is configured we return an empty ID so the picker shows "no | ||
| // selection" rather than a phantom model that does not exist on the server. | ||
| const hasModels = routerModels.litellm && Object.keys(routerModels.litellm).length > 0 | ||
| const id = hasModels |
There was a problem hiding this comment.
When hasModels is true but the configured model ID is absent from the list, getValidatedModelId falls back to defaultModelId. Is there a test covering this case? A mutation that removes the hasModels branch (always returning apiConfiguration.litellmModelId ?? "") would pass the current tests but silently break this validated-fallback path.
| setRefreshStatus("success") | ||
| // Invalidate the react-query router models cache so | ||
| // useSelectedModel picks up the refreshed list. | ||
| queryClient.invalidateQueries({ queryKey: ["routerModels"] }) |
There was a problem hiding this comment.
The query key in useRouterModels is ["routerModels", provider || "all"] (two elements). Passing ["routerModels"] here uses React Query v5 prefix matching, which invalidates all provider subkeys — not just LiteLLM. Should this target ["routerModels", "all"] instead?
|
|
||
| it("should use litellmDefaultModelInfo when selected model not found in routerModels", () => { | ||
| mockUseRouterModels.mockReturnValue({ | ||
| data: { |
There was a problem hiding this comment.
Is there a test for the dynamic transition — render with a non-empty litellm map and a selected model, then re-render with an empty map and assert the selection is preserved? That seems like the primary user-visible scenario the PR is fixing (selection held during a sync that momentarily empties the list).
| @@ -434,3 +434,139 @@ describe("empty cache protection", () => { | |||
| }) | |||
There was a problem hiding this comment.
The in-flight dedup test uses openrouter, whose cache key is the bare provider name. Could a companion test with a key-scoped provider (e.g. requesty) verify that two concurrent calls with different API keys each trigger their own fetch, while two calls with the same key share a single in-flight promise?
github-actions
Bot
added
the
awaiting-author
- poe/lm-studio getModel() pass GetModelsOptions so compound cache key matches; bare-string lookups previously always missed and fell back to defaults - router-provider uses || so empty-string modelId falls back to default instead of forwarding "" to the API - LiteLLM Sync invalidates the exact ["routerModels","litellm"] query rather than the broad ["routerModels"] prefix - document in-flight dedup microtask-safety invariant in modelCache - add tests: list non-empty->empty transition preserves selection; key-scoped in-flight dedup (different keys -> separate fetch)
3 participants
Related GitHub Issue
Closes: #638
Description
Fixes two root bugs causing LiteLLM provider users to lose their selected model:
Bug 1 -- Cache key collision (
src/api/providers/fetchers/modelCache.ts,src/api/providers/router-provider.ts):All URL-scoped providers (LiteLLM, Ollama, LM Studio, Poe, DeepSeek, Requesty) previously shared one cache entry keyed only on the provider name. Switching between profiles backed by different servers silently served the wrong model list, and the stale list persisted across VS Code restarts via the disk cache.
Fix: compound cache key -- URL-scoped providers use
provider:baseUrl; key-scoped providers (LiteLLM, Poe, Requesty) additionally include a short, irreversible discriminator derived from the API key (provider:baseUrl:<discriminator>) so that two different API keys on the same server never share a cache entry. The discriminator is a 32-bit value derived via PBKDF2 and truncated, so the value written to the on-disk cache filename cannot be reversed to identify the API key; this also keeps the derivation off CodeQLs js/insufficient-password-hash sink (Nodes crypto.pbkdf2Sync is not a modeled weak-hash operation), resolving that finding at the source rather than dismissing it. TheRouterProvider.getModel()cold-start fallback is corrected to pass the full options so it resolves the same compound key thatfetchModel()wrote under.Bug 2 -- Silent fallback to hardcoded default (
webview-ui/src/components/ui/hooks/useSelectedModel.ts,src/core/webview/webviewMessageHandler.ts):When the model list was empty,
useSelectedModelreset the configured model ID toclaude-3-7-sonnet-20250219-- a model that typically does not exist on user LiteLLM servers.Four sub-fixes:
""(no selection) rather than a phantom default when nothing is configureduseSelectedModelpicks up the refreshed listrequestRouterModelsmessagewebviewMessageHandler.ts--message.values(current unsaved field state) now takes precedence over saved config via??instead of||, matching the pattern already used for DeepSeekBug 3 -- TypeScript check-types failures (
src/shared/api.ts,src/api/providers/fetchers/modelCache.ts,src/api/providers/fetchers/__tests__/modelCache.spec.ts):Three compile errors introduced by the compound cache key work:
dynamicProviderExtras.litellmdeclaredapiKey: string(required), but the cache key logic supports the no-key case for LiteLLM (URL-only scoping when no key is supplied). Changed toapiKey?: stringand updatedfetchModelsFromProviderto passoptions.apiKey ?? ""to the underlying fetcher.mockCache.getandmockCache.setin the compound-cache-key test were typed as the original NodeCache overloads rather thanMock; fixed with explicit casts.Test Procedure
Automated:
useSelectedModel.spec.ts: the existing test that asserted fallback toclaude-3-7-sonnet-20250219now asserts preservation of the configured model ID; a new test covers the empty-ID case when no model is configured.cd webview-ui && npx vitest run src/components/ui/hooks/__tests__/useSelectedModel.spec.tsManual (Bug 1 -- cache key collision):
baseUrl=http://server-a:4000, select a model.baseUrl=http://server-b:4000.Manual (Bug 2 -- silent fallback):
my-custom-model.my-custom-modelrather than resetting toclaude-3-7-sonnet-20250219.Pre-Submission Checklist
Screenshots / Videos
N/A -- backend cache and hook logic changes; no UI visual changes.
Documentation Updates
Additional Notes
The compound cache key changes the on-disk filename for URL-scoped providers (e.g.
litellm_models.jsonbecomeslitellm_<hash>.json). Existing single-key cache files are simply ignored on first load and will be replaced on the next successful fetch -- no migration needed.Get in Touch
Discord: awschmeder