fix: address PR review for LiteLLM cache key collision fix

- 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)

Author: awschmeder

src/api/providers/fetchers/__tests__/modelCache.spec.ts

@@ -432,6 +432,52 @@ describe("empty cache protection", () => {
 			expect(result1).toEqual(mockModels)
 			expect(result2).toEqual(mockModels)
 		})
+
+		it("scopes in-flight dedup by API key for key-scoped providers", async () => {
+			// In-flight dedup is keyed on the compound cache key, so concurrent refreshes for a
+			// key-scoped provider must dedup only when the API key matches. Two different keys
+			// (different compound keys) each trigger their own fetch; the same key shares one.
+			const mockModels = {
+				"requesty/model": {
+					maxTokens: 4096,
+					contextWindow: 200000,
+					supportsPromptCache: false,
+					description: "Requesty model",
+				},
+			}
+			mockGetRequestyModels.mockResolvedValue(mockModels)
+
+			const { refreshModels } = await import("../modelCache")
+
+			// Different keys -> separate compound keys -> two distinct fetches.
+			const [a, b] = await Promise.all([
+				refreshModels({ provider: "requesty", apiKey: "key-one" }),
+				refreshModels({ provider: "requesty", apiKey: "key-two" }),
+			])
+			expect(mockGetRequestyModels).toHaveBeenCalledTimes(2)
+			expect(a).toEqual(mockModels)
+			expect(b).toEqual(mockModels)
+
+			mockGetRequestyModels.mockClear()
+
+			// Same key -> same compound key -> a single shared in-flight fetch.
+			let resolveShared: (value: typeof mockModels) => void
+			mockGetRequestyModels.mockReturnValue(
+				new Promise<typeof mockModels>((resolve) => {
+					resolveShared = resolve
+				}),
+			)
+
+			const shared1 = refreshModels({ provider: "requesty", apiKey: "same-key" })
+			const shared2 = refreshModels({ provider: "requesty", apiKey: "same-key" })
+
+			expect(mockGetRequestyModels).toHaveBeenCalledTimes(1)
+
+			resolveShared!(mockModels)
+			const [s1, s2] = await Promise.all([shared1, shared2])
+			expect(s1).toEqual(mockModels)
+			expect(s2).toEqual(mockModels)
+		})
 	})
 })
 

src/api/providers/fetchers/modelCache.ts

@@ -310,7 +310,13 @@ export const refreshModels = async (options: GetModelsOptions): Promise<ModelRec
 		}
 	}
 
-	// Create the refresh promise and track it
+	// Create the refresh promise and track it.
+	//
+	// The `finally` cleanup below runs only after the first `await` inside this async
+	// function yields, which cannot happen until the current synchronous run -- including
+	// the `inFlightRefresh.set(cacheKey, ...)` registration below -- has completed. So the
+	// entry is always present in the map before `finally` can delete it; the registration
+	// can never be lost to a microtask race even if the fetch resolves immediately.
 	const refreshPromise = (async (): Promise<ModelRecord> => {
 		try {
 			// Force fresh API fetch - skip getModelsFromCache() check

src/api/providers/lm-studio.ts

@@ -170,7 +170,10 @@ export class LmStudioHandler extends BaseProvider implements SingleCompletionHan
 	}
 
 	override getModel(): { id: string; info: ModelInfo } {
-		const models = getModelsFromCache("lmstudio")
+		const models = getModelsFromCache({
+			provider: "lmstudio",
+			baseUrl: this.options.lmStudioBaseUrl,
+		})
 		if (models && this.options.lmStudioModelId && models[this.options.lmStudioModelId]) {
 			return {
 				id: this.options.lmStudioModelId,

src/api/providers/poe.ts

@@ -38,7 +38,11 @@ export class PoeHandler extends BaseProvider implements SingleCompletionHandler
 
 	override getModel() {
 		const id = this.options.apiModelId ?? poeDefaultModelId
-		const cached = getModelsFromCache("poe")
+		const cached = getModelsFromCache({
+			provider: "poe",
+			apiKey: this.options.poeApiKey,
+			baseUrl: this.options.poeBaseUrl,
+		})
 		const info: ModelInfo = cached?.[id] ?? getPoeDefaultModelInfo()
 		return { id, info }
 	}

src/api/providers/router-provider.ts

@@ -62,7 +62,13 @@ export abstract class RouterProvider extends BaseProvider {
 	}
 
 	override getModel(): { id: string; info: ModelInfo } {
-		const id = this.modelId ?? this.defaultModelId
+		// Use `||` (not `??`) so an empty-string modelId also falls back to the default,
+		// guaranteeing a non-empty id rather than forwarding "" to the API as an invalid
+		// request. Note this guarantees non-empty, not viable: defaultModelId is provider-
+		// supplied and may not be a model that actually exists on the user's server (e.g.
+		// OpenAI-compatible have no inherent default), so a configured-but-empty selection
+		// can still resolve to a model the server rejects.
+		const id = this.modelId || this.defaultModelId
 
 		// First check instance models (populated by fetchModel)
 		if (this.models[id]) {

webview-ui/src/components/settings/providers/LiteLLM.tsx

@@ -57,9 +57,12 @@ export const LiteLLM = ({
 				if (refreshStatus === "loading") {
 					if (!litellmErrorJustReceived.current) {
 						setRefreshStatus("success")
-						// Invalidate the react-query router models cache so
-						// useSelectedModel picks up the refreshed list.
-						queryClient.invalidateQueries({ queryKey: ["routerModels"] })
+						// Invalidate only the LiteLLM router-models query so useSelectedModel
+						// picks up the refreshed list. useSelectedModel reads LiteLLM under the
+						// compound key ["routerModels", "litellm"] (see useRouterModels), so we
+						// target that exact key rather than the bare ["routerModels"] prefix,
+						// which would needlessly invalidate every other provider's query too.
+						queryClient.invalidateQueries({ queryKey: ["routerModels", "litellm"] })
 					}
 					// If litellmErrorJustReceived.current is true, status is already (or will be) "error".
 				}

webview-ui/src/components/ui/hooks/__tests__/useSelectedModel.spec.ts

@@ -602,6 +602,54 @@ describe("useSelectedModel", () => {
 			expect(result.current.info).toEqual(litellmDefaultModelInfo)
 		})
 
+		it("preserves the selected model when the list transitions from populated to empty", () => {
+			// Primary user-visible scenario: a "Sync Models" click momentarily empties the
+			// router-models list before the refreshed list arrives. The selection must be held
+			// across that transition rather than reset.
+			mockUseRouterModels.mockReturnValue({
+				data: {
+					openrouter: {},
+					requesty: {},
+					litellm: {
+						"my-custom-model": {
+							maxTokens: 4096,
+							contextWindow: 8192,
+							supportsImages: false,
+							supportsPromptCache: false,
+						},
+					},
+				},
+				isLoading: false,
+				isError: false,
+			} as any)
+
+			const apiConfiguration: ProviderSettings = {
+				apiProvider: "litellm",
+				litellmModelId: "my-custom-model",
+			}
+
+			const wrapper = createWrapper()
+			const { result, rerender } = renderHook(() => useSelectedModel(apiConfiguration), { wrapper })
+
+			// Initially the configured model resolves from the populated list.
+			expect(result.current.id).toBe("my-custom-model")
+
+			// Simulate the list emptying mid-sync.
+			mockUseRouterModels.mockReturnValue({
+				data: {
+					openrouter: {},
+					requesty: {},
+					litellm: {},
+				},
+				isLoading: false,
+				isError: false,
+			} as any)
+			rerender()
+
+			// Selection is preserved through the empty window.
+			expect(result.current.id).toBe("my-custom-model")
+		})
+
 		it("should use litellmDefaultModelInfo when selected model not found in routerModels", () => {
 			mockUseRouterModels.mockReturnValue({
 				data: {