fix(runtime): word the pinned-503 deadline by its blocker, not as a "limit" (#675)

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<DeadlineNoun, (resetAt: string) => 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:<reason>` 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<CooldownReason, BlockerDescription>;
+
+const BLOCKER_DESCRIPTIONS: ReadonlyMap<string, BlockerDescription> = 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<number, string>,
@@ -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 =

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) {