fix(model-cache): dedupe concurrent fetches and throttle empty-response telemetry - #1072
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe model cache now scopes provider identities, throttles repeated empty-response telemetry, and shares non-auth-scoped in-flight requests between ChangesModel cache behavior
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant getModels
participant refreshModels
participant dedupedFetch
participant providerFetcher
participant telemetry
getModels->>dedupedFetch: request models
refreshModels->>dedupedFetch: reuse request for same key
dedupedFetch->>providerFetcher: fetch models once
providerFetcher-->>dedupedFetch: empty or non-empty result
dedupedFetch->>telemetry: report throttled empty result
dedupedFetch-->>getModels: return result
dedupedFetch-->>refreshModels: return result
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
edelauna
changed the title
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/api/providers/fetchers/__tests__/modelCache.spec.ts (2)
483-493: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBind the LiteLLM mock to the request arguments, not to call order.
mockResolvedValueOncechaining ties each result to invocation order. The test claims endpoint isolation, so assert that each result matches its ownbaseUrl. An implementation change that reorders the two fetches would then fail instead of passing with swapped payloads.♻️ Proposed refactor
- mockGetLiteLLMModels.mockResolvedValueOnce(mockModelsA).mockResolvedValueOnce(mockModelsB) + mockGetLiteLLMModels.mockImplementation(async (_apiKey, baseUrl) => + baseUrl === "http://server-a:4000" ? mockModelsA : mockModelsB, + ) mockGet.mockReturnValue(undefined)🤖 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/fetchers/__tests__/modelCache.spec.ts` around lines 483 - 493, Update the LiteLLM mock setup in the getModels concurrency test to return mockModelsA or mockModelsB based on the request’s baseUrl rather than mockResolvedValueOnce call order. Keep the parallel requests and assertions, ensuring each result remains tied to its corresponding endpoint even if invocation order changes.
884-923: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe comment states behavior the code does not implement.
getModelsexcludes auth-scoped providers frominFlightRefreshentirely (modelCache.tslines 315-320 and 361-363). The map never keys auth-scoped identities. This test passes because no deduplication occurs at all forzooGateway, not because the key is compound. The same assertion would hold for two calls with an identical token.Correct the comment, and assert the actual guarantee.
♻️ Proposed refactor
- // The in-flight fetch map must key on the full compound identity for auth-scoped - // providers too, so a slow fetch for one account's session token can never resolve - // into a concurrent call carrying a different account's token. + // Auth-scoped providers bypass the in-flight map (see AUTH_SCOPED_PROVIDERS), so + // concurrent calls are never deduplicated. A slow fetch for one account's session + // token can therefore never resolve into a call carrying a different account's token.Add a companion case that pins the no-dedup guarantee for an identical token:
it("never deduplicates concurrent zoo-gateway fetches, even for the same token", async () => { freshMockGetZooGatewayModels.mockResolvedValue({}) await Promise.all([ freshGetModels({ provider: providerIdentifiers.zooGateway, apiKey: "same-token" }), freshGetModels({ provider: providerIdentifiers.zooGateway, apiKey: "same-token" }), ]) expect(freshMockGetZooGatewayModels).toHaveBeenCalledTimes(2) })As per coding guidelines: "Use package-local unit tests for pure logic, parsing, state transitions, validation, serialization, request construction, retry decisions, and error handling."
🤖 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/fetchers/__tests__/modelCache.spec.ts` around lines 884 - 923, The test’s comment incorrectly claims auth-scoped fetches are deduplicated by compound identity, while getModels intentionally excludes them from inFlightRefresh. Update the existing test comment and assertions to describe and verify the actual no-deduplication guarantee, and add a companion case using identical tokens that confirms concurrent zoo-gateway calls still invoke freshMockGetZooGatewayModels twice.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 308-320: Normalize joined inFlightRefresh promises separately in
getModels and refreshModels so each API preserves its own failure behavior:
getModels must propagate fetch errors and clean up the shared entry, while
refreshModels must fall back to existing cache or {}. Update the existingRequest
handling around getModels and refreshModels, without changing the shared cache
key or single-flight coordination.
---
Nitpick comments:
In `@src/api/providers/fetchers/__tests__/modelCache.spec.ts`:
- Around line 483-493: Update the LiteLLM mock setup in the getModels
concurrency test to return mockModelsA or mockModelsB based on the request’s
baseUrl rather than mockResolvedValueOnce call order. Keep the parallel requests
and assertions, ensuring each result remains tied to its corresponding endpoint
even if invocation order changes.
- Around line 884-923: The test’s comment incorrectly claims auth-scoped fetches
are deduplicated by compound identity, while getModels intentionally excludes
them from inFlightRefresh. Update the existing test comment and assertions to
describe and verify the actual no-deduplication guarantee, and add a companion
case using identical tokens that confirms concurrent zoo-gateway calls still
invoke freshMockGetZooGatewayModels twice.
🪄 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: 1eb0af9e-2c5f-4f50-b6bb-ac1c2549e4c4
📒 Files selected for processing (2)
src/api/providers/fetchers/__tests__/modelCache.spec.tssrc/api/providers/fetchers/modelCache.ts
edelauna
marked this pull request as ready for review
July 31, 2026 14:04
edelauna
requested review from
JamesRobert20,
hannesrudolph,
navedmerchant and
taltas
as code owners
July 31, 2026 14:04
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/api/providers/fetchers/modelCache.ts (1)
402-448: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInline the
refreshPromiseIIFE.The IIFE existed to register the promise in
inFlightRefreshbefore awaiting it.dedupedFetchnow owns that registration, so the wrapper adds a nesting level and no behavior.refreshModelsis alreadyasync, so the body can run directly.♻️ Proposed simplification
- const refreshPromise = (async (): Promise<ModelRecord> => { - try { - // Force fresh API fetch - skip getModelsFromCache() check - const models = await sharedFetch - const modelCount = Object.keys(models).length + try { + // Force fresh API fetch - skip getModelsFromCache() check + const models = await sharedFetch + const modelCount = Object.keys(models).length - // Get existing cached data for comparison - const existingCache = shouldSkipCache ? undefined : getModelsFromCache(options) - const existingCount = existingCache ? Object.keys(existingCache).length : 0 + // Get existing cached data for comparison + const existingCache = shouldSkipCache ? undefined : getModelsFromCache(options) + const existingCount = existingCache ? Object.keys(existingCache).length : 0 - if (modelCount === 0) { - captureModelCacheEmptyResponseOnce(provider, cacheKey, { - context: "refreshModels", - hasExistingCache: existingCount > 0, - existingCacheSize: existingCount, - }) - if (existingCount > 0) { - return existingCache! - } else { - return {} - } - } + if (modelCount === 0) { + captureModelCacheEmptyResponseOnce(provider, cacheKey, { + context: "refreshModels", + hasExistingCache: existingCount > 0, + existingCacheSize: existingCount, + }) + return existingCount > 0 ? existingCache! : {} + } - reportedEmptyModelResponse.delete(cacheKey) + reportedEmptyModelResponse.delete(cacheKey) - if (!shouldSkipCache) { - memoryCache.set(cacheKey, models) + if (!shouldSkipCache) { + memoryCache.set(cacheKey, models) - await writeModels(cacheKey, models).catch((err) => - console.error(`[refreshModels] Error writing ${cacheKey} models to disk:`, err), - ) - } + await writeModels(cacheKey, models).catch((err) => + console.error(`[refreshModels] Error writing ${cacheKey} models to disk:`, err), + ) + } - return models - } catch (error) { - // ... - console.error(`[refreshModels] Failed to refresh ${cacheKey} models:`, error) - if (shouldSkipCache) { - return {} - } - return getModelsFromCache(options) || {} - } - })() - - return refreshPromise + return models + } catch (error) { + // Log the error for debugging, then return existing cache if available (graceful degradation). + // For auth-scoped providers (zoo-gateway) we MUST NOT return cached models from a prior + // session, since they could belong to a different user -- return empty instead. + console.error(`[refreshModels] Failed to refresh ${cacheKey} models:`, error) + if (shouldSkipCache) { + return {} + } + return getModelsFromCache(options) || {} + }🤖 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/fetchers/modelCache.ts` around lines 402 - 448, Inline the body of the refreshPromise IIFE directly into the surrounding refreshModels flow, removing the unnecessary refreshPromise declaration and invocation. Preserve the existing fetch, cache update, empty-response handling, error fallback, and return behavior; rely on dedupedFetch for inFlightRefresh registration.
🤖 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.
Nitpick comments:
In `@src/api/providers/fetchers/modelCache.ts`:
- Around line 402-448: Inline the body of the refreshPromise IIFE directly into
the surrounding refreshModels flow, removing the unnecessary refreshPromise
declaration and invocation. Preserve the existing fetch, cache update,
empty-response handling, error fallback, and return behavior; rely on
dedupedFetch for inFlightRefresh registration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: feeaad65-3eb1-4404-8730-e9f461acf00d
📒 Files selected for processing (2)
src/api/providers/fetchers/__tests__/modelCache.spec.tssrc/api/providers/fetchers/modelCache.ts
github-actions
Bot
added
the
awaiting-review
github-actions
Bot
removed
the
awaiting-review
…ment, cover in-flight cleanup
github-actions
Bot
added
the
awaiting-review
JamesRobert20
requested changes
Aug 1, 2026
JamesRobert20
left a comment
JamesRobert20
left a comment
Contributor
There was a problem hiding this comment.
Overall this looks solid and nearly ready to merge. One change requested:
Please fix the misleading comment in the zoo-gateway auth-isolation test (modelCache.spec.ts). It currently says isolation comes from compound keys in the in-flight map, but auth-scoped providers bypass dedupedFetch entirely. Rewrite the comment to match that, and optionally add a same-token case that expects two fetcher calls.
Everything else LGTM.
github-actions
Bot
added
awaiting-author
github-actions
Bot
removed
the
awaiting-author
github-actions
Bot
added
the
awaiting-review
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
2 participants
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.
Related GitHub Issue
Refs: #830 (model-cache concurrency and empty-response telemetry — split from #835; independent of #1069, #1070, and #1071)
Description
getModels()had no in-flight deduplication. Two concurrent cache-miss calls for the same provider each fired their own provider fetch, and agetModels()fetch racing arefreshModels()fetch for the same key had no ordering guarantee — whichever call's cache write landed last won, even if it reflected an earlier, staler request.getModels()now shares the sameinFlightRefreshsingle-flight map thatrefreshModels()already used, keyed on the existing compound cache key fromgetCacheKey(). Every caller for a given key — get or refresh — converges on one in-flight fetch.MODEL_CACHE_EMPTY_RESPONSEtelemetry also fired on every empty response, so a persistently broken endpoint re-fired the event on every cache check. It now reports at most once per cache key (captureModelCacheEmptyResponseOnce) until a non-empty response re-arms it. This applies to auth-scoped providers too (zoo-gateway, kimi-code), even though they skip caching entirely.To keep the throttle and in-flight map correctly isolated per credential for auth-scoped providers,
zoo-gatewaywas added toURL_SCOPED_PROVIDERSand bothzoo-gatewayandkimi-codewere added toKEY_SCOPED_PROVIDERS. This only changes the key used for the throttle/in-flight coordination — actual cache reads/writes for these providers stay fully skipped (shouldSkipCacheis unchanged), so a sign-out/sign-in cycle still never serves one account's model list to another.Non-empty cache-write behavior and the graceful-degradation behavior on a failed refresh are both unchanged.
Test Procedure
Ran the model-cache unit suite directly:
43 tests pass, including 12 new cases covering:
getModels()calls share one provider fetchgetModels()andrefreshModels()share the same in-flight fetchPre-Submission Checklist
src/api/providers/fetchers/modelCache.tsand its unit tests. No consent/banner/circuit-breaker/task-completion changes.Documentation Updates
Summary by CodeRabbit