[WRONG BRANCH] fix(gui): bound optimistic active-account reconciliation - #206
[WRONG BRANCH] fix(gui): bound optimistic active-account reconciliation#206luvs01 wants to merge 1 commit into
Conversation
|
✅ Deterministic PR hygiene checks passed. |
📝 WalkthroughWalkthroughThe account pool now tolerates one stale ChangesActive account reconciliation
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
⏳ DRAFT
What to do
Its title has been prefixed with |
github-actions
Bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@gui/src/hooks/useCodexAccountPool.ts`:
- Line 136: Serialize all active-account mutations in useCodexAccountPool
through one shared gate or monotonic mutation revision, covering switch, pause,
bulk-pause, and setAccountPriority. Ensure each management API response updates
activeId and pendingActiveIdRef only when it is still the latest mutation,
preventing late responses from overwriting newer state. Add reverse-order
coverage for switch/pause and pause/priority responses.
- Around line 219-222: Preserve the optimistic active ID in lastGoodByBase when
the pending stale read is intentionally accepted, rather than retaining
prior.activeId when nextActiveId is undefined. Update the cache-write logic
around the pendingActiveIdRef/staleReadsRemaining flow to record the pending ID
after a successful mutation, while remaining consistent with management API
responses. Add a remount test covering this interval and verifying the new
surface retains the optimistic selection.
In `@gui/tests/codex-account-pool-behaviour.test.tsx`:
- Around line 485-488: Replace the fixed 30 ms timeout after switchAccount in
the account-pool test with deterministic synchronization: wait for the expected
post-switch GET request or resolve its deferred mock response before asserting.
Ensure the synchronization specifically proves the background load started by
switchAccount completed, rather than allowing the explicit load to consume the
request allowance.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8b777215-70b9-493d-870c-ce490f4c58f3
📒 Files selected for processing (2)
gui/src/hooks/useCodexAccountPool.tsgui/tests/codex-account-pool-behaviour.test.tsx
| // Set by switchAccount so a background load already in flight cannot roll the active | ||
| // id back to a value the server had not yet committed when that request was issued. | ||
| const pendingActiveIdRef = useRef<{ id: string | null } | null>(null); | ||
| const pendingActiveIdRef = useRef<{ id: string | null; staleReadsRemaining: number } | null>(null); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Sequence all active-account mutations before updating the shared marker.
pendingActiveIdRef is shared by switch, pause, and bulk-pause operations, but these handlers can overlap. A late pause response can overwrite a newer switch result at Lines [394-397] or [480-483]. Conversely, setAccountPriority can clear a newer pause marker at Line [448]. loadGenerationRef orders refresh reads only. It does not order these PUT responses. The next read can therefore consume or accept the wrong active account.
Use one shared active-mutation gate or queue, or attach a monotonic mutation revision and apply activeId and the marker only for the current revision. Add reverse-order tests for switch/pause and pause/priority responses.
As per path instructions, gui/** state changes must stay consistent with management API responses.
Also applies to: 338-341, 394-397, 480-483
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@gui/src/hooks/useCodexAccountPool.ts` at line 136, Serialize all
active-account mutations in useCodexAccountPool through one shared gate or
monotonic mutation revision, covering switch, pause, bulk-pause, and
setAccountPriority. Ensure each management API response updates activeId and
pendingActiveIdRef only when it is still the latest mutation, preventing late
responses from overwriting newer state. Add reverse-order coverage for
switch/pause and pause/priority responses.
Source: Path instructions
| if (pending && pending.staleReadsRemaining > 0 && serverActiveId !== pending.id) { | ||
| // Allow one eventually-consistent response to preserve the accepted value, | ||
| // but ensure a repeated mismatch can reconcile legitimate routing changes. | ||
| pending.staleReadsRemaining -= 1; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve the optimistic active ID in lastGoodByBase.
When Line [219] consumes the stale read, nextActiveId remains undefined. The cache write at Lines [251-253] then keeps prior?.activeId, which is the pre-mutation value. If another surface mounts before the next /active read, Lines [108-110] seed it with that old value, while its new pendingActiveIdRef has no allowance. The new surface can accept the same stale response and lose the optimistic selection.
Update lastGoodByBase when the mutation succeeds, or use the pending ID when recording a load that intentionally preserves the optimistic state. Add a remount test for this interval.
As per path instructions, gui/** state changes must stay consistent with management API responses.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@gui/src/hooks/useCodexAccountPool.ts` around lines 219 - 222, Preserve the
optimistic active ID in lastGoodByBase when the pending stale read is
intentionally accepted, rather than retaining prior.activeId when nextActiveId
is undefined. Update the cache-write logic around the
pendingActiveIdRef/staleReadsRemaining flow to record the pending ID after a
successful mutation, while remaining consistent with management API responses.
Add a remount test covering this interval and verifying the new surface retains
the optimistic selection.
Source: Path instructions
| await act(async () => { | ||
| expect(await seen.current!.switchAccount("a2")).toEqual({ ok: true, activeId: "a2" }); | ||
| await new Promise((resolve) => setTimeout(resolve, 30)); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Replace the fixed delay with deterministic synchronization.
switchAccount starts void load() at gui/src/hooks/useCodexAccountPool.ts Line [349] and returns before that load completes. The 30 ms sleep at Lines [485-487] does not prove that the first post-switch GET ran. A slow test run can execute the assertion before the background read, or let the explicit load() become the read that consumes the allowance.
Resolve a deferred mock response or wait for the expected GET count before asserting.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@gui/tests/codex-account-pool-behaviour.test.tsx` around lines 485 - 488,
Replace the fixed 30 ms timeout after switchAccount in the account-pool test
with deterministic synchronization: wait for the expected post-switch GET
request or resolve its deferred mock response before asserting. Ensure the
synchronization specifically proves the background load started by switchAccount
completed, rather than allowing the explicit load to consume the request
allowance.
1 participant
Motivation
/api/codex-auth/activeresponses such as quota auto-switch or failover, which can leave the UI pinned to a stale active id.Description
pendingActiveIdRefwith a bounded marker that carriesstaleReadsRemainingso at most one mismatched read is absorbed before the controller accepts a newer server value.switchAccount,setAccountPaused, andpauseExhaustedAccountsso background reconciliation cannot permanently ignore real server changes./activeread logic to decrementstaleReadsRemainingon a mismatch and to accept the server id once the allowance is exhausted.a post-switch read accepts a newer server-side active accountingui/tests/codex-account-pool-behaviour.test.tsxthat verifies one absorbed mismatch followed by convergence to a genuine server-selected account.Testing
cd gui && bun test tests/codex-account-pool-behaviour.test.tsx, and the modified test passed.cd gui && bun run lintandcd gui && bun run build, and both completed (build emitted the existing Vite chunk-size advisory only).bun run typecheck, which completed.bun run test/cd gui && bun test tests) which executed hundreds of tests; the GUI/controller tests covering this change passed, while the full repository run encountered unrelated, environment-dependent failures (network/proxy/timeouts and platform zlib differences) not caused by this change.Codex Task
Summary by CodeRabbit
Bug Fixes
Tests