fix: bug sweep — 8 confirmed correctness bugs across refresh, quota, storage, request & budget

lib/forecast.ts

@@ -4,7 +4,7 @@ import type { QuotaCacheData } from "./quota-cache.js";
 import {
 	findQuotaCacheEntryForAccount,
 	isQuotaCacheEntryExhausted,
-	quotaLeftPercentFromUsed,
+	quotaUsedPercentIsExhausted,
 } from "./quota-readiness.js";
 import { getRateLimitResetTimeForFamily } from "./runtime/account-status.js";
 import type { AccountMetadataV3 } from "./storage.js";
@@ -140,8 +140,10 @@ function getLiveQuotaWaitMs(
 		// resets ~7d out; folding that in would overstate the wait by orders of
 		// magnitude and invert the account recommendation (stress audit H5). On a
 		// 429 we honor every future window, since the upstream said "slow down now"
-		// regardless of the usage gauge.
-		if (onlyExhausted && quotaLeftPercentFromUsed(window?.usedPercent) !== 0) {
+		// regardless of the usage gauge. The test is the RAW usedPercent, never the
+		// rounded left-percent: 100 - 99.6 rounds to 0 left, which would bench a
+		// window that still has quota and falsely mark the account "delayed".
+		if (onlyExhausted && !quotaUsedPercentIsExhausted(window?.usedPercent)) {
 			continue;
 		}
 		const resetAt = window?.resetAtMs;
@@ -246,10 +248,15 @@ export function evaluateForecastAccount(
 		input.allAccounts ?? [account],
 	);
 	if (isQuotaCacheEntryExhausted(quotaEntry, now)) {
+		// Only a window that is genuinely at/over 100% used should contribute its
+		// reset time. Testing the ROUNDED left-percent instead would treat a
+		// 99.6%-used sibling as at-limit and fold its (possibly far-future) reset
+		// into the Math.max below, overstating the wait for an account whose
+		// actually-exhausted window recovers much sooner.
 		const resetAts = [quotaEntry?.primary, quotaEntry?.secondary]
 			.filter(
 				(window) =>
-					quotaLeftPercentFromUsed(window?.usedPercent) === 0 &&
+					quotaUsedPercentIsExhausted(window?.usedPercent) &&
 					typeof window?.resetAtMs === "number" &&
 					Number.isFinite(window.resetAtMs) &&
 					window.resetAtMs > now,
@@ -329,6 +336,26 @@ export function evaluateForecastAccount(
 			availability = "delayed";
 		}
 
+		// A live window at 100% used with NO resetAtMs has no known recovery time —
+		// the probe reports it fully consumed and cannot say when it refills. On a
+		// 200 probe getLiveQuotaWaitMs then yields 0 (no reset to wait on) and the
+		// 429 branch never fires, so without this the account would stay "ready" and
+		// be recommended as a healthy pick. Treat that case as exhausted and downgrade
+		// a "ready" account to "delayed". A window WITH a resetAtMs is only *delayed*,
+		// not exhausted: getLiveQuotaWaitMs already contributes its wait above, so a
+		// 429 with known reset times stays a recoverable "delayed" account rather than
+		// a blocked one. The raw-usedPercent test (not rounded left-percent) keeps
+		// 99.6% from being falsely benched.
+		const liveExhausted =
+			(quotaUsedPercentIsExhausted(quota.primary.usedPercent) &&
+				typeof quota.primary.resetAtMs !== "number") ||
+			(quotaUsedPercentIsExhausted(quota.secondary.usedPercent) &&
+				typeof quota.secondary.resetAtMs !== "number");
+		if (liveExhausted) {
+			exhausted = true;
+			if (availability === "ready") availability = "delayed";
+		}
+
 		const primaryUsage = describeQuotaUsage(
 			"primary",
 			quota.primary.usedPercent,

lib/policy/runtime-policy.ts

@@ -9,6 +9,7 @@ import {
 	evaluateBudgetGuard,
 	getBudgetWindowStart,
 	loadBudgetGuardStore,
+	normalizeBudgetKey,
 	type BudgetGuardEvaluation,
 	type BudgetGuardStore,
 } from "../budget-guard.js";
@@ -105,12 +106,19 @@ async function evaluateBudgets(input: {
 	now: number;
 }): Promise<BudgetGuardEvaluation[]> {
 	const keys = new Set<string>();
+	// `global` is already normalize-clean. Project/profile keys can carry uppercase
+	// or spaces, but budget-guard STORES limits only under normalizeBudgetKey (see
+	// upsertBudgetLimit and load-time normalizeStore). Look them up the same way, or
+	// a key like `project:MyApp` never matches its stored `project:myapp` and the
+	// budget is silently unenforced.
 	keys.add("global");
 	if (input.state.project.projectKey) {
-		keys.add(`project:${input.state.project.projectKey}`);
+		const projectKey = normalizeBudgetKey(`project:${input.state.project.projectKey}`);
+		if (projectKey) keys.add(projectKey);
 	}
 	if (input.state.project.profile?.budgetKey) {
-		keys.add(input.state.project.profile.budgetKey);
+		const budgetKey = normalizeBudgetKey(input.state.project.profile.budgetKey);
+		if (budgetKey) keys.add(budgetKey);
 	}
 	const evaluations: BudgetGuardEvaluation[] = [];
 	for (const key of keys) {

lib/quota-readiness.ts

@@ -74,6 +74,22 @@ export function quotaLeftPercentFromUsed(
 	return Math.max(0, Math.min(100, Math.round(100 - usedPercent)));
 }
 
+// A window is exhausted only when it is genuinely at/over 100% used. Base this on
+// the RAW usedPercent, never the rounded quotaLeftPercentFromUsed: rounding
+// 100 - 99.6 = 0.4 down to 0 left-percent would falsely bench a window that still
+// has ~0.4% quota (any usedPercent in (99.5, 100) rounds to 0 left). The header
+// parser preserves fractional used-percent, so this input is expected;
+// quotaLeftPercentFromUsed stays for DISPLAY only.
+export function quotaUsedPercentIsExhausted(
+	usedPercent: number | undefined,
+): boolean {
+	return (
+		typeof usedPercent === "number" &&
+		Number.isFinite(usedPercent) &&
+		usedPercent >= 100
+	);
+}
+
 function quotaWindowIsExhausted(
 	window: QuotaWindowLike | undefined,
 	now = Date.now(),
@@ -96,8 +112,7 @@ function quotaWindowIsExhausted(
 	) {
 		return false;
 	}
-	const leftPercent = quotaLeftPercentFromUsed(window?.usedPercent);
-	return typeof leftPercent === "number" && leftPercent <= 0;
+	return quotaUsedPercentIsExhausted(window?.usedPercent);
 }
 
 export function isQuotaCacheEntryExhausted(

lib/refresh-lease.ts

@@ -11,7 +11,9 @@ import { isRecord } from "./utils.js";
 const log = createLogger("refresh-lease");
 
 const DEFAULT_LEASE_TTL_MS = 30_000;
-const DEFAULT_WAIT_TIMEOUT_MS = 35_000;
+// Exported so the refresh queue can size its acquire-stage eviction threshold
+// above the maximum time a lease acquire() may legitimately block waiting.
+export const DEFAULT_WAIT_TIMEOUT_MS = 35_000;
 const DEFAULT_POLL_INTERVAL_MS = 150;
 const DEFAULT_RESULT_TTL_MS = 20_000;
 const RETRYABLE_IO_ERRORS = new Set(["EBUSY", "EPERM", "EMFILE", "ENFILE"]);
@@ -177,6 +179,20 @@ export class RefreshLeaseCoordinator {
 		this.fsOps = options.fsOps ?? fs;
 	}
 
+	/**
+	 * Resolved maximum time `acquire()` may block waiting for a lease.
+	 *
+	 * Exposed so the refresh queue can size its acquire-stage eviction threshold
+	 * against the budget this coordinator ACTUALLY uses. The budget is
+	 * configurable (constructor option / `CODEX_AUTH_REFRESH_LEASE_WAIT_MS`), so
+	 * sizing eviction off the static `DEFAULT_WAIT_TIMEOUT_MS` would evict an
+	 * acquire that is still legitimately waiting under a larger budget, spawning
+	 * the duplicate refresh (→ `invalid_grant`) the lease exists to prevent.
+	 */
+	get configuredWaitTimeoutMs(): number {
+		return this.waitTimeoutMs;
+	}
+
 	static fromEnvironment(): RefreshLeaseCoordinator {
 		const testMode = process.env.VITEST === "true" || process.env.NODE_ENV === "test";
 		const enabled =
@@ -311,7 +327,14 @@ export class RefreshLeaseCoordinator {
 				if (released) return;
 				released = true;
 				try {
-					if (result) {
+					// Only ever cache a SUCCESSFUL refresh in the cross-process lease.
+					// A `failed` result carries no token material to share; caching it
+					// would make readFreshResult serve the failure verbatim to every
+					// follower for the whole result TTL (DEFAULT_RESULT_TTL_MS), blocking
+					// a real refresh and even escalating to cooldowns. On failure we skip
+					// the cache and still unlink the lock in `finally`, so the next caller
+					// becomes owner and retries immediately.
+					if (result?.type === "success") {
 						await this.writeResult(resultPath, tokenHash, result);
 					}
 				} finally {

lib/refresh-queue.ts

@@ -12,11 +12,19 @@ import { createHash } from "node:crypto";
 import { refreshAccessToken } from "./auth/auth.js";
 import type { TokenResult } from "./types.js";
 import { createLogger } from "./logger.js";
-import { RefreshLeaseCoordinator } from "./refresh-lease.js";
+import { RefreshLeaseCoordinator, DEFAULT_WAIT_TIMEOUT_MS } from "./refresh-lease.js";
 import { isAbortError } from "./utils.js";
 
 const log = createLogger("refresh-queue");
 
+/**
+ * Extra headroom beyond the lease wait budget before an acquire-stage entry is
+ * treated as genuinely stuck. A lease acquire() polls on an interval and does
+ * filesystem work, so it can overshoot its wait deadline slightly; this slack
+ * keeps cleanup from evicting an acquire that is still legitimately waiting.
+ */
+const ACQUIRE_EVICTION_SLACK_MS = 5_000;
+
 /**
  * Non-reversible correlation fingerprint for a token, for logs.
  *
@@ -340,10 +348,31 @@ export class RefreshQueue {
    */
   private cleanup(): void {
     const now = Date.now();
+    // A lease acquire() can legitimately block up to the coordinator's wait
+    // budget. Evicting an acquire-stage entry before that budget elapses would
+    // spawn a duplicate refresh whose refresh token OpenAI has already rotated on
+    // first use (invalid_grant). So an acquire-stage entry is only evicted once it
+    // exceeds BOTH the configured max age AND the lease wait budget plus slack —
+    // never merely maxEntryAgeMs, which can be shorter than the wait budget.
+    //
+    // Read the budget the coordinator was actually CONFIGURED with (constructor
+    // option / CODEX_AUTH_REFRESH_LEASE_WAIT_MS) rather than the static default:
+    // under a larger configured budget the default would evict too early. An
+    // injected test double may not expose the getter, so fall back to the default.
+    const configuredWaitTimeoutMs = this.leaseCoordinator.configuredWaitTimeoutMs;
+    const leaseWaitTimeoutMs =
+      typeof configuredWaitTimeoutMs === "number" &&
+      Number.isFinite(configuredWaitTimeoutMs)
+        ? configuredWaitTimeoutMs
+        : DEFAULT_WAIT_TIMEOUT_MS;
+    const acquireEvictionAgeMs = Math.max(
+      this.maxEntryAgeMs,
+      leaseWaitTimeoutMs + ACQUIRE_EVICTION_SLACK_MS,
+    );
     for (const [token, entry] of this.pending.entries()) {
       const ageMs = now - entry.startedAt;
-      if (ageMs <= this.maxEntryAgeMs) continue;
       if (entry.stage === "acquire") {
+        if (ageMs <= acquireEvictionAgeMs) continue;
         log.warn("Evicting stale refresh entry during lease acquire stage", {
           tokenSuffix: tokenFingerprint(token),
           ageMs,
@@ -352,6 +381,7 @@ export class RefreshQueue {
         this.cleanupRotationMapping(token);
         continue;
       }
+      if (ageMs <= this.maxEntryAgeMs) continue;
       if (!entry.staleWarningLogged) {
         log.warn("Refresh entry exceeded stale warning threshold", {
           tokenSuffix: tokenFingerprint(token),

lib/request/request-transformer.ts

@@ -658,7 +658,8 @@ export function trimInputForFastSession(
 		break;
 	}
 
-	for (let i = Math.max(0, input.length - safeMax); i < input.length; i++) {
+	const tailStart = Math.max(0, input.length - safeMax);
+	for (let i = tailStart; i < input.length; i++) {
 		if (excludedHeadIndexes.has(i)) continue;
 		keepIndexes.add(i);
 	}
@@ -667,7 +668,18 @@ export function trimInputForFastSession(
 	if (trimmed.length === 0) return input;
 	if (input.length <= maxItems && excludedHeadIndexes.size === 0) return input;
 	if (trimmed.length <= safeMax) return trimmed;
-	return trimmed.slice(trimmed.length - safeMax);
+
+	// Kept head items are always the LOWEST kept indexes, so they occupy the first
+	// `keptHead` entries of `trimmed`. Reserve budget for exactly that many.
+	// Recounting the kept indexes below `tailStart` instead would miss a head
+	// instruction that ALSO falls inside the tail window (input.length only just
+	// over safeMax), and the tail slice would then drop it. The two slices cannot
+	// overlap here: keptHead + tailBudget === safeMax < trimmed.length.
+	const tailBudget = Math.max(1, safeMax - keptHead);
+	return [
+		...trimmed.slice(0, keptHead),
+		...trimmed.slice(trimmed.length - tailBudget),
+	];
 }
 
 export interface FastSessionInputTrimPlan {

lib/request/response-handler.ts

@@ -1027,7 +1027,19 @@ export function isEmptyResponse(body: unknown): boolean {
 	if (Object.keys(obj).length === 0) return true;
 
 	const hasOutput =
-		"output" in obj && obj.output !== null && obj.output !== undefined;
+		"output" in obj &&
+		obj.output !== null &&
+		obj.output !== undefined &&
+		(Array.isArray(obj.output)
+			? obj.output.some(
+					(o) =>
+						o !== null &&
+						o !== undefined &&
+						(typeof o !== "object" || Object.keys(o as object).length > 0),
+				)
+			: typeof obj.output === "string"
+				? obj.output.trim() !== ""
+				: true);
 	const hasChoices =
 		"choices" in obj &&
 		Array.isArray(obj.choices) &&

lib/rotation.ts

@@ -198,7 +198,14 @@ export const DEFAULT_TOKEN_BUCKET_CONFIG: TokenBucketConfig = {
 	tokensPerMinute: 6,
 };
 
-const TOKEN_REFUND_WINDOW_MS = 30_000;
+// Must cover the full request lifetime so a token consumed at request start can
+// still be refunded when the request fails at the very end. The runtime proxy
+// refunds on network error / upstream timeout, and the default fetch timeout is
+// 60_000ms (config.ts fetchTimeoutMs) — measured AFTER token consumption and a
+// token refresh. 90_000ms = that 60s timeout plus slack for the refresh and
+// processing, so a genuinely timed-out request's token is reversed instead of
+// leaking (gradual token-bucket starvation -> spurious token-exhausted skips).
+const TOKEN_REFUND_WINDOW_MS = 90_000;
 
 interface TokenBucketEntry {
 	tokens: number;

lib/storage.ts

@@ -811,6 +811,7 @@ async function migrateLegacyProjectStorageIfNeeded(options?: {
 			targetStorage,
 			legacyStorage,
 			normalizeAccountStorage,
+			findMatchingAccountIndex,
 		);
 		const fallbackStorage = targetStorage ?? legacyStorage;
 
@@ -2203,6 +2204,7 @@ export async function importAccounts(
 		mergeImportedAccounts,
 		maxAccounts: ACCOUNT_LIMITS.MAX_ACCOUNTS,
 		deduplicateAccounts,
+		findMatchingAccountIndex,
 		logInfo: (message, details) => {
 			log.info(message, details);
 		},

lib/storage/account-port.ts

@@ -60,6 +60,10 @@ export async function importAccountsSnapshot(params: {
 		deduplicateAccounts: (
 			accounts: AccountStorageV3["accounts"],
 		) => AccountStorageV3["accounts"];
+		findMatchingAccountIndex: (
+			accounts: AccountStorageV3["accounts"],
+			candidate: AccountStorageV3["accounts"][number],
+		) => number | undefined;
 	}) => {
 		newStorage: AccountStorageV3;
 		imported: number;
@@ -70,6 +74,12 @@ export async function importAccountsSnapshot(params: {
 	deduplicateAccounts: (
 		accounts: AccountStorageV3["accounts"],
 	) => AccountStorageV3["accounts"];
+	// Forwarded to `mergeImportedAccounts` so it can re-resolve the manual pin by
+	// identity after dedupe reorders the merged account list.
+	findMatchingAccountIndex: (
+		accounts: AccountStorageV3["accounts"],
+		candidate: AccountStorageV3["accounts"][number],
+	) => number | undefined;
 	logInfo: (message: string, details: Record<string, unknown>) => void;
 }): Promise<{ imported: number; total: number; skipped: number }> {
 	const normalized = await params.readImportFile({
@@ -84,6 +94,7 @@ export async function importAccountsSnapshot(params: {
 				imported: normalized,
 				maxAccounts: params.maxAccounts,
 				deduplicateAccounts: params.deduplicateAccounts,
+				findMatchingAccountIndex: params.findMatchingAccountIndex,
 			});
 			await persist(merged.newStorage);
 			return {