Open
Conversation
Add a request-scoped selector so retries exclude attempted accounts, respect local token-bucket availability, and return retryable waits when local capacity is temporarily exhausted. Co-authored-by: Codex <noreply@openai.com>
Compute per-account availability waits before taking the pool minimum, and tighten 5xx and empty-response tests to assert that retries advance to a different account. Co-authored-by: Codex <noreply@openai.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAccount rotation now uses an attempted-account-key aware selector ( Changes
Sequence DiagramsequenceDiagram
participant Client
participant Retry as RetryLogic (index.ts)
participant AM as AccountManager
participant Snap as AvailabilitySnapshot
participant TBT as TokenBucketTracker
Client->>Retry: Request (model, attemptedAccountKeys)
Retry->>AM: getNextRequestEligibleForFamilyHybrid(family, model, { attemptedAccountKeys, pidOffsetEnabled })
AM->>Snap: evaluate candidate account
Snap->>TBT: getWaitTimeUntilTokenAvailable(candidateIndex, quotaKey)
TBT-->>Snap: waitTime / tokensAvailable
Snap-->>AM: eligible / rateLimited / waitTime
alt eligible account found
AM-->>Retry: ManagedAccount
Retry->>Client: Execute request with account
else no eligible accounts
AM-->>Retry: null
Retry->>Client: Return rate-limited / retry info
else token depleted for candidate
Retry->>Retry: continue to next candidate (add key to attemptedAccountKeys)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
index.ts (1)
2260-2270:⚠️ Potential issue | 🟠 MajorDon’t treat a local bucket miss like a real 429.
consumeToken()has already proven the local bucket is empty. CallingrecordRateLimit()here penalizes the account a second time by lowering health and draining the bucket again, so concurrent misses can keep pushing recovery out and bias hybrid selection away from an otherwise healthy account. This path should rotate without recording a server-side rate limit.Suggested fix
const tokenConsumed = accountManager.consumeToken(account, modelFamily, model); if (!tokenConsumed) { - accountManager.recordRateLimit(account, modelFamily, model); runtimeMetrics.accountRotations++; runtimeMetrics.lastError = `Local token bucket depleted for account ${account.index + 1} (${modelFamily}${model ? `:${model}` : ""})`; runtimeMetrics.lastErrorCategory = "rate-limit-local"; logWarn( `Skipping account ${account.index + 1}: local token bucket depleted for ${modelFamily}${model ? `:${model}` : ""}`, ); continue; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@index.ts` around lines 2260 - 2270, The local token-bucket miss path should not record a server-side 429: in the branch where accountManager.consumeToken(account, modelFamily, model) returns false, remove the call to accountManager.recordRateLimit(account, modelFamily, model) so we don't penalize health or drain tokens again; instead simply increment runtimeMetrics.accountRotations, set runtimeMetrics.lastError and lastErrorCategory ("rate-limit-local"), log the warning, and continue rotation (preserving the existing logWarn and continue behavior).test/index.test.ts (1)
2289-2313:⚠️ Potential issue | 🟡 MinorAssert the computed wait, not just the generic 429.
This test injects a
12_000ms family wait, but never verifies that the handler actually surfaces that delay. It will still pass ifgetMinWaitTimeForFamily()is ignored and the code falls back to a generic “all accounts are rate-limited” response, which leaves the new pool-min wait logic unprotected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/index.test.ts` around lines 2289 - 2313, The test currently verifies only a generic 429 message but must assert the computed family wait (12_000 ms) is surfaced; update the assertion after calling sdk.fetch (and before/after checking response.status) to verify the handler includes the wait returned by getMinWaitTimeForFamily — e.g., check response.headers.get("Retry-After") or that await response.text() contains the "12000" (or equivalent seconds value) returned by createRequestAwareManager/getMinWaitTimeForFamily so the new pool-min wait logic is actually exercised.
🧹 Nitpick comments (1)
test/index-retry.test.ts (1)
58-60: HonorattemptedIndicesin this mock.Right now this stub behaves like the old selector, so a retry path that accidentally reuses a tried account can still look green in this suite.
Proposed mock adjustment
- getNextRequestEligibleForFamilyHybrid() { - return this.getCurrentOrNextForFamily(); + getNextRequestEligibleForFamilyHybrid( + _family?: unknown, + _model?: unknown, + options?: { attemptedIndices?: ReadonlySet<number> }, + ) { + if (options?.attemptedIndices?.has(0)) return null; + return this.getCurrentOrNextForFamily(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/index-retry.test.ts` around lines 58 - 60, The mock's getNextRequestEligibleForFamilyHybrid currently just delegates to getCurrentOrNextForFamily and ignores the attemptedIndices tracking, so update the mock implementation of getNextRequestEligibleForFamilyHybrid to consult the mock's attemptedIndices set/array and skip any indices already present (mirroring the real selector's retry behavior) before returning a candidate; if no unattempted candidate exists return the same null/undefined path the real getCurrentOrNextForFamily would. Ensure you reference the mock's attemptedIndices and the methods getNextRequestEligibleForFamilyHybrid and getCurrentOrNextForFamily when making the change so tests cannot reuse already-tried accounts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/accounts.test.ts`:
- Around line 959-1010: The tests calling getTokenTracker().drain(...) leak
process-global tracker state; wrap the describe("getMinWaitTimeForFamily", ...)
tests (or each test) with cleanup so token trackers are reset between tests:
call resetTrackers() (or otherwise reinitialize the global tracker) in a
beforeEach/afterEach surrounding the AccountManager-based tests (or add inline
resetTrackers() at the start/end of each test) to ensure getTokenTracker(),
AccountManager, and getMinWaitTimeForFamily() run with a clean tracker state and
avoid order-dependent failures.
In `@test/index.test.ts`:
- Around line 2529-2538: The mock wrapper getNextRequestEligibleForFamilyHybrid
currently returns null if the single candidate from
customManager.getCurrentOrNextForFamilyHybrid(family, currentModel) is in
attemptedIndices, which incorrectly stops traversal; change it to loop: obtain
attemptedIndices as before, then repeatedly call
customManager.getCurrentOrNextForFamilyHybrid(family, currentModel) and if
candidate is null return null, if candidate.index is in attemptedIndices
continue the loop (advancing the underlying cursor) until you find a candidate
whose index is not in attemptedIndices, then return that candidate; keep the
same parameter names (family, currentModel, options) and use attemptedIndices
and customManager for identification.
---
Outside diff comments:
In `@index.ts`:
- Around line 2260-2270: The local token-bucket miss path should not record a
server-side 429: in the branch where accountManager.consumeToken(account,
modelFamily, model) returns false, remove the call to
accountManager.recordRateLimit(account, modelFamily, model) so we don't penalize
health or drain tokens again; instead simply increment
runtimeMetrics.accountRotations, set runtimeMetrics.lastError and
lastErrorCategory ("rate-limit-local"), log the warning, and continue rotation
(preserving the existing logWarn and continue behavior).
In `@test/index.test.ts`:
- Around line 2289-2313: The test currently verifies only a generic 429 message
but must assert the computed family wait (12_000 ms) is surfaced; update the
assertion after calling sdk.fetch (and before/after checking response.status) to
verify the handler includes the wait returned by getMinWaitTimeForFamily — e.g.,
check response.headers.get("Retry-After") or that await response.text() contains
the "12000" (or equivalent seconds value) returned by
createRequestAwareManager/getMinWaitTimeForFamily so the new pool-min wait logic
is actually exercised.
---
Nitpick comments:
In `@test/index-retry.test.ts`:
- Around line 58-60: The mock's getNextRequestEligibleForFamilyHybrid currently
just delegates to getCurrentOrNextForFamily and ignores the attemptedIndices
tracking, so update the mock implementation of
getNextRequestEligibleForFamilyHybrid to consult the mock's attemptedIndices
set/array and skip any indices already present (mirroring the real selector's
retry behavior) before returning a candidate; if no unattempted candidate exists
return the same null/undefined path the real getCurrentOrNextForFamily would.
Ensure you reference the mock's attemptedIndices and the methods
getNextRequestEligibleForFamilyHybrid and getCurrentOrNextForFamily when making
the change so tests cannot reuse already-tried accounts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4116a0c8-1761-42af-92e5-95ff561e1dd4
📒 Files selected for processing (7)
index.tslib/accounts.tslib/rotation.tstest/accounts.test.tstest/index-retry.test.tstest/index.test.tstest/rotation-integration.test.ts
Handle disabled token refill waits safely, stop double-penalizing local bucket misses, and tighten rotation mocks and wait assertions around the new request-scoped selector. Co-authored-by: Codex <noreply@openai.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 8 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="test/accounts.test.ts">
<violation number="1" location="test/accounts.test.ts:1041">
P2: Mutating `tracker.config.tokensPerMinute = 0` leaks to all subsequent tests because `resetTrackers()` only clears bucket entries, not the config on the singleton instance. Restore the original value after the test to prevent silent cross-test contamination.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/accounts.ts`:
- Around line 423-435: getAccountAvailabilitySnapshot incorrectly uses the
mutable account.index as the token-bucket identity (via
tokenTracker.getTokens(account.index, quotaKey)), which causes bucket/wait state
to leak when removeAccount renumbers indices; change the tracker key to a stable
per-account identifier (e.g., account.id or another immutable unique field)
instead of account.index, and update all token-tracker usages (getTokens,
get/set wait time, etc.) and getTokenTracker initialization to use that stable
key; alternatively, if adding a stable id is not possible, ensure removeAccount
remaps or clears token-tracker state for shifted indices (adjusting
getMinWaitTimeForFamily and related functions accordingly) so state does not
follow the new occupant.
In `@test/index.test.ts`:
- Around line 304-306: The mock implementation of
getNextRequestEligibleForFamilyHybrid returns accounts[0] unconditionally and
ignores attemptedIndices, which can mask traversal/retry bugs; update the mock
(getNextRequestEligibleForFamilyHybrid) to consult attemptedIndices and return
null if index 0 is present in attemptedIndices (otherwise return accounts[0]),
so even the single-account default mocks the real contract's behavior of not
reusing already-attempted indices.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d8e5e2e0-47ae-42ac-a6eb-0aaa1d38f0ab
📒 Files selected for processing (6)
index.tslib/accounts.tstest/accounts.test.tstest/index-retry.test.tstest/index.test.tstest/rotation.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- index.ts
Clear index-keyed tracker state when accounts are renumbered, harden the default request-selector mocks, and extend token wait coverage for the final review findings. Co-authored-by: Codex <noreply@openai.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/accounts.ts (1)
949-956:⚠️ Potential issue | 🟡 MinorRemap toast debounce state when indices shift.
lastToastAccountIndexis also keyed byaccount.index. Aftersplice()+ reindexing, the new occupant of that index inherits the previous account's debounce window, so account-switch toasts can be suppressed for the wrong account.Suggested fix
this.accounts.splice(idx, 1); this.accounts.forEach((acc, index) => { acc.index = index; }); + if (this.lastToastAccountIndex === idx) { + this.lastToastAccountIndex = -1; + } else if (this.lastToastAccountIndex > idx) { + this.lastToastAccountIndex -= 1; + } // Trackers are keyed by account index, so any removal that renumbers accounts // must clear in-memory tracker state to avoid leaking waits/health to new occupants. resetTrackers();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/accounts.ts` around lines 949 - 956, After removing an account and reindexing via this.accounts.splice(...) and the forEach that sets acc.index, also update the debounce map lastToastAccountIndex so entries keyed by numeric indices are shifted down to reflect the removed index (so new occupants don't inherit another account's debounce). Implement this by iterating the existing lastToastAccountIndex keys, for any key > removedIdx move its value to key-1 (delete the old key), delete the removedIdx entry, and then call resetTrackers() as before; reference this.accounts.splice, acc.index reassigning, lastToastAccountIndex, and resetTrackers() when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/accounts.ts`:
- Around line 662-723: The retry bookkeeping uses the mutable array index
(account.index) which breaks when removeAccount renumbers entries; switch
attemptedIndices to use a stable per-account key (e.g., account.id or a new
immutable property like account.uid) instead of account.index: update the
map/filter that builds accountsWithMetrics to test and return that stable key,
update where selections are marked as attempted to add the stable key, and
ensure selectHybridAccount is invoked/updated to accept and return that stable
key (then map back to the current array index to set
currentAccountIndexByFamily, cursorByFamily and account.lastUsed); also update
any early-path checks (the currentIndex fast-path) to compare attemptedIndices
against the stable key rather than account.index.
---
Outside diff comments:
In `@lib/accounts.ts`:
- Around line 949-956: After removing an account and reindexing via
this.accounts.splice(...) and the forEach that sets acc.index, also update the
debounce map lastToastAccountIndex so entries keyed by numeric indices are
shifted down to reflect the removed index (so new occupants don't inherit
another account's debounce). Implement this by iterating the existing
lastToastAccountIndex keys, for any key > removedIdx move its value to key-1
(delete the old key), delete the removedIdx entry, and then call resetTrackers()
as before; reference this.accounts.splice, acc.index reassigning,
lastToastAccountIndex, and resetTrackers() when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: da1c572f-2d40-4c72-9361-73cbb3ed6851
📒 Files selected for processing (4)
lib/accounts.tstest/accounts.test.tstest/index.test.tstest/rotation.test.ts
Switch request attempt bookkeeping to a stable per-account key, clear toast debounce state on renumbering, and add the remaining review-driven regression coverage. Co-authored-by: Codex <noreply@openai.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/accounts.test.ts (1)
1036-1050: Avoid mutating internal tracker state in tests; pass configuration at construction instead.The test directly mutates
tracker.config.tokensPerMinuteand relies on try/finally to restore it. SincegetTokenTrackeraccepts an optional config parameter, usegetTokenTracker({ tokensPerMinute: 0 })to configure the tracker at construction, eliminating the need for internal state mutation and restoration.🔧 Suggested approach
const manager = new AccountManager(undefined, stored as never); - const tracker = getTokenTracker() as unknown as { - config: { tokensPerMinute: number }; - drain: (accountIndex: number, quotaKey?: string, drainAmount?: number) => void; - }; - const originalTokensPerMinute = tracker.config.tokensPerMinute; - try { - tracker.config.tokensPerMinute = 0; - tracker.drain(0, "codex", 50); + resetTrackers(); + const tracker = getTokenTracker({ tokensPerMinute: 0 }) as unknown as { + drain: (accountIndex: number, quotaKey?: string, drainAmount?: number) => void; + }; + tracker.drain(0, "codex", 50); - const waitTime = manager.getMinWaitTimeForFamily("codex"); - expect(waitTime).toBe(Number.MAX_SAFE_INTEGER); - } finally { - tracker.config.tokensPerMinute = originalTokensPerMinute; - } + const waitTime = manager.getMinWaitTimeForFamily("codex"); + expect(waitTime).toBe(Number.MAX_SAFE_INTEGER);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/accounts.test.ts` around lines 1036 - 1050, The test mutates internal tracker state by setting tracker.config.tokensPerMinute directly; instead construct the tracker with the desired config via getTokenTracker({ tokensPerMinute: 0 }) and use that instance for tracker.drain and assertions so you don't need the try/finally restore. Update the test to create the tracker with getTokenTracker({ tokensPerMinute: 0 }) and then call tracker.drain(0, "codex", 50) and verify manager.getMinWaitTimeForFamily("codex") returns Number.MAX_SAFE_INTEGER, referencing AccountManager, getTokenTracker, tracker.drain and manager.getMinWaitTimeForFamily.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/accounts.ts`:
- Around line 219-225: The request-attempt key uses mutable auth fields causing
mismatches; change getRequestAttemptKey to use a stable per-account identifier
instead (add a new immutableId on ManagedAccount assigned once when the account
object is created/loaded, e.g., a UUID) and include that immutableId (plus
organizationId if desired) in the key instead of refreshToken/accountId; also
ensure all account construction/loading code sets immutableId when missing so
attemptedAccountKeys.has(getRequestAttemptKey(account)) continues to match even
after updateFromAuth() rotates tokens or accountId is hydrated.
---
Nitpick comments:
In `@test/accounts.test.ts`:
- Around line 1036-1050: The test mutates internal tracker state by setting
tracker.config.tokensPerMinute directly; instead construct the tracker with the
desired config via getTokenTracker({ tokensPerMinute: 0 }) and use that instance
for tracker.drain and assertions so you don't need the try/finally restore.
Update the test to create the tracker with getTokenTracker({ tokensPerMinute: 0
}) and then call tracker.drain(0, "codex", 50) and verify
manager.getMinWaitTimeForFamily("codex") returns Number.MAX_SAFE_INTEGER,
referencing AccountManager, getTokenTracker, tracker.drain and
manager.getMinWaitTimeForFamily.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ac5d9c8b-77d8-48e4-be27-943f0ec2197b
📒 Files selected for processing (7)
index.tslib/accounts.tslib/rotation.tstest/accounts.test.tstest/index-retry.test.tstest/index.test.tstest/rotation-integration.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/index-retry.test.ts
There was a problem hiding this comment.
2 issues found across 7 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="lib/accounts.ts">
<violation number="1" location="lib/accounts.ts:221">
P1: Attempted-account keys are derived from mutable account fields, so a refresh can change the key mid-request and let the same account bypass attempted-key filtering.</violation>
</file>
<file name="test/rotation-integration.test.ts">
<violation number="1" location="test/rotation-integration.test.ts:176">
P2: The new assertion is too weak: `toBeGreaterThan(0)` can pass from immediate fractional refill even when tracker state was not actually cleared after renumbering.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Use a per-account immutable id for request attempt tracking so token refresh and account-id hydration cannot let the same account bypass attempted-key filtering within one request. Co-authored-by: Codex <noreply@openai.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/accounts.ts (1)
685-707: Simplify the IIFE check for current account eligibility.The inline IIFE on lines 688-691 obscures the intent. A simpler pattern would improve readability.
♻️ Suggested simplification
- if ( - currentIndex >= 0 && - currentIndex < count && - ((currentAccount) => - !!currentAccount && !attemptedAccountKeys.has(getRequestAttemptKey(currentAccount)))( - this.accounts[currentIndex], - ) - ) { - const currentAccount = this.accounts[currentIndex]; + const currentAccount = currentIndex >= 0 && currentIndex < count + ? this.accounts[currentIndex] + : undefined; + if ( + currentAccount && + !attemptedAccountKeys.has(getRequestAttemptKey(currentAccount)) + ) { if (currentAccount) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/accounts.ts` around lines 685 - 707, The code uses an unnecessary IIFE to test this.accounts[currentIndex] and attemptedAccountKeys — replace that IIFE with a direct, readable check: retrieve const currentAccount = this.accounts[currentIndex] and use if (currentIndex >= 0 && currentIndex < count && currentAccount && !attemptedAccountKeys.has(getRequestAttemptKey(currentAccount))) { ... } then keep the existing body that calls getAccountAvailabilitySnapshot(currentAccount, family, model, tokenTracker), sets currentAccount.lastUsed = nowMs() when eligible, or assigns alreadyCheckedCurrentIndex = currentIndex otherwise; remove the IIFE wrapper entirely.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/accounts.ts`:
- Around line 685-707: The code uses an unnecessary IIFE to test
this.accounts[currentIndex] and attemptedAccountKeys — replace that IIFE with a
direct, readable check: retrieve const currentAccount =
this.accounts[currentIndex] and use if (currentIndex >= 0 && currentIndex <
count && currentAccount &&
!attemptedAccountKeys.has(getRequestAttemptKey(currentAccount))) { ... } then
keep the existing body that calls getAccountAvailabilitySnapshot(currentAccount,
family, model, tokenTracker), sets currentAccount.lastUsed = nowMs() when
eligible, or assigns alreadyCheckedCurrentIndex = currentIndex otherwise; remove
the IIFE wrapper entirely.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bd0f7dcb-fa93-4b73-abc8-3a96d48ded7b
📒 Files selected for processing (2)
lib/accounts.tstest/rotation-integration.test.ts
Simplify the current-account fast path and extend the removal reset test to assert both token and health tracker reset behavior for surviving accounts. Co-authored-by: Codex <noreply@openai.com>
Reindex health and token tracker entries after account removal so surviving accounts keep their own state instead of being globally reset. Co-authored-by: Codex <noreply@openai.com>
ndycode
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens request-time multi-account rotation by introducing a dedicated request-eligible selector, stabilizing per-request attempt identity, and improving wait-time and tracker behavior across account removal/renumbering.
Changes:
- Add stable per-account request attempt keys and a request-time selector that skips already-attempted accounts.
- Improve wait-time computation by including local token bucket recovery and per-account blocker maxima.
- Reindex token/health trackers (and toast debounce state) when accounts are removed and indices are renumbered; expand regression coverage for rotation and retry paths.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
lib/rotation.ts |
Adds token wait helper + tracker reindexing after account removal. |
lib/accounts.ts |
Adds immutable account identity + request-eligible selector + updated wait computation + tracker reindex hook on removal. |
index.ts |
Switches fetch loop to request-eligible selector, uses attempted-key set, and continues traversal on local token depletion. |
test/rotation.test.ts |
Adds unit coverage for token wait-time helper. |
test/accounts.test.ts |
Adds coverage for token-refill wait, mixed blockers, and disabled-refill semantics. |
test/index.test.ts |
Expands request-time rotation coverage (network/5xx/empty/local-bucket/wait-path) and updates mocks for attempted keys. |
test/index-retry.test.ts |
Updates retry mock to honor attempted keys. |
test/rotation-integration.test.ts |
Adds integration coverage for attempted-key skipping and tracker reset after renumbering. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lib/accounts.ts
Outdated
| return waitTimes.length > 0 ? Math.min(...waitTimes) : 0; | ||
| if (waitTimes.length === 0) return 0; | ||
| const minWait = Math.min(...waitTimes); | ||
| return Number.isFinite(minWait) ? minWait : Number.MAX_SAFE_INTEGER; |
lib/accounts.ts
Outdated
Comment on lines
+450
to
+452
| const quotaKey = model ? `${family}:${model}` : family; | ||
| const tokensAvailable = tokenTracker.getTokens(account.index, quotaKey); | ||
| const eligible = enabled && !rateLimited && !coolingDown && tokensAvailable >= 1; |
Comment on lines
+2130
to
+2133
| vi.mocked(fetchHelpers.createCodexHeaders).mockImplementation( | ||
| (_init, _accountId, accessToken) => | ||
| new Headers({ authorization: `Bearer ${String(accessToken)}` }), | ||
| ); |
Comment on lines
+124
to
+136
| reindexAfterRemoval(removedIndex: number): void { | ||
| const nextEntries = new Map<string, HealthEntry>(); | ||
| for (const [key, entry] of this.entries) { | ||
| const [indexText, ...quotaParts] = key.split(":"); | ||
| const index = Number.parseInt(indexText ?? "", 10); | ||
| if (!Number.isFinite(index)) continue; | ||
| if (index === removedIndex) continue; | ||
| const nextIndex = index > removedIndex ? index - 1 : index; | ||
| const nextKey = quotaParts.length > 0 ? `${nextIndex}:${quotaParts.join(":")}` : `${nextIndex}`; | ||
| nextEntries.set(nextKey, entry); | ||
| } | ||
| this.entries = nextEntries; | ||
| } |
Comment on lines
+435
to
+437
| } | ||
|
|
||
| private getAccountAvailabilitySnapshot( |
There was a problem hiding this comment.
Math.POSITIVE_INFINITY can escape into waitTimes but is not guarded before Math.min
when tokenWaitMs is Number.POSITIVE_INFINITY (zero-refill bucket) the code correctly pushes it:
accountWaits.push(Number.isFinite(tokenWaitMs) ? tokenWaitMs : Number.POSITIVE_INFINITY);and Math.max(...accountWaits) → Infinity lands in waitTimes. that is the intended code path. however, the final Math.min(...waitTimes) will return Infinity if every account contributed only an infinite wait, and that value propagates to callers (handled in index.ts via hasFiniteWait). the risk is a future caller of getMinWaitTimeForFamily that does not guard against Infinity (e.g., uses the result directly as a setTimeout delay). setTimeout(fn, Infinity) on windows never fires, silently stalling the retry loop.
worth adding a doc-comment on getMinWaitTimeForFamily calling out the Infinity contract explicitly, so future callers are warned:
/**
* Returns the minimum wait time (ms) before any enabled account is expected to
* be available for `family`/`model`. Returns 0 if at least one account is
* immediately eligible. Returns `Number.POSITIVE_INFINITY` when all accounts
* are blocked by a zero-refill token bucket and no finite wait can be computed.
* Callers MUST guard against `Infinity` before passing the result to `setTimeout`.
*/
getMinWaitTimeForFamily(...)Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/accounts.ts
Line: 435-437
Comment:
**`Math.POSITIVE_INFINITY` can escape into `waitTimes` but is not guarded before `Math.min`**
when `tokenWaitMs` is `Number.POSITIVE_INFINITY` (zero-refill bucket) the code correctly pushes it:
```typescript
accountWaits.push(Number.isFinite(tokenWaitMs) ? tokenWaitMs : Number.POSITIVE_INFINITY);
```
and `Math.max(...accountWaits)` → `Infinity` lands in `waitTimes`. that is the intended code path. however, the final `Math.min(...waitTimes)` will return `Infinity` if every account contributed only an infinite wait, and that value propagates to callers (handled in `index.ts` via `hasFiniteWait`). the risk is a future caller of `getMinWaitTimeForFamily` that does not guard against `Infinity` (e.g., uses the result directly as a `setTimeout` delay). `setTimeout(fn, Infinity)` on windows never fires, silently stalling the retry loop.
worth adding a doc-comment on `getMinWaitTimeForFamily` calling out the `Infinity` contract explicitly, so future callers are warned:
```typescript
/**
* Returns the minimum wait time (ms) before any enabled account is expected to
* be available for `family`/`model`. Returns 0 if at least one account is
* immediately eligible. Returns `Number.POSITIVE_INFINITY` when all accounts
* are blocked by a zero-refill token bucket and no finite wait can be computed.
* Callers MUST guard against `Infinity` before passing the result to `setTimeout`.
*/
getMinWaitTimeForFamily(...)
```
How can I resolve this? If you propose a fix, please make it concise.
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
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.
Summary
Root cause
The fetch loop tracked attempted accounts, but the general hybrid selector could still legally return the current or fallback account. That contract mismatch let traversal terminate early after a repeated selection instead of moving to another eligible account. The remaining edge cases were that retry bookkeeping still used mutable values (
account.index, then later refresh/account fields), which could drift if the account pool was renumbered or the selected account refreshed mid-request.Final branch tip
fix/account-rotation-traversal9d6fdd0Validation
npm run lintnpm run typechecknpm testnpm run buildRegression coverage
index.ts:2260recordRateLimitis not called:test/index.test.ts:2192lib/accounts.ts:176,lib/accounts.ts:219,lib/accounts.ts:662,index.ts:2087max(...)then poolmin(...):lib/accounts.ts:933lib/accounts.ts:955test/accounts.test.ts:896test/rotation.test.ts:18612sdelay:test/index.test.ts:2289test/index.test.ts:304test/index-retry.test.ts:58test/rotation-integration.test.ts:159,test/rotation-integration.test.ts:171Windows concurrency and token redaction coverage
test/rotation-integration.test.ts:274,test/storage-async.test.ts:271test/logger.test.ts:744test/logger.test.ts:764Live check
opencode run "Reply with READY only." --model=openai/gpt-5-codex --variant medium --agent livecheckREADYReview status
note: greptile review for oc-chatgpt-multi-auth. cite files like
lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.Greptile Summary
this pr hardens the request-time account rotation loop by replacing the mutable-index
attemptedset with a stable per-accountimmutableIdkey, introducing a dedicatedgetNextRequestEligibleForFamilyHybridselector that filters attempted accounts before selection (not after), fixing abreak→continuethat was silently aborting traversal on local token bucket misses, and computing pool wait time asmin(per-account max(...))rather than a flat pool-wide min — which correctly surfaces the earliest-available account. tracker reindexing on account removal and toast-index remapping are also addressed. regression coverage is thorough across network errors, 5xx, empty-response, consumed-token races, indefinite-wait, and windows-oriented reindexing paths.key changes:
attempted: Set<number>→Set<string>keyed onaccount.immutableId, immune to index drift from removals or token refreshgetNextRequestEligibleForFamilyHybridpre-filters attempted and ineligible accounts so the selector can never return a repeated or unavailable accountconsumeTokenmiss on the async window nowcontinues to the next account instead of breaking out of the loop;recordRateLimitis no longer called for local bucket misses (correct — only remote 429s earn that penalty)getMinWaitTimeForFamilyincludes token-bucket recovery time and propagatesNumber.POSITIVE_INFINITYfor zero-refill configs;index.tsguards this withhasFiniteWaitto prevent sleeping indefinitelyreindexAfterRemovalon bothHealthScoreTrackerandTokenBucketTrackerkeeps per-account history correctly aligned after pool renumbering — directly addresses the windows antivirus-triggered removal scenarioimmutableIdis transient (regenerated per plugin load), which is safe becauseattemptedis local to a single request cycle and never persistedConfidence Score: 4/5
Infinitycontract ongetMinWaitTimeForFamilyworth noting before mergeInfinityreturn contract ongetMinWaitTimeForFamilythat future callers could misuse withsetTimeout. no data-loss or correctness regressions identified.getMinWaitTimeForFamilycan returnNumber.POSITIVE_INFINITY; callers must guard before passing tosetTimeoutor similar timer APIsImportant Files Changed
immutableId,getNextRequestEligibleForFamilyHybrid,getAccountAvailabilitySnapshot, and tracker reindexing on removal; logic is sound butgetMinWaitTimeForFamilycan returnInfinitywith no contract doc for callersreindexAfterRemovalto both trackers andgetWaitTimeUntilTokenAvailabletoTokenBucketTracker; key-parsing logic for colon-delimited quota keys is correct and covers model-qualified keysattemptedtoSet<string>, replaces the broken hybrid selector withgetNextRequestEligibleForFamilyHybrid, fixesbreak→continueand removes erroneousrecordRateLimiton local bucket miss, and guards infinite wait from triggeringsetTimeoutgetWaitTimeUntilTokenAvailableandreindexAfterRemovalunit tests; one test description ("partially drained") is misleading — the drain call fully empties the bucketbeforeEach/afterEachtracker resets, newgetMinWaitTimeForFamilytests (local recovery, mixed blockers, zero-refill), and threegetNextRequestEligibleForFamilyHybridunit tests covering skip, ineligible-current, and null-fallback pathsgetNextRequestEligibleForFamilyHybridandgetRequestAttemptKey; addscreateRequestAwareManagerhelper and six new integration tests covering network error, 5xx, empty-response, consumeToken race, indefinite wait, and local bucket depletion rotationgetNextRequestEligibleForFamilyHybridandgetRequestAttemptKeythat honor attempted-key semantics; aligns retry test mock with the new request-selector contractSequence Diagram
sequenceDiagram participant FetchLoop as fetch loop participant Selector as getNextRequestEligibleForFamilyHybrid participant Snapshot as getAccountAvailabilitySnapshot participant TokenTracker as TokenBucketTracker participant Manager as AccountManager FetchLoop->>Selector: call with family, model, attemptedKeys Selector->>Snapshot: check current account eligibility Snapshot->>TokenTracker: getTokens(account.index, quotaKey) Snapshot-->>Selector: eligible, tokensAvailable, rateLimited, coolingDown alt eligible and not in attemptedKeys Selector-->>FetchLoop: return current account else ineligible or already attempted Selector->>Snapshot: check each remaining candidate Selector->>Selector: selectHybridAccount on eligible subset Selector-->>FetchLoop: return selected account or null end FetchLoop->>Manager: getRequestAttemptKey returns immutableId FetchLoop->>Manager: consumeToken alt token consumed FetchLoop->>FetchLoop: execute fetch request else token lost to async race FetchLoop->>FetchLoop: continue to next account end FetchLoop->>Manager: getMinWaitTimeForFamily Manager->>TokenTracker: getWaitTimeUntilTokenAvailable per account Manager-->>FetchLoop: min of per-account max or Infinity alt hasFiniteWait and waitMs gt 0 FetchLoop->>FetchLoop: sleep then retry else Infinity or zero FetchLoop-->>FetchLoop: surface indefinitely message or continue endPrompt To Fix All With AI
Last reviewed commit: 332c319
Context used: