Skip to content

fix(providers): keep the quota report cache when pool readings are only restamped - #7992

Merged
lidge-jun merged 4 commits into
devfrom
fix/7956-quota-report-cache-signature
Oct 30, 2026
Merged

lidge-jun merged 4 commits into
devfrom
fix/7956-quota-report-cache-signature

Conversation

@lidge-jun

@lidge-jun lidge-jun commented Oct 30, 2026 •

Copy link
Copy Markdown
Owner

Summary

The provider quota report cache never committed while Codex traffic was flowing through a pool-mode openai provider. As a result, every ocx provider quota / GET /api/provider-quotas read re-probed every provider, including Anthropic's rate-limited /api/oauth/usage. A single transient probe failure also dropped that provider's row from the response. Closes #7956 (closed manually after merge, since this targets dev). Thanks to @Caesar7812 for the precise root-cause report.

Cause: the pool aggregation part of the cache key (quotaSignatureValue in src/providers/quota/report-cache.ts) hashed each account's updatedAt. Every proxied response re-observes the account and advances that timestamp without changing any value. fetchProviderQuotaReports commits only when the key it computed before probing still matches after probing, and it reuses last-good rows only from an exact key match. Neither held while traffic flowed.

Fix: aggregation reads updatedAt only to classify a reading as fresh or stale (CODEX_CAPACITY_MAX_QUOTA_AGE_MS). The signature now carries that fresh/stale verdict instead of the raw timestamp. Any change in percent, reset, plan, pause, reauth or active account still changes the key, and so does a fresh/stale transition. Timestamp-only restamps no longer do, so the cache and the bounded last-good fallback behave as CACHE_TTL_MS and LAST_GOOD_MAX_AGE_MS intend. A displayed per-account updatedAt can lag by at most the existing 5-minute TTL. One clock stays in the key: for an exhausted short window with no reset, shortObservedAt alone decides whether the window still blocks (a 5-minute freshness rule), so it must not lag.

Before: re-observing an unchanged pool account and then reading quotas probes Anthropic again, and if that probe fails, the anthropic row disappears. After: the read is served from cache, and a failed probe keeps the last-good anthropic row.

Verification

  • Two regression tests in tests/providers/provider-quota.test.ts: restamped pool observations keep the report cache and last-good rows (#7956) (cache reuse plus last-good retention on a 503) and a restamp during an outstanding probe still lets that probe commit (#7956). Restamps use a controlled clock (setSystemTime), and afterEach restores it. With report-cache.ts restored to current dev, all three Provider quota report cache never commits while Codex traffic flows, so every quota read re-probes Anthropic and drops its row on a single failed read #7956 cases fail; with the fix, all three pass.
  • bun test tests/providers/provider-quota.test.ts: 170 pass, 0 fail.
  • bun test tests/providers/provider-quota-observed-marker.test.ts: 3 pass. bun test tests/providers/provider-account-quota.test.ts: 68 pass. Run together with provider-capacity, state-store-sweeper and gui-codex-usage-score-parity in one process, the same 4 cases fail with or without this change, a pre-existing cross-file isolation issue that this PR does not touch.
  • only an exhausted reset-less short window keeps its observation clock in the key (#7956) (same file): red with the source change stashed, green with it. provider-quota.test.ts plus provider-quota-observed-marker.test.ts at the final head: 174 pass, 0 fail.
  • bun run typecheck: passes. bun run structure:check: passes (report-cache.ts has no owning structure doc). tests/providers/provider-quota.test.ts stays under its file-size cap (3714 of 3763).
  • Fresh independent terra reviews: NEAR-PASS on b994b45 (one P2, sleep-based restamps, fixed with a controlled clock in 16c87b8), then PASS on the final head 16c87b8 with no findings. On a local merge with dev 03d8bd0, provider-quota, observed-marker and file-size-ratchet give 183 pass, 0 fail; typecheck and structure:check pass.
  • NOT RUN locally: bun run test:changed and the full bun run test. The build host is shared by three maintenance lanes and is at critical load, so per the AGENTS.md resource exception, breadth is left to PR CI.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed. (No user-facing contract change; the in-code comments in quota.ts and report-cache.ts were updated.)
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults. (No credential, auth or logging change. Account ids and emails still never enter the key.)

…ly restamped

While Codex traffic flows, every proxied response re-observes the pool
accounts' quota and advances updatedAt without changing any value. The pool
aggregation signature hashed that timestamp, so the 5-minute provider quota
report cache never committed: every quota read re-probed every provider,
including Anthropic's rate-limited usage endpoint, and a single transient
probe failure dropped that provider's row because no last-good row matched
the new key.

Aggregation reads updatedAt only to classify a reading as fresh or stale, so
the signature now carries that verdict instead of the raw timestamp. Value
changes and fresh/stale transitions still change the key.

Refs #7956
@lidge-jun
lidge-jun requested a review from Ingwannu as a code owner October 30, 2026 11:46
@coderabbitai

coderabbitai Bot commented Oct 30, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 12 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

Learn how review limits work.

Review configuration:

Run configuration
Commits

Reviewing files that changed from the base of the PR and between 6218a9a and 16c87b8.

Files selected for processing: 3
  • src/providers/quota.ts
  • src/providers/quota/report-cache.ts
  • tests/providers/provider-quota.test.ts
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added the bug Something isn't working label Oct 30, 2026
Review on #7992: prove the reported race itself is fixed (updatedAt advancing
between the pre-probe key and the commit key), not only invalidation of an
already committed cache. Also correct the signature comment: displayed
window timestamps can lag a restamp by up to the cache TTL.
@lidge-jun

Copy link
Copy Markdown
Owner Author

Fresh independent review of 4948d79: PASS-WITH-FIXES. Responses, fixed in e1337bc except where noted:

  • SHOULD-FIX, test the outstanding-probe race: added a restamp during an outstanding probe still lets that probe commit (#7956). It holds the Anthropic probe open, restamps the pool account, releases the probe, and asserts that the result committed: the next ordinary read makes no new Anthropic call and keeps both rows. With the two source files restored to the base 3a70dd4, both Provider quota report cache never commits while Codex traffic flows, so every quota read re-probes Anthropic and drops its row on a single failed read #7956 tests fail (Anthropic probed twice, expected once). With the fix, both pass.
  • NIT, comment accuracy: the signature comment now says that displayed window timestamps (updatedAt, shortObservedAt) can lag a restamp by up to the report cache TTL.
  • SHOULD-FIX, freshness boundary race: not changed, with reasons. The pre-probe key and the pool aggregation each evaluate freshness immediately after awaiting the same prefetched account snapshot, milliseconds apart. A stale aggregation can be committed under a fresh key only if an account crosses its 30-minute freshness boundary within that gap and is restamped before the commit key is computed. An account sitting at the 30-minute idle boundary is by definition not receiving the traffic that restamps it. If it happens, one account shows as stale for at most the 5-minute TTL; routing does not read this cache. Binding a single evaluation clock through every provider probe would touch the shared probe interface for a cosmetic, TTL-bounded case. If a maintainer prefers that, it can be a follow-up.

No security boundary is involved: no credential, auth or logging change, and no account id or email enters the key.

@github-actions

github-actions Bot commented Oct 30, 2026 •

Copy link
Copy Markdown
Contributor

✅ Deterministic PR hygiene checks passed.

… short window in the key

Its shortObservedAt alone decides whether that window still blocks, over a five-minute freshness window, so a restamp must still miss the report cache for it. Other observation clocks stay out of the key (#7956).
Bun.sleep(5) only usually produced a different Date.now(); setSystemTime makes each restamp distinct, and afterEach restores the clock.
@lidge-jun

Copy link
Copy Markdown
Owner Author

Maintainer integration into dev under the MAINTAINERS.md dev exception (lane L4b, maintainer lidge-jun; L4b took this PR over from the paused earlier L4 lane on coordinator instruction).

  • Exact head: 16c87b85000dcc3bd6c9530a76b5fe751a787949.
  • Exact-head CI: Cross-platform CI run 41051922491 completed successfully: gates, test changed, api usage, docker smoke, npm-global ubuntu-latest and storage policy. Catalog async contracts, hygiene (12:26Z, after earlier concurrency cancellations), resolve-pr and the latest enforce-target (12:27Z) also succeeded. structure gate, the full shards and macOS/Windows jobs were skipped by path and policy.
  • Review: the final fresh independent terra review is PASS with no findings. The earlier P2 about sleep-based restamps was fixed with a controlled clock.
  • Union check against current dev 03d8bd0ce: git merge-tree is clean. On a local merge, provider-quota, provider-quota-observed-marker and file-size-ratchet give 183 pass, 0 fail, and typecheck and structure:check pass. provider-quota.test.ts is at 3715 lines, under its 3763 cap.
  • No security-boundary surface is touched; account ids and emails still never enter the cache key.

@lidge-jun
lidge-jun merged commit 8063be2 into dev Oct 30, 2026
31 of 33 checks passed
@lidge-jun
lidge-jun deleted the fix/7956-quota-report-cache-signature branch October 30, 2026 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant