diff --git a/docs/reference/error-contracts.md b/docs/reference/error-contracts.md index 4ec3888bc..af9d8b4b3 100644 --- a/docs/reference/error-contracts.md +++ b/docs/reference/error-contracts.md @@ -77,7 +77,7 @@ The default-on localhost Responses proxy returns JSON error payloads with a stab | `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_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. +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). --- diff --git a/lib/runtime-rotation-proxy.ts b/lib/runtime-rotation-proxy.ts index b85eefa15..eb2f9333b 100644 --- a/lib/runtime-rotation-proxy.ts +++ b/lib/runtime-rotation-proxy.ts @@ -1044,6 +1044,47 @@ function writePoolExhausted(params: { }); } +/** + * Build the JSON `error` body for a pinned-account 503 response. Extracted so + * the null-reason desync path (`reason: null`, no parenthetical in `message`) + * can be unit-tested without standing up a full proxy. The shape mirrors + * `writePoolExhausted` so consumers can handle both 503 codes uniformly. See + * issue #486. + */ +export interface PinnedUnavailableErrorBody { + message: string; + code: "codex_pinned_account_unavailable"; + pinnedAccountIndex: number | null; + reason: string | null; + account_skip_reasons: Record; +} + +export function buildPinnedUnavailableErrorBody( + pinnedIndex: number | null | undefined, + accountSkipReasons: ReadonlyMap, +): PinnedUnavailableErrorBody { + const normalizedPinnedIndex = + typeof pinnedIndex === "number" ? pinnedIndex : null; + const skipReason = + normalizedPinnedIndex !== null + ? accountSkipReasons.get(normalizedPinnedIndex) ?? null + : null; + const reasonSuffix = skipReason ? ` (${skipReason})` : ""; + const displayIndex = (normalizedPinnedIndex ?? 0) + 1; + return { + message: `Pinned account ${displayIndex} is currently unavailable${reasonSuffix}; run \`codex-multi-auth status\` for details, or \`codex-multi-auth unpin\` to allow rotation.`, + code: "codex_pinned_account_unavailable", + pinnedAccountIndex: normalizedPinnedIndex, + reason: skipReason, + account_skip_reasons: Object.fromEntries( + [...accountSkipReasons.entries()].map(([index, reason]) => [ + String(index), + reason, + ]), + ), + }; +} + async function withTimeout( promise: Promise, timeoutMs: number, @@ -1738,19 +1779,25 @@ export async function startRuntimeRotationProxy( // When a manual pin is set and the pinned account is unavailable, do // NOT silently fall through to rotation. Hard-fail with a 503 so the // user is informed the pin cannot be honored. See issue #474. + // + // Surface the runtime skip reason in both the human-readable message + // and a structured `reason` field, mirroring `writePoolExhausted`. A + // null reason indicates a forecast/runtime state desync (the pinned + // account was selected but no skip reason was recorded) — see #486. if (isPinned) { + const errorBody = buildPinnedUnavailableErrorBody( + pinnedIndex, + accountSkipReasons, + ); + if (errorBody.reason === null) { + status.lastError = `pinned-503 missing skip reason (pinnedIndex=${pinnedIndex})`; + } await usageRecorder?.record({ outcome: "failure", statusCode: HTTP_STATUS.SERVICE_UNAVAILABLE, errorCode: "codex_pinned_account_unavailable", }); - writeJson(res, HTTP_STATUS.SERVICE_UNAVAILABLE, { - error: { - message: `Pinned account ${(pinnedIndex ?? 0) + 1} is currently unavailable; run \`codex-multi-auth status\` for details, or \`codex-multi-auth unpin\` to allow rotation.`, - code: "codex_pinned_account_unavailable", - pinnedAccountIndex: pinnedIndex, - }, - }); + writeJson(res, HTTP_STATUS.SERVICE_UNAVAILABLE, { error: errorBody }); return; } diff --git a/test/issue-474-pin-end-to-end.test.ts b/test/issue-474-pin-end-to-end.test.ts index ce175be46..aa05ba6b1 100644 --- a/test/issue-474-pin-end-to-end.test.ts +++ b/test/issue-474-pin-end-to-end.test.ts @@ -279,8 +279,163 @@ describe("issue #474 — end-to-end pin honored over real HTTP proxy", () => { expect(thirdResult.bodyText).toContain( "codex_pinned_account_unavailable", ); + // Issue #486: the 503 body must surface the runtime skip reason so + // users can diagnose why the pin cannot be honored without scraping + // `codex-multi-auth status` logs out-of-band. + const thirdBody = JSON.parse(thirdResult.bodyText) as { + error: { + code: string; + pinnedAccountIndex: number | null; + reason: string | null; + account_skip_reasons: Record; + message: string; + }; + }; + expect(thirdBody.error.reason).toBe("disabled"); + expect(thirdBody.error.message).toContain("(disabled)"); + expect(thirdBody.error.pinnedAccountIndex).toBe(otherAccountIndex); + expect( + thirdBody.error.account_skip_reasons[String(otherAccountIndex)], + ).toBe("disabled"); // No additional upstream call — the proxy refused before issuing one. expect(upstreamCalls).toHaveLength(2); }, ); + + it( + "surfaces 'rate-limited' skip reason in pinned 503 body (issue #486)", + async () => { + const storagePath = makeTmpStoragePath(); + const now = Date.now(); + const initialStorage = createStorage(now); + const pinnedIndex = 1; + writeStorageFile(storagePath, { + ...initialStorage, + pinnedAccountIndex: pinnedIndex, + affinityGeneration: 1, + }); + setStoragePathDirect(storagePath); + + const accountManager = new AccountManager(undefined, initialStorage); + openManagers.push(accountManager); + + const pinned = accountManager.getAccountByIndex(pinnedIndex); + expect(pinned).not.toBeNull(); + if (!pinned) throw new Error("setup failed"); + // Match the family the proxy will resolve from `model: "gpt-5-codex"`. + // `getModelFamily("gpt-5-codex")` returns "gpt-5-codex", not "codex", + // so the rate-limit must be keyed under that family for the runtime + // skip-reason check to detect it. + accountManager.markRateLimitedWithReason( + pinned, + 60_000, + "gpt-5-codex", + "quota", + ); + + const upstreamCalls: number[] = []; + const fetchImpl: typeof fetch = async (_input, init) => { + const headers = new Headers(init?.headers); + const auth = headers.get("authorization") ?? ""; + const token = auth.replace(/^Bearer\s+/i, ""); + const index = initialStorage.accounts.findIndex( + (a) => a.accessToken === token, + ); + upstreamCalls.push(index); + return new Response(JSON.stringify({ ok: true, account: index }), { + status: HTTP_STATUS.OK, + headers: { "content-type": "application/json" }, + }); + }; + + const proxy = await startRuntimeRotationProxy({ + accountManager, + fetchImpl, + upstreamBaseUrl: "https://example.test/backend-api", + clientApiKey: CLIENT_API_KEY, + }); + openServers.push(proxy); + + const result = await postViaHttp( + proxy, + { model: "gpt-5-codex", stream: false }, + "/v1/responses", + ); + expect(result.status).toBe(HTTP_STATUS.SERVICE_UNAVAILABLE); + const body = JSON.parse(result.bodyText) as { + error: { + code: string; + reason: string | null; + account_skip_reasons: Record; + message: string; + }; + }; + expect(body.error.code).toBe("codex_pinned_account_unavailable"); + expect(body.error.reason).toBe("rate-limited"); + expect(body.error.message).toContain("(rate-limited)"); + expect(body.error.account_skip_reasons[String(pinnedIndex)]).toBe( + "rate-limited", + ); + expect(upstreamCalls).toHaveLength(0); + }, + ); + + it( + "surfaces a cooling-down skip reason in pinned 503 body (issue #486)", + async () => { + const storagePath = makeTmpStoragePath(); + const now = Date.now(); + const initialStorage = createStorage(now); + const pinnedIndex = 0; + writeStorageFile(storagePath, { + ...initialStorage, + pinnedAccountIndex: pinnedIndex, + affinityGeneration: 1, + }); + setStoragePathDirect(storagePath); + + const accountManager = new AccountManager(undefined, initialStorage); + openManagers.push(accountManager); + + const pinned = accountManager.getAccountByIndex(pinnedIndex); + if (!pinned) throw new Error("setup failed"); + accountManager.markAccountCoolingDown(pinned, 60_000, "auth-failure"); + + const upstreamCalls: number[] = []; + const fetchImpl: typeof fetch = async () => { + upstreamCalls.push(-1); + return new Response("{}", { status: HTTP_STATUS.OK }); + }; + + const proxy = await startRuntimeRotationProxy({ + accountManager, + fetchImpl, + upstreamBaseUrl: "https://example.test/backend-api", + clientApiKey: CLIENT_API_KEY, + }); + openServers.push(proxy); + + const result = await postViaHttp( + proxy, + { model: "gpt-5-codex", stream: false }, + "/v1/responses", + ); + expect(result.status).toBe(HTTP_STATUS.SERVICE_UNAVAILABLE); + const body = JSON.parse(result.bodyText) as { + error: { + code: string; + reason: string | null; + account_skip_reasons: Record; + message: string; + }; + }; + expect(body.error.code).toBe("codex_pinned_account_unavailable"); + expect(body.error.reason).toBe("cooling-down:auth-failure"); + expect(body.error.message).toContain("(cooling-down:auth-failure)"); + expect(body.error.account_skip_reasons[String(pinnedIndex)]).toBe( + "cooling-down:auth-failure", + ); + expect(upstreamCalls).toHaveLength(0); + }, + ); }); diff --git a/test/issue-474-pin-honored.test.ts b/test/issue-474-pin-honored.test.ts index 933fe49f4..4ca1d4128 100644 --- a/test/issue-474-pin-honored.test.ts +++ b/test/issue-474-pin-honored.test.ts @@ -5,6 +5,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { AccountManager } from "../lib/accounts.js"; import { clearCircuitBreakers } from "../lib/circuit-breaker.js"; import { + buildPinnedUnavailableErrorBody, chooseAccount, readPinnedAccountIndexFromDisk, resetPinCacheForTesting, @@ -462,6 +463,279 @@ describe("issue #474 — manual pin honored by runtime proxy", () => { }); }); + // Issue #486: chooseAccount must record a skip reason for every pinned + // unavailability path so the runtime proxy can surface it in the 503 body. + describe("chooseAccount populates skipReasons for pinned unavailability", () => { + it("records 'rate-limited' when the pinned account is rate-limited", () => { + const now = Date.now(); + const storage = createStorage(now, 3); + const accountManager = new AccountManager(undefined, storage); + const pinned = accountManager.getAccountByIndex(1); + if (!pinned) throw new Error("setup failed"); + accountManager.markRateLimitedWithReason( + pinned, + 60_000, + "codex", + "quota", + ); + const skipReasons = new Map(); + + const result = chooseAccount({ + accountManager, + sessionAffinityStore: null, + sessionKey: null, + family: "codex", + model: null, + attemptedIndexes: new Set(), + now, + policy: null, + pinnedIndex: 1, + skipReasons, + }); + + expect(result).toBeNull(); + expect(skipReasons.get(1)).toBe("rate-limited"); + }); + + it("records 'cooling-down:auth-failure' when the pinned account is cooling down", () => { + const now = Date.now(); + const storage = createStorage(now, 3); + const accountManager = new AccountManager(undefined, storage); + const pinned = accountManager.getAccountByIndex(0); + if (!pinned) throw new Error("setup failed"); + accountManager.markAccountCoolingDown(pinned, 60_000, "auth-failure"); + const skipReasons = new Map(); + + const result = chooseAccount({ + accountManager, + sessionAffinityStore: null, + sessionKey: null, + family: "codex", + model: null, + attemptedIndexes: new Set(), + now, + policy: null, + pinnedIndex: 0, + skipReasons, + }); + + expect(result).toBeNull(); + expect(skipReasons.get(0)).toBe("cooling-down:auth-failure"); + }); + + it("records 'disabled' when the pinned account is disabled", () => { + const now = Date.now(); + const storage = createStorage(now, 3); + const accountManager = new AccountManager(undefined, storage); + accountManager.setAccountEnabled(2, false); + const skipReasons = new Map(); + + const result = chooseAccount({ + accountManager, + sessionAffinityStore: null, + sessionKey: null, + family: "codex", + model: null, + attemptedIndexes: new Set(), + now, + policy: null, + pinnedIndex: 2, + skipReasons, + }); + + expect(result).toBeNull(); + expect(skipReasons.get(2)).toBe("disabled"); + }); + + it("records 'policy-blocked' when the pinned account is policy-blocked", () => { + const now = Date.now(); + const storage = createStorage(now, 3); + const accountManager = new AccountManager(undefined, storage); + const skipReasons = new Map(); + + const result = chooseAccount({ + accountManager, + sessionAffinityStore: null, + sessionKey: null, + family: "codex", + model: null, + attemptedIndexes: new Set(), + now, + policy: { + allowed: true, + statusCode: 200, + reasons: [], + errorCode: null, + projectKey: null, + blockedAccountIndexes: new Set([1]), + scoreBoostByAccount: {}, + budgetEvaluations: [], + }, + pinnedIndex: 1, + skipReasons, + }); + + expect(result).toBeNull(); + expect(skipReasons.get(1)).toBe("policy-blocked"); + }); + + it("records 'missing' when the pinned index is out of range", () => { + const now = Date.now(); + const storage = createStorage(now, 2); + const accountManager = new AccountManager(undefined, storage); + const skipReasons = new Map(); + + const result = chooseAccount({ + accountManager, + sessionAffinityStore: null, + sessionKey: null, + family: "codex", + model: null, + attemptedIndexes: new Set(), + now, + policy: null, + pinnedIndex: 5, + skipReasons, + }); + + expect(result).toBeNull(); + expect(skipReasons.get(5)).toBe("missing"); + }); + + it("records 'already-attempted' when the pinned index was already tried", () => { + const now = Date.now(); + const storage = createStorage(now, 3); + const accountManager = new AccountManager(undefined, storage); + const skipReasons = new Map(); + + const result = chooseAccount({ + accountManager, + sessionAffinityStore: null, + sessionKey: null, + family: "codex", + model: null, + attemptedIndexes: new Set([0]), + now, + policy: null, + pinnedIndex: 0, + skipReasons, + }); + + expect(result).toBeNull(); + expect(skipReasons.get(0)).toBe("already-attempted"); + }); + + it("records 'workspace-disabled' when every workspace on the pinned account is disabled", () => { + const now = Date.now(); + const storage = createStorage(now, 3); + const accountManager = new AccountManager(undefined, storage); + const pinned = accountManager.getAccountByIndex(2); + if (!pinned) throw new Error("setup failed"); + pinned.workspaces = [{ id: "ws-1", enabled: false }]; + const skipReasons = new Map(); + + const result = chooseAccount({ + accountManager, + sessionAffinityStore: null, + sessionKey: null, + family: "codex", + model: null, + attemptedIndexes: new Set(), + now, + policy: null, + pinnedIndex: 2, + skipReasons, + }); + + expect(result).toBeNull(); + expect(skipReasons.get(2)).toBe("workspace-disabled"); + }); + + it("records 'circuit-open' when the pinned account's circuit breaker is open", () => { + const now = Date.now(); + const storage = createStorage(now, 3); + const accountManager = new AccountManager(undefined, storage); + const pinned = accountManager.getAccountByIndex(1); + if (!pinned) throw new Error("setup failed"); + // Default failure threshold is 3 — trip the breaker explicitly so + // `getAccountRuntimeSkipReason` resolves to "circuit-open". + accountManager.recordFailure(pinned, "codex"); + accountManager.recordFailure(pinned, "codex"); + accountManager.recordFailure(pinned, "codex"); + const skipReasons = new Map(); + + const result = chooseAccount({ + accountManager, + sessionAffinityStore: null, + sessionKey: null, + family: "codex", + model: null, + attemptedIndexes: new Set(), + now, + policy: null, + pinnedIndex: 1, + skipReasons, + }); + + expect(result).toBeNull(); + expect(skipReasons.get(1)).toBe("circuit-open"); + }); + }); + + // Issue #486: cover the null-reason desync branch directly. `chooseAccount` + // is supposed to record a skip reason for every pinned-unavailability path, + // but if internal state desyncs (e.g. forecast says "rotate" yet runtime + // state is missing a reason), the proxy still needs to produce a 503 body + // with an explicit `reason: null` instead of silently masking the gap. + describe("buildPinnedUnavailableErrorBody", () => { + it("returns reason: null and omits the parenthetical when no skip reason was recorded", () => { + const body = buildPinnedUnavailableErrorBody( + 1, + new Map(), + ); + expect(body.code).toBe("codex_pinned_account_unavailable"); + expect(body.reason).toBeNull(); + expect(body.pinnedAccountIndex).toBe(1); + expect(body.account_skip_reasons).toEqual({}); + expect(body.message).toBe( + "Pinned account 2 is currently unavailable; run `codex-multi-auth status` for details, or `codex-multi-auth unpin` to allow rotation.", + ); + expect(body.message).not.toContain("("); + }); + + it("surfaces the skip reason and appends it to the message when present", () => { + const skipReasons = new Map([[1, "rate-limited"]]); + const body = buildPinnedUnavailableErrorBody(1, skipReasons); + expect(body.reason).toBe("rate-limited"); + expect(body.account_skip_reasons).toEqual({ "1": "rate-limited" }); + expect(body.message).toContain("Pinned account 2"); + expect(body.message).toContain("(rate-limited)"); + }); + + it("handles a null pinnedIndex without throwing and emits reason: null", () => { + const body = buildPinnedUnavailableErrorBody( + null, + new Map(), + ); + expect(body.pinnedAccountIndex).toBeNull(); + expect(body.reason).toBeNull(); + expect(body.message).toContain("Pinned account 1"); + }); + + it("mirrors the full accountSkipReasons map even when the pinned entry is unknown", () => { + const skipReasons = new Map([ + [0, "rate-limited"], + [2, "disabled"], + ]); + const body = buildPinnedUnavailableErrorBody(1, skipReasons); + expect(body.reason).toBeNull(); + expect(body.account_skip_reasons).toEqual({ + "0": "rate-limited", + "2": "disabled", + }); + }); + }); + describe("readPinnedAccountIndexFromDisk", () => { it("returns the pinnedAccountIndex written to disk", () => { const path = makeTmpStoragePath();