Skip to content

[WRONG BRANCH] fix(gui): bound optimistic active-account reconciliation #206

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
luvs01 wants to merge 1 commit into
base: main
Choose a base branch
from
+40 −6
Draft

[WRONG BRANCH] fix(gui): bound optimistic active-account reconciliation #206

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
23 changes: 17 additions & 6 deletions gui/src/hooks/useCodexAccountPool.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ export function useCodexAccountPool(apiBase: string, enabled = true): CodexAccou
const loadGenerationRef = useRef(0);
// 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);

@coderabbitai coderabbitai Bot Aug 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🗄️ 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

const observersRef = useRef<Set<CodexAccountLoadObserver> | null>(null);
if (observersRef.current === null) observersRef.current = new Set();
// Last /active payload an actual read returned. Surfaces that mount after a
Expand Down Expand Up @@ -216,8 +216,10 @@ export function useCodexAccountPool(apiBase: string, enabled = true): CodexAccou
if (loadGenerationRef.current === generation) {
const serverActiveId = active.activeCodexAccountId ?? null;
const pending = pendingActiveIdRef.current;
if (pending && serverActiveId !== pending.id) {
// Stale read: keep the accepted value and let the next load reconcile.
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;
Comment on lines +219 to +222

@coderabbitai coderabbitai Bot Aug 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🗄️ 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

} else {
pendingActiveIdRef.current = null;
nextActiveId = serverActiveId;
Expand Down Expand Up @@ -333,7 +335,10 @@ export function useCodexAccountPool(apiBase: string, enabled = true): CodexAccou
if (!response.ok) throw new Error("account switch failed");
const result = await response.json().catch(() => ({})) as { activeCodexAccountId?: string | null };
const selectedId = result.activeCodexAccountId ?? id;
pendingActiveIdRef.current = { id: selectedId ?? null };
pendingActiveIdRef.current = {
id: selectedId ?? null,
staleReadsRemaining: 1,
};
setActiveId(selectedId ?? null);
// A manual selection pins its target until the account drains or routing moves off
// it. The badge follows the id, not /active's `pinned` boolean, so a same-tier
Expand Down Expand Up @@ -386,7 +391,10 @@ export function useCodexAccountPool(apiBase: string, enabled = true): CodexAccou
)));
if (Object.prototype.hasOwnProperty.call(result, "activeCodexAccountId")) {
const nextActiveId = result.activeCodexAccountId ?? null;
pendingActiveIdRef.current = { id: nextActiveId };
pendingActiveIdRef.current = {
id: nextActiveId,
staleReadsRemaining: 1,
};
setActiveId(nextActiveId);
}
// Deliberately NOT cross-gated against the switch and order writes, even though
Expand Down Expand Up @@ -469,7 +477,10 @@ export function useCodexAccountPool(apiBase: string, enabled = true): CodexAccou
)));
if (Object.prototype.hasOwnProperty.call(result, "activeCodexAccountId")) {
const nextActiveId = result.activeCodexAccountId ?? null;
pendingActiveIdRef.current = { id: nextActiveId };
pendingActiveIdRef.current = {
id: nextActiveId,
staleReadsRemaining: 1,
};
setActiveId(nextActiveId);
}
// Conditional for the same reason as the single-account pause above: clearing
Expand Down
23 changes: 23 additions & 0 deletions gui/tests/codex-account-pool-behaviour.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -471,6 +471,29 @@ test("an accepted manual switch moves the pin before reconciliation lands", asyn
});
});

test("a post-switch read accepts a newer server-side active account", async () => {
accounts = [
{ id: "a1", email: "main", isMain: true, paused: false, priority: 0, hasCredential: true, quota: null },
{ id: "a2", email: "selected", isMain: false, paused: false, priority: 0, hasCredential: true, quota: null },
{ id: "a3", email: "failover", isMain: false, paused: false, priority: 0, hasCredential: true, quota: null },
];
const seen = await mountController();

// The PUT accepts a2, but routing legitimately moves to a3 before the
// reconciliation read. That fresh response must retire the optimistic marker.
activeGetId = "a3";
await act(async () => {
expect(await seen.current!.switchAccount("a2")).toEqual({ ok: true, activeId: "a2" });
await new Promise((resolve) => setTimeout(resolve, 30));
});
Comment on lines +485 to +488

@coderabbitai coderabbitai Bot Aug 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

// One mismatch may be the eventually-consistent response the optimistic marker
// exists to absorb.
expect(seen.current!.activeId).toBe("a2");

await act(async () => { await seen.current!.load(); });
expect(seen.current!.activeId).toBe("a3");
});

test("the main sentinel writes through to its distinct account row", async () => {
const seen = await mountController();

Expand Down
Loading