feat: quota-aware account selection + fix premature model unavailability - #211
DavyMassoneto wants to merge 1 commit into
Conversation
- Move setModelUnavailable from per-account loop to all-accounts-exhausted path - Clear model unavailability on successful fallback - Add in-memory quota cache with background refresh (5min active, 20min exhausted) - Integrate quota cache in account selection to skip exhausted accounts - Mark accounts as exhausted from 429 when no cached quota data exists - Populate quota cache from dashboard usage endpoint
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a sophisticated quota management system that significantly enhances the routing of requests to provider accounts. By implementing an in-memory cache and intelligent selection strategies, the system can now proactively avoid rate-limited accounts and prevent unnecessary service interruptions. This leads to more reliable and efficient request handling, improving the overall stability and performance of the application. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 introduces a quota-aware account selection mechanism and fixes a bug related to premature model unavailability. While the logic for quota management appears sound, there are significant security concerns regarding access control and data handling. Specifically, the new usage API endpoint is vulnerable to IDOR, and the chat handler is susceptible to log injection due to insufficient sanitization of user-supplied fields. Additionally, my review includes suggestions to improve code clarity and robustness in the new caching logic and account selection.
| async function backgroundRefreshTick() { | ||
| const now = Date.now(); | ||
|
|
||
| for (const entry of cache.values()) { | ||
| const age = now - entry.fetchedAt; | ||
|
|
||
| if (entry.exhausted) { | ||
| // If resetAt has passed, refetch immediately | ||
| if (entry.nextResetAt && new Date(entry.nextResetAt).getTime() <= now) { | ||
| refreshEntry(entry); | ||
| continue; | ||
| } | ||
| // Recheck exhausted accounts every 20 minutes | ||
| if (age >= EXHAUSTED_REFRESH_MS) { | ||
| refreshEntry(entry); | ||
| } | ||
| } else { | ||
| // Refresh active accounts every 5 minutes | ||
| if (age >= ACTIVE_TTL_MS) { | ||
| refreshEntry(entry); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The current implementation of backgroundRefreshTick uses a fire-and-forget pattern for refreshEntry calls inside the loop. While this is functional due to the internal error handling in refreshEntry, it can make the code harder to reason about.
To improve clarity and robustness, I suggest collecting all the refresh tasks into an array of promises and then using Promise.allSettled to handle them. This makes the parallel, non-blocking execution explicit and is a more standard pattern for managing multiple concurrent asynchronous operations.
async function backgroundRefreshTick() {
const now = Date.now();
const refreshPromises: Promise<void>[] = [];
for (const entry of cache.values()) {
const age = now - entry.fetchedAt;
if (entry.exhausted) {
// If resetAt has passed, refetch immediately
if (entry.nextResetAt && new Date(entry.nextResetAt).getTime() <= now) {
refreshPromises.push(refreshEntry(entry));
continue;
}
// Recheck exhausted accounts every 20 minutes
if (age >= EXHAUSTED_REFRESH_MS) {
refreshPromises.push(refreshEntry(entry));
}
} else {
// Refresh active accounts every 5 minutes
if (age >= ACTIVE_TTL_MS) {
refreshPromises.push(refreshEntry(entry));
}
}
}
if (refreshPromises.length > 0) {
// Execute refreshes in parallel but don't block the tick.
// Using Promise.allSettled to ensure all promises are handled.
Promise.allSettled(refreshPromises);
}
}| const orderedConnections = | ||
| withQuota.length > 0 ? [...withQuota, ...exhaustedQuota] : availableConnections; |
There was a problem hiding this comment.
The logic to create orderedConnections can be simplified. The ternary operator is redundant because if withQuota.length is 0, then exhaustedQuota will contain all the elements of availableConnections. In that case, [...withQuota, ...exhaustedQuota] (which becomes just exhaustedQuota) is equivalent to availableConnections. You can simplify this to a single, more direct expression.
| const orderedConnections = | |
| withQuota.length > 0 ? [...withQuota, ...exhaustedQuota] : availableConnections; | |
| const orderedConnections = [...withQuota, ...exhaustedQuota]; |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| // Active entries expire after ACTIVE_TTL | ||
| if (age > ACTIVE_TTL_MS) return false; | ||
|
|
||
| return entry.exhausted; |
There was a problem hiding this comment.
WARNING: Dead code — this line returns entry.exhausted but at this point in the function, entry.exhausted must be false (we already returned for exhausted entries at line 131). Since active entries within TTL should return false, this could be simplified to return false; for clarity, or removed entirely as the logic flow already guarantees the correct return value.
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Other Observations (not in diff)
Files Reviewed (4 files)
Overall Assessment: The quota management system is well-designed with:
The single warning is minor (dead code) and doesn't affect functionality. The implementation is solid and follows existing patterns in the codebase. |
* chore(deps): drain the Dependabot queue — 7 of 10 alerts Lockfile-only bumps; no manifest touched, so nothing changes for consumers. Root package-lock.json: hono 4.13.0 -> 4.13.7 (#215 #216 #217, medium, patched 4.13.5) csv-parse 7.0.1 -> 7.0.2 (#213, medium) joi 18.2.3 -> 18.2.8 (#211 #212, low, patched 18.2.4/18.2.5) @omniroute/opencode-plugin: toml 4.1.1 -> 4.3.0 (#209, HIGH, patched 4.1.2) @omniroute/opencode-plugin-v2: esbuild 0.28.1 -> 0.28.2 (#210, low) — the direct copy only; see below. The plugin-v2 diff looks large but is one package: esbuild ships 27 platform binaries, each carrying version + resolved + integrity. Three alerts stay open, deliberately: #218 extract-zip (HIGH) and #214 adm-zip (medium) have NO published patch. Both are dev-scope. Closing them needs an upstream release or a decision to replace the dependency — neither belongs in a lockfile bump. #210 esbuild is only half-closed. `node_modules/esbuild` is on 0.28.2, but `tsup` pins `esbuild: ^0.27.0`, so its nested copy stays at 0.27.7 — inside the vulnerable range (>= 0.27.3, < 0.28.1). Updating tsup does not move it (8.5.1 is already current). Forcing it would take an `overrides` entry pushing a major of esbuild inside the bundler, which is exactly the change that breaks a build silently, for a LOW dev-only alert. Left for an upstream tsup release. check:lockfile passes on all three, including the workspace lock/manifest consistency check. check:tracked-artifacts OK. * chore(deps): bump js-yaml to 4.3.2 (root + electron) Two more HIGH alerts arrived after the first sweep: #220 js-yaml (root package-lock.json) >= 4.0.0, < 4.3.2 #219 js-yaml (electron/package-lock.json) >= 4.0.0, < 4.3.2 The root's own js-yaml was already on 5.4.1; the vulnerable copies were the ones nested under @yarnpkg/parsers, lockfile-lint, xmlbuilder2 (root) and the direct dependency in electron. All now 4.3.2. Four version lines, nothing else. #221 smol-toml (HIGH, <= 1.7.0) is NOT closed here. The root is on 1.8.0; the vulnerable 1.6.1 sits under @openai/codex-security, which pins it as an EXACT version rather than a range, so `npm update` cannot move it. Bumping codex-security itself (0.1.24 -> 0.1.26) does not help — 0.1.26 pins the same 1.6.1 — so that bump was reverted rather than carried along for no benefit. Closing #221 needs an upstream codex-security release or an `overrides` entry, the same trade already declined for #210/tsup: forcing a transitive pin from outside is how a build breaks silently. Note that @openai/codex-security is also the package carrying the unpatched extract-zip (#218), so one upstream release would likely clear both. * chore(deps): override smol-toml to 1.8.0 and raise the js-yaml floor Closes #221 (smol-toml, HIGH, DoS via malformed TOML, vulnerable <= 1.7.0). @openai/codex-security pins smol-toml at 1.6.1 as an EXACT version, so no `npm update` reaches it. This repo already uses `overrides` as its standard tool for exactly that situation — the block carries 20+ entries, including the scoped-by-parent form and the `qs`/`fast-uri`/`ip-address` entries that back earlier security bumps — so a scoped override is the idiomatic fix here, not a new mechanism: "@openai/codex-security": { "smol-toml": "^1.8.0" } The nested copy deduplicates to the root's existing 1.8.0, which two other consumers (the root itself and knip) already run, so the version is proven in this tree. The whole lockfile diff is the 14 lines of the removed 1.6.1 entry. Also raised the `@yarnpkg/parsers` js-yaml floor from ^4.3.1 to ^4.3.2, so the override documents the patched version rather than permitting the vulnerable one it was written against. Not fixed, and not fixable by version — verified against the npm registry rather than trusting the advisory metadata: #218 extract-zip — latest published IS 2.0.1, the vulnerable version. Dev scope, via @openai/codex-security. No release to move to. #214 adm-zip — latest published IS 0.6.0, the top of the vulnerable range (>= 0.5.9, <= 0.6.0). RUNTIME scope, via onnxruntime-node's ^0.5.16, and the repo already overrides adm-zip to ^0.6.0. No release to move to. Both need an upstream fix or a decision to replace the dependency; neither is a lockfile change. adm-zip being runtime rather than dev makes it the one worth tracking. #210 esbuild stays open too. A flat `overrides: { esbuild: ^0.28.2 }` in opencode-plugin-v2 does close it — npm then reports 0 vulnerabilities — but it requires regenerating that lockfile from scratch: 823 lines, 96 packages moved, for a LOW dev-only alert, and a major esbuild bump inside tsup cannot be validated here without a real install of that package. Tried, measured, reverted. Left for an upstream tsup release. check:lockfile OK on all lockfiles including the workspace consistency check; check:tracked-artifacts OK; prettier clean.
1 participant
Summary
setModelUnavailablethat was blocking models after a single account 429Changes
chat.ts: MovesetModelUnavailableto all-accounts-exhausted path, clear on successful fallback, mark 429 in quota cachequotaCache.ts: New module — quota cache with TTL-aware background refresh (5min active, 20min exhausted)auth.ts: IntegrateisAccountQuotaExhaustedto prioritize accounts with available quotausage/[connectionId]/route.ts: Populate quota cache from dashboard usage endpointinstrumentation.ts: Start background refresh on server boot