fix(accounts): invalidate runtime tracker key on removeAccount (HI-01)#419
Conversation
getRuntimeTrackerKey caches the resolved tracker key on the account via _runtimeTrackerKey. For accounts without accountId/email, the key falls back to the numeric account.index. When removeAccount splices the pool and reindexes survivors (acc.index = index), the cached numeric key is not invalidated, so later lookups keep consulting the pre-reindex position. Rotation, health, and token-bucket state for that account all mismatch the current pool order. Clear _runtimeTrackerKey on each survivor whose cached key was numeric, right after the reindex loop. Identity-based string keys remain stable because accountId/email are not affected by array-position changes, so they do not need invalidation. Regression test exercises the remove-then-select path: a pool of four accounts (mixed refresh-only and identity-bearing) primes the runtime tracker, records a health failure under the pre-reindex key, removes the identity-bearing account at index 1, and asserts the surviving refresh-only account reports its new numeric index (1) instead of the stale cached value (2). The identity account's string key is verified to survive the reindex unchanged. Audit reference: .sisyphus/notepads/deep-audit/reports/accounts-rotation.json HI-01
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
📝 WalkthroughWalkthroughmodified Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
notes for review
🚥 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 docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 1335-1345: The numeric runtime-tracker cache
(acc._runtimeTrackerKey) is cleared on splice but lib/rotation.ts still keeps
health/token entries keyed by the old numeric indices, allowing a later
refresh-only survivor to inherit another account’s state; update the rotation
trackers (e.g., the health and token maps used in rotation.ts—referenced where
entries are read/removed around the functions handling rotation and health
checks) to also clear any entries whose numeric keys fall in the shifted index
range when you remove an account, or switch refresh-only accounts to a stable
per-account runtime id returned by getRuntimeAccountIdentityKey and use that
stable id as the tracker key; also add a vitest regression that removes one slot
with two refresh-only survivors after it and asserts no state bleed, update any
affected queues to handle EBUSY/429 retries, and ensure logging changes do not
leak tokens/emails.
In `@test/accounts.test.ts`:
- Around line 2914-2919: Update the test in test/accounts.test.ts to cover
numeric key reuse: make the post-removal survivor account (the one initialized
with refreshToken "token-d") a refresh-only account by removing/omitting its
email so it can shift into numeric slot 2, and keep a separate identity account
(with an email) for the existing string-key assertions; after the removal/shift
assert that the new numeric key 2 has pristine state (no stale health/token
entries) to catch the bleed case referenced in lib/accounts.ts:1335, and ensure
assertions remain deterministic under vitest (no real secret mocks or skipped
asserts).
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b0850a31-0480-40d0-a6ef-9e218505ba63
📒 Files selected for processing (2)
lib/accounts.tstest/accounts.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (2)
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/accounts.ts
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/accounts.test.ts
🔇 Additional comments (1)
lib/accounts.ts (1)
1319-1326: this type-only cleanup looks safe.lib/accounts.ts:1319 keeps the family pointer snapshots explicit without changing removeAccount behavior. no auth rotation or logging concern here.
Follow-up for PR #419 review comments. Invalidate cached numeric tracker keys was not sufficient on its own: the rotation health/token tracker maps still retained entries keyed by the old numeric slots, so a later refresh-only survivor could inherit another account's state after reindex. Fix: clear numeric-keyed tracker entries at or above the removed index for both HealthScoreTracker and TokenBucketTracker, then keep the regression test fully numeric by making the shifted survivor refresh-only and asserting both health and token state are pristine after reindex.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| getHealthTracker().clearNumericKeysAtOrAbove(idx); | ||
| getTokenTracker().clearNumericKeysAtOrAbove(idx); |
There was a problem hiding this comment.
clearNumericKeysAtOrAbove does not exist
clearNumericKeysAtOrAbove is called on both HealthScoreTracker and TokenBucketTracker here, but neither class defines this method in lib/rotation.ts. HealthScoreTracker (line 77) and TokenBucketTracker (line 198) expose only getScore/recordSuccess/recordRateLimit/recordFailure/reset/clear and getTokens/tryConsume/refundToken/drain/reset/clear respectively — no clearNumericKeysAtOrAbove on either.
This means:
- TypeScript strict-mode compilation rejects both calls (contradicting the "typecheck clean" claim).
- At runtime, every call to
removeAccountthrowsTypeError: getHealthTracker(...).clearNumericKeysAtOrAbove is not a function. - The regression test added in this PR cannot pass as written.
The method needs to be added to both classes in rotation.ts. Sketch of the required implementation — the internal key format is JSON.stringify([numericStringOrId, quotaKey ?? null]), so numeric-slot entries can be identified and pruned by threshold:
// HealthScoreTracker — add inside the class body
clearNumericKeysAtOrAbove(threshold: number): void {
for (const key of this.entries.keys()) {
const parsed = JSON.parse(key) as [string, string | null];
const n = Number(parsed[0]);
if (Number.isInteger(n) && String(n) === parsed[0] && n >= threshold) {
this.entries.delete(key);
}
}
}
// TokenBucketTracker — identical shape, targets this.buckets
clearNumericKeysAtOrAbove(threshold: number): void {
for (const key of this.buckets.keys()) {
const parsed = JSON.parse(key) as [string, string | null];
const n = Number(parsed[0]);
if (Number.isInteger(n) && String(n) === parsed[0] && n >= threshold) {
this.buckets.delete(key);
}
}
}Without this, the HI-01 fix is incomplete and breaks all account removal paths.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/accounts.ts
Line: 1336-1337
Comment:
**missing method definition — `clearNumericKeysAtOrAbove` does not exist**
`clearNumericKeysAtOrAbove` is called on both `HealthScoreTracker` and `TokenBucketTracker` here, but neither class defines this method in `lib/rotation.ts`. `HealthScoreTracker` (line 77) and `TokenBucketTracker` (line 198) expose only `getScore/recordSuccess/recordRateLimit/recordFailure/reset/clear` and `getTokens/tryConsume/refundToken/drain/reset/clear` respectively — no `clearNumericKeysAtOrAbove` on either.
This means:
- TypeScript strict-mode compilation rejects both calls (contradicting the "typecheck clean" claim).
- At runtime, every call to `removeAccount` throws `TypeError: getHealthTracker(...).clearNumericKeysAtOrAbove is not a function`.
- The regression test added in this PR cannot pass as written.
The method needs to be added to both classes in `rotation.ts`. Sketch of the required implementation — the internal key format is `JSON.stringify([numericStringOrId, quotaKey ?? null])`, so numeric-slot entries can be identified and pruned by threshold:
```typescript
// HealthScoreTracker — add inside the class body
clearNumericKeysAtOrAbove(threshold: number): void {
for (const key of this.entries.keys()) {
const parsed = JSON.parse(key) as [string, string | null];
const n = Number(parsed[0]);
if (Number.isInteger(n) && String(n) === parsed[0] && n >= threshold) {
this.entries.delete(key);
}
}
}
// TokenBucketTracker — identical shape, targets this.buckets
clearNumericKeysAtOrAbove(threshold: number): void {
for (const key of this.buckets.keys()) {
const parsed = JSON.parse(key) as [string, string | null];
const n = Number(parsed[0]);
if (Number.isInteger(n) && String(n) === parsed[0] && n >= threshold) {
this.buckets.delete(key);
}
}
}
```
Without this, the HI-01 fix is incomplete and breaks all account removal paths.
How can I resolve this? If you propose a fix, please make it concise.
The rollup integration exposed that PR #419's branch updates accounts.ts to call HealthScoreTracker.clearNumericKeysAtOrAbove() and TokenBucketTracker.clearNumericKeysAtOrAbove(), but the helper methods never landed in lib/rotation.ts on that branch. Add the two tiny helper methods directly on the rollup branch so the full integration set typechecks and the rollup PR is self-contained. PR #419 itself should receive the same patch before final merge.
Merge the reviewed rollup fix branch after deep audit, full verification battery, and real pack/install smoke test. This lands the integrated fix set from PRs #414-#429 and #431 via one reviewed branch, including the two rollup-only integration commits: - 7f50455 fix(rollup): add missing numeric tracker cleanup helpers from PR #419 - 056ad18 test(rollup): restore auth-list empty-state expectation to current behavior Rationale: the rollup branch was the only branch deep-audited end-to-end and pack/install smoke-tested as a complete artifact. Baseline failures in codex-bin-wrapper / benchmark-runtime-path-script already exist on main and are not regressions from this merge.
1 participant
Summary
.sisyphus/notepads/deep-audit/reports/accounts-rotation.json:_runtimeTrackerKeycached to a numeric index goes stale afterremoveAccountreindex.removeAccount.accountId/emailare not affected by array-position changes.Why this matters
getRuntimeTrackerKeycaches the resolved key onaccount._runtimeTrackerKey. When an account has noaccountId/email,getRuntimeAccountIdentityKeyfalls back toaccount.index. AfterremoveAccountsplices the pool and reindexes survivors withacc.index = index, the cached numeric key is never invalidated — subsequent rotation, health, and token-bucket lookups all consult the stale (pre-reindex) position.Test
Added
invalidates numeric runtime tracker key after removeAccount reindex (HI-01)totest/accounts.test.tsunder theactive-account pointer dangledescribe. The test:_runtimeTrackerKeyon both a refresh-only account (index 2) and an identity account (index 3).recordFailuredegradation under the pre-reindex numeric key and captures the health score.Validation
npm test -- accounts rotation→ 276/276 passing (9 files).npm test(full suite) → 3530/3530 passing (232 files). Test count delta inaccounts.test.ts: 144 → 145 (+1).npm run typecheck→ clean.npm run lint→ clean.Constraints honoured
as any,@ts-ignore,@ts-expect-error.origin/mainat3f1c1fe(currentorigin/main; audit was authored against1f6da97but the_runtimeTrackerKeycaching logic is identical at both points).Audit reference:
.sisyphus/notepads/deep-audit/reports/accounts-rotation.jsonHI-01.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 fixes HI-01 by invalidating the cached
_runtimeTrackerKeyon numeric-keyed accounts afterremoveAccountreindexes the pool, and adds a follow-up commit that also purges stale numeric tracker entries viaclearNumericKeysAtOrAbove. the_runtimeTrackerKeyinvalidation in theforEachloop is correct, but the follow-up commit callsgetHealthTracker().clearNumericKeysAtOrAbove(idx)andgetTokenTracker().clearNumericKeysAtOrAbove(idx)— a method that does not exist on eitherHealthScoreTrackerorTokenBucketTrackerinlib/rotation.ts.clearNumericKeysAtOrAboveis missing from both tracker classes; everyremoveAccountcall will throwTypeErrorat runtime and fail TypeScript strict compilation, making the fix net-broken despite the correct intent.Confidence Score: 2/5
not safe to merge — every account removal throws TypeError at runtime due to missing method definition
the P0 missing-method bug makes the entire
removeAccountpath throw unconditionally; the correct part of the fix (numeric key invalidation in forEach) is sound but unreachable untilclearNumericKeysAtOrAboveis added to both tracker classes in rotation.tslib/rotation.ts needs
clearNumericKeysAtOrAbove(threshold: number): voidadded to bothHealthScoreTrackerandTokenBucketTrackerImportant Files Changed
clearNumericKeysAtOrAbove+_runtimeTrackerKeyinvalidation inremoveAccount, butclearNumericKeysAtOrAboveis not defined on either tracker class — runtime TypeError on every account removaltoken-dfixture block has inconsistent indentation and the test cannot currently pass due to the missing method in rotation.tsSequence Diagram
sequenceDiagram participant Caller participant AccountManager participant HealthScoreTracker participant TokenBucketTracker participant accounts as accounts[] Caller->>AccountManager: removeAccount(account) AccountManager->>accounts: splice(idx, 1) AccountManager->>HealthScoreTracker: clearNumericKeysAtOrAbove(idx) ❌ undefined Note over HealthScoreTracker: TypeError — method missing in rotation.ts AccountManager->>TokenBucketTracker: clearNumericKeysAtOrAbove(idx) ❌ undefined AccountManager->>accounts: forEach → acc.index = newIndex Note over accounts: if _runtimeTrackerKey is numeric → delete (✅ correct) AccountManager-->>Caller: true (never reached)Prompt To Fix All With AI
Reviews (2): Last reviewed commit: "fix(accounts): clear shifted numeric tra..." | Re-trigger Greptile