fix(model-cache): dedupe concurrent fetches and throttle empty-response telemetry - #1072
Draft
edelauna wants to merge 1 commit into
Draft
fix(model-cache): dedupe concurrent fetches and throttle empty-response telemetry#1072edelauna wants to merge 1 commit into
edelauna wants to merge 1 commit into
Conversation
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://cdn.github.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
Comment on lines
+308
to
+320
| // Route the cache-miss fetch through the same single-flight coordinator refreshModels() | ||
| // uses (inFlightRefresh), keyed on the same compound cacheKey. Without this, concurrent | ||
| // getModels() calls for the same key each independently miss the cache and fire their own | ||
| // redundant provider fetch, and a getModels() fetch racing a refreshModels() fetch for the | ||
| // same key has no ordering guarantee -- whichever call's memoryCache.set() lands last wins, | ||
| // even if it started (and thus reflects) an earlier, staler request. Sharing the map means | ||
| // every caller for a given key -- get or refresh -- converges on one in-flight fetch. | ||
| if (!shouldSkipCache) { | ||
| const existingRequest = inFlightRefresh.get(cacheKey) | ||
| if (existingRequest) { | ||
| return existingRequest | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate refreshModels callers and check for try/catch or .catch() handling.
set -euo pipefail
rg -n -C 8 --type=ts -g '!**/__tests__/**' '\brefreshModels\s*\('Repository: Zoo-Code-Org/Zoo-Code
Length of output: 159
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Candidate files =="
fd -a 'modelCache.ts|.*modelCache.*' . | sed 's#^\./##'
echo
echo "== ModelCache outline =="
ast-grep outline src/api/providers/fetchers/modelCache.ts --view compact 2>/dev/null || true
echo
echo "== Relevant modelCache sections =="
sed -n '1,120p' src/api/providers/fetchers/modelCache.ts
printf '\n---\n'
sed -n '260,480p' src/api/providers/fetchers/modelCache.ts
echo
echo "== Broader refreshModels mentions =="
rg -n --type=ts -g '!**/__tests__/**' '\brefreshModels\s*\b|\binFlightRefresh\b' . || trueRepository: Zoo-Code-Org/Zoo-Code
Length of output: 20272
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== modelCache.spec outline =="
ast-grep outline src/api/providers/fetchers/__tests__/modelCache.spec.ts --view compact 2>/dev/null || true
echo
echo "== modelCache.spec relevant sections =="
wc -l src/api/providers/fetchers/__tests__/inFlightRefresh getModels join getModels join refreshModels getModels refreshModels inFlightRefresh getModels getModels getModels
sed -n '1,220p' src/api/providers/fetchers/__tests__/modelCache.spec.ts
printf '\n---\n'
sed -n '220,480p' src/api/providers/fetchers/__tests__/modelCache.spec.ts
printf '\n---\n'
sed -n '480,720p' src/api/providers/fetchers/__tests__/modelCache.spec.ts
echo
echo "== Search in-flight behavior in tests =="
rg -n --type=ts -C 6 'inFlightRefresh|concurrent|same key|re-throw|degrade|cached fetch error|graceful|failed|join|same flight|getModels|refreshModels' src/api/providers/fetchers/__tests__/modelCache.spec.ts || trueRepository: Zoo-Code-Org/Zoo-Code
Length of output: 777
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== modelCache.spec outline =="
ast-grep outline src/api/providers/fetchers/__tests__/modelCache.spec.ts --view compact 2>/dev/null || true
echo
echo "== modelCache.spec relevant sections =="
wc -l src/api/providers/fetchers/__tests__/modelCache.spec.ts
sed -n '1,220p' src/api/providers/fetchers/__tests__/modelCache.spec.ts
printf '\n---\n'
sed -n '220,480p' src/api/providers/fetchers/__tests__/modelCache.spec.ts
printf '\n---\n'
sed -n '480,720p' src/api/providers/fetchers/__tests__/modelCache.spec.ts
echo
echo "== Search in-flight behavior in tests =="
rg -n --type=ts -C 6 'inFlightRefresh|concurrent|same key|re-throw|degrade|cached fetch error|graceful|failed|join|same flight|getModels|refreshModels' src/api/providers/fetchers/__tests__/modelCache.spec.ts || trueRepository: Zoo-Code-Org/Zoo-Code
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Caller files =="
for f in webview-ui/src/components/settings/providers/Unbound.tsx webview-ui/src/components/settings/providers/LiteLLM.tsx webview-ui/src/components/settings/providers/Poe.tsx webview-ui/src/components/settings/providers/Ollama.tsx webview-ui/src/components/settings/providers/Moonshot.tsx webview-ui/src/components/settings/providers/Requesty.tsx webview-ui/src/components/settings/providers/OpenCodeGo.tsx; do
if [ -f "$f" ]; then
echo "--- $f ---"
nl -ba "$f" | sed -n '1,175p'
fi
done
echo
echo "== refreshModels call sites with handler context =="
rg -n -C 5 --type tsx --type ts -g 'webview-ui/src/components/settings/providers/**' 'refreshModels\s*\(' webview-ui/src/components/settings/providers || true
echo
echo "== initializeModelCacheRefresh caller context =="
sed -n '460,520p' src/api/providers/fetchers/modelCache.ts
rg -n -C 4 'initializeModelCacheRefresh\s*\(' . --type ts --type tsx || true
echo
echo "== Behavioral probe for Promise rejection propagation on shared promise =="
node - <<'JS'
let inFlightRefresh = new Map()
function makeGetModels() {
const promise = Promise.reject(new Error("getModels fetch failed"))
inFlightRefresh.set("k", promise)
return promise
}
function makeRefreshModels() {
const existingRequest = inFlightRefresh.get("k")
if (existingRequest) {
return existingRequest
}
}
const get = makeGetModels()
const refresh = makeRefreshModels()
for (const [name, promise] of [["get", get], ["refresh", refresh]]) {
promise.then(
(value) => console.log(name, "resolved", value),
(error) => console.log(name, "rejected", error.message)
)
}
process.off("unhandledRejection", () => {})
// Keep Node 2+ seconds to observe unhandled rejection if no handler existed after joining.
setTimeout(() => {
console.log(JSON.stringify({ getRejectionPropagates: get.status === "rejected", refreshRejectionPropagates: refresh.status === "rejected" }), null, 2)
}, 50)
JSRepository: Zoo-Code-Org/Zoo-Code
Length of output: 283
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Caller files =="
for f in webview-ui/src/components/settings/providers/Unbound.tsx webview-ui/src/components/settings/providers/LiteLLM.tsx webview-ui/src/components/settings/providers/Poe.tsx webview-ui/src/components/settings/providers/Ollama.tsx webview-ui/src/components/settings/providers/Moonshot.tsx webview-ui/src/components/settings/providers/Requesty.tsx webview-ui/src/components/settings/providers/OpenCodeGo.tsx; do
if [ -f "$f" ]; then
echo "--- $f ---"
awk '{printf "%6d\t%s\n", NR, $0}' "$f" | sed -n '1,185p'
fi
done
echo
echo "== refreshModels call sites with handler context =="
rg -n -C 5 --type tsx --type ts -g 'webview-ui/src/components/settings/providers/**' 'refreshModels\s*\(' webview-ui/src/components/settings/providers || true
echo
echo "== initializeModelCacheRefresh caller context =="
awk '{printf "%6d\t%s\n", NR, $0}' src/api/providers/fetchers/modelCache.ts | sed -n '460,520p'
rg -n -C 4 'initializeModelCacheRefresh\s*\(' . --type ts --type tsx || true
echo
echo "== Behavioral probe for Promise rejection propagation on shared promise =="
node - <<'JS'
const inFlightRefresh = new Map()
let getError, refreshError
const unhandled = []
process.on("unhandledRejection", (rej) => unhandled.push(rej.message))
process.on("rejectionHandled", (promise) => {});
function makeGetModels() {
const promise = Promise.reject(new Error("getModels fetch failed"))
inFlightRefresh.set("k", promise)
return promise
}
function makeRefreshModels() {
const existingRequest = inFlightRefresh.get("k")
if (existingRequest) return existingRequest
}
const get = makeGetModels()
const refresh = makeRefreshModels()
get.then(
() => {},
(e) => { getError = e && e.message }
)
refresh.then(
() => {},
(e) => { refreshError = e && e.message }
)
process.off("unhandledRejection", () => {})
// The joined promise is not unhandled if any call site attaches a rejection handler.
setTimeout(() => {
console.log(JSON.stringify({ get: getError, refresh: refreshError, unhandled }, null, 2))
}, 50)
JSRepository: Zoo-Code-Org/Zoo-Code
Length of output: 49822
Normalize the joined in-flight promise at each entry point.
getModels rereleases and deletes the shared inFlightRefresh promise on fetch failure, while refreshModels degrades to existing cache or {}. A refreshModels call joining a failed getModels request now rejects instead of returning degraded cache, and a getModels call joining a failed refreshModels request now hides the fetch error. Wrap the joined promise according to each API's contract, or split the in-flight map.
🤖 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 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.
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
1 participant
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