Skip to content

Commit cb70148

Browse files
committed
fix(gui): stop an order write and a manual switch from racing the pin
Both PUTs move the manual pin, in opposite directions -- the active route sets it, the order route clears it -- and each handler now applies its edge optimistically rather than waiting for the reload. They were gated on separate refs, so they could overlap, and response order is not request order: whichever reply landed last won the client's pin state regardless of which write the server processed last. The dashboard could sit on the inverse of the server's pin until some later reload happened to correct it. Cross-gate them in both directions, since either can be the newer statement. A refused mutation returns "busy", which both call sites drop without a toast, so the order control and the switch confirm are also disabled while the other is in flight -- otherwise the gate would only turn a visible race into an invisible no-op. Found by CodeRabbit on #715.
1 parent 0dc6483 commit cb70148

7 files changed

Lines changed: 97 additions & 9 deletions

gui/src/components/CodexAccountPool.tsx

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -299,6 +299,7 @@ export default function CodexAccountPool({ apiBase, accountModeState = null, ban
299299
pauseBusy={pauseUpdatingId !== null || pausingExhausted}
300300
onPriorityChange={(entry, priority) => { void changePriority(entry, priority); }}
301301
priorityUpdatingId={priorityUpdatingId}
302+
switchingId={switchingId}
302303
pinnedId={activePinnedId}
303304
onOpenReset={openResetPopup}
304305
onCopyDoctor={copyDoctor}
@@ -332,6 +333,7 @@ export default function CodexAccountPool({ apiBase, accountModeState = null, ban
332333
pauseBusy={pauseUpdatingId !== null || pausingExhausted}
333334
onPriorityChange={(entry, priority) => { void changePriority(entry, priority); }}
334335
priorityUpdatingId={priorityUpdatingId}
336+
switchingId={switchingId}
335337
pinnedId={activePinnedId}
336338
onReauth={openReauth}
337339
onEditAlias={editAlias}
@@ -365,6 +367,7 @@ export default function CodexAccountPool({ apiBase, accountModeState = null, ban
365367
mainEmail={main?.email}
366368
accountModeState={accountModeState}
367369
switchingId={switchingId}
370+
orderBusy={priorityUpdatingId !== null}
368371
onCancel={() => setConfirm(null)}
369372
onConfirm={() => { void setActive(confirm.id === "__main__" ? "__main__" : confirm.id); }}
370373
/>

gui/src/components/codex-account-pool-cards.tsx

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ export function CodexAccountPoolCards({
2929
pauseBusy,
3030
onPriorityChange,
3131
priorityUpdatingId,
32+
switchingId,
3233
pinnedId = null,
3334
onReauth,
3435
onEditAlias,
@@ -48,6 +49,8 @@ export function CodexAccountPoolCards({
4849
pauseBusy: boolean;
4950
onPriorityChange: (account: CodexAccountEntry, priority: number) => void;
5051
priorityUpdatingId: string | null;
52+
/** In-flight manual switch, which writes the same pin an order write clears. */
53+
switchingId: string | null;
5154
/**
5255
* The account an operator pinned by hand, which is not always the selected one: under
5356
* round-robin and fill-first the pin caps selection at its own tier while the cursor
@@ -145,7 +148,9 @@ export function CodexAccountPoolCards({
145148
// Every row, not just the one being written: the controller serializes order
146149
// writes behind one mutation ref, so a second row's pick would come back "busy"
147150
// and be dropped with no toast. Same global lock the pause button uses.
148-
disabled={priorityUpdatingId !== null}
151+
// A pending switch counts too — it writes the same pin this clears, so the
152+
// controller refuses to overlap them, and that refusal is equally silent.
153+
disabled={priorityUpdatingId !== null || switchingId !== null}
149154
onChange={(priority) => onPriorityChange(a, priority)}
150155
/>
151156
{showReauth

gui/src/components/codex-account-pool-main-card.tsx

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ export function CodexAccountPoolMainCard({
2929
pauseBusy,
3030
onPriorityChange,
3131
priorityUpdatingId,
32+
switchingId,
3233
pinnedId = null,
3334
onOpenReset,
3435
onCopyDoctor,
@@ -46,6 +47,8 @@ export function CodexAccountPoolMainCard({
4647
pauseBusy: boolean;
4748
onPriorityChange: (entry: CodexAccountEntry, priority: number) => void;
4849
priorityUpdatingId: string | null;
50+
/** In-flight manual switch, which writes the same pin an order write clears. */
51+
switchingId: string | null;
4952
/**
5053
* The account carrying the pin, which is not always the one routing is on: the pin caps
5154
* selection at its own tier, and under round-robin or fill-first the cursor still moves
@@ -139,8 +142,9 @@ export function CodexAccountPoolMainCard({
139142
selectId={`codex-account-priority-${mainSwitchEntry.id}`}
140143
// Any in-flight order write, not just this card's: order writes share one mutation
141144
// ref, so a pick made during another card's write returns "busy" and is dropped
142-
// silently. Mirrors pauseBusy.
143-
disabled={priorityUpdatingId !== null}
145+
// silently. Mirrors pauseBusy. A pending switch counts too — it writes the same
146+
// pin this clears, so the controller refuses to overlap them, just as silently.
147+
disabled={priorityUpdatingId !== null || switchingId !== null}
144148
onChange={(priority) => onPriorityChange(mainSwitchEntry, priority)}
145149
/>
146150
)}

gui/src/components/codex-account-switch-modal.tsx

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,13 +9,20 @@ export function CodexAccountSwitchModal({
99
mainEmail,
1010
accountModeState,
1111
switchingId,
12+
orderBusy = false,
1213
onCancel,
1314
onConfirm,
1415
}: {
1516
confirm: CodexAccountEntry;
1617
mainEmail?: string;
1718
accountModeState: CodexAccountModeState | null;
1819
switchingId: string | null;
20+
/**
21+
* An in-flight selection-order write. It clears the pin this switch would set, so the
22+
* controller refuses to run the two together and drops the loser without a toast --
23+
* the button has to be unavailable rather than silently ineffective.
24+
*/
25+
orderBusy?: boolean;
1926
onCancel: () => void;
2027
onConfirm: () => void;
2128
}) {
@@ -59,7 +66,7 @@ export function CodexAccountSwitchModal({
5966
)}
6067
<div className="modal-actions">
6168
<button type="button" className="btn btn-ghost" onClick={onCancel}>{t("codexAuth.cancel")}</button>
62-
<button type="button" className="btn btn-primary" disabled={Boolean(switchingId)} onClick={onConfirm}>
69+
<button type="button" className="btn btn-primary" disabled={Boolean(switchingId) || orderBusy} onClick={onConfirm}>
6370
{switchingId ? t("pws.accountSwitching") : t(accountModeState === "direct" ? "codexAuth.prepareForPool" : "codexAuth.setAsNext")}
6471
</button>
6572
</div>

gui/src/hooks/useCodexAccountPool.ts

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -228,7 +228,12 @@ export function useCodexAccountPool(apiBase: string, enabled = true): CodexAccou
228228
}, []);
229229

230230
const switchAccount = useCallback(async (id: string | null) => {
231-
if (switchingRef.current) return { ok: false, reason: "busy" } as const;
231+
// Cross-gated with the order write, not just with itself: both PUTs move the pin,
232+
// and in opposite directions, so letting them overlap lets the client settle on
233+
// the inverse of the server's final pin until a reload happens to correct it.
234+
// Response order is not request order, so neither optimistic write can tell
235+
// whether it is the newer statement.
236+
if (switchingRef.current || priorityMutationRef.current) return { ok: false, reason: "busy" } as const;
232237
switchingRef.current = id ?? "__main__";
233238
setSwitchingId(id ?? "__main__");
234239
try {
@@ -308,7 +313,9 @@ export function useCodexAccountPool(apiBase: string, enabled = true): CodexAccou
308313
}, [apiBase, load]);
309314

310315
const setAccountPriority = useCallback(async (id: string, priority: number | null) => {
311-
if (priorityMutationRef.current) return { ok: false, reason: "busy" } as const;
316+
// The other half of the cross-gate in switchAccount: an order write clears the
317+
// pin that a switch sets, so the two cannot be in flight together.
318+
if (priorityMutationRef.current || switchingRef.current) return { ok: false, reason: "busy" } as const;
312319
priorityMutationRef.current = { accountId: id };
313320
setPriorityUpdatingId(id);
314321
try {

gui/tests/codex-account-pool-behaviour.test.tsx

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ let priorityResponseOk = true;
3030
let nextPriorityResponseGate: Promise<void> | null = null;
3131
let nextPauseResponseGate: Promise<void> | null = null;
3232
let nextActiveResponseGate: Promise<void> | null = null;
33+
let nextActivePutGate: Promise<void> | null = null;
3334
let activePinned = false;
3435
let activePinnedAccountId: string | null = null;
3536
let omitPinnedAccountId = false;
@@ -55,6 +56,7 @@ beforeEach(() => {
5556
nextPriorityResponseGate = null;
5657
nextPauseResponseGate = null;
5758
nextActiveResponseGate = null;
59+
nextActivePutGate = null;
5860
activePinned = false;
5961
activePinnedAccountId = null;
6062
omitPinnedAccountId = false;
@@ -121,6 +123,9 @@ beforeEach(() => {
121123
if (path.startsWith("codex-auth/active")) {
122124
if (init?.method === "PUT") {
123125
const body = JSON.parse(String(init.body)) as { accountId: string | null };
126+
const putGate = nextActivePutGate;
127+
nextActivePutGate = null;
128+
if (putGate) await putGate;
124129
activePinnedAccountId = body.accountId;
125130
return {
126131
ok: true,
@@ -399,6 +404,49 @@ test("an accepted selection-order write clears the pin before reconciliation lan
399404
});
400405
});
401406

407+
test("an order write and a manual switch refuse to overlap in either direction", async () => {
408+
// Both PUTs move the pin, in opposite directions, and each applies its edge
409+
// optimistically. Response order is not request order, so overlapping them can leave
410+
// the client on the inverse of the server's final pin until a reload corrects it.
411+
accounts = [
412+
{ id: "a1", email: "main", isMain: true, paused: false, priority: 0, hasCredential: true, quota: null },
413+
{ id: "a2", email: "pool", isMain: false, paused: false, priority: 0, hasCredential: true, quota: null },
414+
];
415+
const seen = await mountController();
416+
417+
let releasePriority!: () => void;
418+
nextPriorityResponseGate = new Promise<void>(resolve => { releasePriority = resolve; });
419+
let priorityResult: unknown;
420+
let switchDuringOrder: unknown;
421+
await act(async () => {
422+
const orderWrite = seen.current!.setAccountPriority("a2", 2).then(r => { priorityResult = r; });
423+
switchDuringOrder = await seen.current!.switchAccount("a1");
424+
releasePriority();
425+
await orderWrite;
426+
});
427+
await act(async () => { await new Promise((r) => setTimeout(r, 30)); });
428+
429+
expect(switchDuringOrder).toEqual({ ok: false, reason: "busy" });
430+
expect(priorityResult).toEqual({ ok: true });
431+
432+
let releaseSwitch!: () => void;
433+
nextActivePutGate = new Promise<void>(resolve => { releaseSwitch = resolve; });
434+
let switchResult: unknown;
435+
let orderDuringSwitch: unknown;
436+
await act(async () => {
437+
const switchWrite = seen.current!.switchAccount("a2").then(r => { switchResult = r; });
438+
orderDuringSwitch = await seen.current!.setAccountPriority("a2", 1);
439+
releaseSwitch();
440+
await switchWrite;
441+
});
442+
await act(async () => { await new Promise((r) => setTimeout(r, 30)); });
443+
444+
expect(orderDuringSwitch).toEqual({ ok: false, reason: "busy" });
445+
expect(switchResult).toEqual({ ok: true, activeId: "a2" });
446+
// The refused order write must not have moved the row either.
447+
expect(seen.current!.accounts.find(account => account.id === "a2")?.priority).toBe(2);
448+
});
449+
402450
test("an accepted manual switch moves the pin before reconciliation lands", async () => {
403451
activePinnedAccountId = "a1";
404452
const seen = await mountController();

gui/tests/codex-account-pool-controller.test.ts

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ test("main and added account cards expose the same persisted pause control", asy
4545
expect(addedCards).toContain('t(a.paused ? "codexAuth.resume" : "codexAuth.pause")');
4646
});
4747

48-
test("both cards expose the selection-order control, and the mutation has its own gate", async () => {
48+
test("both cards expose the selection-order control, and pin writes cannot overlap", async () => {
4949
const pool = await read("../src/components/CodexAccountPool.tsx");
5050
const mainCard = await read("../src/components/codex-account-pool-main-card.tsx");
5151
const addedCards = await read("../src/components/codex-account-pool-cards.tsx");
@@ -76,13 +76,27 @@ test("both cards expose the selection-order control, and the mutation has its ow
7676

7777
expect(hook).toContain("/api/codex-auth/accounts/priority");
7878
// Sharing pauseMutationRef would make a pause toggle and an order change on two
79-
// different accounts reject each other for no reason.
79+
// different accounts reject each other for no reason. The manual switch is the one
80+
// deliberate exception: it writes the same pin an order write clears, so overlapping
81+
// them lets the client settle on the inverse of the server's final pin. Cross-gated
82+
// in both directions, since either can be the newer statement.
8083
const priorityMutation = hook.slice(
8184
hook.indexOf("const setAccountPriority"),
8285
hook.indexOf("const pauseExhaustedAccounts"),
8386
);
84-
expect(priorityMutation).toContain("if (priorityMutationRef.current) return");
87+
expect(priorityMutation).toMatch(/if \(priorityMutationRef\.current \|\| switchingRef\.current\) return/);
8588
expect(priorityMutation).not.toContain("pauseMutationRef");
89+
90+
const switchMutation = hook.slice(hook.indexOf("const switchAccount"), hook.indexOf("const saveAlias"));
91+
expect(switchMutation).toMatch(/if \(switchingRef\.current \|\| priorityMutationRef\.current\) return/);
92+
93+
// A refused mutation returns "busy", which both call sites drop without a toast, so
94+
// each control must be unavailable while the other is in flight rather than silently
95+
// ineffective.
96+
for (const card of [mainCard, addedCards]) {
97+
expect(card).toContain("disabled={priorityUpdatingId !== null || switchingId !== null}");
98+
}
99+
expect(pool).toContain("orderBusy={priorityUpdatingId !== null}");
86100
});
87101

88102
test("the pool header exposes one bulk action backed by the atomic endpoint", async () => {

0 commit comments

Comments
 (0)