fix: isolate combo cache by endpoint and credential#40
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the in-memory combo cache to isolate entries by endpoint and a non-reversible credential digest (SHA-256) using a Map, and adds corresponding integration tests. The review feedback suggests adding defensive guards to handle missing configuration properties (baseUrl or apiKey) gracefully, and optimizing the fallback check in isComboModel by iterating directly over the cache values instead of allocating a new array via the spread operator.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
|
||
| // Check cache first | ||
| if (comboCache && Date.now() - comboCache.timestamp < COMBO_CACHE_TTL) { | ||
| const cacheKey = getComboCacheKey(baseUrl, apiKey); |
There was a problem hiding this comment.
If baseUrl or apiKey is missing or undefined in the configuration, calling getComboCacheKey will throw a synchronous TypeError (e.g., when calling .replace on undefined or passing a non-string to createHash().update()). Adding a defensive guard here ensures that we handle missing configuration gracefully by logging a warning and returning null instead of crashing the process.
if (!baseUrl || !apiKey) {
warn('Missing baseUrl or apiKey in config');
return null;
}
const cacheKey = getComboCacheKey(baseUrl, apiKey);There was a problem hiding this comment.
Addressed in the latest push: fetchComboData() now returns null before deriving a cache key when either value is empty, with regression coverage that proves no fetch occurs.
|
|
||
| return false; | ||
| // Fallback: check all endpoint/credential-specific combo caches. | ||
| return [...comboCaches.values()].some((cached) => cached.combos.has(model.id)); |
There was a problem hiding this comment.
Using the spread operator [...comboCaches.values()] creates a new array allocation on every call to isComboModel. Since this function can be called frequently (e.g., when filtering or enriching lists of models), we can optimize this by iterating directly over the iterator using a for...of loop, avoiding unnecessary memory allocation and improving performance.
for (const cached of comboCaches.values()) {
if (cached.combos.has(model.id)) {
return true;
}
}
return false;There was a problem hiding this comment.
Addressed: isComboModel() now iterates comboCaches.values() directly without allocating an array.
Code Review Roast 🔥Verdict: No Issues Found | Recommendation: Merge Oh wait, this PR is actually clean. I need to sit down. I had my flamethrower warmed up and everything. 📊 Overall: Like finding a unicorn in production — I didn't think clean PRs existed anymore, but here we are. Files Reviewed (3 files)
Previous Review Summaries (2 snapshots, latest commit 5f9be7a)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 5f9be7a)Verdict: 2 Suggestions Found | Recommendation: Address before merge Overview
Issue Details (click to expand)
🏆 Best part: The credential hashing and cache-key isolation are actually well-designed — SHA-256 digests for cache identities is a clean way to avoid credential leakage. 💀 Worst part: The unbounded 📊 Overall: Like a first pancake — the shape is right and the ingredients are good, but the edges are a bit ragged. Close, but needs a trim. Files Reviewed (2 files)
Fix these issues in Kilo Cloud Model used for review: kilo-auto/free Previous review (commit 9bffc65)Verdict: No Issues Found | Recommendation: Merge Oh wait, this PR is actually clean. I need to sit down. I had my flamethrower warmed up and everything. 📊 Overall: Like finding a unicorn in production — I didn't think clean PRs existed anymore, but here we are. Files Reviewed (2 files)
Model used for review: kilo-auto/free Reviewed by step-3.7-flash · Input: 193.6K · Output: 16K · Cached: 1.1M |
9bffc65 to
5f9be7a
Compare
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| // In-memory cache for combo data | ||
| let comboCache: ComboCache | null = null; | ||
| // Cache entries are isolated by endpoint and a non-reversible credential digest. | ||
| const comboCaches = new Map<string, ComboCache>(); |
There was a problem hiding this comment.
🔥 The Roast: This Map is a hoarder's paradise — it accepts every endpoint/credential pair it ever meets and never throws anything away. The old comboCache was a single variable that got overwritten on every fetch, naturally bounding memory. Now comboCaches grows indefinitely until clearComboCache() is called. In a long-running process or server that rotates credentials, you've just scheduled a memory leak.
🩹 The Fix: Evict stale entries proactively. On a cache miss, prune any entries older than COMBO_CACHE_TTL before inserting the fresh one. This keeps the Map bounded without relying on explicit cleanup calls.
📏 Severity: suggestion
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Addressed in the latest push.
fetchComboData() now calls pruneExpiredComboCaches() before inserting a fresh entry, so identities older than COMBO_CACHE_TTL are removed proactively. This keeps the multi-entry map bounded without waiting for an explicit clear.
Regression: expired combo cache entries are pruned on write.
| @@ -122,7 +138,7 @@ export async function fetchComboData( | |||
| * Clear the combo cache | |||
| */ | |||
There was a problem hiding this comment.
🔥 The Roast: clearComboCache() is still living in the singleton era. It unconditionally wipes ALL endpoint/credential caches, which means callers like clearModelCache(config, apiKey) in src/models.ts:386 — designed to clear a single config's data — also nukes every other endpoint's combo cache. That's emptying the entire office fridge because your yogurt expired.
🩹 The Fix: Either accept an optional cache key parameter for selective deletion, or update clearModelCache to only clear the matching combo cache entry when specific config and apiKey are provided. Match the granularity to the caller's intent.
📏 Severity: suggestion
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Addressed in the latest push.
clearComboCache(config?) is now selective:
- with
baseUrl+apiKey→ delete only that endpoint/credential identity - without config → clear the full map (legacy behavior)
clearModelCache(config, apiKey) routes through the selective form so a single-config model-cache clear no longer wipes every combo identity.
Regression: clearComboCache can target one endpoint/credential pair.
Address review feedback on the multi-entry combo cache: - prune expired endpoint/credential entries before cache writes - allow clearComboCache(config) to target one identity - keep full clear when no config is provided - route clearModelCache through the selective combo clear - add regression coverage for selective clear and TTL prune
Review follow-upPushed an additional commit addressing the remaining review suggestions on the multi-entry combo cache:
Focused combo tests: all passing. Pre-existing flaky logger alphabetical-mtime test is unrelated to this change. |
1 participant
Summary
isComboModel()fallback behavior across all keyed cachesRoot cause
fetchComboData()previously reused one process-global cache entry based only on age. A second OmniRoute endpoint or credential could therefore receive the first configuration's combo definitions for up to five minutes.Verification
npm test: 69 passed, 0 failednpm testgit diff --checkCloses #39