From 4eae8b0fd57ef1c107330bd2de1881e687eadeca Mon Sep 17 00:00:00 2001 From: Mike Bannister Date: Wed, 19 Aug 2026 20:40:40 -0400 Subject: [PATCH 1/5] fix(runtime): word the pinned-503 deadline by its blocker, not as a "limit" (#675) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The recovery deadline in the pinned-account 503 is max(rate-limit reset, cooldown end, circuit-breaker next-attempt) since #671, but the sentence called every one of them "the recorded limit" — so during the 2026-08-20 provider outage a 30-second breaker deadline printed as a limit reset and a backend incident read as a blown subscription quota, with the internal token "(circuit-open)" beside it. Skip-reason precedence makes the misdirection provable: rate-limited is tested before circuit-open, so a circuit-open verdict means the account was not rate limited at all. Derive the parenthetical and the deadline noun from the blocker class: a genuine rate limit keeps quota phrasing, a breaker or error cooldown names upstream errors as the cause and the timestamp as the next attempt, an auth or legacy rate-limit cooldown says the cooldown ends, and unknown tokens such as the retry loop's selection verdicts pass through verbatim with neutral recovery wording. Only the human message changes: the machine-readable reason keeps the raw token, the status stays 503, and permanent blockers keep their suppressed deadline. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01WBFDUprxzJ6YmuyxkbwB6k --- lib/request/rate-limit-decision.ts | 75 +++++++++++++++++++++- test/issue-474-pin-end-to-end.test.ts | 6 +- test/rate-limit-decision.test.ts | 91 ++++++++++++++++++++++++++- test/runtime-rotation-proxy.test.ts | 4 +- 4 files changed, 170 insertions(+), 6 deletions(-) diff --git a/lib/request/rate-limit-decision.ts b/lib/request/rate-limit-decision.ts index 388ae34e4..97ecfe307 100644 --- a/lib/request/rate-limit-decision.ts +++ b/lib/request/rate-limit-decision.ts @@ -213,6 +213,73 @@ export interface PinnedUnavailableContext { 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. + */ +function describePinnedBlocker(skipReason: string | null): { + parenthetical: string | null; + deadline: (resetAt: string) => string; +} { + switch (skipReason) { + case null: + return { + parenthetical: null, + deadline: (resetAt) => + `the account is expected to be available again at ${resetAt}`, + }; + case "rate-limited": + return { + parenthetical: "rate-limited", + deadline: (resetAt) => `the rate limit resets at ${resetAt}`, + }; + case "circuit-open": + return { + parenthetical: "paused after repeated upstream errors", + deadline: (resetAt) => `the next attempt is allowed at ${resetAt}`, + }; + case "cooling-down:server-error": + return { + parenthetical: "cooling down after upstream server errors", + deadline: (resetAt) => `the next attempt is allowed at ${resetAt}`, + }; + case "cooling-down:network-error": + return { + parenthetical: "cooling down after network errors", + deadline: (resetAt) => `the next attempt is allowed at ${resetAt}`, + }; + case "cooling-down:auth-failure": + return { + parenthetical: "cooling down after authentication failures", + deadline: (resetAt) => `the cooldown ends at ${resetAt}`, + }; + case "cooling-down:rate-limit": + return { + parenthetical: "cooling down after a rate limit", + deadline: (resetAt) => `the cooldown ends at ${resetAt}`, + }; + case "cooling-down": + return { + parenthetical: "cooling down", + deadline: (resetAt) => `the cooldown ends at ${resetAt}`, + }; + default: + // Permanent blockers never reach the deadline clause (the call site + // suppresses their reset time), and future or internal tokens such as + // the retry loop's "already-attempted" stay legible verbatim. + return { + parenthetical: skipReason, + deadline: (resetAt) => + `the account is expected to be available again at ${resetAt}`, + }; + } +} + export function buildPinnedUnavailableErrorBody( pinnedIndex: number | null | undefined, accountSkipReasons: ReadonlyMap, @@ -224,7 +291,10 @@ export function buildPinnedUnavailableErrorBody( normalizedPinnedIndex !== null ? accountSkipReasons.get(normalizedPinnedIndex) ?? null : null; - const reasonSuffix = skipReason ? ` (${skipReason})` : ""; + const blocker = describePinnedBlocker(skipReason); + const reasonSuffix = blocker.parenthetical + ? ` (${blocker.parenthetical})` + : ""; // 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 +320,7 @@ 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}` : ""; + 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/test/issue-474-pin-end-to-end.test.ts b/test/issue-474-pin-end-to-end.test.ts index 4717d7574..026bb8f10 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 2fd4f85f0..122d2aebc 100644 --- a/test/rate-limit-decision.test.ts +++ b/test/rate-limit-decision.test.ts @@ -311,13 +311,102 @@ 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", + }, + { + reason: "cooling-down:server-error", + parenthetical: "(cooling down after upstream server errors)", + deadlineNoun: "the next attempt is allowed at", + }, + { + reason: "cooling-down:network-error", + parenthetical: "(cooling down after network errors)", + deadlineNoun: "the next attempt is allowed 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("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"); + }); + 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 1735cfbf5..dfdfe89a2 100644 --- a/test/runtime-rotation-proxy.test.ts +++ b/test/runtime-rotation-proxy.test.ts @@ -844,7 +844,9 @@ 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"); + expect(payload.error.message).toContain( + "the account is expected to be available again at", + ); expect(payload.error.message).toContain("launcher"); expect(payload.error.message).not.toContain("unpin"); }); From 1fec5bf8e75b0636a995cff565143eb0a23d9a53 Mon Sep 17 00:00:00 2001 From: Mike Bannister Date: Wed, 19 Aug 2026 20:53:31 -0400 Subject: [PATCH 2/5] fix(runtime): only word the recovery deadline as a rate-limit reset when the rate limit supplies it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round 1 (Greptile P1): skip-reason precedence reports rate-limited whenever a live limit exists, but the advertised deadline is the max of every gating record — a breaker tripped seconds before a limit expires ends later, and the quota phrasing would attribute that breaker timestamp to the rate limit. The call site now measures the rate-limit records' own bound (getRateLimitRecoveryTimeForFamily, the two selection keys without the cooldown) and the quota phrasing is used only when that bound is the recovery deadline; otherwise the sentence falls back to the neutral availability wording, which is true regardless of which record holds the account. A context without the measurement keeps trusting the skip reason, so the JSON contract and every other caller are unchanged. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01WBFDUprxzJ6YmuyxkbwB6k --- lib/request/rate-limit-decision.ts | 40 ++++++++++++++++++++++---- lib/runtime-rotation-proxy.ts | 18 +++++++++++- lib/runtime/account-status.ts | 31 ++++++++++++++++++++ test/rate-limit-decision.test.ts | 45 ++++++++++++++++++++++++++++++ 4 files changed, 127 insertions(+), 7 deletions(-) diff --git a/lib/request/rate-limit-decision.ts b/lib/request/rate-limit-decision.ts index 97ecfe307..f76512d67 100644 --- a/lib/request/rate-limit-decision.ts +++ b/lib/request/rate-limit-decision.ts @@ -210,6 +210,15 @@ export interface PinnedUnavailableContext { pinSource?: "forced" | "manual" | null; /** Epoch ms when the blocking record ends (rate limit or cooldown). */ resetAtMs?: number | null; + /** + * Epoch ms when the rate-limit records alone stop gating the request, + * when the caller knows it. `resetAtMs` 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 — the quota phrasing is only + * used when the rate limit itself is that bound. Omitted entirely means + * unknown: trust the skip reason. + */ + rateLimitResetAtMs?: number | null; now?: number; } @@ -222,7 +231,10 @@ export interface PinnedUnavailableContext { * 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): { +function describePinnedBlocker( + skipReason: string | null, + rateLimitBoundsRecovery: boolean, +): { parenthetical: string | null; deadline: (resetAt: string) => string; } { @@ -236,7 +248,13 @@ function describePinnedBlocker(skipReason: string | null): { case "rate-limited": return { parenthetical: "rate-limited", - deadline: (resetAt) => `the rate limit resets at ${resetAt}`, + // 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 reset when the rate limit actually supplies it. + deadline: rateLimitBoundsRecovery + ? (resetAt) => `the rate limit resets at ${resetAt}` + : (resetAt) => + `the account is expected to be available again at ${resetAt}`, }; case "circuit-open": return { @@ -291,10 +309,6 @@ export function buildPinnedUnavailableErrorBody( normalizedPinnedIndex !== null ? accountSkipReasons.get(normalizedPinnedIndex) ?? null : null; - const blocker = describePinnedBlocker(skipReason); - const reasonSuffix = blocker.parenthetical - ? ` (${blocker.parenthetical})` - : ""; // On the desync path the pin index is unknown (null); claiming "account 1" // there would contradict the machine-readable pinnedAccountIndex: null. const accountPhrase = @@ -320,6 +334,20 @@ 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; + // A context without the key at all means the caller did not measure the + // rate-limit bound (older callers, unit seams): trust the skip reason. A + // present key whose value is not a live bound at-or-past the recovery + // deadline — null, undefined, or earlier — demotes to the neutral + // phrasing, which stays true for a live rate limit either way. + const rateLimitBoundsRecovery = + !("rateLimitResetAtMs" in (context ?? {})) || + (typeof context?.rateLimitResetAtMs === "number" && + resetAtMs !== null && + context.rateLimitResetAtMs >= resetAtMs); + const blocker = describePinnedBlocker(skipReason, 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. diff --git a/lib/runtime-rotation-proxy.ts b/lib/runtime-rotation-proxy.ts index 4a617ff28..e1aca831d 100644 --- a/lib/runtime-rotation-proxy.ts +++ b/lib/runtime-rotation-proxy.ts @@ -73,7 +73,10 @@ import { responseHeadersForClient, withTimeout, } from "./request/stream-failover-runtime.js"; -import { getAccountRecoveryTimeForFamily } from "./runtime/account-status.js"; +import { + getAccountRecoveryTimeForFamily, + getRateLimitRecoveryTimeForFamily, +} from "./runtime/account-status.js"; import { chooseAccount } from "./runtime/rotation-account-selection.js"; import { createRotationProxyState, @@ -1655,6 +1658,18 @@ async function handleRequestInner( pinnedStateRecoveryAtMs ?? 0, pinnedCircuitRecoveryAtMs ?? 0, ); + // The rate-limit records' own bound, so the message only words the + // recovery deadline as a rate-limit reset when the rate limit is in + // fact what supplies it — a breaker or cooldown can end later. + const pinnedRateLimitResetAtMs = + pinnedAccount === null + ? null + : getRateLimitRecoveryTimeForFamily( + pinnedAccount, + state.now(), + context.family, + context.model, + ); const errorBody = buildPinnedUnavailableErrorBody( pinnedIndex, accountSkipReasons, @@ -1663,6 +1678,7 @@ async function handleRequestInner( pinSource: typeof state.forcedAccountIndex === "number" ? "forced" : "manual", resetAtMs: pinnedResetAtMs, + rateLimitResetAtMs: pinnedRateLimitResetAtMs, now: state.now(), }, ); diff --git a/lib/runtime/account-status.ts b/lib/runtime/account-status.ts index 24df22636..8e6769547 100644 --- a/lib/runtime/account-status.ts +++ b/lib/runtime/account-status.ts @@ -75,6 +75,37 @@ export function getAccountRecoveryTimeForFamily( return latest; } +/** + * The rate-limit portion of getAccountRecoveryTimeForFamily: the latest + * active bound among exactly the two keys selection consults, with cooldowns + * excluded. The pinned-503 uses it to decide whether the recovery 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. + */ +export function getRateLimitRecoveryTimeForFamily( + account: { + rateLimitResetTimes?: Record; + }, + now: number, + 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)]); + } + return latest; +} + export function formatRateLimitEntry( account: { rateLimitResetTimes?: Record }, now: number, diff --git a/test/rate-limit-decision.test.ts b/test/rate-limit-decision.test.ts index 122d2aebc..0a3de8f17 100644 --- a/test/rate-limit-decision.test.ts +++ b/test/rate-limit-decision.test.ts @@ -389,6 +389,51 @@ describe("buildPinnedUnavailableErrorBody", () => { ); }); + 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, + rateLimitResetAtMs: resetAtMs, + 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 rateLimitResetAtMs = 1_700_000_010_000; + const resetAtMs = 1_700_000_030_000; + const body = buildPinnedUnavailableErrorBody( + 0, + new Map([[0, "rate-limited"]]), + { + pinSource: "forced", + resetAtMs, + rateLimitResetAtMs, + 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 From 09d586d33ae381fac7ffcb05f4958c9656a69ae2 Mon Sep 17 00:00:00 2001 From: Mike Bannister Date: Wed, 19 Aug 2026 21:01:30 -0400 Subject: [PATCH 3/5] test(runtime): prove the proxy threads the rate-limit bound through the pinned 503 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round 2: an end-to-end case where a live rate limit and a later server-error cooldown both gate the pinned account — reset_at advertises the cooldown's later deadline while the sentence keeps the (rate-limited) parenthetical and the neutral availability wording instead of calling that timestamp a rate-limit reset. The record is keyed by the requested model's own family; a "codex"-keyed record would not gate a gpt-5-codex request. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01WBFDUprxzJ6YmuyxkbwB6k --- test/runtime-rotation-proxy.test.ts | 57 +++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/test/runtime-rotation-proxy.test.ts b/test/runtime-rotation-proxy.test.ts index dfdfe89a2..70ddbd451 100644 --- a/test/runtime-rotation-proxy.test.ts +++ b/test/runtime-rotation-proxy.test.ts @@ -1018,6 +1018,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)); From f923c7abc59936eafc7080cca14cc0485427ea3d Mon Sep 17 00:00:00 2001 From: ndycode Date: Fri, 21 Aug 2026 05:56:50 +0800 Subject: [PATCH 4/5] refactor(runtime): derive both pinned-503 recovery bounds from one pass getRateLimitRecoveryTimeForFamily was a third copy of a walk that already existed twice in this module: getAccountRecoveryTimeForFamily above it and getRateLimitResetTimeForModel below, the latter documented with the identical contract ("the latest active bound among exactly the two keys selection consults"). Its only delta was accepting a nullable model. Three byte-identical `consider` closures and three copies of the getQuotaKey(family) / getQuotaKey(family, model) pair meant any change to the key set selection consults -- the drift that produced the prefix-vs-exact key bugs in #670/#671 -- had to land in three places, and missing the newest one would silently mis-word the pinned 503 rather than fail a test. Collapse them onto getAccountRecoveryBoundsForFamily, which returns both bounds from one pass over rateLimitResetTimes against one `now`. The two existing helpers stay as named views over it, so no caller changes. Callers that need both bounds can now take them from a single call, which is what the pinned-503 body does next: measuring them separately let a record expire between the two walks and reported a rate-limited pin as bounded by something else. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0199PddR9aYf5VsE6mnCb1Fa --- lib/runtime/account-status.ts | 134 +++++++++++++++++++--------------- 1 file changed, 77 insertions(+), 57 deletions(-) diff --git a/lib/runtime/account-status.ts b/lib/runtime/account-status.ts index 8e6769547..1af268db1 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,30 +120,11 @@ 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 rate-limit portion of getAccountRecoveryTimeForFamily: the latest - * active bound among exactly the two keys selection consults, with cooldowns - * excluded. The pinned-503 uses it to decide whether the recovery 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. - */ +/** The `rateLimitAtMs` bound alone; see getAccountRecoveryBoundsForFamily. */ export function getRateLimitRecoveryTimeForFamily( account: { rateLimitResetTimes?: Record; @@ -92,18 +133,8 @@ export function getRateLimitRecoveryTimeForFamily( 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)]); - } - return latest; + return getAccountRecoveryBoundsForFamily(account, now, family, model) + .rateLimitAtMs; } export function formatRateLimitEntry( @@ -144,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; } From 826553e3c6eee7b20cd0a295969de29744932e43 Mon Sep 17 00:00:00 2001 From: ndycode Date: Fri, 21 Aug 2026 05:57:15 +0800 Subject: [PATCH 5/5] fix(runtime): word the pinned 503 from the pin's live blocker, not the loop's verdict The blocker-aware wording was fed accountSkipReasons -- the SELECTION verdict -- so it missed the dominant path of #675. When the pinned account takes a 503/429 during the request, the retry loop re-enters selection, sees the pin in attemptedIndexes and records "already-attempted" over whatever class actually blocks it. describePinnedBlocker then fell to its default arm and printed "Pinned account 1 is currently unavailable (already-attempted); the account is expected to be available again at ...": an internal token and a class-less deadline, the exact complaint the issue was filed about. The block already re-read the pin's live runtime state 40 lines above, but used it only for the permanent-blocker test. Thread that live verdict through as `currentSkipReason` and word the sentence from it whenever the recorded verdict does not itself name a blocker class. A recorded verdict that does name one still wins, because the selection-only classes ("missing", "policy-blocked") have no runtime-state equivalent. The machine-readable `reason` is untouched and still reports what selection decided. The end-to-end 429 test asserted the old sentence, so it was locking the bug in; it now reads "(rate-limited); the rate limit resets at ...". Also on this path: - Replace the `rateLimitResetAtMs` deadline field with an explicit `recoveryBound: "rate-limit" | "other" | "unknown"`. The old shape branched on key PRESENCE (`"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 TypeScript could not express and no compiler could check. A regression test now pins both shapes to the same output. - Table-drive describePinnedBlocker. Eight switch arms produced four distinct deadline strings from nine copies of the same closure, and the cooldown arms were hand-written literals with no compile-time link to CooldownReason, so a fifth reason would fall through and leak `cooling-down:` raw into the sentence -- reintroducing the very leak this PR removes. The cooldown table is now `satisfies Record`, making that omission a build error. - Unify the cooldown deadline noun. server-error and network-error said "the next attempt is allowed at" while auth-failure, rate-limit and bare cooling-down said "the cooldown ends at", though all five are bounded by the same coolingDownUntil field -- an operator comparing two 503s from one mechanism saw two different nouns. - Read the clock once. Building one body called state.now() four times and walked rateLimitResetTimes twice, so a record expiring between the two walks made a genuinely rate-limited pin print the neutral sentence. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0199PddR9aYf5VsE6mnCb1Fa --- lib/request/rate-limit-decision.ts | 237 +++++++++++++++++++--------- lib/runtime-rotation-proxy.ts | 51 +++--- test/rate-limit-decision.test.ts | 97 +++++++++++- test/runtime-rotation-proxy.test.ts | 12 +- 4 files changed, 293 insertions(+), 104 deletions(-) diff --git a/lib/request/rate-limit-decision.ts b/lib/request/rate-limit-decision.ts index f76512d67..f7196a5e9 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"; @@ -211,17 +212,124 @@ export interface PinnedUnavailableContext { /** Epoch ms when the blocking record ends (rate limit or cooldown). */ resetAtMs?: number | null; /** - * Epoch ms when the rate-limit records alone stop gating the request, - * when the caller knows it. `resetAtMs` 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 — the quota phrasing is only - * used when the rate limit itself is that bound. Omitted entirely means - * unknown: trust the skip reason. + * 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. */ - rateLimitResetAtMs?: number | null; + 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 @@ -238,64 +346,33 @@ function describePinnedBlocker( parenthetical: string | null; deadline: (resetAt: string) => string; } { - switch (skipReason) { - case null: - return { - parenthetical: null, - deadline: (resetAt) => - `the account is expected to be available again at ${resetAt}`, - }; - case "rate-limited": - return { - parenthetical: "rate-limited", - // 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 reset when the rate limit actually supplies it. - deadline: rateLimitBoundsRecovery - ? (resetAt) => `the rate limit resets at ${resetAt}` - : (resetAt) => - `the account is expected to be available again at ${resetAt}`, - }; - case "circuit-open": - return { - parenthetical: "paused after repeated upstream errors", - deadline: (resetAt) => `the next attempt is allowed at ${resetAt}`, - }; - case "cooling-down:server-error": - return { - parenthetical: "cooling down after upstream server errors", - deadline: (resetAt) => `the next attempt is allowed at ${resetAt}`, - }; - case "cooling-down:network-error": - return { - parenthetical: "cooling down after network errors", - deadline: (resetAt) => `the next attempt is allowed at ${resetAt}`, - }; - case "cooling-down:auth-failure": - return { - parenthetical: "cooling down after authentication failures", - deadline: (resetAt) => `the cooldown ends at ${resetAt}`, - }; - case "cooling-down:rate-limit": - return { - parenthetical: "cooling down after a rate limit", - deadline: (resetAt) => `the cooldown ends at ${resetAt}`, - }; - case "cooling-down": - return { - parenthetical: "cooling down", - deadline: (resetAt) => `the cooldown ends at ${resetAt}`, - }; - default: - // Permanent blockers never reach the deadline clause (the call site - // suppresses their reset time), and future or internal tokens such as - // the retry loop's "already-attempted" stay legible verbatim. - return { - parenthetical: skipReason, - deadline: (resetAt) => - `the account is expected to be available again at ${resetAt}`, - }; + 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( @@ -334,17 +411,27 @@ 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; - // A context without the key at all means the caller did not measure the - // rate-limit bound (older callers, unit seams): trust the skip reason. A - // present key whose value is not a live bound at-or-past the recovery - // deadline — null, undefined, or earlier — demotes to the neutral - // phrasing, which stays true for a live rate limit either way. - const rateLimitBoundsRecovery = - !("rateLimitResetAtMs" in (context ?? {})) || - (typeof context?.rateLimitResetAtMs === "number" && - resetAtMs !== null && - context.rateLimitResetAtMs >= resetAtMs); - const blocker = describePinnedBlocker(skipReason, rateLimitBoundsRecovery); + // "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})` : ""; diff --git a/lib/runtime-rotation-proxy.ts b/lib/runtime-rotation-proxy.ts index e1aca831d..573316c49 100644 --- a/lib/runtime-rotation-proxy.ts +++ b/lib/runtime-rotation-proxy.ts @@ -73,10 +73,7 @@ import { responseHeadersForClient, withTimeout, } from "./request/stream-failover-runtime.js"; -import { - getAccountRecoveryTimeForFamily, - getRateLimitRecoveryTimeForFamily, -} from "./runtime/account-status.js"; +import { getAccountRecoveryBoundsForFamily } from "./runtime/account-status.js"; import { chooseAccount } from "./runtime/rotation-account-selection.js"; import { createRotationProxyState, @@ -1635,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) @@ -1658,18 +1664,17 @@ async function handleRequestInner( pinnedStateRecoveryAtMs ?? 0, pinnedCircuitRecoveryAtMs ?? 0, ); - // The rate-limit records' own bound, so the message only words the - // recovery deadline as a rate-limit reset when the rate limit is in - // fact what supplies it — a breaker or cooldown can end later. + // 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 = - pinnedAccount === null - ? null - : getRateLimitRecoveryTimeForFamily( - pinnedAccount, - state.now(), - context.family, - context.model, - ); + pinnedRecoveryBounds?.rateLimitAtMs ?? null; + const recoveryBound = + pinnedResetAtMs !== null && + pinnedRateLimitResetAtMs !== null && + pinnedRateLimitResetAtMs >= pinnedResetAtMs + ? "rate-limit" + : "other"; const errorBody = buildPinnedUnavailableErrorBody( pinnedIndex, accountSkipReasons, @@ -1678,8 +1683,12 @@ async function handleRequestInner( pinSource: typeof state.forcedAccountIndex === "number" ? "forced" : "manual", resetAtMs: pinnedResetAtMs, - rateLimitResetAtMs: pinnedRateLimitResetAtMs, - 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/test/rate-limit-decision.test.ts b/test/rate-limit-decision.test.ts index 0a3de8f17..99c11a92f 100644 --- a/test/rate-limit-decision.test.ts +++ b/test/rate-limit-decision.test.ts @@ -329,15 +329,20 @@ describe("buildPinnedUnavailableErrorBody", () => { 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 next attempt is allowed at", + deadlineNoun: "the cooldown ends at", }, { reason: "cooling-down:network-error", parenthetical: "(cooling down after network errors)", - deadlineNoun: "the next attempt is allowed at", + deadlineNoun: "the cooldown ends at", }, { reason: "cooling-down:auth-failure", @@ -397,7 +402,7 @@ describe("buildPinnedUnavailableErrorBody", () => { { pinSource: "manual", resetAtMs, - rateLimitResetAtMs: resetAtMs, + recoveryBound: "rate-limit", now: 1_700_000_000_000, }, ); @@ -412,7 +417,6 @@ describe("buildPinnedUnavailableErrorBody", () => { // 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 rateLimitResetAtMs = 1_700_000_010_000; const resetAtMs = 1_700_000_030_000; const body = buildPinnedUnavailableErrorBody( 0, @@ -420,7 +424,7 @@ describe("buildPinnedUnavailableErrorBody", () => { { pinSource: "forced", resetAtMs, - rateLimitResetAtMs, + recoveryBound: "other", now: 1_700_000_000_000, }, ); @@ -452,6 +456,89 @@ describe("buildPinnedUnavailableErrorBody", () => { 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 70ddbd451..dec529222 100644 --- a/test/runtime-rotation-proxy.test.ts +++ b/test/runtime-rotation-proxy.test.ts @@ -844,9 +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 account is expected to be available again 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"); });