Repository navigation
fix(quota-gatherer): bound provider concurrency + retain last-known snapshot on error (#1268) - #3996
Conversation
…napshot on error (#1268)
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Warning Review limit reachedYou'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 38 minutes. View limit details
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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2676120ed1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| run.entries = retained; | ||
| run.retained = true; |
There was a problem hiding this comment.
When a provider fails on the normal 15-minute tick, its retained entries are typically only 15 minutes old and still have state: "ok". quota-current.ts considers those entries fresh for 20 minutes, and both overlayQuotaCurrent() and summarize() ignore the heartbeat's per-provider error. Consequently, the failed fetch now appears healthy/current for roughly five minutes instead of surfacing the failure immediately; preserving measuredAt alone does not make retained data stale. Carry an explicit retention/failure signal through to the reader so it can display last-known data without reporting the failed provider as current.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in the follow-up commit on this PR — thank you, the 15-min tick vs 20-min freshness bound point is correct and preservation of measuredAt alone was not enough.
What changed:
QuotaCurrentEntrygains an explicitretainedFromFailedTick?: stringmarker (model.ts), stamped by the gatherer on retained copies via the newmarkRetainedhelper (quota-gather-map.ts) — originalmeasuredAtpreserved, previous-tick objects untouched.entryState()in quota-current.ts — the single classification point behind bothoverlayQuotaCurrent()andsummarize()— now classifies any entry carrying the marker asstaleimmediately, regardless of the freshness bound. The pool keeps showing the last-known windows under the stale state instead of reading healthy/current.- Tests: quota-current.test.ts gains "a retained entry is stale immediately, even inside the freshness bound (perf(quota-plane): parallel provider fetching + per-provider timeout budget for the 15-min tick #1268)" (entry 5 min old + marker → pool stale, dated windows kept); quota-gather-map.test.ts gains the
markRetainedcopy/stamp test. Quota test files: 96/96 pass on the laptop.
The heartbeat per-provider error also remains as before; the reader-facing signal is now the entry marker + stale classification.
…classify stale immediately (#1268)
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Follow-up to issue #1268 from triage task 2527 (queue task 4978). The CLI half of #1268 already landed in OpenChamber; these are the two remaining gatherer gaps in
pm-stack/aggregator/scripts/gather-quota.ts:1. Bounded provider concurrency — the tick no longer fires one unbounded
Promise.allover the whole provider catalogue (~30 simultaneous dashboard fetches). Providers now run throughmapWithConcurrency(new, insrc/quota-gather-map.tswhere it is unit-testable): default 6 in flight — matching the CLI — configurable viaQUOTA_GATHER_CONCURRENCY. A non-positive/invalid limit degrades to sequential, never back to unbounded; results keep catalogue order.2. Last-known retention on timeout/error — an errored provider run with no ok row of its own no longer writes fresh error entries over (or drops) its last good data. The gatherer reads the previous tick's
quota-current.jsonand retains that provider'sokentries verbatim (lastKnownEntries, also inquota-gather-map.ts). Retained entries keep their originalmeasuredAt, so the hub's existing freshness classification surfaces them as stale — visible staleness, not fresh-looking data. The heartbeat still reports the run aserrorwith its reason, now with the retained row count, and the tick log marks the runretained-last-known. Partial failures (some credential rows ok) keep the fresh entries untouched, as before.Tests —
test/quota-gather-map.test.tsgains 6 tests: concurrency bound enforced (max in-flight == limit) with order preserved, over-large limit, empty input, invalid limit degrading to sequential, rejection propagation; retention returns ok entries verbatim and never error/unconfigured or unknown-provider entries. Laptop results: quota test files 94/94 pass; full aggregator suite 926 pass / 1 fail — the slice3 registry failure is identical on clean main in this environment (pre-existing, unrelated);tsc --noEmitshows the same 12 pre-existing errors as main, none in the changed files;bun buildof the gatherer script succeeds.Scope note: #1268 itself stays open — this packet covers the gatherer follow-up only (the parent issue also tracks the broader perf item); the issue gets the completion note, not a close.
🤖 Queue worker (repo-worker-code), lane A — task 4978
Need help on this PR? Tag
@codesmith-botwith what you need.