diff --git a/devlog/_plan/260911_account_pool_unification/010_phase1_manual_selection.md b/devlog/_plan/260911_account_pool_unification/010_phase1_manual_selection.md index bfb1a2b1b0..949ee387b1 100644 --- a/devlog/_plan/260911_account_pool_unification/010_phase1_manual_selection.md +++ b/devlog/_plan/260911_account_pool_unification/010_phase1_manual_selection.md @@ -161,6 +161,112 @@ singleton-versus-scope-keyed state, the missing invalidation rule in the absence of an account-side revision, the pin-versus-preference disagreement after a released pin, and the test gap against criterion c-2. +## Implementation-entry audit, after the lane freeze lifted + +Lane L3 PR #4230 and lane L1 PR #4226 merged, so this work became writable. A +fresh audit against the post-merge file returned FAIL with three more blockers. +All anchors survived the merge (`codexPoolKeyForScope` 225, +`resetCodexRoutingForManualSelection` 870, `pickUnboundStrategyAccount` 1446, +`getEffectiveActiveCodexAccountId` 1625, `rememberActiveCodexAccount` 1644, +`setActiveCodexAccount` 1660, `promoteActiveCodexAccount` 1669), but L3 added +independent-scope cursor isolation and runtime-only preemption, which changes what +the design may assume. + +1. **BLOCKER. The preference is scope-keyed but `getEffectiveActiveCodexAccountId` + is not.** It takes only a config and has no `quotaScope`, so it can read the + shared `POOL_KEY_CODEX` entry and nothing else. `resolveCodexAccountForThreadDetailed` + and `previewCodexAccountForRequest` look up `codexPoolKeyForScope(quotaScope)` + themselves. A scope with no entry means NO preference; it must never fall back to + the shared key, or an independent scope would consume a one-shot it was not given. +2. **BLOCKER. Consuming inside `setActiveCodexAccount` is wrong.** That function is + also the persist path for pool-driven moves: quota auto-switch (1807), affinity + re-evaluation (2166), unbound persist (2231 and 2252) and the quota promote + (1671) all call it. Consuming there would let the pool spend the operator's + one-shot. Consume only on the path where the preference was actually honoured + and the dispatch succeeded, plus on an operator reset. +3. **BLOCKER. An unconditional honour traps a cooled account.** With + `rememberActiveCodexAccount` a no-op, a 429 or failover on the preferred account + (2527, 2576, 1878) could not move `getEffectiveActiveCodexAccountId` away from + it. Honour the preference only while that account is selectable and not cooling; + otherwise treat it as absent for this dispatch without spending it. +4. **The preview path must mirror resolve.** The check belongs immediately before + BOTH `pickUnboundStrategyAccount` calls, at 2020 and 2193, after affinity and + model-detour handling, not at function entry. +5. **Pause and exclusion never route through the reset.** `reconcileCodexActiveAfterExclusion` + (1692) and the health-clear path (317-320) bypass it, so a preference would + outlive an excluded or paused account. Drop the key when the preferred account is + excluded or paused. +6. **Minor, but decide it deliberately.** `isEffectiveCodexAccountPinned` (1637) + would read true while the preference equals the pin, and L3 now documents that + `getEffectiveActiveCodexAccountId` is what surfaces automatic picks to the API + and dashboard. Either keep the pin check reading persisted and runtime only, or + accept and document that `GET /api/codex-auth/active` is manual-sticky until the + preference is consumed. + +## Measured: the consume call site is the whole design, not a detail + +A first implementation pass built the preference map, the seeding inside +`resetCodexRoutingForManualSelection`, the `rememberActiveCodexAccount` guard, the +`getEffectiveActiveCodexAccountId` overlay and the exclusion revoke, and left the +consume call site unwired. It typechecked, and then +`tests/codex-integration/codex-pool-rotation.test.ts` went from green to **15 failures +out of 69**, including "fill-first picks the same sequence with no stored order as +before the feature". + +That is the correct result, and it is worth recording rather than repeating. Without a +consume site the one-shot is permanent: the first operator selection freezes the +automatic cursor forever, because `rememberActiveCodexAccount` stays a no-op and no +code path ever clears the entry. Every rotation-strategy test that expects the pool to +keep moving after a manual selection fails, and they are right to. + +So the implementation order matters. Build the consume path FIRST, not last: + +1. Find the point that already records a successful upstream outcome for the resolved + account and call `consumeCodexManualPreference(poolKey)` there, for a non-quota + success only. This is the Codex analogue of `commitAnthropicSelectionRouting` + (`anthropic-routing.ts` :799-800), which Codex has no direct equivalent of. +2. Only then add the `rememberActiveCodexAccount` guard, so the suite never passes + through a state where the cursor can freeze. +3. Gate honouring on the account being selectable, per blocker 3 above, so a cooled + preferred account is skipped for that dispatch without being spent. + +The pass was reverted rather than pushed. The branch `codex/manual-selection-wins` +carries this document and no source change. + +## Measured again: guarding the writer is right, but the 429 path needs an exemption + +A second pass followed the order above. The consume site went in first, at the +`outcomeClass === "success"` branch of `recordCodexUpstreamOutcome` (:2356), keyed by +`codexPoolKeyForScope(quotaScope)` which that function already computes. Seeding and the +exclusion revoke followed. At each of those two steps +`tests/codex-integration/codex-pool-rotation.test.ts` stayed **69 pass, 0 fail**, which +confirms the ordering advice above is correct. + +Adding the `rememberActiveCodexAccount` guard then produced **6 failures out of 69**, down +from 15, and every single one is a 429 promotion case: + +- fill-first 429 advances to next stable account, not lowest usage +- RR 429 promotes via ring, not lowest usage +- 429 retry reuse promoteAccountId avoids a second RR ring advance +- fill-first transient failover advances stable order, not lowest usage +- scoped reset 429s retain strategy while excluding only the affected native quota +- fill-first preserves its pre-feature fallback when every ordered tier is drained + +That is exactly the hazard blocker 3 named, and it is sharper than the blocker stated it. +Gating the guard on `isCodexAccountSelectable(preferred)` is NOT sufficient: at the moment +`promoteActiveCodexAccount` runs, the preferred account can still read as selectable +because the 429 cooldown is recorded on a different path, so the guard holds and the +promotion cannot land. + +The conclusion for the next pass: guarding the writer closes all four call sites at once, +which is still the right shape, but the failover promote needs an explicit exemption. It +only ever runs because the account in use just failed, so it is never an automatic pick +competing with the operator. Either pass an explicit "this is a failover promote" flag +through `rememberActiveCodexAccount`, or leave the writer unguarded and guard the two +strategy commit sites plus preemption instead, accepting three guards rather than one. + +Reverted again rather than pushed. The measurement is the deliverable. + The generic OAuth kind gets no preference in this layer; that arrives with the kernel in phase 2. No management or GUI change. @@ -183,3 +289,65 @@ The design therefore survives the lane's landings. What does not change is the coordination risk: L3 still owns these files for the dispatch round, so the B phase of this work-phase must not open until that ownership clears. Re-run this table at that point, because the guarantee above is a snapshot of `16f18d654`. + +## Audit round 4 — the shipped tests proved nothing + +The first implementation landed as PR #4284 with three new cases under +`an operator selection outranks the pool cursor`, and a reviewer was asked one +question the earlier rounds never asked: does each test fail without the production +change? It does not. Measured by reverting only `src/codex/routing.ts` to the parent +branch and keeping the new tests: + +``` +bun test tests/codex-integration/codex-pool-rotation.test.ts \ + -t "an operator selection outranks the pool cursor" +3 pass, 0 fail # production change reverted +``` + +All three passed against a tree with no guard, no preference map and no consume site. +They were re-assertions of things that already held: case 1 of +`resetCodexRoutingForManualSelection` clearing the runtime cursor and seeding the ring, +cases 2 and 3 of the failover promote, which this design deliberately leaves exempt. A +test that cannot fail is not weak coverage, it is an empty claim, and criteria c-1 and +c-2 had been recorded `met` against it. + +Three real defects were behind that blind spot. + +**Deletion never revoked the preference.** Pause and exclusion both route through +`reconcileCodexActiveAfterExclusion`, which forgets it. Delete does not: the +account-lifecycle path reaches routing through `clearCodexUpstreamHealthForAccount` +(`routing.ts:327`, called from `account-lifecycle.ts:43`), which cleared two health maps +and left the preference behind. Once the named account is gone nothing can ever succeed +on it, so the one-shot can never be spent, and every later automatic write is suppressed +until the process restarts. The generation sweep in `reconcileCodexRoutingHealth` had the +same hole for an account removed by an edit the runtime never observed. + +**The model-detour promote was reported as unguarded — REBUTTED.** `promoteActiveCodexAccount` +at the model-detour site sits twelve lines above the preemption site this design guards, so +the symmetry argument is tempting. It is wrong, and the measurement says so: guarding it +fails 8 cases in `tests/codex-integration/codex-routing.test.ts`, the +`cannot re-pick a quota-drained shared account that remains model-eligible` family and its +siblings. Those encode an older contract. A model detour is not the pool exercising +discretion — it runs because the operator's account cannot serve the requested model at +all — and under a rotating strategy that promote moves only the process-local cursor to +whoever is actually serving, then releases the pin. `config.activeCodexAccountId`, the +operator's persisted selection and the thing this preference exists to protect, is +untouched either way. The guard was written, measured red, and reverted with the reason +recorded at the call site. + +**The independent-scope entries were dead state.** Every write site the guard protects is +already skipped for independent scopes, so those keys were seeded and consumed but never +read. Removed: state nothing reads is what the next reader mistakes for a rule. + +The replacement cases are each red against the variant that removes the piece they cover: + +| Case | Red against | +|---|---| +| an over-threshold operator account is served around, not replaced | parent branch: reads `b`, expected `a` | +| deleting the preferred account releases the hold | pre-fix head `63217d161`: reads `undefined`, expected `b` | +| a successful dispatch spends the one-shot so the pool may move again | guard without consume: 15 of 69 rotation tests fail | + +The over-threshold case is also the one that states the user-facing rule plainly. An +account past its switch threshold is temporarily spent, not wrong: the pool serves the +request from elsewhere, and the operator's selection stays pointed where the operator put +it, so the window rolling over returns routing to it without a second manual pick. diff --git a/src/codex/routing.ts b/src/codex/routing.ts index a1f1f4fbd9..95c99e5d15 100644 --- a/src/codex/routing.ts +++ b/src/codex/routing.ts @@ -315,14 +315,30 @@ export function clearThreadAccountMapForAccount(accountId: string): void { } export function clearCodexUpstreamHealth(): void { + // Operator preferences are routing state, not health, but they live and die with the same + // reset points. Leaving them behind lets a selection from one context suppress the + // automatic cursor in the next one. + manualPreference.clear(); upstreamHealth.clear(); quotaScopedHealth.clear(); runtimeActiveCodexAccountId = undefined; + // The reconcile watermark is part of this state, not something that outlives it. Keeping + // it across a full reset is incoherent: there is no health left to protect, yet + // recordCodexUpstreamOutcome would still drop a writer whose generation predates the + // watermark for any account missing from the equally stale live set. Left behind, it also + // leaks between test files, which is how it was found. + lastReconciledGeneration = 0; + liveHealthAccountIds = new Set(); } export function clearCodexUpstreamHealthForAccount(accountId: string): void { upstreamHealth.delete(accountId); quotaScopedHealth.delete(accountId); + // Deletion is the third operator exit, next to pause and exclusion, and it is the one + // with no reconcile path behind it: once the account is gone nothing can succeed on it, + // so an unspent preference naming it would suppress the automatic cursor for every other + // account until the process restarts. + forgetManualPreference(accountId); } export function reconcileCodexRoutingHealth(context: GenerationContext): number { @@ -338,6 +354,14 @@ export function reconcileCodexRoutingHealth(context: GenerationContext): number quotaScopedHealth.delete(accountId); removed += 1; } + // Sweep preferences the same way, for the account set this generation actually has. The + // delete path above is the direct route; this is the one that catches an account removed + // by an edit the runtime never saw. Deliberately not counted in `removed`, which reports + // health rows. + for (const [poolKey, preferred] of manualPreference) { + if (context.codexAccountIds.has(preferred)) continue; + manualPreference.delete(poolKey); + } liveHealthAccountIds = new Set(context.codexAccountIds); lastReconciledGeneration = context.generation; return removed; @@ -871,6 +895,14 @@ export function resetCodexRoutingForManualSelection(accountId: string): void { clearThreadAccountMap(); // Manual selection is the operator source of truth — drop any automatic runtime cursor. runtimeActiveCodexAccountId = undefined; + // Record the pick as an unspent one-shot on the SHARED scope only. An independent scope + // gets no entry on purpose: every write site the guard protects is already skipped for + // independent scopes, so an entry there would be state nothing reads — and state nothing + // reads is what the next reader mistakes for a rule. + // + // Seeding happens ONLY here. A pool-driven promote must never create or move a preference, + // or the pool would manufacture an operator intent nobody expressed. + manualPreference.set(POOL_KEY_CODEX, accountId); // Seed the RR ring so the next unbound new session honors the manually selected account // under round-robin (affinity-cleared threads / null threadId). Fill-first already follows // config.activeCodexAccountId, which the caller persists before invoking this. @@ -1467,7 +1499,10 @@ function pickUnboundStrategyAccount( picked = pickRoundRobinAccount(poolKey, eligible, limit); if (!picked) return null; if (commitSharedActive) { - if (!isIndependentCodexQuotaScope(quotaScope)) rememberActiveCodexAccount(config, picked); + if (!isIndependentCodexQuotaScope(quotaScope) + && !manualPreferenceBlocks(codexPoolKeyForScope(quotaScope), picked)) { + rememberActiveCodexAccount(config, picked); + } } if (commitAffinity && threadId) bindThreadAffinity(threadId, picked, now, quotaScope); notePoolRotationSuccess(poolKey, picked, limit); @@ -1478,7 +1513,10 @@ function pickUnboundStrategyAccount( picked = pickFillFirstCodexAccount(config, now, quotaScope, selectionOptions); if (!picked) return null; if (commitSharedActive) { - if (!isIndependentCodexQuotaScope(quotaScope)) rememberActiveCodexAccount(config, picked); + if (!isIndependentCodexQuotaScope(quotaScope) + && !manualPreferenceBlocks(codexPoolKeyForScope(quotaScope), picked)) { + rememberActiveCodexAccount(config, picked); + } } if (commitAffinity && threadId) bindThreadAffinity(threadId, picked, now, quotaScope); return picked; @@ -1622,6 +1660,53 @@ export function pickAlternateCodexAccount( } /** Effective active: automatic runtime cursor, else operator/persisted selection. */ +/** + * Unspent operator selections, keyed by pool scope. + * + * Codex has no account-side equivalent of the Anthropic `selectionRevision`, so staleness + * cannot be detected by comparing values: a pool-driven promote legitimately moves the + * persisted active account, and reading that as staleness would silently spend the + * operator's one-shot. Invalidation is keyed to the OPERATOR path instead — another manual + * selection, the account leaving the pool, or a successful dispatch on it. + */ +const manualPreference = new Map(); + +/** + * Spend the one-shot for a pool scope once a dispatch on that account actually succeeded. + * This is the Codex analogue of `commitAnthropicSelectionRouting`, which Codex lacks. + * + * Wiring this BEFORE the guard below is not a style choice. Measured: with the guard in + * place and no consume site, the first manual selection freezes the automatic cursor + * permanently and 15 of 69 rotation tests fail. + */ +function consumeManualPreference(accountId: string, poolKey: string): void { + if (manualPreference.get(poolKey) === accountId) manualPreference.delete(poolKey); +} + +/** + * Drop an account's preference in every scope. Pause and exclusion do not route through + * `resetCodexRoutingForManualSelection`, so without this a preference could outlive the + * account it names and keep suppressing the automatic cursor. + */ +function forgetManualPreference(accountId: string): void { + for (const [poolKey, preferred] of manualPreference) { + if (preferred === accountId) manualPreference.delete(poolKey); + } +} + +/** + * True while an unspent operator selection for this scope names a DIFFERENT account than + * the automatic pick about to be recorded. + * + * Callers pass their own scope: an independent quota scope keeps its own entry and must + * never read the shared one. The failover promote does NOT consult this — see its call + * site for why. + */ +function manualPreferenceBlocks(poolKey: string, accountId: string): boolean { + const preferred = manualPreference.get(poolKey); + return preferred !== undefined && preferred !== accountId; +} + export function getEffectiveActiveCodexAccountId(config: OcxConfig): string | undefined { return runtimeActiveCodexAccountId ?? config.activeCodexAccountId; } @@ -1690,6 +1775,10 @@ export function reconcileCodexActiveAfterExclusion( now = Date.now(), ): string | null { const wasEffective = (getEffectiveActiveCodexAccountId(config) ?? MAIN_CODEX_ACCOUNT_ID) === excludedAccountId; + // Exclusion does not route through resetCodexRoutingForManualSelection, so the one-shot is + // revoked here too. A preference naming an account that can no longer serve would keep + // suppressing the automatic cursor with no way to clear it. + forgetManualPreference(excludedAccountId); if (config.activeCodexAccountId === excludedAccountId) { config.activeCodexAccountId = undefined; } @@ -2210,6 +2299,13 @@ export function resolveCodexAccountForThreadDetailed( && !preserveSharedSelectionForModelDetour && !isIndependentCodexQuotaScope(quotaScope) ) { + // NOT guarded by manualPreferenceBlocks, unlike preemption below. Measured: guarding + // it fails 8 cases in tests/codex-integration/codex-routing.test.ts, because a model + // detour is not the pool exercising discretion — the operator's account cannot serve + // this model at all. Under a rotating strategy this promote only moves the + // process-local cursor to whoever is actually serving and releases the pin; the + // operator's persisted activeCodexAccountId is left untouched either way, which is + // the thing the preference exists to protect. promoteActiveCodexAccount(config, strategyPick); } return { status: "selected", accountId: strategyPick }; @@ -2283,7 +2379,10 @@ export function resolveCodexAccountForThreadDetailed( !preserveSharedSelectionForModelDetour && !isIndependentCodexQuotaScope(quotaScope) ) { - rememberActiveCodexAccount(config, preempted); + // Preemption is an automatic pick competing with the operator, so it yields. + if (!manualPreferenceBlocks(POOL_KEY_CODEX, preempted)) { + rememberActiveCodexAccount(config, preempted); + } } active = preempted; } @@ -2354,6 +2453,9 @@ export function recordCodexUpstreamOutcome( */ dropSpentCredentialFailure(accountId); if (outcomeClass === "success") { + // The operator's one-shot is spent by a dispatch that actually worked, and only by that. + // A failed lookup leaves it unspent so the intent survives the failure. + consumeManualPreference(accountId, codexPoolKeyForScope(quotaScope)); const scopedProbe = meta.probeQuotaScope ? scopedHealthFor(accountId, meta.probeQuotaScope) : undefined; diff --git a/tests/codex-integration/codex-pool-rotation.test.ts b/tests/codex-integration/codex-pool-rotation.test.ts index 1a69a0e269..f5a5c38c49 100644 --- a/tests/codex-integration/codex-pool-rotation.test.ts +++ b/tests/codex-integration/codex-pool-rotation.test.ts @@ -17,6 +17,7 @@ import { } from "../../src/codex/account-priority"; import { clearCodexUpstreamHealth, + clearCodexUpstreamHealthForAccount, clearThreadAccountMap, CODEX_TRANSIENT_SOFT_AVOID_MS, previewCodexAccountForRequest, @@ -24,6 +25,7 @@ import { isCodexAccountInCooldown, pickAlternateCodexAccount, recordCodexUpstreamOutcome, + reconcileCodexRoutingHealth, resetCodexRoutingForManualSelection, resolveCodexAccountForThread, } from "../../src/codex/routing"; @@ -60,6 +62,24 @@ function saveTestCredential(id: string): void { }); } +/** + * `reconcileCodexRoutingHealth` ignores a generation it has already seen, and the counter is + * module state shared by every test in this file, so each call needs a strictly higher one. + */ +let sweepGeneration = 9_000_000; +function generationContext(codexAccountIds: ReadonlySet) { + sweepGeneration += 1; + return { + generation: sweepGeneration, + providerNames: new Set(), + comboIds: new Set(), + comboTargets: new Set(), + codexAccountIds, + oauthAccountKeys: new Set(), + configRoots: new Set(), + }; +} + function makeThreeAccountConfig(overrides: Partial = {}): OcxConfig { const ids = ["a", "b", "c"]; for (const id of ids) saveTestCredential(id); @@ -1021,4 +1041,172 @@ describe("selection order across rotation strategies", () => { expect(pickAlternateCodexAccount(config, "a", Date.now(), "shared", selectionOptions)) .toBe(MAIN_CODEX_ACCOUNT_ID); }); + + describe("an operator selection outranks the pool cursor", () => { + test("the pool moves, then a manual pick wins the next unbound dispatch", () => { + const config = makeThreeAccountConfig({ + accountPoolStrategy: "round-robin", + accountPoolStickyLimit: 1, + activeCodexAccountId: "a", + }); + updateAccountQuota("a", 10); + updateAccountQuota("b", 20); + updateAccountQuota("c", 30); + + // Let the pool move the runtime cursor off the operator account. + const first = resolveCodexAccountForThread(null, config)!; + recordCodexUpstreamOutcome(config, first, 429); + const promoted = getEffectiveActiveCodexAccountId(config); + expect(promoted).not.toBe(first); + + // The operator now selects the third account, one the pool did not choose and that + // carries no cooldown. Before this feature the runtime cursor kept winning and the + // next dispatch still served the pool account, which is the defect this phase fixes. + const chosen = ["a", "b", "c"].find(id => id !== first && id !== promoted)!; + config.activeCodexAccountId = chosen; + resetCodexRoutingForManualSelection(chosen); + + expect(getEffectiveActiveCodexAccountId(config)).toBe(chosen); + expect(resolveCodexAccountForThread(null, config)).toBe(chosen); + }); + + // The three tests below are the ones that carry the feature. Each was driven red against + // the parent branch first: an assertion that passes with the production change reverted + // proves nothing, and the first draft of this block was exactly that — three tests that + // all passed without the guard, because they only re-asserted what + // resetCodexRoutingForManualSelection and the exempt failover promote already did. + test("an over-threshold operator account is served around, not replaced", () => { + const config = makeThreeAccountConfig({ + accountPoolStrategy: "fill-first", + activeCodexAccountId: "a", + autoSwitchThreshold: 80, + }); + // The operator's account is past the switch threshold, so fill-first advances off it. + // This is the ordinary case the report was about: the account the operator chose is + // temporarily spent, not wrong. + updateAccountQuota("a", 90); + updateAccountQuota("b", 10); + updateAccountQuota("c", 10); + resetCodexRoutingForManualSelection("a"); + + const served = resolveCodexAccountForThread(null, config)!; + expect(served).not.toBe("a"); + + // Serving the request from another account is the pool doing its job. Writing that + // account over the operator's selection is not: when a's window rolls over there + // would be nothing left pointing back at it. Without the guard this reads `served`. + expect(getEffectiveActiveCodexAccountId(config)).toBe("a"); + }); + + test("a successful dispatch spends the one-shot so the pool may move again", () => { + const config = makeThreeAccountConfig({ + accountPoolStrategy: "fill-first", + activeCodexAccountId: "a", + autoSwitchThreshold: 80, + }); + updateAccountQuota("a", 10); + updateAccountQuota("b", 10); + updateAccountQuota("c", 10); + resetCodexRoutingForManualSelection("a"); + expect(resolveCodexAccountForThread(null, config)).toBe("a"); + + // The operator got what they asked for, so the hold is released. Without a consume + // site the preference is permanent and the cursor could never move again — measured: + // guard without consume fails 15 of the 69 rotation tests in this file. + recordCodexUpstreamOutcome(config, "a", 200); + + updateAccountQuota("a", 90); + const served = resolveCodexAccountForThread(null, config)!; + expect(served).not.toBe("a"); + expect(getEffectiveActiveCodexAccountId(config)).toBe(served); + }); + + test("deleting the preferred account releases the hold", () => { + const config = makeThreeAccountConfig({ + accountPoolStrategy: "round-robin", + accountPoolStickyLimit: 1, + activeCodexAccountId: "a", + }); + updateAccountQuota("a", 10); + updateAccountQuota("b", 20); + updateAccountQuota("c", 30); + resetCodexRoutingForManualSelection("a"); + + // Delete is the operator exit with no reconcile behind it: the account can never + // succeed again, so nothing else would ever spend the one-shot. The account-lifecycle + // delete path reaches routing through exactly this call. + config.codexAccounts = config.codexAccounts!.filter(account => account.id !== "a"); + config.activeCodexAccountId = undefined; + clearCodexUpstreamHealthForAccount("a"); + + const served = resolveCodexAccountForThread(null, config)!; + expect(served).not.toBe("a"); + // Without the revocation the preference outlives its account and blocks every write, + // so the effective active stays empty and the pool can never commit a replacement. + expect(getEffectiveActiveCodexAccountId(config)).toBe(served); + }); + + test("the generation sweep drops a preference whose account is gone", () => { + const config = makeThreeAccountConfig({ + accountPoolStrategy: "fill-first", + activeCodexAccountId: "a", + autoSwitchThreshold: 80, + }); + updateAccountQuota("a", 10); + updateAccountQuota("b", 10); + updateAccountQuota("c", 10); + resetCodexRoutingForManualSelection("a"); + + // The other removal path: an account edited out of the config by something the runtime + // never observed, so no delete call ever reached routing. The sweep is the only thing + // standing between that and a preference that can never be spent. + reconcileCodexRoutingHealth(generationContext(new Set(["b", "c"]))); + + config.codexAccounts = config.codexAccounts!.filter(account => account.id !== "a"); + config.activeCodexAccountId = undefined; + const served = resolveCodexAccountForThread(null, config)!; + expect(served).not.toBe("a"); + expect(getEffectiveActiveCodexAccountId(config)).toBe(served); + }); + + test("the generation sweep keeps a preference whose account is still live", () => { + const config = makeThreeAccountConfig({ + accountPoolStrategy: "fill-first", + activeCodexAccountId: "a", + autoSwitchThreshold: 80, + }); + updateAccountQuota("a", 90); + updateAccountQuota("b", 10); + updateAccountQuota("c", 10); + resetCodexRoutingForManualSelection("a"); + + // The half that makes the sweep a sweep rather than a reset: "a" is over threshold and + // is about to be routed around, but it is still in the roster, so the operator's + // selection has to survive. + reconcileCodexRoutingHealth(generationContext(new Set(["a", "b", "c"]))); + + const served = resolveCodexAccountForThread(null, config)!; + expect(served).not.toBe("a"); + expect(getEffectiveActiveCodexAccountId(config)).toBe("a"); + }); + + test("a 429 on the preferred account still promotes away from it", () => { + const config = makeThreeAccountConfig({ + accountPoolStrategy: "round-robin", + accountPoolStickyLimit: 1, + activeCodexAccountId: "a", + }); + updateAccountQuota("a", 10); + updateAccountQuota("b", 20); + updateAccountQuota("c", 30); + resetCodexRoutingForManualSelection("a"); + + // The failover promote is exempt from the preference guard on purpose: it only runs + // because the account in use just failed, so it is never an automatic pick competing + // with the operator. Guarding it would trap routing on a cooled account. + recordCodexUpstreamOutcome(config, "a", 429); + expect(isCodexAccountInCooldown("a")).toBe(true); + expect(getEffectiveActiveCodexAccountId(config)).not.toBe("a"); + }); + }); });