Conversation
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. |
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 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. Comment |
![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: b72673105b
ℹ️ 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".
| if (filteredFlights.size >= MAX_CONCURRENT_FILTERED_AGGREGATES) { | ||
| throw new Error("too many concurrent filtered usage aggregates"); |
There was a problem hiding this comment.
When four distinct scans are active, a filtered management /api/usage request reaches this throw, but logs-usage-routes.ts catches it and returns HTTP 200 with zero totals and error: "read_failed". Consumers such as gui/src/pages/Usage.tsx and src/cli/observe.ts accept successful responses without checking that field, so temporary saturation is displayed as genuine zero usage. Translate this overload condition to a non-success response, or otherwise ensure clients reject the synthetic summary.
Useful? React with 👍 / 👎.
| if (filteredFlights.size >= MAX_CONCURRENT_FILTERED_AGGREGATES) { | ||
| throw new Error("too many concurrent filtered usage aggregates"); |
There was a problem hiding this comment.
When four other keys are scanning, this guard also rejects a request whose own key already has a current entry in retainedFilteredAggregates, because the unchanged-cache check does not happen until refreshFilteredAggregate. That request would perform no ledger scan, yet four slow callers can deny the otherwise available cached report; enforce the limit only on the rebuild/append path that actually invokes the scanner. The changed structure contract specifically describes this as a bound on concurrent filtered scans.
AGENTS.md reference: structure/AGENTS.md:L9-L10
Useful? React with 👍 / 👎.
|
✅ Deterministic PR hygiene checks passed. |
github-actions
Bot
changed the title
⏳ DRAFT
What to do
Its title has been prefixed with |
1 participant
Motivation
GET /v1/usagepath could be abused by an authenticated client key to spawn many distinct whole-ledger scans, amplifying file-descriptor, memory, and CPU use and enabling an authenticated DoS.Description
MAX_CONCURRENT_FILTERED_AGGREGATES = 4and reject new distinct filtered aggregate flights when that cap is reached insrc/server/management/usage-aggregate-cache.tsby throwing an error before starting a refresh.Promiseand reuse its result.tests/usage/usage-aggregate-cache.test.tsthat asserts four distinct blocked scans run concurrently, the fifth distinct scan is rejected, and a duplicate caller joins the existing flight.structure/gui-and-management-api.mdto document the filtered-scan concurrency invariant so the architecture notes reflect the runtime bound.Testing
node_modules/.bin/bun test tests/usage/usage-aggregate-cache.test.ts tests/server/hub-usage.test.ts, and the changed-subsystem tests passed (focused run passed).node_modules/.bin/bun run typecheck,node_modules/.bin/bun run structure:check, andnode_modules/.bin/bun run privacy:scan, which succeeded after a small docs formatting adjustment to satisfy the structure check.node_modules/.bin/bun run test; the focused changes are covered and pass, while the complete parallel suite contains unrelated failures in the environment and serial fixtures (these failures are out of scope for this focused change).Codex Task