feat(combos): add reset-aware routing strategy#2015
Conversation
|
still need to test some stuff, i'll reopen once tests are finished |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ffde066951
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (config.exhaustionGuard > 0 && sessionRemaining < config.exhaustionGuard) { | ||
| score *= Math.max(0.05, sessionRemaining / config.exhaustionGuard); | ||
| } |
There was a problem hiding this comment.
When a Codex account is at or above the weekly quota preflight threshold but its weekly reset is soon, this score can still rank it ahead of a usable account because the exhaustion guard only applies to sessionRemaining. In that scenario getProviderCredentialsWithQuotaPreflight will reject the forced connection as quota-exhausted and the combo then falls back on every request, adding avoidable latency/noisy failures even though the reset-aware sorter already had the weekly window data needed to skip or heavily penalize it.
Useful? React with 👍 / 👎.
![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 new 'reset-aware' routing strategy for model combos, specifically designed to optimize Codex account usage by balancing remaining quota against 5h and weekly reset windows. The implementation includes scoring logic, UI configuration options, and unit tests. Feedback focuses on performance and reliability improvements, specifically suggesting the use of concurrency for connection fetching, implementing limits on simultaneous quota requests to avoid network bursts, and addressing potential clock drift issues in reset urgency calculations.
| const connectionById = new Map<string, Record<string, unknown>>(); | ||
| const expandedTargets: ResolvedComboTarget[] = []; | ||
|
|
||
| for (const target of targets) { |
There was a problem hiding this comment.
The loop iterates through targets and performs sequential await on getCodexConnectionsForTarget. While there is a local connectionCache, the initial calls for each unique provider will be sequential. Consider using Promise.all to fetch connections for all targets concurrently to improve performance, especially for combos with multiple providers.
| const scoredTargets = await Promise.all( | ||
| expandedTargets.map(async (target, index) => { | ||
| let quota: unknown = null; | ||
| if (isCodexTarget(target) && target.connectionId) { | ||
| try { | ||
| quota = await fetchCodexQuota( | ||
| target.connectionId, | ||
| connectionById.get(target.connectionId) | ||
| ); | ||
| } catch (error) { | ||
| log.warn?.( | ||
| "COMBO", | ||
| `Reset-aware quota fetch failed for connection=${target.connectionId}: ${error instanceof Error ? error.message : String(error)}` | ||
| ); | ||
| } | ||
| } | ||
| const { score } = scoreResetAwareQuota(quota, config); | ||
| return { target, score, index }; | ||
| }) | ||
| ); |
There was a problem hiding this comment.
Using Promise.all to fetch quotas for all expandedTargets concurrently is efficient, but it could lead to a burst of network requests if a combo contains a large number of Codex accounts. Since fetchCodexQuota has a 60s cache, this is mostly an issue on the first request or after cache expiration. If the number of accounts is expected to be very high, consider implementing a concurrency limit for these background fetches.
| function getResetUrgency(resetAt: string | null | undefined, windowMs: number): number { | ||
| if (!resetAt) return 0.5; | ||
| const resetTime = new Date(resetAt).getTime(); | ||
| if (!Number.isFinite(resetTime)) return 0.5; | ||
| const msUntilReset = resetTime - Date.now(); | ||
| if (msUntilReset <= 0) return 1; | ||
| return clamp01(1 - msUntilReset / windowMs); | ||
| } |
There was a problem hiding this comment.
The getResetUrgency function relies on Date.now(), which uses the system clock. If the server's system time is significantly out of sync with the Codex provider's clock, the urgency calculation (and thus the routing) may be inaccurate. While codexQuotaFetcher.ts attempts to normalize resetAt using Date.now() when reset_after_seconds is provided, a direct comparison with system time is still a potential point of drift.
1 participant
Summary
reset-awareCombo routing strategy for Codex account pools.Related Issues
Validation
npm run lintnpm run test:unitnpm run test:coverage>= 60%for statements, lines, functions, and branchesSonarQube PR analysis was not run locally.
Tests Added Or Updated
Coverage Notes
82.78%75.13%85.28%82.78%Reviewer Notes
resetAwareSessionWeightresetAwareWeeklyWeightresetAwareTieBandPercentresetAwareExhaustionGuardPercentv22.22.2.