Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 24 additions & 2 deletions lib/accounts.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1316,16 +1316,38 @@ export class AccountManager {

// Snapshot family pointers before splice so we can distinguish "was
// pointing at the removed account" from "was pointing past it".
const priorCursor: Record<ModelFamily, number> = {} as Record<ModelFamily, number>;
const priorActive: Record<ModelFamily, number> = {} as Record<ModelFamily, number>;
const priorCursor: Record<ModelFamily, number> = {} as Record<
ModelFamily,
number
>;
const priorActive: Record<ModelFamily, number> = {} as Record<
ModelFamily,
number
>;
for (const family of MODEL_FAMILIES) {
priorCursor[family] = this.cursorByFamily[family];
priorActive[family] = this.currentAccountIndexByFamily[family];
}

this.accounts.splice(idx, 1);
// Clear numeric-keyed tracker state in the shifted range. After reindex,
// any refresh-only account that moved from N to N-1 must not inherit the
// stale health/token entries that used to belong to the old numeric slot.
getHealthTracker().clearNumericKeysAtOrAbove(idx);
getTokenTracker().clearNumericKeysAtOrAbove(idx);
Comment on lines +1336 to +1337

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P0 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:

// 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.

Fix in Codex

this.accounts.forEach((acc, index) => {
acc.index = index;
// Invalidate the cached runtime tracker key when it was keyed by
// numeric index (fallback path in getRuntimeAccountIdentityKey).
// After the splice+reindex above, a remaining account that was at
// index N (e.g. 3) may now live at index N-1 (e.g. 2); if we keep
// the previously cached numeric key, rotation/health/token state
// queries would consult the stale position and mismatch the
// current one. Identity-based string keys remain stable because
// accountId/email are not affected by array position changes.
if (typeof acc._runtimeTrackerKey === "number") {
acc._runtimeTrackerKey = undefined;
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
});

if (this.accounts.length === 0) {
Expand Down
115 changes: 115 additions & 0 deletions test/accounts.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2888,6 +2888,121 @@ describe("AccountManager", () => {
expect(otherAccount).not.toBeNull();
expect(otherAccount?.enabled).not.toBe(false);
});

it("invalidates numeric runtime tracker key after removeAccount reindex (HI-01)", () => {
// Regression: _runtimeTrackerKey was cached to the numeric
// account.index when no accountId/email was present. After
// removeAccount spliced the array and reindexed surviving
// accounts, the cached numeric key still pointed at the
// old (stale) position, so getRuntimeTrackerKey returned the
// wrong key and rotation/health/token tracker lookups
// mismatched the current index. Identity-based string keys
// must continue to survive a reindex unchanged.
const now = Date.now();
const stored = {
version: 3 as const,
activeIndex: 0,
accounts: [
{ refreshToken: "token-a", addedAt: now, lastUsed: now },
{
refreshToken: "token-b",
email: "remove-me@example.com",
addedAt: now,
lastUsed: now,
},
{ refreshToken: "token-c", addedAt: now, lastUsed: now },
{
refreshToken: "token-d",
addedAt: now,
lastUsed: now,
},
],
};

const manager = new AccountManager(undefined, stored);

const refreshOnlyBefore = manager.getAccountByIndex(2)!;
const refreshOnlyAfterShift = manager.getAccountByIndex(3)!;

// Prime the cache: getRuntimeTrackerKey writes
// _runtimeTrackerKey on first access. The refresh-only
// account caches its numeric index (2); the identity
// account caches its string identity key.
const refreshKeyBefore = getRuntimeTrackerKey(refreshOnlyBefore);
const refreshOnlyAfterShiftKeyBefore = getRuntimeTrackerKey(
refreshOnlyAfterShift,
);
expect(refreshKeyBefore).toBe(2);
expect(refreshOnlyAfterShiftKeyBefore).toBe(3);
expect(refreshOnlyBefore._runtimeTrackerKey).toBe(2);
expect(refreshOnlyAfterShift._runtimeTrackerKey).toBe(3);

// Record observable tracker state keyed by the current
// tracker key so we can prove the same account is
// consulted under its new key after the reindex. Use the
// health tracker: that is what recordFailure mutates via
// getRuntimeTrackerKey.
const healthTracker = getHealthTracker();
const tokenTracker = getTokenTracker();
const initialRefreshScore = healthTracker.getScore(
refreshKeyBefore,
"codex",
);
manager.recordFailure(refreshOnlyBefore, "codex");
tokenTracker.tryConsume(refreshKeyBefore, "codex");
const postFailureScore = healthTracker.getScore(
refreshKeyBefore,
"codex",
);
expect(postFailureScore).toBeLessThan(initialRefreshScore);
expect(tokenTracker.getTokens(refreshKeyBefore, "codex")).toBeLessThan(
50,
);

// Remove the identity-bearing account at index 1. The
// refresh-only account that was at index 2 now lives at
// index 1 after the splice + reindex.
const victim = manager.getAccountByIndex(1)!;
expect(manager.removeAccount(victim)).toBe(true);

const refreshOnlyAfter = manager.getAccountByIndex(1);
expect(refreshOnlyAfter).not.toBeNull();
expect(refreshOnlyAfter?.refreshToken).toBe("token-c");
expect(refreshOnlyAfter?.index).toBe(1);

// getRuntimeTrackerKey must now report the NEW numeric
// index (1), not the stale cached value (2). Before the
// fix, the stale cache caused this assertion to fail with 2.
const refreshKeyAfter = getRuntimeTrackerKey(refreshOnlyAfter!);
expect(refreshKeyAfter).toBe(1);
expect(refreshOnlyAfter?._runtimeTrackerKey).toBe(1);

// The health score recorded under the pre-reindex key (2)
// must NOT bleed into the post-reindex key (1). Querying
// the new key should return the default (pristine) score.
// This proves the runtime tracker consults the refreshed
// key after reindex.
expect(healthTracker.getScore(refreshKeyAfter, "codex")).toBe(
initialRefreshScore,
);

// The next refresh-only survivor shifts from numeric key 3 to 2.
// The stale numeric state for old key 2 must NOT bleed onto it.
const refreshOnlyShifted = manager.getAccountByIndex(2);
expect(refreshOnlyShifted).not.toBeNull();
expect(refreshOnlyShifted?.refreshToken).toBe("token-d");
expect(refreshOnlyShifted?.index).toBe(2);
const refreshOnlyShiftedKeyAfter = getRuntimeTrackerKey(
refreshOnlyShifted!,
);
expect(refreshOnlyShiftedKeyAfter).toBe(2);
expect(healthTracker.getScore(refreshOnlyShiftedKeyAfter, "codex")).toBe(
initialRefreshScore,
);
expect(tokenTracker.getTokens(refreshOnlyShiftedKeyAfter, "codex")).toBe(
50,
);
});
});

describe("flushPendingSave", () => {
Expand Down