From 75d3fc9d1f699435e853f448c3db68261858b38e Mon Sep 17 00:00:00 2001 From: Mike Bannister Date: Sat, 15 Aug 2026 16:03:53 -0400 Subject: [PATCH 1/7] fix(runtime): tell the pinned-503 truth about pin source and reset time MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The pinned-account 503 always advised `codex-multi-auth unpin`, but the pin honored there is state.forcedAccountIndex ?? the persisted switch pin — and for a forced pin (--account / CODEX_MULTI_AUTH_FORCE_ACCOUNT_INDEX) unpin clears nothing, so the advice was wrong exactly where the message appears most: launcher-managed sessions that pin per invocation. The body also carried no recovery time even when the skip reason was a time-bounded record whose reset moment sat in the store. buildPinnedUnavailableErrorBody now takes optional context: pin_source ("forced" pins get a relaunch remedy instead of unpin), and reset_at/retry_after_ms threaded from the blocking record — the family's rate-limit record for a rate-limited skip, coolingDownUntil for a cooldown — with the message naming the reset moment when one is known. The proxy call site distinguishes the pin source it already tracks and resolves the reset for the request's family. Absent context, the body and message are byte-identical to before (the issue-474 expectations pass unchanged). --- lib/request/rate-limit-decision.ts | 42 +++++++++++++++++++++++++++++- lib/runtime-rotation-proxy.ts | 32 +++++++++++++++++++++++ test/rate-limit-decision.test.ts | 34 ++++++++++++++++++++++++ 3 files changed, 107 insertions(+), 1 deletion(-) diff --git a/lib/request/rate-limit-decision.ts b/lib/request/rate-limit-decision.ts index 88cddb962..6f775e5f1 100644 --- a/lib/request/rate-limit-decision.ts +++ b/lib/request/rate-limit-decision.ts @@ -185,12 +185,31 @@ export interface PinnedUnavailableErrorBody { code: "codex_pinned_account_unavailable"; pinnedAccountIndex: number | null; reason: string | null; + /** How the pin was set; forced pins are not cleared by `unpin`. */ + pin_source: "forced" | "manual" | null; + /** When the blocking record ends, when the skip reason is time-bounded. */ + reset_at: string | null; + retry_after_ms: number | null; account_skip_reasons: Record; } +export interface PinnedUnavailableContext { + /** + * "forced" when the pin came from the wrapper's forced-account mode + * (`--account` / CODEX_MULTI_AUTH_FORCE_ACCOUNT_INDEX), "manual" when it + * came from `switch`. `unpin` clears only the manual kind, so the remedy + * line must not suggest it for a forced pin. + */ + pinSource?: "forced" | "manual" | null; + /** Epoch ms when the blocking record ends (rate limit or cooldown). */ + resetAtMs?: number | null; + now?: number; +} + export function buildPinnedUnavailableErrorBody( pinnedIndex: number | null | undefined, accountSkipReasons: ReadonlyMap, + context?: PinnedUnavailableContext, ): PinnedUnavailableErrorBody { const normalizedPinnedIndex = typeof pinnedIndex === "number" ? pinnedIndex : null; @@ -205,11 +224,32 @@ export function buildPinnedUnavailableErrorBody( normalizedPinnedIndex === null ? "The pinned account" : `Pinned account ${normalizedPinnedIndex + 1}`; + const pinSource = context?.pinSource ?? null; + const resetAtMs = + typeof context?.resetAtMs === "number" && + Number.isFinite(context.resetAtMs) && + context.resetAtMs > 0 + ? context.resetAtMs + : null; + const now = context?.now ?? Date.now(); + const retryAfterMs = resetAtMs !== null ? Math.max(0, resetAtMs - now) : null; + const resetAt = resetAtMs !== null ? new Date(resetAtMs).toISOString() : null; + const waitSuffix = + resetAt !== null ? `; the recorded limit resets at ${resetAt}` : ""; + // A forced pin belongs to the launching process, not to ndy's persisted + // pin state, so `unpin` would clear nothing — say what actually helps. + const remedy = + pinSource === "forced" + ? "the pin was set by this session's launcher, so relaunch to select a different account" + : "run `codex-multi-auth status` for details, or `codex-multi-auth unpin` to allow rotation"; return { - message: `${accountPhrase} is currently unavailable${reasonSuffix}; run \`codex-multi-auth status\` for details, or \`codex-multi-auth unpin\` to allow rotation.`, + message: `${accountPhrase} is currently unavailable${reasonSuffix}${waitSuffix}; ${remedy}.`, code: "codex_pinned_account_unavailable", pinnedAccountIndex: normalizedPinnedIndex, reason: skipReason, + pin_source: pinSource, + reset_at: resetAt, + retry_after_ms: retryAfterMs, account_skip_reasons: Object.fromEntries( [...accountSkipReasons.entries()].map(([index, reason]) => [ String(index), diff --git a/lib/runtime-rotation-proxy.ts b/lib/runtime-rotation-proxy.ts index f447b985e..bc8cc0dca 100644 --- a/lib/runtime-rotation-proxy.ts +++ b/lib/runtime-rotation-proxy.ts @@ -72,6 +72,7 @@ import { responseHeadersForClient, withTimeout, } from "./request/stream-failover-runtime.js"; +import { getRateLimitResetTimeForFamily } from "./runtime/account-status.js"; import { chooseAccount } from "./runtime/rotation-account-selection.js"; import { createRotationProxyState, @@ -1573,9 +1574,40 @@ async function handleRequestInner( // null reason indicates a forecast/runtime state desync (the pinned // account was selected but no skip reason was recorded) — see #486. if (isPinned) { + const pinnedAccount = + typeof pinnedIndex === "number" + ? accountManager.getAccountByIndex(pinnedIndex) + : null; + const pinnedSkipReason = + typeof pinnedIndex === "number" + ? accountSkipReasons.get(pinnedIndex) ?? null + : null; + // A rate-limited skip is bounded by the family record and a cooldown + // by coolingDownUntil; anything else has no known recovery moment. + const pinnedResetAtMs = + pinnedAccount === null || pinnedSkipReason === null + ? null + : pinnedSkipReason === "rate-limited" + ? getRateLimitResetTimeForFamily( + pinnedAccount, + state.now(), + context.family, + ) + : pinnedSkipReason.startsWith("cooling-down") && + typeof pinnedAccount.coolingDownUntil === "number" && + pinnedAccount.coolingDownUntil > state.now() + ? pinnedAccount.coolingDownUntil + : null; const errorBody = buildPinnedUnavailableErrorBody( pinnedIndex, accountSkipReasons, + { + // typeof check so a forced index of 0 still reads as forced. + pinSource: + typeof state.forcedAccountIndex === "number" ? "forced" : "manual", + resetAtMs: pinnedResetAtMs, + now: state.now(), + }, ); if (errorBody.reason === null) { state.status.lastError = `pinned-503 missing skip reason (pinnedIndex=${pinnedIndex})`; diff --git a/test/rate-limit-decision.test.ts b/test/rate-limit-decision.test.ts index 40d6cf371..4410d3c42 100644 --- a/test/rate-limit-decision.test.ts +++ b/test/rate-limit-decision.test.ts @@ -295,5 +295,39 @@ describe("buildPinnedUnavailableErrorBody", () => { expect(body.message).not.toContain("Pinned account 1"); expect(body.message).not.toContain("("); expect(body.account_skip_reasons).toEqual({}); + expect(body.pin_source).toBeNull(); + expect(body.reset_at).toBeNull(); + expect(body.retry_after_ms).toBeNull(); + }); + + it("tailors the remedy to a forced pin and threads the recorded reset", () => { + const resetAtMs = 1_700_000_030_000; + const body = buildPinnedUnavailableErrorBody( + 0, + new Map([[0, "rate-limited"]]), + { pinSource: "forced", resetAtMs, now: 1_700_000_000_000 }, + ); + expect(body.pin_source).toBe("forced"); + expect(body.reset_at).toBe(new Date(resetAtMs).toISOString()); + expect(body.retry_after_ms).toBe(30_000); + expect(body.message).toContain( + `the recorded limit resets at ${new Date(resetAtMs).toISOString()}`, + ); + // A forced pin is not cleared by `unpin`; the remedy must not suggest it. + expect(body.message).toContain("set by this session's launcher"); + expect(body.message).not.toContain("unpin"); + }); + + it("keeps the unpin advice for manual pins and nulls an unknown reset", () => { + const body = buildPinnedUnavailableErrorBody( + 1, + new Map([[1, "disabled"]]), + { pinSource: "manual" }, + ); + expect(body.pin_source).toBe("manual"); + expect(body.reset_at).toBeNull(); + expect(body.retry_after_ms).toBeNull(); + expect(body.message).toContain("codex-multi-auth unpin"); + expect(body.message).not.toContain("resets at"); }); }); From 199cda59b140b7399836d8c0ee22d9c53842325f Mon Sep 17 00:00:00 2001 From: Mike Bannister Date: Sat, 15 Aug 2026 16:32:38 -0400 Subject: [PATCH 2/7] fix(runtime): derive pinned-503 recovery from account state, not the skip reason Review follow-ups on both fronts of the recovery metadata: - A direct 429 or network error on the pinned account reaches the 503 with the retry loop's selection verdict (already-attempted) as its skip reason, so gating the reset lookup on rate-limited/cooling-down strings suppressed recovery info exactly where it was freshest. The reset now comes straight from the account's persisted state. - With several overlapping records for the family, the account stays skipped until the LAST one expires, so the earliest reset would send a client straight back into a 503. getAccountRecoveryTimeForFamily returns the latest matching bound (records plus active cooldown), leaving getRateLimitResetTimeForFamily's earliest-reset semantics to its wait-display callers. Covered by test/account-status.test.ts (helper semantics) and two runtime proxy regressions (test/runtime-rotation-proxy.test.ts) that force a pinned account through a direct 429 and a network-error cooldown and assert the 503 carries pin_source, reason, reset_at, and retry_after_ms. --- lib/runtime-rotation-proxy.ts | 33 +++++------ lib/runtime/account-status.ts | 34 ++++++++++++ test/account-status.test.ts | 54 ++++++++++++++++++ test/runtime-rotation-proxy.test.ts | 85 +++++++++++++++++++++++++++++ 4 files changed, 187 insertions(+), 19 deletions(-) diff --git a/lib/runtime-rotation-proxy.ts b/lib/runtime-rotation-proxy.ts index bc8cc0dca..d7c56cc81 100644 --- a/lib/runtime-rotation-proxy.ts +++ b/lib/runtime-rotation-proxy.ts @@ -72,7 +72,7 @@ import { responseHeadersForClient, withTimeout, } from "./request/stream-failover-runtime.js"; -import { getRateLimitResetTimeForFamily } from "./runtime/account-status.js"; +import { getAccountRecoveryTimeForFamily } from "./runtime/account-status.js"; import { chooseAccount } from "./runtime/rotation-account-selection.js"; import { createRotationProxyState, @@ -1578,26 +1578,21 @@ async function handleRequestInner( typeof pinnedIndex === "number" ? accountManager.getAccountByIndex(pinnedIndex) : null; - const pinnedSkipReason = - typeof pinnedIndex === "number" - ? accountSkipReasons.get(pinnedIndex) ?? null - : null; - // A rate-limited skip is bounded by the family record and a cooldown - // by coolingDownUntil; anything else has no known recovery moment. + // Recovery comes from the pinned account's persisted state, not the + // recorded skip reason: a direct 429/cooldown on the pinned account + // reaches this 503 as "already-attempted" (the retry loop's selection + // verdict) while the record it just wrote is what actually bounds + // recovery — and with several overlapping records the account stays + // skipped until the LAST one expires, so the latest bound is the one + // worth advertising. const pinnedResetAtMs = - pinnedAccount === null || pinnedSkipReason === null + pinnedAccount === null ? null - : pinnedSkipReason === "rate-limited" - ? getRateLimitResetTimeForFamily( - pinnedAccount, - state.now(), - context.family, - ) - : pinnedSkipReason.startsWith("cooling-down") && - typeof pinnedAccount.coolingDownUntil === "number" && - pinnedAccount.coolingDownUntil > state.now() - ? pinnedAccount.coolingDownUntil - : null; + : getAccountRecoveryTimeForFamily( + pinnedAccount, + state.now(), + context.family, + ); const errorBody = buildPinnedUnavailableErrorBody( pinnedIndex, accountSkipReasons, diff --git a/lib/runtime/account-status.ts b/lib/runtime/account-status.ts index 267cf663e..0f72e8ed4 100644 --- a/lib/runtime/account-status.ts +++ b/lib/runtime/account-status.ts @@ -38,6 +38,40 @@ export function getRateLimitResetTimeForFamily( return minReset; } +/** + * The moment the account becomes usable again for `family`: the LATEST + * matching rate-limit record plus any active cooldown. Distinct from + * getRateLimitResetTimeForFamily, whose earliest-reset answer feeds wait + * displays: the account stays skipped while ANY matching record is active, + * so a retry hint built from the earliest reset would send clients back + * into a 503. Null when nothing bounds recovery. + */ +export function getAccountRecoveryTimeForFamily( + account: { + rateLimitResetTimes?: Record; + coolingDownUntil?: number; + }, + now: number, + family: ModelFamily, +): number | null { + let latest: number | null = null; + const consider = (value: number | undefined): void => { + if (typeof value !== "number" || !Number.isFinite(value)) return; + if (value <= now) return; + if (latest === null || value > latest) latest = value; + }; + const times = account.rateLimitResetTimes; + if (times) { + const prefix = `${family}:`; + for (const [key, value] of Object.entries(times)) { + if (key !== family && !key.startsWith(prefix)) continue; + consider(value); + } + } + consider(account.coolingDownUntil); + return latest; +} + export function formatRateLimitEntry( account: { rateLimitResetTimes?: Record }, now: number, diff --git a/test/account-status.test.ts b/test/account-status.test.ts index b4313d24b..7ad30f753 100644 --- a/test/account-status.test.ts +++ b/test/account-status.test.ts @@ -1,6 +1,7 @@ import { describe, expect, it } from "vitest"; import { formatRateLimitEntry, + getAccountRecoveryTimeForFamily, getRateLimitResetTimeForFamily, resolveActiveIndex, } from "../lib/runtime/account-status.js"; @@ -101,3 +102,56 @@ describe("account status helpers", () => { ).toBe("resets in 4000ms"); }); }); + +describe("getAccountRecoveryTimeForFamily", () => { + it("returns the LATEST matching reset so a retry lands after real recovery", () => { + // Two overlapping records for the family: the account stays skipped + // until the last one expires, so the earliest reset would send a + // client straight back into a 503. + expect( + getAccountRecoveryTimeForFamily( + { rateLimitResetTimes: { codex: 3_000, "codex:5h": 9_000 } }, + 1_000, + "codex", + ), + ).toBe(9_000); + }); + + it("ignores other families and expired records", () => { + expect( + getAccountRecoveryTimeForFamily( + { rateLimitResetTimes: { "gpt-5.2": 9_000, codex: 500 } }, + 1_000, + "codex", + ), + ).toBeNull(); + }); + + it("folds an active cooldown into the recovery moment", () => { + expect( + getAccountRecoveryTimeForFamily( + { rateLimitResetTimes: { codex: 3_000 }, coolingDownUntil: 7_000 }, + 1_000, + "codex", + ), + ).toBe(7_000); + expect( + getAccountRecoveryTimeForFamily( + { coolingDownUntil: 2_000 }, + 1_000, + "codex", + ), + ).toBe(2_000); + }); + + it("returns null when nothing bounds recovery", () => { + expect(getAccountRecoveryTimeForFamily({}, 1_000, "codex")).toBeNull(); + expect( + getAccountRecoveryTimeForFamily( + { coolingDownUntil: 900 }, + 1_000, + "codex", + ), + ).toBeNull(); + }); +}); diff --git a/test/runtime-rotation-proxy.test.ts b/test/runtime-rotation-proxy.test.ts index a892bf923..83c007519 100644 --- a/test/runtime-rotation-proxy.test.ts +++ b/test/runtime-rotation-proxy.test.ts @@ -799,6 +799,91 @@ describe("runtime rotation proxy", () => { expect(calls).toHaveLength(0); }); + it("carries pin source and recovery metadata when the forced account 429s directly", async () => { + const now = Date.now(); + const accountManager = new AccountManager(undefined, createStorage(now, 2)); + const { calls, fetchImpl } = createRecordingFetch( + () => + new Response('{"error":{"message":"rate limited"}}', { + status: HTTP_STATUS.TOO_MANY_REQUESTS, + headers: { + "content-type": "application/json", + "retry-after": "120", + }, + }), + ); + const proxy = await startProxy({ + accountManager, + fetchImpl, + options: { forcedAccountIndex: 0 }, + }); + + const response = await postResponses(proxy, { + model: "gpt-5-codex", + stream: true, + input: [{ type: "message", role: "user", content: "hi" }], + }); + + expect(response.status).toBe(HTTP_STATUS.SERVICE_UNAVAILABLE); + // One direct attempt on the pin, then fail-hard — never a rotation. + expect(calls).toHaveLength(1); + const payload = (await response.json()) as { + error: { + code: string; + pin_source: string | null; + reason: string | null; + reset_at: string | null; + retry_after_ms: number | null; + message: string; + }; + }; + expect(payload.error.code).toBe("codex_pinned_account_unavailable"); + expect(payload.error.pin_source).toBe("forced"); + // The retry loop's selection verdict — recovery metadata must not + // depend on this string, only on the account's persisted state. + expect(payload.error.reason).toBe("already-attempted"); + expect(payload.error.retry_after_ms).toBeGreaterThan(0); + expect(Date.parse(payload.error.reset_at ?? "")).toBeGreaterThan(now); + expect(payload.error.message).toContain("the recorded limit resets at"); + expect(payload.error.message).toContain("launcher"); + expect(payload.error.message).not.toContain("unpin"); + }); + + it("carries cooldown recovery metadata when the forced account fails with a network error", async () => { + const now = Date.now(); + const accountManager = new AccountManager(undefined, createStorage(now, 2)); + const { calls, fetchImpl } = createRecordingFetch(() => { + throw new TypeError("fetch failed"); + }); + const proxy = await startProxy({ + accountManager, + fetchImpl, + options: { forcedAccountIndex: 0 }, + }); + + const response = await postResponses(proxy, { + model: "gpt-5-codex", + stream: true, + input: [{ type: "message", role: "user", content: "hi" }], + }); + + expect(response.status).toBe(HTTP_STATUS.SERVICE_UNAVAILABLE); + expect(calls).toHaveLength(1); + const payload = (await response.json()) as { + error: { + code: string; + pin_source: string | null; + reset_at: string | null; + retry_after_ms: number | null; + }; + }; + expect(payload.error.code).toBe("codex_pinned_account_unavailable"); + expect(payload.error.pin_source).toBe("forced"); + // The network-error cooldown bounds recovery. + expect(payload.error.retry_after_ms).toBeGreaterThan(0); + expect(Date.parse(payload.error.reset_at ?? "")).toBeGreaterThan(now); + }); + it("reads the forced account from CODEX_MULTI_AUTH_FORCE_ACCOUNT_INDEX env when no option is passed (#623)", async () => { // This is the exact mechanism the pin uses to cross the launcher -> detached // app-helper process boundary: no option, env only. From 5b76c68542c4ee8c5087ede3b5d1c5ad07b400ed Mon Sep 17 00:00:00 2001 From: Mike Bannister Date: Sat, 15 Aug 2026 16:53:00 -0400 Subject: [PATCH 3/7] fix(runtime): include the circuit deadline in pinned-503 recovery MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An open circuit breaker outlives the short failure cooldowns that tripped it, so recovery derived only from the persisted account state advertised an early reset — or none at all once the cooldown lapsed — while requests kept 503ing until the breaker's own deadline. The 503 recovery is now the later of the account-state bound and the breaker's next-attempt time, exposed through AccountManager.getCircuitRecoveryTime over the breaker's existing getTimeUntilAvailable. A proxy regression trips the default breaker on a forced pin (with the network-error cooldown zeroed so every request records a failure) and asserts the advertised recovery is the circuit deadline, not the elapsed cooldown. --- lib/accounts.ts | 16 +++++++++ lib/runtime-rotation-proxy.ts | 15 ++++++++- test/runtime-rotation-proxy.test.ts | 51 +++++++++++++++++++++++++++++ 3 files changed, 81 insertions(+), 1 deletion(-) diff --git a/lib/accounts.ts b/lib/accounts.ts index 8b7152e3f..5854ce9ce 100644 --- a/lib/accounts.ts +++ b/lib/accounts.ts @@ -1294,6 +1294,22 @@ export class AccountManager { return getCircuitBreaker(getAccountCircuitKey(account)).isAvailable(); } + /** + * When the account's circuit breaker will admit an attempt again, as an + * epoch-ms deadline — null when it already would. Lets the pinned-503 + * recovery metadata cover circuit-open skips, whose deadline lives in the + * breaker rather than the persisted account record. + */ + getCircuitRecoveryTime( + account: ManagedAccount, + now = Date.now(), + ): number | null { + const waitMs = getCircuitBreaker( + getAccountCircuitKey(account), + ).getTimeUntilAvailable(now); + return waitMs > 0 ? now + waitMs : null; + } + incrementAuthFailures(account: ManagedAccount): number { account.consecutiveAuthFailures = (account.consecutiveAuthFailures ?? 0) + 1; diff --git a/lib/runtime-rotation-proxy.ts b/lib/runtime-rotation-proxy.ts index d7c56cc81..949ee2e94 100644 --- a/lib/runtime-rotation-proxy.ts +++ b/lib/runtime-rotation-proxy.ts @@ -1585,7 +1585,7 @@ async function handleRequestInner( // recovery — and with several overlapping records the account stays // skipped until the LAST one expires, so the latest bound is the one // worth advertising. - const pinnedResetAtMs = + const pinnedStateRecoveryAtMs = pinnedAccount === null ? null : getAccountRecoveryTimeForFamily( @@ -1593,6 +1593,19 @@ async function handleRequestInner( state.now(), context.family, ); + // An open circuit outlives the short failure cooldowns that tripped + // it; its deadline lives in the breaker, not the account record. + const pinnedCircuitRecoveryAtMs = + pinnedAccount === null + ? null + : accountManager.getCircuitRecoveryTime(pinnedAccount, state.now()); + const pinnedResetAtMs = + pinnedStateRecoveryAtMs === null && pinnedCircuitRecoveryAtMs === null + ? null + : Math.max( + pinnedStateRecoveryAtMs ?? 0, + pinnedCircuitRecoveryAtMs ?? 0, + ); const errorBody = buildPinnedUnavailableErrorBody( pinnedIndex, accountSkipReasons, diff --git a/test/runtime-rotation-proxy.test.ts b/test/runtime-rotation-proxy.test.ts index 83c007519..282ecde3b 100644 --- a/test/runtime-rotation-proxy.test.ts +++ b/test/runtime-rotation-proxy.test.ts @@ -884,6 +884,57 @@ describe("runtime rotation proxy", () => { expect(Date.parse(payload.error.reset_at ?? "")).toBeGreaterThan(now); }); + it("advertises the circuit deadline once repeated failures open the pinned account's breaker", async () => { + const now = Date.now(); + const accountManager = new AccountManager(undefined, createStorage(now, 2)); + const { calls, fetchImpl } = createRecordingFetch(() => { + throw new TypeError("fetch failed"); + }); + // Zero the network-error cooldown so every request reaches upstream + // and records a breaker failure; otherwise the cooldown absorbs the + // retries and the breaker never opens. + vi.stubEnv("CODEX_AUTH_NETWORK_ERROR_COOLDOWN_MS", "0"); + const proxy = await startProxy({ + accountManager, + fetchImpl, + options: { forcedAccountIndex: 0 }, + }); + vi.unstubAllEnvs(); + const body = { + model: "gpt-5-codex", + stream: true, + input: [{ type: "message", role: "user", content: "hi" }], + }; + + // Three failing requests trip the default breaker (threshold 3). + for (let attempt = 0; attempt < 3; attempt += 1) { + const failed = await postResponses(proxy, body); + expect(failed.status).toBe(HTTP_STATUS.SERVICE_UNAVAILABLE); + } + expect(calls).toHaveLength(3); + + const response = await postResponses(proxy, body); + expect(response.status).toBe(HTTP_STATUS.SERVICE_UNAVAILABLE); + // Circuit-open skips before any upstream attempt. + expect(calls).toHaveLength(3); + const payload = (await response.json()) as { + error: { + code: string; + pin_source: string | null; + reset_at: string | null; + retry_after_ms: number | null; + }; + }; + expect(payload.error.code).toBe("codex_pinned_account_unavailable"); + expect(payload.error.pin_source).toBe("forced"); + // The breaker's 30s reset outlives the short network-error cooldown + // that tripped it; the advertised recovery must be the circuit + // deadline, not the already-elapsed cooldown. + expect(payload.error.retry_after_ms).toBeGreaterThan(10_000); + expect(payload.error.retry_after_ms).toBeLessThanOrEqual(30_000); + expect(Date.parse(payload.error.reset_at ?? "")).toBeGreaterThan(now); + }); + it("reads the forced account from CODEX_MULTI_AUTH_FORCE_ACCOUNT_INDEX env when no option is passed (#623)", async () => { // This is the exact mechanism the pin uses to cross the launcher -> detached // app-helper process boundary: no option, env only. From c112ddc9f7903f7fe93d330fd12eb8663500765b Mon Sep 17 00:00:00 2001 From: Mike Bannister Date: Sat, 15 Aug 2026 17:15:49 -0400 Subject: [PATCH 4/7] fix(runtime): bound pinned-503 recovery by the keys that gate the request MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Selection consults exactly two rate-limit keys per request — the family-wide key and the requested model's key (isRateLimitedForFamily) — so another model's record in the same family never blocks the request and must not inflate its advertised recovery. getAccountRecoveryTimeForFamily now takes the model and considers only those gating keys plus the active cooldown; the proxy passes the request's model through. Unit coverage pins both directions: an unrelated model's later record is ignored, and a model-scoped record alone does not gate a model-less request. --- lib/runtime-rotation-proxy.ts | 1 + lib/runtime/account-status.ts | 22 ++++++++++-------- test/account-status.test.ts | 43 +++++++++++++++++++++++++++++++---- 3 files changed, 51 insertions(+), 15 deletions(-) diff --git a/lib/runtime-rotation-proxy.ts b/lib/runtime-rotation-proxy.ts index 949ee2e94..c11bb6e1e 100644 --- a/lib/runtime-rotation-proxy.ts +++ b/lib/runtime-rotation-proxy.ts @@ -1592,6 +1592,7 @@ async function handleRequestInner( pinnedAccount, state.now(), context.family, + context.model, ); // An open circuit outlives the short failure cooldowns that tripped // it; its deadline lives in the breaker, not the account record. diff --git a/lib/runtime/account-status.ts b/lib/runtime/account-status.ts index 0f72e8ed4..afc8b312a 100644 --- a/lib/runtime/account-status.ts +++ b/lib/runtime/account-status.ts @@ -39,12 +39,16 @@ export function getRateLimitResetTimeForFamily( } /** - * The moment the account becomes usable again for `family`: the LATEST - * matching rate-limit record plus any active cooldown. Distinct from + * The moment the account becomes usable again for a `family`/`model` + * request: the LATEST bound among the records that actually gate that + * request plus any active cooldown. Two deliberate differences from * getRateLimitResetTimeForFamily, whose earliest-reset answer feeds wait - * displays: the account stays skipped while ANY matching record is active, - * so a retry hint built from the earliest reset would send clients back - * into a 503. Null when nothing bounds recovery. + * displays: the account stays skipped while ANY gating record is active, + * so the earliest reset would send clients back into a 503 — and only the + * keys selection consults (`family`, plus `family:` when a model is + * known; see isRateLimitedForFamily) may contribute, because another + * model's record does not block this request and would overstate its + * recovery. Null when nothing bounds recovery. */ export function getAccountRecoveryTimeForFamily( account: { @@ -53,6 +57,7 @@ export function getAccountRecoveryTimeForFamily( }, now: number, family: ModelFamily, + model?: string | null, ): number | null { let latest: number | null = null; const consider = (value: number | undefined): void => { @@ -62,11 +67,8 @@ export function getAccountRecoveryTimeForFamily( }; const times = account.rateLimitResetTimes; if (times) { - const prefix = `${family}:`; - for (const [key, value] of Object.entries(times)) { - if (key !== family && !key.startsWith(prefix)) continue; - consider(value); - } + consider(times[family]); + if (model) consider(times[`${family}:${model}`]); } consider(account.coolingDownUntil); return latest; diff --git a/test/account-status.test.ts b/test/account-status.test.ts index 7ad30f753..6effc24a8 100644 --- a/test/account-status.test.ts +++ b/test/account-status.test.ts @@ -104,19 +104,52 @@ describe("account status helpers", () => { }); describe("getAccountRecoveryTimeForFamily", () => { - it("returns the LATEST matching reset so a retry lands after real recovery", () => { - // Two overlapping records for the family: the account stays skipped - // until the last one expires, so the earliest reset would send a - // client straight back into a 503. + it("returns the LATEST gating reset so a retry lands after real recovery", () => { + // Family-wide and requested-model records overlap: the account stays + // skipped until the later one expires, so the earliest reset would + // send a client straight back into a 503. expect( getAccountRecoveryTimeForFamily( - { rateLimitResetTimes: { codex: 3_000, "codex:5h": 9_000 } }, + { + rateLimitResetTimes: { + codex: 3_000, + "codex:gpt-5-codex": 9_000, + }, + }, 1_000, "codex", + "gpt-5-codex", ), ).toBe(9_000); }); + it("ignores records that do not gate the request", () => { + // Another model's record in the same family does not block this + // request (selection checks only the family key and the requested + // model's key), so it must not inflate the advertised recovery. + expect( + getAccountRecoveryTimeForFamily( + { + rateLimitResetTimes: { + codex: 3_000, + "codex:gpt-5.3-codex": 9_000, + }, + }, + 1_000, + "codex", + "gpt-5-codex", + ), + ).toBe(3_000); + // Without a model only the family-wide key gates. + expect( + getAccountRecoveryTimeForFamily( + { rateLimitResetTimes: { "codex:gpt-5-codex": 9_000 } }, + 1_000, + "codex", + ), + ).toBeNull(); + }); + it("ignores other families and expired records", () => { expect( getAccountRecoveryTimeForFamily( From 2a03c2b812f90edc220e6c9510834544133c8737 Mon Sep 17 00:00:00 2001 From: Mike Bannister Date: Sat, 15 Aug 2026 17:41:36 -0400 Subject: [PATCH 5/7] fix(runtime): no timed recovery under a permanent pinned blocker, and reuse getQuotaKey MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups: a disabled, workspace-disabled, auth-invalidated, policy-blocked, or out-of-range pinned account stays unselectable after any concurrent rate-limit record or cooldown expires, so the 503 no longer advertises that record's expiry — selection rejects such an account before any attempt, so the recorded skip reason is reliably the permanent one and gates the suppression. A proxy regression pins a disabled account carrying an active record and asserts reset_at and retry_after_ms stay null. The recovery helper also derives its record keys through getQuotaKey instead of a hand-rolled template, so the shape cannot drift from what markRateLimitedWithReason persists. --- lib/runtime-rotation-proxy.ts | 31 +++++++++++++++++++-- lib/runtime/account-status.ts | 5 ++-- test/runtime-rotation-proxy.test.ts | 42 +++++++++++++++++++++++++++++ 3 files changed, 74 insertions(+), 4 deletions(-) diff --git a/lib/runtime-rotation-proxy.ts b/lib/runtime-rotation-proxy.ts index c11bb6e1e..3f04b1261 100644 --- a/lib/runtime-rotation-proxy.ts +++ b/lib/runtime-rotation-proxy.ts @@ -3,6 +3,7 @@ import { createServer, type IncomingMessage, type Server, type ServerResponse } import type { Socket } from "node:net"; import { AccountManager, + AUTH_INVALIDATION_MARKER, extractAccountId, type ManagedAccount, } from "./accounts.js"; @@ -166,6 +167,19 @@ function toUrlHost(host: string): string { // failures surfaced only as a last-write-wins status.lastError string. Logs are // level-gated and carry the per-request correlation id set in handleRequest. const proxyLog = createLogger("runtime-proxy"); + +/** + * Pinned skip reasons that no timer clears: the account stays unselectable + * after any concurrent rate-limit record or cooldown expires, so the pinned + * 503 must not advertise that record's expiry as a recovery time. + */ +const PINNED_PERMANENT_SKIP_REASONS: ReadonlySet = new Set([ + "missing", + "disabled", + "workspace-disabled", + "policy-blocked", + AUTH_INVALIDATION_MARKER, +]); const DEFAULT_QUOTA_REMAINING_THRESHOLD = 10; /** @internal Stable identity key for in-memory quota snapshots across reloads. */ @@ -1578,7 +1592,19 @@ async function handleRequestInner( typeof pinnedIndex === "number" ? accountManager.getAccountByIndex(pinnedIndex) : null; - // Recovery comes from the pinned account's persisted state, not the + const pinnedSkipReason = + typeof pinnedIndex === "number" + ? accountSkipReasons.get(pinnedIndex) ?? null + : null; + // A permanent blocker (disabled, no enabled workspace, invalidated + // auth, policy block, out-of-range pin) outlives every timed record, + // so advertising a record's expiry would invite a retry into another + // 503. Selection rejects such an account before any attempt, so the + // recorded skip reason is the permanent one in exactly these cases. + const pinnedBlockedPermanently = + pinnedSkipReason !== null && + PINNED_PERMANENT_SKIP_REASONS.has(pinnedSkipReason); + // Otherwise recovery comes from the pinned account's persisted state, not the // recorded skip reason: a direct 429/cooldown on the pinned account // reaches this 503 as "already-attempted" (the retry loop's selection // verdict) while the record it just wrote is what actually bounds @@ -1601,7 +1627,8 @@ async function handleRequestInner( ? null : accountManager.getCircuitRecoveryTime(pinnedAccount, state.now()); const pinnedResetAtMs = - pinnedStateRecoveryAtMs === null && pinnedCircuitRecoveryAtMs === null + pinnedBlockedPermanently || + (pinnedStateRecoveryAtMs === null && pinnedCircuitRecoveryAtMs === null) ? null : Math.max( pinnedStateRecoveryAtMs ?? 0, diff --git a/lib/runtime/account-status.ts b/lib/runtime/account-status.ts index afc8b312a..c6d47bafb 100644 --- a/lib/runtime/account-status.ts +++ b/lib/runtime/account-status.ts @@ -1,3 +1,4 @@ +import { getQuotaKey } from "../accounts/rate-limits.js"; import type { ModelFamily } from "../prompts/codex.js"; export function resolveActiveIndex( @@ -67,8 +68,8 @@ export function getAccountRecoveryTimeForFamily( }; const times = account.rateLimitResetTimes; if (times) { - consider(times[family]); - if (model) consider(times[`${family}:${model}`]); + consider(times[getQuotaKey(family)]); + if (model) consider(times[getQuotaKey(family, model)]); } consider(account.coolingDownUntil); return latest; diff --git a/test/runtime-rotation-proxy.test.ts b/test/runtime-rotation-proxy.test.ts index 282ecde3b..21158983b 100644 --- a/test/runtime-rotation-proxy.test.ts +++ b/test/runtime-rotation-proxy.test.ts @@ -935,6 +935,48 @@ describe("runtime rotation proxy", () => { expect(Date.parse(payload.error.reset_at ?? "")).toBeGreaterThan(now); }); + it("suppresses timed recovery when a permanent blocker holds the pinned account", async () => { + const now = Date.now(); + const accountManager = new AccountManager(undefined, createStorage(now, 2)); + const pinned = accountManager.getAccountByIndex(0); + if (!pinned) throw new Error("setup failed"); + // Disabled outlives the record: after the rate limit expires the + // account is still unselectable, so no recovery time is honest. + pinned.enabled = false; + pinned.rateLimitResetTimes = { codex: now + 60_000 }; + const { calls, fetchImpl } = createRecordingFetch(() => + textEventStream("data: should-not-be-reached\n\n"), + ); + const proxy = await startProxy({ + accountManager, + fetchImpl, + options: { forcedAccountIndex: 0 }, + }); + + const response = await postResponses(proxy, { + model: "gpt-5-codex", + stream: true, + input: [{ type: "message", role: "user", content: "hi" }], + }); + + expect(response.status).toBe(HTTP_STATUS.SERVICE_UNAVAILABLE); + expect(calls).toHaveLength(0); + const payload = (await response.json()) as { + error: { + code: string; + reason: string | null; + reset_at: string | null; + retry_after_ms: number | null; + message: string; + }; + }; + expect(payload.error.code).toBe("codex_pinned_account_unavailable"); + expect(payload.error.reason).toBe("disabled"); + expect(payload.error.reset_at).toBeNull(); + expect(payload.error.retry_after_ms).toBeNull(); + expect(payload.error.message).not.toContain("resets at"); + }); + it("reads the forced account from CODEX_MULTI_AUTH_FORCE_ACCOUNT_INDEX env when no option is passed (#623)", async () => { // This is the exact mechanism the pin uses to cross the launcher -> detached // app-helper process boundary: no option, env only. From 9caadc99615ac42ced591f3d851f1cb7704344e3 Mon Sep 17 00:00:00 2001 From: ndycode Date: Sun, 16 Aug 2026 22:24:46 +0800 Subject: [PATCH 6/7] fix(runtime): keep the pinned 503 alive on a corrupt deadline, and document the new contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four follow-ups on this PR's own surface, none of which the bots raised. - `new Date(resetAtMs).toISOString()` was guarded only by `Number.isFinite` and `> 0`. resetAtMs is read straight off persisted account state (rateLimitResetTimes, coolingDownUntil); markAccountCoolingDown clamps only the low side and nothing re-validates either on load. A finite but absurd value past the ECMAScript time limit therefore threw RangeError inside handleRequestInner, whose outer catch replies `codex_runtime_rotation_proxy_error` 500 — so a corrupt deadline turned the pinned 503 into a generic error carrying no pinnedAccountIndex, no reason, and no account_skip_reasons. Exactly the diagnostics this PR exists to deliver, lost on the one input that most needs them. Such a value bounds nothing usable, so it is now read as "no known recovery". - docs/reference/error-contracts.md still described `codex_pinned_account_unavailable` as a manual-pin-only condition and still told integrators to run `unpin` — the advice this PR proves wrong for forced pins — and did not mention pin_source, reset_at, or retry_after_ms at all. The entry now covers both pin kinds and a field table documents the three additions, including that reset_at is null under a permanent blocker and that retry_after_ms is the latest bound for one account while the pool-exhausted code reports the earliest across the pool. - The circuit-breaker regression called `vi.unstubAllEnvs()` between `startProxy` and its assertions. If startProxy rejected, the unstub never ran and a zero network-error cooldown leaked into every later test in the file — the shared afterEach clears trackers and breakers but not env stubs. Restored in a finally, and scoped to the one variable rather than unstubbing everything an enclosing hook may have set. - Dropped a stray author reference from a comment shipping in the published package source. Tests: two cases in test/rate-limit-decision.test.ts pin the guard - an out-of-range deadline yields null reset_at/retry_after_ms while the reason, pin source, and index survive, and a deadline at the exact limit still reports. The first throws RangeError without the fix. Suites: test/rate-limit-decision.test.ts, test/runtime-rotation-proxy.test.ts, test/documentation.test.ts. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01WYnWb16vmd1XdS33GPtdxi --- docs/reference/error-contracts.md | 12 +++++++++- lib/request/rate-limit-decision.ts | 21 ++++++++++++++--- test/rate-limit-decision.test.ts | 35 +++++++++++++++++++++++++++++ test/runtime-rotation-proxy.test.ts | 26 ++++++++++++++++----- 4 files changed, 84 insertions(+), 10 deletions(-) diff --git a/docs/reference/error-contracts.md b/docs/reference/error-contracts.md index 80a3de63e..ffe3228d9 100644 --- a/docs/reference/error-contracts.md +++ b/docs/reference/error-contracts.md @@ -120,11 +120,21 @@ The default-on localhost Responses proxy returns JSON error payloads with a stab | `runtime_rotation_proxy_unauthorized` | `401` | Local request did not include the per-process proxy client key | | `runtime_rotation_proxy_payload_too_large` | `413` | Request body exceeded the proxy safety cap | | `codex_runtime_rotation_pool_exhausted` | `429` or `503` | No managed account can currently service the runtime request | -| `codex_pinned_account_unavailable` | `503` | A manual pin is set (via `codex-multi-auth switch`) but the pinned account is rate-limited, cooling down, disabled, or blocked by policy. Run `codex-multi-auth status` for details, or `codex-multi-auth unpin` to allow rotation | +| `codex_pinned_account_unavailable` | `503` | A pin is in force — either a manual pin (`codex-multi-auth switch`) or a forced pin set per invocation by the launcher (`--account` / `CODEX_MULTI_AUTH_FORCE_ACCOUNT_INDEX`) — but the pinned account is rate-limited, cooling down, disabled, or blocked by policy. The remedy depends on `pin_source`: a manual pin clears with `codex-multi-auth unpin`, a forced pin does not and needs a relaunch | | `codex_runtime_rotation_proxy_error` | `500` | Proxy failed before forwarding the request | Pool exhaustion includes a `reason`, `retry_after_ms`, and a hint to run `codex-multi-auth rotation status`. Pinned-account-unavailable responses include a `pinnedAccountIndex` field identifying the pinned account, a structured `reason` field carrying the runtime skip reason (for example `rate-limited`, `cooling-down:auth-failure`, `circuit-open`, `disabled`, `workspace-disabled`, `policy-blocked`, `missing`, `already-attempted`) or `null` when no reason was recorded, and an `account_skip_reasons` map keyed by account index that mirrors the pool-exhausted response shape. The human-readable `message` appends the same reason in parentheses when present (see issue #486). +Pinned-account-unavailable responses also carry three recovery fields: + +| Field | Type | Meaning | +|-------|------|---------| +| `pin_source` | `"forced"` \| `"manual"` \| `null` | How the pin was set. `manual` came from `codex-multi-auth switch` and clears with `unpin`; `forced` came from this session's launcher (`--account` / `CODEX_MULTI_AUTH_FORCE_ACCOUNT_INDEX`) and `unpin` will NOT clear it — relaunch to select a different account. The `message` carries the matching remedy | +| `reset_at` | ISO-8601 string \| `null` | When the pinned account next becomes selectable: the latest of its still-active rate-limit record for the request's family and model, any active cooldown, and its circuit breaker's next-admission deadline. `null` when nothing bounds recovery, and deliberately `null` under a permanent blocker (`disabled`, `workspace-disabled`, `policy-blocked`, `missing`, token invalidation), where no timer clears the condition | +| `retry_after_ms` | number \| `null` | The same moment as milliseconds from now. Note this is the *latest* bound for the single pinned account, whereas `codex_runtime_rotation_pool_exhausted` reports the *earliest* recovery across the whole pool | + +`retry_after_ms` is advisory; it is not emitted as a `Retry-After` header. + Account policy pause/drain is enforced through runtime policy evaluation and contributes to selection skip reasons such as `policy-blocked`. --- diff --git a/lib/request/rate-limit-decision.ts b/lib/request/rate-limit-decision.ts index 6f775e5f1..926febdbd 100644 --- a/lib/request/rate-limit-decision.ts +++ b/lib/request/rate-limit-decision.ts @@ -193,6 +193,12 @@ export interface PinnedUnavailableErrorBody { account_skip_reasons: Record; } +/** + * Largest epoch-ms value `new Date(...).toISOString()` accepts; anything beyond + * throws RangeError (ECMAScript time-value limit, ±100,000,000 days). + */ +const MAX_ECMASCRIPT_TIME_VALUE = 8_640_000_000_000_000; + export interface PinnedUnavailableContext { /** * "forced" when the pin came from the wrapper's forced-account mode @@ -225,10 +231,19 @@ export function buildPinnedUnavailableErrorBody( ? "The pinned account" : `Pinned account ${normalizedPinnedIndex + 1}`; const pinSource = context?.pinSource ?? null; + // Upper bound as well as lower: resetAtMs comes from persisted account state + // (rateLimitResetTimes, coolingDownUntil), and markAccountCoolingDown clamps + // only the low side while nothing re-validates either on load. A finite but + // absurd deadline past the ECMAScript time limit would make toISOString below + // throw a RangeError inside handleRequestInner, collapsing this diagnostic 503 + // into a generic 500 that carries no pinnedAccountIndex, reason, or skip map — + // the exact payload this branch exists to deliver. Such a value bounds nothing + // usable anyway, so treat it as "no known recovery" instead. const resetAtMs = typeof context?.resetAtMs === "number" && Number.isFinite(context.resetAtMs) && - context.resetAtMs > 0 + context.resetAtMs > 0 && + context.resetAtMs <= MAX_ECMASCRIPT_TIME_VALUE ? context.resetAtMs : null; const now = context?.now ?? Date.now(); @@ -236,8 +251,8 @@ export function buildPinnedUnavailableErrorBody( const resetAt = resetAtMs !== null ? new Date(resetAtMs).toISOString() : null; const waitSuffix = resetAt !== null ? `; the recorded limit resets at ${resetAt}` : ""; - // A forced pin belongs to the launching process, not to ndy's persisted - // pin state, so `unpin` would clear nothing — say what actually helps. + // A forced pin belongs to the launching process, not to the persisted pin + // state, so `unpin` would clear nothing — say what actually helps. const remedy = pinSource === "forced" ? "the pin was set by this session's launcher, so relaunch to select a different account" diff --git a/test/rate-limit-decision.test.ts b/test/rate-limit-decision.test.ts index 4410d3c42..2fd4f85f0 100644 --- a/test/rate-limit-decision.test.ts +++ b/test/rate-limit-decision.test.ts @@ -331,3 +331,38 @@ describe("buildPinnedUnavailableErrorBody", () => { expect(body.message).not.toContain("resets at"); }); }); + +describe("buildPinnedUnavailableErrorBody recovery bounds", () => { + it("drops an out-of-range deadline instead of throwing RangeError", () => { + // resetAtMs comes from persisted account state and nothing re-validates + // it on load, so a corrupted coolingDownUntil can be finite, positive, + // and still past the ECMAScript time limit. toISOString would throw and + // collapse this 503 into a generic 500 that carries none of the + // diagnostics below. + const body = buildPinnedUnavailableErrorBody( + 0, + new Map([[0, "rate-limited"]]), + { pinSource: "forced", resetAtMs: 1e18, now: 1_700_000_000_000 }, + ); + + expect(body.reset_at).toBeNull(); + expect(body.retry_after_ms).toBeNull(); + expect(body.code).toBe("codex_pinned_account_unavailable"); + expect(body.pinnedAccountIndex).toBe(0); + expect(body.reason).toBe("rate-limited"); + expect(body.pin_source).toBe("forced"); + expect(body.message).not.toContain("resets at"); + }); + + it("still reports a deadline at the edge of the valid range", () => { + const maxTimeValue = 8_640_000_000_000_000; + const body = buildPinnedUnavailableErrorBody( + 0, + new Map([[0, "rate-limited"]]), + { resetAtMs: maxTimeValue, now: 1_700_000_000_000 }, + ); + + expect(body.reset_at).toBe(new Date(maxTimeValue).toISOString()); + expect(body.retry_after_ms).toBe(maxTimeValue - 1_700_000_000_000); + }); +}); diff --git a/test/runtime-rotation-proxy.test.ts b/test/runtime-rotation-proxy.test.ts index 21158983b..534da008b 100644 --- a/test/runtime-rotation-proxy.test.ts +++ b/test/runtime-rotation-proxy.test.ts @@ -893,13 +893,27 @@ describe("runtime rotation proxy", () => { // Zero the network-error cooldown so every request reaches upstream // and records a breaker failure; otherwise the cooldown absorbs the // retries and the breaker never opens. + // Restored in a finally: if startProxy rejects, an inline unstub never + // runs and the zero cooldown leaks into every later test in this file — + // the shared afterEach does not clear env stubs. Scoped to the one + // variable rather than vi.unstubAllEnvs(), which would also clear stubs + // an enclosing hook set. + const previousNetworkErrorCooldown = + process.env.CODEX_AUTH_NETWORK_ERROR_COOLDOWN_MS; + let proxy: Awaited>; vi.stubEnv("CODEX_AUTH_NETWORK_ERROR_COOLDOWN_MS", "0"); - const proxy = await startProxy({ - accountManager, - fetchImpl, - options: { forcedAccountIndex: 0 }, - }); - vi.unstubAllEnvs(); + try { + proxy = await startProxy({ + accountManager, + fetchImpl, + options: { forcedAccountIndex: 0 }, + }); + } finally { + vi.stubEnv( + "CODEX_AUTH_NETWORK_ERROR_COOLDOWN_MS", + previousNetworkErrorCooldown, + ); + } const body = { model: "gpt-5-codex", stream: true, From 07b0f91ce8dc93905b378d59f5e79d98390ce962 Mon Sep 17 00:00:00 2001 From: ndycode Date: Sun, 16 Aug 2026 22:34:44 +0800 Subject: [PATCH 7/7] fix(runtime): do not let "already-attempted" hide a pin this request just disabled MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Greptile was right, and the gate was wrong in a way the earlier reasoning missed. `PINNED_PERMANENT_SKIP_REASONS` was matched only against the RECORDED skip reason, on the assumption that selection rejects a permanently blocked pin before any attempt so the recorded reason is reliably the permanent one. That holds for state present before the request. It does not hold for state this request creates. A workspace-disabled 402/403 calls `setAccountEnabled(index, false)` in the retry loop and then continues. The next selection pass sees the pin in `attemptedIndexes` and records `already-attempted`, so the disable never reaches the recorded reason — while the same branch's `recordFailure` can be the third that opens the breaker. The 503 then advertised the circuit's ~30s reset as recovery for an account no timer will ever re-admit, so every retry after `retry_after_ms` lands on another 503, forever. The permanence check now also re-reads the pin's CURRENT runtime state via `getManagedAccountRuntimeSkipReason`, which is authoritative about enabled/workspace/auth-invalidation. The recorded reason is still consulted because it carries the selection-only verdicts (`missing`, `policy-blocked`) that account state cannot express. `reason` itself keeps reporting the selection verdict, unchanged — only the recovery metadata is suppressed. Also: docs/reference/error-contracts.md and the pin-source docblock named `CODEX_MULTI_AUTH_FORCE_ACCOUNT_INDEX`, which settings.md documents as "internal ... not intended to be set by hand". User-facing text now names the public selector `CODEX_MULTI_AUTH_FORCE_ACCOUNT`, with the internal variable mentioned only as what the wrapper resolves it into. Test: a forced pin takes two network failures, then a workspace-disabled 403 that both trips the breaker and disables the account; the 503 must carry null reset_at/retry_after_ms. Without the fix it reports the circuit reset instead. Suites: test/runtime-rotation-proxy.test.ts, test/rate-limit-decision.test.ts. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01WYnWb16vmd1XdS33GPtdxi --- docs/reference/error-contracts.md | 4 +- lib/request/rate-limit-decision.ts | 3 +- lib/runtime-rotation-proxy.ts | 29 +++++++++++-- test/runtime-rotation-proxy.test.ts | 67 +++++++++++++++++++++++++++++ 4 files changed, 96 insertions(+), 7 deletions(-) diff --git a/docs/reference/error-contracts.md b/docs/reference/error-contracts.md index ffe3228d9..6d298dca8 100644 --- a/docs/reference/error-contracts.md +++ b/docs/reference/error-contracts.md @@ -120,7 +120,7 @@ The default-on localhost Responses proxy returns JSON error payloads with a stab | `runtime_rotation_proxy_unauthorized` | `401` | Local request did not include the per-process proxy client key | | `runtime_rotation_proxy_payload_too_large` | `413` | Request body exceeded the proxy safety cap | | `codex_runtime_rotation_pool_exhausted` | `429` or `503` | No managed account can currently service the runtime request | -| `codex_pinned_account_unavailable` | `503` | A pin is in force — either a manual pin (`codex-multi-auth switch`) or a forced pin set per invocation by the launcher (`--account` / `CODEX_MULTI_AUTH_FORCE_ACCOUNT_INDEX`) — but the pinned account is rate-limited, cooling down, disabled, or blocked by policy. The remedy depends on `pin_source`: a manual pin clears with `codex-multi-auth unpin`, a forced pin does not and needs a relaunch | +| `codex_pinned_account_unavailable` | `503` | A pin is in force — either a manual pin (`codex-multi-auth switch`) or a forced pin set per invocation by the launcher (`--account` / `CODEX_MULTI_AUTH_FORCE_ACCOUNT`) — but the pinned account is rate-limited, cooling down, disabled, or blocked by policy. The remedy depends on `pin_source`: a manual pin clears with `codex-multi-auth unpin`, a forced pin does not and needs a relaunch | | `codex_runtime_rotation_proxy_error` | `500` | Proxy failed before forwarding the request | Pool exhaustion includes a `reason`, `retry_after_ms`, and a hint to run `codex-multi-auth rotation status`. Pinned-account-unavailable responses include a `pinnedAccountIndex` field identifying the pinned account, a structured `reason` field carrying the runtime skip reason (for example `rate-limited`, `cooling-down:auth-failure`, `circuit-open`, `disabled`, `workspace-disabled`, `policy-blocked`, `missing`, `already-attempted`) or `null` when no reason was recorded, and an `account_skip_reasons` map keyed by account index that mirrors the pool-exhausted response shape. The human-readable `message` appends the same reason in parentheses when present (see issue #486). @@ -129,7 +129,7 @@ Pinned-account-unavailable responses also carry three recovery fields: | Field | Type | Meaning | |-------|------|---------| -| `pin_source` | `"forced"` \| `"manual"` \| `null` | How the pin was set. `manual` came from `codex-multi-auth switch` and clears with `unpin`; `forced` came from this session's launcher (`--account` / `CODEX_MULTI_AUTH_FORCE_ACCOUNT_INDEX`) and `unpin` will NOT clear it — relaunch to select a different account. The `message` carries the matching remedy | +| `pin_source` | `"forced"` \| `"manual"` \| `null` | How the pin was set. `manual` came from `codex-multi-auth switch` and clears with `unpin`; `forced` came from this session's launcher (`--account` / `CODEX_MULTI_AUTH_FORCE_ACCOUNT`) and `unpin` will NOT clear it — relaunch to select a different account. The `message` carries the matching remedy | | `reset_at` | ISO-8601 string \| `null` | When the pinned account next becomes selectable: the latest of its still-active rate-limit record for the request's family and model, any active cooldown, and its circuit breaker's next-admission deadline. `null` when nothing bounds recovery, and deliberately `null` under a permanent blocker (`disabled`, `workspace-disabled`, `policy-blocked`, `missing`, token invalidation), where no timer clears the condition | | `retry_after_ms` | number \| `null` | The same moment as milliseconds from now. Note this is the *latest* bound for the single pinned account, whereas `codex_runtime_rotation_pool_exhausted` reports the *earliest* recovery across the whole pool | diff --git a/lib/request/rate-limit-decision.ts b/lib/request/rate-limit-decision.ts index 926febdbd..388ae34e4 100644 --- a/lib/request/rate-limit-decision.ts +++ b/lib/request/rate-limit-decision.ts @@ -202,7 +202,8 @@ const MAX_ECMASCRIPT_TIME_VALUE = 8_640_000_000_000_000; export interface PinnedUnavailableContext { /** * "forced" when the pin came from the wrapper's forced-account mode - * (`--account` / CODEX_MULTI_AUTH_FORCE_ACCOUNT_INDEX), "manual" when it + * (`--account` / CODEX_MULTI_AUTH_FORCE_ACCOUNT, which the wrapper resolves + * into the internal CODEX_MULTI_AUTH_FORCE_ACCOUNT_INDEX), "manual" when it * came from `switch`. `unpin` clears only the manual kind, so the remedy * line must not suggest it for a forced pin. */ diff --git a/lib/runtime-rotation-proxy.ts b/lib/runtime-rotation-proxy.ts index 3f04b1261..4a617ff28 100644 --- a/lib/runtime-rotation-proxy.ts +++ b/lib/runtime-rotation-proxy.ts @@ -1599,11 +1599,32 @@ async function handleRequestInner( // A permanent blocker (disabled, no enabled workspace, invalidated // auth, policy block, out-of-range pin) outlives every timed record, // so advertising a record's expiry would invite a retry into another - // 503. Selection rejects such an account before any attempt, so the - // recorded skip reason is the permanent one in exactly these cases. + // 503. + // + // The recorded reason alone is not enough to detect one. It is the + // SELECTION verdict, and this request can make the pin permanently + // unselectable after selection already ran: a workspace-disabled + // 402/403 calls setAccountEnabled(index, false) above and then + // continues, so the next pass records "already-attempted" and the + // disable never surfaces. With a breaker tripped by the same failure + // the 503 would then advertise the circuit's ~30s reset for an + // account no timer will ever re-admit. Re-read the pin's CURRENT + // runtime state so a permanent blocker cannot hide behind the + // verdict; the recorded reason still covers the selection-only + // verdicts ("missing", "policy-blocked") that state cannot express. + const pinnedCurrentSkipReason = + pinnedAccount === null + ? null + : accountManager.getManagedAccountRuntimeSkipReason( + pinnedAccount, + context.family, + context.model, + ); const pinnedBlockedPermanently = - pinnedSkipReason !== null && - PINNED_PERMANENT_SKIP_REASONS.has(pinnedSkipReason); + (pinnedSkipReason !== null && + PINNED_PERMANENT_SKIP_REASONS.has(pinnedSkipReason)) || + (pinnedCurrentSkipReason !== null && + PINNED_PERMANENT_SKIP_REASONS.has(pinnedCurrentSkipReason)); // Otherwise recovery comes from the pinned account's persisted state, not the // recorded skip reason: a direct 429/cooldown on the pinned account // reaches this 503 as "already-attempted" (the retry loop's selection diff --git a/test/runtime-rotation-proxy.test.ts b/test/runtime-rotation-proxy.test.ts index 534da008b..1735cfbf5 100644 --- a/test/runtime-rotation-proxy.test.ts +++ b/test/runtime-rotation-proxy.test.ts @@ -884,6 +884,73 @@ describe("runtime rotation proxy", () => { expect(Date.parse(payload.error.reset_at ?? "")).toBeGreaterThan(now); }); + it("suppresses recovery when this request itself disables the pinned account", async () => { + const now = Date.now(); + const accountManager = new AccountManager(undefined, createStorage(now, 1)); + // Two network failures record breaker failures without disabling; the + // third response is a workspace-disabled 403, which records the failure + // that opens the breaker AND calls setAccountEnabled(index, false). + // Selection on the next pass sees the pin in attemptedIndexes and + // records "already-attempted", so the disable never reaches the recorded + // skip reason — the 503 must still refuse to advertise the circuit's + // ~30s reset for an account no timer will re-admit. + const { calls, fetchImpl } = createRecordingFetch((_call, attempt) => { + if (attempt < 3) throw new TypeError("fetch failed"); + return new Response( + JSON.stringify({ + error: { code: "workspace_disabled", message: "workspace has been disabled" }, + }), + { status: HTTP_STATUS.FORBIDDEN, headers: { "content-type": "application/json" } }, + ); + }); + const previousNetworkErrorCooldown = + process.env.CODEX_AUTH_NETWORK_ERROR_COOLDOWN_MS; + let proxy: Awaited>; + vi.stubEnv("CODEX_AUTH_NETWORK_ERROR_COOLDOWN_MS", "0"); + try { + proxy = await startProxy({ + accountManager, + fetchImpl, + options: { forcedAccountIndex: 0 }, + }); + } finally { + vi.stubEnv( + "CODEX_AUTH_NETWORK_ERROR_COOLDOWN_MS", + previousNetworkErrorCooldown, + ); + } + const body = { + model: "gpt-5-codex", + stream: true, + input: [{ type: "message", role: "user", content: "hi" }], + }; + + for (let attempt = 0; attempt < 2; attempt += 1) { + const failed = await postResponses(proxy, body); + expect(failed.status).toBe(HTTP_STATUS.SERVICE_UNAVAILABLE); + } + expect(calls).toHaveLength(2); + + const response = await postResponses(proxy, body); + expect(response.status).toBe(HTTP_STATUS.SERVICE_UNAVAILABLE); + expect(calls).toHaveLength(3); + expect(accountManager.getAccountByIndex(0)?.enabled).toBe(false); + + const payload = (await response.json()) as { + error: { + code: string; + pin_source: string | null; + reset_at: string | null; + retry_after_ms: number | null; + }; + }; + expect(payload.error.code).toBe("codex_pinned_account_unavailable"); + expect(payload.error.pin_source).toBe("forced"); + // The account is disabled for good; no timer clears that. + expect(payload.error.reset_at).toBeNull(); + expect(payload.error.retry_after_ms).toBeNull(); + }); + it("advertises the circuit deadline once repeated failures open the pinned account's breaker", async () => { const now = Date.now(); const accountManager = new AccountManager(undefined, createStorage(now, 2));