From 31c163c26d7be518d2039e16dede8ada9f3e3d67 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Wed, 26 Aug 2026 11:58:59 +0900 Subject: [PATCH] fix(codex): classify a sub-day quota header window as the 5h burst (Plus/Team) Codex removed the 5-hour rate limit some time ago and has now restored it for Plus and Team, while Pro stays weekly-only. OpenCodex reads the same account state from two wires and only one of them classifies windows by duration. parseUsageQuota has always used the duration: anything under 24h is a burst window. parseUpstreamQuotaHeaders knew only "explicitly monthly, or else weekly", so once the 5h window returned it filed the burst reading as the weekly one. Identical upstream data, before this change: headers {primary 97% / 300 min, secondary 12% / 10080 min} parseUpstreamQuotaHeaders -> {weeklyPercent: 97} parseUsageQuota -> {shortPercent: 97, weeklyPercent: 12} Three consequences, worst last: the real weekly reading is discarded; the GUI shows a weekly bar at 100% and no 5h bar, so the operator cannot tell which limit they hit; and a 5h-exhausted account keeps weeklyPercent=100 after the burst window resets, so pool routing avoids a healthy account until an unrelated WHAM refresh overwrites it. The header parser now derives its threshold from the same constant the WHAM parser uses, rather than repeating the number. A declared sub-day primary becomes shortPercent/shortResetAt/shortWindowSeconds and vacates the primary slot so the secondary can be what it always was: the weekly reading. A primary with no declared duration is untouched, because legacy payloads omit the header and guessing there would reclassify every account that predates the field. src/routing/quota.ts changes with it, and must. codexAccountQuotaEvidence scored headroom from weekly and monthly only. That was survivable while the broken parser wrote 5h values into weeklyPercent - routing saw the burst by accident. Fixing the parser alone moves a 5h-exhausted account from 0.03 to 0.88 headroom and routes it straight into a 429. The bug was cancelling itself out; removing one half without the other is worse than leaving both. Tests: five header-classification cases including the 1439/1440-minute boundary and the legacy no-duration path; a new parity suite asserting the two parsers assign identical upstream data to the same windows; three routing regressions covering burst-exhausted, fully-exhausted, and weekly-bound accounts. Falsified both ways - reverting the parser branch fails 7, reverting the routing fold fails 1. bun run typecheck exit 0. bun run test 0 fail. --- src/codex/quota.ts | 46 ++++++++-- src/routing/quota.ts | 10 +++ tests/codex-quota-parser-parity.test.ts | 112 ++++++++++++++++++++++++ tests/rate-limit-reset-credits.test.ts | 92 +++++++++++++++++++ 4 files changed, 253 insertions(+), 7 deletions(-) create mode 100644 tests/codex-quota-parser-parity.test.ts diff --git a/src/codex/quota.ts b/src/codex/quota.ts index ae9152c0ed..3248186375 100644 --- a/src/codex/quota.ts +++ b/src/codex/quota.ts @@ -95,6 +95,9 @@ const MONTHLY_WINDOW_MIN_SECONDS = 28 * 24 * 60 * 60; */ const WEEKLY_WINDOW_MIN_SECONDS = 24 * 60 * 60; const MONTHLY_WINDOW_MIN_MINUTES = MONTHLY_WINDOW_MIN_SECONDS / 60; +// Derived, never written as a literal: the header parser and the WHAM parser must not be able +// to drift to different thresholds, which is the class of defect this pair exists to prevent. +const WEEKLY_WINDOW_MIN_MINUTES = WEEKLY_WINDOW_MIN_SECONDS / 60; const accountQuota = new Map(); let lastReconciledGeneration = 0; @@ -219,14 +222,24 @@ function isExplicitMonthlyWindow(window: WhamUsageWindow | null | undefined): bo } function isExplicitMonthlyWindowMinutes(windowMinutes: unknown): boolean { - const minutes = typeof windowMinutes === "number" - ? windowMinutes - : typeof windowMinutes === "string" && windowMinutes.trim() !== "" - ? Number(windowMinutes) + const minutes = windowMinutes_(windowMinutes); + return minutes !== undefined && minutes >= MONTHLY_WINDOW_MIN_MINUTES; +} + +/** The header wire reports a window duration in MINUTES; WHAM reports it in seconds. */ +function windowMinutes_(value: unknown): number | undefined { + const minutes = typeof value === "number" + ? value + : typeof value === "string" && value.trim() !== "" + ? Number(value) : undefined; - return typeof minutes === "number" - && Number.isFinite(minutes) - && minutes >= MONTHLY_WINDOW_MIN_MINUTES; + return typeof minutes === "number" && Number.isFinite(minutes) ? minutes : undefined; +} + +/** Minutes-domain twin of isExplicitShortWindow. Same strict `<`, same 24h discriminator. */ +function isExplicitShortWindowMinutes(value: unknown): boolean { + const minutes = windowMinutes_(value); + return minutes !== undefined && minutes > 0 && minutes < WEEKLY_WINDOW_MIN_MINUTES; } @@ -342,6 +355,12 @@ export function parseUpstreamQuotaHeaders(headers: Headers): Omit typeof value === "number" && Number.isFinite(value)); const maxPercent = percents.length > 0 ? Math.max(...percents) : undefined; // Credits-only snapshots prove neither usage nor exhaustion. Unknown must not @@ -49,6 +55,10 @@ function codexAccountQuotaEvidence(accountId: string, plan?: string): RouteQuota const resets = [ ...(monthly ? [] : [quota.weeklyResetAt]), quota.monthlyResetAt, + // Pair the reset with the window that can actually gate the next request: a burst-limited + // account recovers in hours, and reporting a distant weekly reset would defer a retry that + // is already safe. + quota.shortResetAt, ].filter((value): value is number => typeof value === "number" && Number.isFinite(value)) .filter(value => value > Date.now()); return { diff --git a/tests/codex-quota-parser-parity.test.ts b/tests/codex-quota-parser-parity.test.ts new file mode 100644 index 0000000000..1f909f4f4e --- /dev/null +++ b/tests/codex-quota-parser-parity.test.ts @@ -0,0 +1,112 @@ +import { describe, expect, it } from "bun:test"; +import { + clearAccountQuota, + parseUpstreamQuotaHeaders, + parseUsageQuota, + setAccountQuotaFromParsed, +} from "../src/codex/quota"; +import { codexPoolQuotaEvidence } from "../src/routing/quota"; + +/** + * The two quota parsers, pinned against each other. + * + * Codex reports the same account state twice: as response headers on every request, and as a + * WHAM usage payload on refresh. `parseUsageQuota` classified windows by DURATION from the + * start; `parseUpstreamQuotaHeaders` only knew "explicitly monthly, or else weekly". While + * Codex had no 5-hour window that difference was invisible. When the window came back for Plus + * and Team, the header path started filing a 5h reading as the weekly one, discarding the real + * weekly value and leaving the account exhausted long after the burst reset. + * + * This is the assertion that would have caught it: the parsers must agree about WHICH WINDOW a + * number belongs to, whichever wire it arrived on. It compares window assignment rather than + * whole objects, because the WHAM payload also carries provenance and Spark windows the header + * wire does not. + */ +describe("quota parser parity: headers and WHAM agree on window assignment", () => { + const cases = [ + { name: "Plus/Team 5h burst + 7-day weekly", minutes: 300, seconds: 18_000, primary: 97, secondary: 12 }, + { name: "Pro weekly-only", minutes: 10_080, seconds: 604_800, primary: 80, secondary: undefined }, + { name: "monthly plan with a weekly secondary", minutes: 43_800, seconds: 2_628_000, primary: 100, secondary: 22 }, + { name: "sub-hour burst", minutes: 15, seconds: 900, primary: 40, secondary: 5 }, + ] as const; + + /** Which field each percent landed in — the only thing both wires can be compared on. */ + function assignment(quota: Record | null): Record { + return { + shortPercent: quota?.shortPercent, + weeklyPercent: quota?.weeklyPercent, + monthlyPercent: quota?.monthlyPercent, + }; + } + + for (const testCase of cases) { + it(`agrees on ${testCase.name}`, () => { + const headers = new Headers({ + "x-codex-primary-used-percent": String(testCase.primary), + "x-codex-primary-window-minutes": String(testCase.minutes), + ...(testCase.secondary !== undefined + ? { + "x-codex-secondary-used-percent": String(testCase.secondary), + "x-codex-secondary-window-minutes": "10080", + } + : {}), + }); + const wham = parseUsageQuota({ + plan_type: "plus", + rate_limit: { + primary_window: { used_percent: testCase.primary, limit_window_seconds: testCase.seconds }, + ...(testCase.secondary !== undefined + ? { secondary_window: { used_percent: testCase.secondary, limit_window_seconds: 604_800 } } + : {}), + }, + }); + + expect(assignment(parseUpstreamQuotaHeaders(headers) as Record)) + .toEqual(assignment(wham as Record)); + }); + } + + it("the burst duration survives the header round trip in seconds", () => { + // The header wire speaks minutes and the stored field is seconds; a unit slip here would be + // silent, since both numbers are plausible durations. + const quota = parseUpstreamQuotaHeaders(new Headers({ + "x-codex-primary-used-percent": "50", + "x-codex-primary-window-minutes": "300", + })); + expect(quota?.shortWindowSeconds).toBe(18_000); + }); +}); + +/** + * The regression the parser fix would otherwise have introduced. + * + * `codexAccountQuotaEvidence` scored headroom from weekly and monthly only. That was survivable + * while the broken parser wrote 5h readings into `weeklyPercent` — routing saw the burst by + * accident. Correcting the parser without this fold would take a 5h-exhausted account from 3% + * headroom to 88% and route straight into a 429. + */ +describe("routing headroom accounts for the burst window", () => { + it("a 5h-exhausted account keeps low headroom despite a healthy weekly", () => { + clearAccountQuota(); + setAccountQuotaFromParsed("burst-acct", { shortPercent: 97, weeklyPercent: 12 }); + const evidence = codexPoolQuotaEvidence([{ accountId: "burst-acct", plan: "plus" }]); + expect(evidence.known).toBe(true); + expect(evidence.headroom).toBeLessThanOrEqual(0.05); + }); + + it("a fully exhausted burst window reports exhausted", () => { + clearAccountQuota(); + setAccountQuotaFromParsed("burst-dead", { shortPercent: 100, weeklyPercent: 8 }); + const evidence = codexPoolQuotaEvidence([{ accountId: "burst-dead", plan: "plus" }]); + expect(evidence.exhausted).toBe(true); + }); + + it("a healthy burst window does not suppress a real weekly limit", () => { + // The fold must not invert: the maximum still governs, so a near-full weekly still bites. + clearAccountQuota(); + setAccountQuotaFromParsed("weekly-bound", { shortPercent: 3, weeklyPercent: 96 }); + const evidence = codexPoolQuotaEvidence([{ accountId: "weekly-bound", plan: "plus" }]); + expect(evidence.headroom).toBeLessThanOrEqual(0.05); + }); +}); + diff --git a/tests/rate-limit-reset-credits.test.ts b/tests/rate-limit-reset-credits.test.ts index b775700c8d..45b9950fdf 100644 --- a/tests/rate-limit-reset-credits.test.ts +++ b/tests/rate-limit-reset-credits.test.ts @@ -481,4 +481,96 @@ describe("rate-limit reset credits", () => { }); }); }); + + /** + * Codex removed the 5-hour window and restored it for Plus and Team (Pro stays weekly-only). + * The header parser classified every non-monthly primary as weekly, so a 5h reading landed in + * weeklyPercent, the real weekly value was discarded, and the account stayed "exhausted" long + * after the burst window reset. + */ + describe("header window duration classification (5h restoration)", () => { + it("files a 5h primary as the burst window and the 7-day secondary as weekly", () => { + clearAccountQuota(); + const headers = new Headers({ + "x-codex-primary-used-percent": "97", + "x-codex-primary-window-minutes": "300", + "x-codex-primary-reset-at": "1787401330", + "x-codex-secondary-used-percent": "12", + "x-codex-secondary-window-minutes": "10080", + "x-codex-secondary-reset-at": "1788000000", + }); + applyAccountQuotaFromUpstreamHeaders("burst-A", headers); + expect(getAccountQuota("burst-A")).toEqual({ + shortPercent: 97, + shortResetAt: 1787401330, + shortWindowSeconds: 18000, + weeklyPercent: 12, + weeklyResetAt: 1788000000, + updatedAt: expect.any(Number), + }); + }); + + it("an exhausted burst window does not poison the weekly reading", () => { + // The damaging half of the bug: weeklyPercent=100 survives the 5h reset and keeps the + // account out of the pool until an unrelated WHAM refresh overwrites it. + clearAccountQuota(); + applyAccountQuotaFromUpstreamHeaders("burst-B", new Headers({ + "x-codex-primary-used-percent": "100", + "x-codex-primary-window-minutes": "300", + "x-codex-secondary-used-percent": "8", + "x-codex-secondary-window-minutes": "10080", + })); + const quota = getAccountQuota("burst-B"); + expect(quota?.shortPercent).toBe(100); + expect(quota?.weeklyPercent).toBe(8); + }); + + it("a sub-day window at 1439 minutes is short; 1440 minutes is not", () => { + // Strict `<` against the 24h discriminator, matching isExplicitShortWindow exactly. + clearAccountQuota(); + applyAccountQuotaFromUpstreamHeaders("edge-below", new Headers({ + "x-codex-primary-used-percent": "60", + "x-codex-primary-window-minutes": "1439", + })); + expect(getAccountQuota("edge-below")?.shortPercent).toBe(60); + expect(getAccountQuota("edge-below")?.weeklyPercent).toBeUndefined(); + + clearAccountQuota(); + applyAccountQuotaFromUpstreamHeaders("edge-at", new Headers({ + "x-codex-primary-used-percent": "60", + "x-codex-primary-window-minutes": "1440", + })); + expect(getAccountQuota("edge-at")?.weeklyPercent).toBe(60); + expect(getAccountQuota("edge-at")?.shortPercent).toBeUndefined(); + }); + + it("a primary with no declared duration stays weekly", () => { + // Duration classification is opt-in on a DECLARED duration. Legacy payloads omit the + // header, and guessing there would reclassify every account that predates the field. + clearAccountQuota(); + applyAccountQuotaFromUpstreamHeaders("legacy-A", new Headers({ + "x-codex-primary-used-percent": "80", + "x-codex-primary-reset-at": "1787000000", + })); + expect(getAccountQuota("legacy-A")).toEqual({ + weeklyPercent: 80, + weeklyResetAt: 1787000000, + updatedAt: expect.any(Number), + }); + }); + + it("the reset instant travels with its own window", () => { + clearAccountQuota(); + applyAccountQuotaFromUpstreamHeaders("reset-A", new Headers({ + "x-codex-primary-used-percent": "50", + "x-codex-primary-window-minutes": "300", + "x-codex-primary-reset-at": "1787401330", + })); + const quota = getAccountQuota("reset-A"); + expect(quota?.shortResetAt).toBe(1787401330); + // The primary reset belongs to the burst window; it must not be reported as the weekly one. + expect(quota?.weeklyResetAt).toBeUndefined(); + }); + }); + });