diff --git a/lib/request/rate-limit-decision.ts b/lib/request/rate-limit-decision.ts index 388ae34e..f7196a5e 100644 --- a/lib/request/rate-limit-decision.ts +++ b/lib/request/rate-limit-decision.ts @@ -1,5 +1,6 @@ import { HTTP_STATUS, MAX_RATE_LIMIT_DELAY_MS } from "../constants.js"; import type { ExhaustionReason } from "../runtime/rotation-server-types.js"; +import type { CooldownReason } from "../storage/public-types.js"; import type { TokenResult } from "../types.js"; import { isRecord } from "../utils.js"; @@ -210,9 +211,170 @@ export interface PinnedUnavailableContext { pinSource?: "forced" | "manual" | null; /** Epoch ms when the blocking record ends (rate limit or cooldown). */ resetAtMs?: number | null; + /** + * Which class of record supplies `resetAtMs`. That deadline is the max + * across every gating record, so a `rate-limited` skip reason can carry a + * deadline supplied by a breaker or cooldown that ends later, and the + * quota phrasing is only true when the rate limit itself is that bound. + * + * - `"rate-limit"` — the rate-limit records bound it; word it as a reset. + * - `"other"` — a cooldown or breaker ends later and bounds it instead. + * - `"unknown"` (the default) — the caller did not measure; trust the + * skip reason. + * + * Deliberately a value rather than the presence of a second deadline + * field. The previous shape branched on `"rateLimitResetAtMs" in context`, + * so a caller that spread the key with an `undefined` value got the + * opposite wording from one that omitted it — a contract no type could + * express and no compiler could check. + */ + recoveryBound?: "rate-limit" | "other" | "unknown"; + /** + * The pin's CURRENT runtime skip reason, re-read from account state at the + * moment the 503 is built. `accountSkipReasons` holds the SELECTION + * verdict, and on the dominant path the retry loop has already overwritten + * it with its own bookkeeping token ("already-attempted") — wording the + * sentence from that is what issue #675 reported. Omitted when the caller + * did not re-read runtime state. + */ + currentSkipReason?: string | null; now?: number; } +/** + * The human sentence derives its parenthetical and its deadline noun from the + * blocker class. The deadline is max(rate-limit reset, cooldown end, breaker + * next-attempt), so calling it a "limit" is only true for the rate-limit + * class: during a provider outage a tripped breaker or a server-error + * cooldown printed "the recorded limit resets at …", and operators read a + * backend incident as a blown subscription quota. Only the message changes — + * the machine-readable `reason` keeps the raw skip token. + */ +type DeadlineNoun = + | "available-again" + | "next-attempt" + | "cooldown-ends" + | "rate-limit-reset"; + +const DEADLINE_SENTENCES: Record string> = { + "available-again": (resetAt) => + `the account is expected to be available again at ${resetAt}`, + "next-attempt": (resetAt) => `the next attempt is allowed at ${resetAt}`, + "cooldown-ends": (resetAt) => `the cooldown ends at ${resetAt}`, + "rate-limit-reset": (resetAt) => `the rate limit resets at ${resetAt}`, +}; + +interface BlockerDescription { + parenthetical: string; + deadlineNoun: DeadlineNoun; +} + +/** + * Every cooldown variant is bounded by the same `coolingDownUntil` field, so + * they all word their deadline the same way. Two of them used to say "the next + * attempt is allowed at" while the rest said "the cooldown ends at", which + * reads as two different mechanisms to an operator comparing two 503s. + * + * `satisfies` is what keeps this honest: a new CooldownReason becomes a + * compile error here rather than falling through to the verbatim arm below, + * which would print the raw `cooling-down:` token in the sentence — + * the internal-token leak issue #675 was filed about. + */ +const COOLDOWN_DESCRIPTIONS = { + "auth-failure": { + parenthetical: "cooling down after authentication failures", + deadlineNoun: "cooldown-ends", + }, + "network-error": { + parenthetical: "cooling down after network errors", + deadlineNoun: "cooldown-ends", + }, + "server-error": { + parenthetical: "cooling down after upstream server errors", + deadlineNoun: "cooldown-ends", + }, + "rate-limit": { + parenthetical: "cooling down after a rate limit", + deadlineNoun: "cooldown-ends", + }, +} satisfies Record; + +const BLOCKER_DESCRIPTIONS: ReadonlyMap = new Map< + string, + BlockerDescription +>([ + [ + "rate-limited", + // Upgraded to "rate-limit-reset" below when the caller confirms the + // rate limit is what bounds the advertised deadline. + { parenthetical: "rate-limited", deadlineNoun: "available-again" }, + ], + [ + "circuit-open", + { + parenthetical: "paused after repeated upstream errors", + deadlineNoun: "next-attempt", + }, + ], + ["cooling-down", { parenthetical: "cooling down", deadlineNoun: "cooldown-ends" }], + ...Object.entries(COOLDOWN_DESCRIPTIONS).map( + ([reason, description]): [string, BlockerDescription] => [ + `cooling-down:${reason}`, + description, + ], + ), +]); + +/** Whether the sentence has real wording for this token, or must echo it raw. */ +function isDescribedBlocker(skipReason: string): boolean { + return BLOCKER_DESCRIPTIONS.has(skipReason); +} + +/** + * The human sentence derives its parenthetical and its deadline noun from the + * blocker class. The deadline is max(rate-limit reset, cooldown end, breaker + * next-attempt), so calling it a "limit" is only true for the rate-limit + * class: during a provider outage a tripped breaker or a server-error + * cooldown printed "the recorded limit resets at …", and operators read a + * backend incident as a blown subscription quota. Only the message changes — + * the machine-readable `reason` keeps the raw skip token. + */ +function describePinnedBlocker( + skipReason: string | null, + rateLimitBoundsRecovery: boolean, +): { + parenthetical: string | null; + deadline: (resetAt: string) => string; +} { + if (skipReason === null) { + return { + parenthetical: null, + deadline: DEADLINE_SENTENCES["available-again"], + }; + } + const described = BLOCKER_DESCRIPTIONS.get(skipReason); + if (described === undefined) { + // Permanent blockers never reach the deadline clause (the call site + // suppresses their reset time), and future or internal tokens stay + // legible verbatim. + return { + parenthetical: skipReason, + deadline: DEADLINE_SENTENCES["available-again"], + }; + } + // A breaker or cooldown can outlive the rate limit; the deadline is the max + // of every gating record, so it is only worded as the limit's own reset + // when the rate limit actually supplies it. + const deadlineNoun: DeadlineNoun = + skipReason === "rate-limited" && rateLimitBoundsRecovery + ? "rate-limit-reset" + : described.deadlineNoun; + return { + parenthetical: described.parenthetical, + deadline: DEADLINE_SENTENCES[deadlineNoun], + }; +} + export function buildPinnedUnavailableErrorBody( pinnedIndex: number | null | undefined, accountSkipReasons: ReadonlyMap, @@ -224,7 +386,6 @@ export function buildPinnedUnavailableErrorBody( normalizedPinnedIndex !== null ? accountSkipReasons.get(normalizedPinnedIndex) ?? null : null; - const reasonSuffix = skipReason ? ` (${skipReason})` : ""; // On the desync path the pin index is unknown (null); claiming "account 1" // there would contradict the machine-readable pinnedAccountIndex: null. const accountPhrase = @@ -250,8 +411,31 @@ export function buildPinnedUnavailableErrorBody( 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}` : ""; + // "unknown" (the default) means the caller did not measure which record + // bounds the deadline — older callers and unit seams — so trust the skip + // reason. Only a measured "other" demotes to the neutral phrasing, which + // stays true for a live rate limit either way. + const rateLimitBoundsRecovery = (context?.recoveryBound ?? "unknown") !== "other"; + // The machine-readable `reason` below stays the recorded SELECTION verdict; + // only the human sentence follows the blocker actually gating the pin. A + // recorded verdict that names a real blocker class wins, because the + // selection-only classes ("missing", "policy-blocked") have no + // runtime-state equivalent and would be lost. Otherwise the re-read runtime + // state is the truth and the recorded token is the retry loop's own + // bookkeeping — the dominant path in issue #675, where a 503/429 on the pin + // re-enters selection and records "already-attempted" over the real class. + const describedSkipReason = + skipReason !== null && isDescribedBlocker(skipReason) + ? skipReason + : context?.currentSkipReason ?? skipReason; + const blocker = describePinnedBlocker( + describedSkipReason, + rateLimitBoundsRecovery, + ); + const reasonSuffix = blocker.parenthetical + ? ` (${blocker.parenthetical})` + : ""; + const waitSuffix = resetAt !== null ? `; ${blocker.deadline(resetAt)}` : ""; // 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 = diff --git a/lib/runtime-rotation-proxy.ts b/lib/runtime-rotation-proxy.ts index 4a617ff2..573316c4 100644 --- a/lib/runtime-rotation-proxy.ts +++ b/lib/runtime-rotation-proxy.ts @@ -73,7 +73,7 @@ import { responseHeadersForClient, withTimeout, } from "./request/stream-failover-runtime.js"; -import { getAccountRecoveryTimeForFamily } from "./runtime/account-status.js"; +import { getAccountRecoveryBoundsForFamily } from "./runtime/account-status.js"; import { chooseAccount } from "./runtime/rotation-account-selection.js"; import { createRotationProxyState, @@ -1632,21 +1632,30 @@ 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 pinnedStateRecoveryAtMs = + // + // One clock for the whole body. The recovery bound and the + // rate-limit bound are compared against each other below, so + // reading state.now() per lookup would let a record expire between + // them and report a rate-limited pin as bounded by something else. + const evaluatedAtMs = state.now(); + // Both bounds come from a single pass over the account's records. + const pinnedRecoveryBounds = pinnedAccount === null ? null - : getAccountRecoveryTimeForFamily( + : getAccountRecoveryBoundsForFamily( pinnedAccount, - state.now(), + evaluatedAtMs, context.family, context.model, ); + const pinnedStateRecoveryAtMs = + pinnedRecoveryBounds?.recoveryAtMs ?? null; // 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()); + : accountManager.getCircuitRecoveryTime(pinnedAccount, evaluatedAtMs); const pinnedResetAtMs = pinnedBlockedPermanently || (pinnedStateRecoveryAtMs === null && pinnedCircuitRecoveryAtMs === null) @@ -1655,6 +1664,17 @@ async function handleRequestInner( pinnedStateRecoveryAtMs ?? 0, pinnedCircuitRecoveryAtMs ?? 0, ); + // The message only words the recovery deadline as a rate-limit reset + // when the rate-limit records are in fact what supplies it — a + // breaker or cooldown can end later and then bounds it instead. + const pinnedRateLimitResetAtMs = + pinnedRecoveryBounds?.rateLimitAtMs ?? null; + const recoveryBound = + pinnedResetAtMs !== null && + pinnedRateLimitResetAtMs !== null && + pinnedRateLimitResetAtMs >= pinnedResetAtMs + ? "rate-limit" + : "other"; const errorBody = buildPinnedUnavailableErrorBody( pinnedIndex, accountSkipReasons, @@ -1663,7 +1683,12 @@ async function handleRequestInner( pinSource: typeof state.forcedAccountIndex === "number" ? "forced" : "manual", resetAtMs: pinnedResetAtMs, - now: state.now(), + recoveryBound, + // The recorded verdict above is the retry loop's + // "already-attempted" on the dominant path; the re-read runtime + // state is what the operator-facing sentence should describe. + currentSkipReason: pinnedCurrentSkipReason, + now: evaluatedAtMs, }, ); if (errorBody.reason === null) { diff --git a/lib/runtime/account-status.ts b/lib/runtime/account-status.ts index 24df2263..1af268db 100644 --- a/lib/runtime/account-status.ts +++ b/lib/runtime/account-status.ts @@ -39,18 +39,78 @@ export function getRateLimitResetTimeForFamily( return minReset; } +/** The value when it is still a live bound at `now`, otherwise null. */ +function activeBound(value: number | undefined, now: number): number | null { + if (typeof value !== "number" || !Number.isFinite(value)) return null; + return value > now ? value : null; +} + +/** The later of two bounds, treating null as "does not bound". */ +function laterBound(a: number | null, b: number | null): number | null { + if (a === null) return b; + if (b === null) return a; + return a > b ? a : b; +} + +export interface AccountRecoveryBounds { + /** + * The moment the account becomes usable again: `rateLimitAtMs` widened by + * any active cooldown, because the account stays skipped while ANY gating + * record is active. Null when nothing bounds recovery. + */ + recoveryAtMs: number | null; + /** + * The rate-limit records' own contribution to `recoveryAtMs`, cooldowns + * excluded. The pinned-503 uses it to decide whether the deadline it + * advertises is in fact the rate limit's own reset — with a breaker or + * cooldown ending later, the full recovery bound outlives the rate limit + * and must not be worded as its reset. Null when no rate-limit record + * gates the request. + */ + rateLimitAtMs: number | null; +} + /** - * 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 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. + * Both recovery bounds for a `family`/`model` request, from ONE pass over + * `rateLimitResetTimes` against a single `now`. + * + * Two deliberate differences from getRateLimitResetTimeForFamily, whose + * earliest-reset answer feeds wait displays: the latest bound wins, because + * 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. + * + * A caller that needs both bounds must take them from one call. Measuring + * them separately lets a record expire between the two walks, which reports + * a rate-limited account as if a cooldown bounded its recovery. */ +export function getAccountRecoveryBoundsForFamily( + account: { + rateLimitResetTimes?: Record; + coolingDownUntil?: number; + }, + now: number, + family: ModelFamily, + model?: string | null, +): AccountRecoveryBounds { + const times = account.rateLimitResetTimes; + const rateLimitAtMs = times + ? laterBound( + activeBound(times[getQuotaKey(family)], now), + model ? activeBound(times[getQuotaKey(family, model)], now) : null, + ) + : null; + return { + rateLimitAtMs, + recoveryAtMs: laterBound( + rateLimitAtMs, + activeBound(account.coolingDownUntil, now), + ), + }; +} + +/** The `recoveryAtMs` bound alone; see getAccountRecoveryBoundsForFamily. */ export function getAccountRecoveryTimeForFamily( account: { rateLimitResetTimes?: Record; @@ -60,19 +120,21 @@ export function getAccountRecoveryTimeForFamily( family: ModelFamily, model?: string | null, ): 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) { - consider(times[getQuotaKey(family)]); - if (model) consider(times[getQuotaKey(family, model)]); - } - consider(account.coolingDownUntil); - return latest; + return getAccountRecoveryBoundsForFamily(account, now, family, model) + .recoveryAtMs; +} + +/** The `rateLimitAtMs` bound alone; see getAccountRecoveryBoundsForFamily. */ +export function getRateLimitRecoveryTimeForFamily( + account: { + rateLimitResetTimes?: Record; + }, + now: number, + family: ModelFamily, + model?: string | null, +): number | null { + return getAccountRecoveryBoundsForFamily(account, now, family, model) + .rateLimitAtMs; } export function formatRateLimitEntry( @@ -113,17 +175,6 @@ export function getRateLimitResetTimeForModel( family: ModelFamily, model: string, ): number | null { - const times = account.rateLimitResetTimes; - if (!times) return 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; - }; - - consider(times[getQuotaKey(family)]); - consider(times[getQuotaKey(family, model)]); - return latest; + return getAccountRecoveryBoundsForFamily(account, now, family, model) + .rateLimitAtMs; } diff --git a/test/issue-474-pin-end-to-end.test.ts b/test/issue-474-pin-end-to-end.test.ts index 4717d757..026bb8f1 100644 --- a/test/issue-474-pin-end-to-end.test.ts +++ b/test/issue-474-pin-end-to-end.test.ts @@ -437,7 +437,11 @@ describe("issue #474 — end-to-end pin honored over real HTTP proxy", () => { }; 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)"); + // The raw token stays in the machine-readable reason above; the human + // sentence translates it into the blocker it names. + expect(body.error.message).toContain( + "(cooling down after authentication failures)", + ); expect(body.error.account_skip_reasons[String(pinnedIndex)]).toBe( "cooling-down:auth-failure", ); diff --git a/test/rate-limit-decision.test.ts b/test/rate-limit-decision.test.ts index 2fd4f85f..99c11a92 100644 --- a/test/rate-limit-decision.test.ts +++ b/test/rate-limit-decision.test.ts @@ -311,13 +311,234 @@ describe("buildPinnedUnavailableErrorBody", () => { 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()}`, + `the rate 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"); }); + // The deadline is max(rate-limit reset, cooldown end, breaker next-attempt), + // so "limit" is only true for the rate-limit class. During the 2026-08-20 + // provider outage, a breaker deadline printed as "the recorded limit resets + // at …" read as a blown subscription quota; the noun now follows the + // blocker, while the machine-readable `reason` keeps the raw token. + for (const { reason, parenthetical, deadlineNoun } of [ + { + reason: "circuit-open", + parenthetical: "(paused after repeated upstream errors)", + deadlineNoun: "the next attempt is allowed at", + }, + // Every `cooling-down:*` variant is bounded by the same + // `coolingDownUntil` field, so they share one deadline noun: two of + // them used to say "the next attempt is allowed at" while the rest + // said "the cooldown ends at", which reads as two different mechanisms + // to an operator comparing two 503s from the same cause. + { + reason: "cooling-down:server-error", + parenthetical: "(cooling down after upstream server errors)", + deadlineNoun: "the cooldown ends at", + }, + { + reason: "cooling-down:network-error", + parenthetical: "(cooling down after network errors)", + deadlineNoun: "the cooldown ends at", + }, + { + reason: "cooling-down:auth-failure", + parenthetical: "(cooling down after authentication failures)", + deadlineNoun: "the cooldown ends at", + }, + { + reason: "cooling-down:rate-limit", + parenthetical: "(cooling down after a rate limit)", + deadlineNoun: "the cooldown ends at", + }, + { + reason: "cooling-down", + parenthetical: "(cooling down)", + deadlineNoun: "the cooldown ends at", + }, + ] as const) { + it(`words a \`${reason}\` deadline as transient recovery, not a limit`, () => { + const resetAtMs = 1_700_000_030_000; + const body = buildPinnedUnavailableErrorBody( + 1, + new Map([[1, reason]]), + { pinSource: "forced", resetAtMs, now: 1_700_000_000_000 }, + ); + // The JSON contract is untouched: raw token, same deadline fields. + expect(body.reason).toBe(reason); + expect(body.reset_at).toBe(new Date(resetAtMs).toISOString()); + expect(body.retry_after_ms).toBe(30_000); + expect(body.message).toContain(parenthetical); + expect(body.message).toContain( + `${deadlineNoun} ${new Date(resetAtMs).toISOString()}`, + ); + expect(body.message).not.toContain("limit resets"); + expect(body.message).not.toContain("circuit-open"); + }); + } + + it("keeps quota phrasing for a genuine rate limit", () => { + const resetAtMs = 1_700_000_030_000; + const body = buildPinnedUnavailableErrorBody( + 0, + new Map([[0, "rate-limited"]]), + { pinSource: "manual", resetAtMs, now: 1_700_000_000_000 }, + ); + expect(body.reason).toBe("rate-limited"); + expect(body.message).toContain("(rate-limited)"); + expect(body.message).toContain( + `the rate limit resets at ${new Date(resetAtMs).toISOString()}`, + ); + }); + + it("keeps quota phrasing when the rate limit supplies the recovery bound", () => { + const resetAtMs = 1_700_000_030_000; + const body = buildPinnedUnavailableErrorBody( + 0, + new Map([[0, "rate-limited"]]), + { + pinSource: "manual", + resetAtMs, + recoveryBound: "rate-limit", + now: 1_700_000_000_000, + }, + ); + expect(body.message).toContain( + `the rate limit resets at ${new Date(resetAtMs).toISOString()}`, + ); + }); + + it("does not call a later breaker or cooldown deadline a rate-limit reset", () => { + // Skip-reason precedence reports "rate-limited" whenever a live limit + // exists, but the recovery bound is the max of every gating record — a + // breaker tripped seconds ago can end after a limit about to expire. + // Wording that later timestamp as the limit's reset would be the same + // misattribution this change removes from the transient classes. + const resetAtMs = 1_700_000_030_000; + const body = buildPinnedUnavailableErrorBody( + 0, + new Map([[0, "rate-limited"]]), + { + pinSource: "forced", + resetAtMs, + recoveryBound: "other", + now: 1_700_000_000_000, + }, + ); + // Contract untouched: reset_at still advertises the full recovery bound. + expect(body.reason).toBe("rate-limited"); + expect(body.reset_at).toBe(new Date(resetAtMs).toISOString()); + expect(body.message).toContain("(rate-limited)"); + expect(body.message).toContain( + `the account is expected to be available again at ${new Date(resetAtMs).toISOString()}`, + ); + expect(body.message).not.toContain("limit resets"); + }); + + it("passes an unknown skip token through verbatim with neutral recovery wording", () => { + // The retry loop reports selection verdicts like "already-attempted" + // through the same seam; those must stay legible without claiming a + // limit or inventing a translation. + const resetAtMs = 1_700_000_030_000; + const body = buildPinnedUnavailableErrorBody( + 0, + new Map([[0, "already-attempted"]]), + { pinSource: "forced", resetAtMs, now: 1_700_000_000_000 }, + ); + expect(body.reason).toBe("already-attempted"); + expect(body.message).toContain("(already-attempted)"); + expect(body.message).toContain( + `the account is expected to be available again at ${new Date(resetAtMs).toISOString()}`, + ); + expect(body.message).not.toContain("limit resets"); + }); + + // The dominant #675 path: the pin takes a 503/429 during the request, the + // retry loop re-enters selection, and selection records "already-attempted" + // over whatever class actually blocks the account. Wording the sentence + // from that recorded verdict is what shipped an internal token and a + // class-less deadline to operators. + it("words the sentence from the re-read runtime blocker, not the loop's verdict", () => { + const resetAtMs = 1_700_000_030_000; + const body = buildPinnedUnavailableErrorBody( + 0, + new Map([[0, "already-attempted"]]), + { + pinSource: "forced", + resetAtMs, + recoveryBound: "other", + currentSkipReason: "circuit-open", + now: 1_700_000_000_000, + }, + ); + // The machine-readable contract still reports what selection decided. + expect(body.reason).toBe("already-attempted"); + // The human sentence describes what is actually gating the pin. + expect(body.message).toContain("(paused after repeated upstream errors)"); + expect(body.message).toContain( + `the next attempt is allowed at ${new Date(resetAtMs).toISOString()}`, + ); + expect(body.message).not.toContain("already-attempted"); + expect(body.message).not.toContain("circuit-open"); + }); + + it("keeps a recorded selection-only verdict that runtime state cannot express", () => { + // "policy-blocked" has no runtime-state equivalent, so a null re-read + // must not erase it from the sentence. + const body = buildPinnedUnavailableErrorBody( + 0, + new Map([[0, "policy-blocked"]]), + { pinSource: "manual", currentSkipReason: null }, + ); + expect(body.reason).toBe("policy-blocked"); + expect(body.message).toContain("(policy-blocked)"); + }); + + it("prefers the recorded verdict when it already names a blocker class", () => { + // A recorded "rate-limited" is a real class; a later re-read that has + // moved on to the cooldown it caused must not relabel the 503. + const resetAtMs = 1_700_000_030_000; + const body = buildPinnedUnavailableErrorBody( + 0, + new Map([[0, "rate-limited"]]), + { + resetAtMs, + recoveryBound: "rate-limit", + currentSkipReason: "cooling-down:rate-limit", + now: 1_700_000_000_000, + }, + ); + expect(body.message).toContain("(rate-limited)"); + expect(body.message).toContain( + `the rate limit resets at ${new Date(resetAtMs).toISOString()}`, + ); + }); + + // The bound used to be inferred from whether a second deadline key was + // present on the context object, so a caller that spread the key with an + // `undefined` value got the opposite wording from one that omitted it — + // with nothing in the type to say so. Both shapes must now agree. + it("treats an explicit undefined recovery bound as unmeasured, not as `other`", () => { + const resetAtMs = 1_700_000_030_000; + const omitted = buildPinnedUnavailableErrorBody( + 0, + new Map([[0, "rate-limited"]]), + { resetAtMs, now: 1_700_000_000_000 }, + ); + const explicitUndefined = buildPinnedUnavailableErrorBody( + 0, + new Map([[0, "rate-limited"]]), + { resetAtMs, recoveryBound: undefined, now: 1_700_000_000_000 }, + ); + expect(explicitUndefined.message).toBe(omitted.message); + expect(explicitUndefined.message).toContain( + `the rate limit resets at ${new Date(resetAtMs).toISOString()}`, + ); + }); + it("keeps the unpin advice for manual pins and nulls an unknown reset", () => { const body = buildPinnedUnavailableErrorBody( 1, diff --git a/test/runtime-rotation-proxy.test.ts b/test/runtime-rotation-proxy.test.ts index 1735cfbf..dec52922 100644 --- a/test/runtime-rotation-proxy.test.ts +++ b/test/runtime-rotation-proxy.test.ts @@ -844,7 +844,15 @@ describe("runtime rotation proxy", () => { 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"); + // This is the dominant #675 path, and the reason above is exactly why + // it was reported: selection had already overwritten the real class + // with its own bookkeeping token by the time the body was built. The + // sentence follows the pin's re-read runtime state instead, so the 429 + // that just landed is described as the rate limit it is — while the + // machine-readable `reason` still reports the selection verdict. + expect(payload.error.message).toContain("(rate-limited)"); + expect(payload.error.message).toContain("the rate limit resets at"); + expect(payload.error.message).not.toContain("already-attempted"); expect(payload.error.message).toContain("launcher"); expect(payload.error.message).not.toContain("unpin"); }); @@ -1016,6 +1024,63 @@ describe("runtime rotation proxy", () => { expect(Date.parse(payload.error.reset_at ?? "")).toBeGreaterThan(now); }); + it("does not word a later cooldown deadline as the rate-limit reset (#675)", 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"); + // Both records gate the account. Selection reports "rate-limited" by + // precedence, but recovery is bounded by whichever record ends last — + // here a server-error cooldown that outlives the limit by 50s. The + // sentence must not attribute that later timestamp to the rate limit. + // The record is keyed by the requested model's family ("gpt-5-codex" + // maps to its own family, not "codex"), or it would not gate at all. + pinned.rateLimitResetTimes = { "gpt-5-codex": now + 10_000 }; + pinned.coolingDownUntil = now + 60_000; + pinned.cooldownReason = "server-error"; + 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("rate-limited"); + // The machine contract still advertises the full recovery bound: the + // cooldown's end, not the limit's earlier reset. + expect(payload.error.retry_after_ms).toBeGreaterThan(10_000); + expect(payload.error.retry_after_ms).toBeLessThanOrEqual(60_000); + expect(Date.parse(payload.error.reset_at ?? "")).toBeGreaterThan( + now + 10_000, + ); + // The sentence names the blocker but keeps the deadline neutral. + expect(payload.error.message).toContain("(rate-limited)"); + expect(payload.error.message).toContain( + "the account is expected to be available again at", + ); + expect(payload.error.message).not.toContain("limit resets"); + }); + 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));