diff --git a/src/codex/routing.ts b/src/codex/routing.ts index 4eb35cc208..321d267aae 100644 --- a/src/codex/routing.ts +++ b/src/codex/routing.ts @@ -1628,7 +1628,7 @@ function transientDetourAccount( now: number, quotaScope?: CodexQuotaScope, selectionOptions?: CodexAccountUsabilityOptions, - allowFreshPick = true, + mode: "commit" | "peek" = "commit", ): string | null { const held = entry.transientDetourAccountId; if ( @@ -1641,11 +1641,12 @@ function transientDetourAccount( ) { return held; } - // A fresh pick is a side effect under round-robin: pickRoundRobinAccount commits and advances - // the ring. The preview path is contractually read-only, so it reports a detour only once the - // request path has actually chosen one, rather than moving the ring to answer a question. - if (!allowFreshPick) return null; - return pickAlternateCodexAccount(config, entry.accountId, now, quotaScope, selectionOptions); + // Preview must name the same account resolve would, including before any detour has been + // recorded -- but without advancing the round-robin ring, which is the one side effect in + // the selection path. + return mode === "peek" + ? peekAlternateCodexAccount(config, entry.accountId, now, quotaScope, selectionOptions) + : pickAlternateCodexAccount(config, entry.accountId, now, quotaScope, selectionOptions); } /** Earliest future shared short/weekly reset; missing evidence and ties use usage order. */ @@ -1943,6 +1944,32 @@ export function pickAlternateCodexAccount( return pickLowestUsageCodexAccount(config, excludeId, now, quotaScope, selectionOptions); } +/** + * The account {@link pickAlternateCodexAccount} WOULD return, without returning it. + * + * Only the round-robin branch has a side effect -- `pickRoundRobinAccount` commits the pick and + * advances the ring -- so every other strategy delegates rather than growing a second copy of + * the selection rule that could drift from it. + * + * This exists because preview and resolve have to agree on the FIRST transient detour, not just + * on later ones. Preview feeds subagent model-availability scoring, so a preview that reported + * the bound account while resolve was about to serve from a cool sibling could retire a model + * over usage the request would never have touched. + */ +function peekAlternateCodexAccount( + config: OcxConfig, + excludeId: string, + now: number, + quotaScope?: CodexQuotaScope, + selectionOptions?: CodexAccountUsabilityOptions, +): string | null { + if (accountPoolStrategyForScope(config, quotaScope) === "round-robin") { + const eligible = getEligiblePoolAccounts(config, excludeId, now, quotaScope, selectionOptions); + return peekRoundRobinAccount(codexPoolKeyForScope(quotaScope), eligible, stickyLimitForConfig(config)); + } + return pickAlternateCodexAccount(config, excludeId, now, quotaScope, selectionOptions); +} + /** Effective active: automatic runtime cursor, else operator/persisted selection. */ /** * Unspent operator selections, keyed by pool scope. @@ -2330,8 +2357,10 @@ function previewReusableAffinityAccount( !isTransientHoldExpired(entry, now) && isTransientOnlyAffinityBlock(config, entry, now, quotaScope, selectionOptions) ) { - const detour = transientDetourAccount(config, entry, now, quotaScope, selectionOptions, false); + const detour = transientDetourAccount(config, entry, now, quotaScope, selectionOptions, "peek"); if (detour !== null && detour !== entry.accountId) return detour; + // Nowhere to detour still means the thread keeps its account, so preview says so too. + return entry.accountId; } return null; } @@ -2407,7 +2436,19 @@ function resetFirstAffinityReplacement( const usage = computeCodexUsageScore(getAccountQuota(entry.accountId), getPoolAccountPlanForSelection(config, entry.accountId, selectionOptions), now); if (!mayRebindAffinityForQuota(config, entry.accountId, usage, threshold, selectionOptions)) return null; const candidates = getEligiblePoolAccounts(config, entry.accountId, now, quotaScope, selectionOptions, true) - .filter(id => hasCodexQuotaHeadroom(config, id, selectionOptions, now)); + // Headroom alone answers true for an UNMEASURED account, which is the right default for an + // unbound request and the wrong bet for a bound one. The quota strategy already excludes + // those through the strictly-cooler compare; reset ordering has no such compare, so it has + // to say it. Moving a warm conversation onto an account nobody has a reading for is a + // guess, not an improvement. + .filter(id => { + if (!hasCodexQuotaHeadroom(config, id, selectionOptions, now)) return false; + return !isUnknownUsage(computeCodexUsageScore( + getAccountQuota(id), + getPoolAccountPlanForSelection(config, id, selectionOptions), + now, + )); + }); return pickResetFirstCodexAccount(config, candidates, now, selectionOptions); } @@ -2672,12 +2713,17 @@ export function resolveCodexAccountForThreadDetailed( && isTransientOnlyAffinityBlock(config, detourEntry, now, quotaScope, selectionOptions) ) { const lane = transientDetourAccount(config, detourEntry, now, quotaScope, selectionOptions); + detourEntry.transientHoldSince ??= now; + detourEntry.lastUsedAt = now; if (lane !== null && lane !== detourEntry.accountId) { - detourEntry.transientHoldSince ??= now; detourEntry.transientDetourAccountId = lane; - detourEntry.lastUsedAt = now; return { status: "selected", accountId: lane }; } + // A provider-wide outage soft-avoids every sibling, so there is nowhere to detour. + // That is a statement about where this request can go, not about who owns the + // conversation: dropping the pin here would rebuild the cold prefix elsewhere for + // exactly the failure mode the hold exists to survive. + return { status: "selected", accountId: detourEntry.accountId }; } // Detour expiry or invalidation must not expire the ordinary task. Drop only // this model lane and select from ordinary/shared state below. @@ -2746,14 +2792,18 @@ export function resolveCodexAccountForThreadDetailed( && isTransientOnlyAffinityBlock(config, entry, now, quotaScope, selectionOptions) ) { const detour = transientDetourAccount(config, entry, now, quotaScope, selectionOptions); + entry.transientHoldSince ??= now; + entry.lastUsedAt = now; if (detour !== null && detour !== entry.accountId) { - entry.transientHoldSince ??= now; entry.transientDetourAccountId = detour; - entry.lastUsedAt = now; // Deliberately no promoteActiveCodexAccount and no rebind: this is one request routing // around a blip, not the pool deciding where the conversation now lives. return { status: "selected", accountId: detour }; } + // No sibling can take it either -- the usual shape of a provider-wide 503. The binding + // survives: "cannot send right now" and "forget which account owns this conversation" + // are different answers, and conflating them is what the hold was added to stop. + return { status: "selected", accountId: entry.accountId }; } // A model-only exclusion does not invalidate the shared task binding. Health, // generation, pause, cooldown, and failure evidence still retire it normally. diff --git a/tests/codex-integration/codex-pool-rotation.test.ts b/tests/codex-integration/codex-pool-rotation.test.ts index cdf7120cb1..335e5e183e 100644 --- a/tests/codex-integration/codex-pool-rotation.test.ts +++ b/tests/codex-integration/codex-pool-rotation.test.ts @@ -1472,6 +1472,63 @@ describe("selection order across rotation strategies", () => { expect(resolveCodexAccountForThread(threadId, config, recovered)).toBe("a"); }); + test("preview names the same detour as resolve before any detour is recorded", () => { + const config = makeThreeAccountConfig({ + accountPoolStrategy: "quota", + autoSwitchThreshold: 80, + activeCodexAccountId: "a", + upstreamFailoverThreshold: 3, + }); + const threadId = "preview-first-detour-thread"; + updateAccountQuota("a", 10); + updateAccountQuota("b", 20); + updateAccountQuota("c", 30); + const start = Date.now(); + expect(resolveCodexAccountForThread(threadId, config, start)).toBe("a"); + + recordCodexUpstreamOutcome(config, "a", 503, { now: start }); + recordCodexUpstreamOutcome(config, "a", 503, { now: start }); + recordCodexUpstreamOutcome(config, "a", 503, { now: start }); + + // Preview FIRST, before any detour exists. Subagent fallback scores this account's usage to + // decide whether a model is still reachable, so a preview that named the bound account here + // would retire a model over usage the request was never going to touch. + const previewed = previewCodexAccountForRequest(threadId, config, start); + const served = resolveCodexAccountForThread(threadId, config, start); + expect(previewed).toBe(served); + expect(served).not.toBe("a"); + }); + + test("a transient block with nowhere to detour keeps the binding", () => { + const config = makeThreeAccountConfig({ + accountPoolStrategy: "quota", + autoSwitchThreshold: 80, + activeCodexAccountId: "a", + upstreamFailoverThreshold: 3, + }); + const threadId = "provider-wide-outage-thread"; + updateAccountQuota("a", 10); + updateAccountQuota("b", 20); + updateAccountQuota("c", 30); + const start = Date.now(); + expect(resolveCodexAccountForThread(threadId, config, start)).toBe("a"); + + // A provider-wide 503 hits every account, so every sibling is soft-avoided too and the + // detour has nowhere to go. Losing the binding here would rebuild the cold prefix somewhere + // else for exactly the failure the hold exists to survive. + for (const id of ["a", "b", "c"]) { + recordCodexUpstreamOutcome(config, id, 503, { now: start }); + recordCodexUpstreamOutcome(config, id, 503, { now: start }); + recordCodexUpstreamOutcome(config, id, 503, { now: start }); + } + expect(resolveCodexAccountForThread(threadId, config, start)).toBe("a"); + expect(previewCodexAccountForRequest(threadId, config, start)).toBe("a"); + + // Once the outage clears the thread is still on its own warm account, with no rebind. + const recovered = start + 6 * 60_000; + expect(resolveCodexAccountForThread(threadId, config, recovered)).toBe("a"); + }); + test("a transient hold that outlives its window releases the binding", () => { const config = makeThreeAccountConfig({ accountPoolStrategy: "quota",