Skip to content

perf: avoid decoding usage events for Codex activity - #3528

Merged
steipete merged 2 commits into
steipete:mainfrom
brzvsk:perf/codex-activity-aggregate-read
Sep 10, 2026
Merged

perf: avoid decoding usage events for Codex activity#3528
steipete merged 2 commits into
steipete:mainfrom
brzvsk:perf/codex-activity-aggregate-read

Conversation

@brzvsk

@brzvsk brzvsk commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Cached Codex token activity now reads scoped daily aggregates instead of loading and decoding every usage-event payload. Account/root and time-zone validation, coverage, pending scans, retained retries, identity reconciliation, and detailed monetary/project/session reports keep their existing behavior. The nearby cached-read methods also return typed optional executor results directly, removing redundant double-optional wrapping.

This addresses one verified refresh-time CPU and memory hotspot related to #3247. It does not claim to resolve that report’s separate WebKit or whole-app energy symptoms. No parser hash, schema, pricing, refresh cadence, or credential-ownership change is introduced.

Maintainer proof:

  • On unchanged main, the new real-fetcher regression recorded 1,024 event reads, 190,528 payload bytes, and 1,024 decodes. The retained-retry case also read eight unused events. All four assertions failed before the fix.
  • After the fix, 70 focused tests passed: zero event payload reads/decodes, identical totals and coverage, foreign-account exclusion, time-zone and root rejection, and incomplete/retry handling.
  • make check passed with zero violations; the Codex parser hash remains 4969a789db679c93.
  • make test: all 1,057 selections across 89 groups passed on the first attempt, with zero retries or timeouts (1,008.5 seconds). Exact-head CI remains the final pre-merge gate.
  • Independent P0–P2 review found no actionable issues, including the optional-return cleanup and ownership boundaries.

The contributor’s release-core benchmark on independent private-cache copies measured median activity-call CPU of 1.385110 seconds before and 0.234250 seconds after, with complete output parity and unchanged stored-record fingerprints. Those are contributor measurements of this read path, not a prediction of whole-app battery savings.

Includes documentation and an Unreleased performance entry. Thanks @brzvsk!

@clawsweeper

clawsweeper Bot commented Sep 9, 2026

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 9, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-09T19:25:06.737860Z 8729fc1 PR opened
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Sep 9, 2026
@clawsweeper

clawsweeper Bot commented Sep 9, 2026

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed September 10, 2026, 12:37 AM ET / 04:37 UTC (Revision 4).

ClawSweeper review

What this changes

The PR reads cached Codex activity from daily totals without decoding individual events, simplifies optional returns, and adds regression coverage and documentation.

Merge readiness

Ready for maintainer review

This remains a useful optimization absent from current main and v0.58.0; source review and reported benchmark results support landing it, with no actionable correctness findings.

Priority: P2
Reviewed head: 4b9ee5e081a98e8345e3c41e7adc20869db27149

Review scores

Measure Result What it means
Overall readiness 🦞 diamond lobster (5/6) A small optimization supported by real-cache measurements, behavior-parity coverage, and reported full validation.
Proof confidence 🦞 diamond lobster (5/6) Sufficient (live_output): The supplied release-core benchmark exercises the changed cached-activity reader against independent real private-cache copies and reports lower CPU with identical output and unchanged stored records; focused fetcher tests supplement that evidence.
Patch quality 🦞 diamond lobster (5/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Verified Sufficient (live_output): The supplied release-core benchmark exercises the changed cached-activity reader against independent real private-cache copies and reports lower CPU with identical output and unchanged stored records; focused fetcher tests supplement that evidence.
Evidence reviewed 8 items Repository policy: Read the complete root AGENTS.md; no nested AGENTS.md or matching maintainer-note files were found. Applied provider isolation, focused coverage, and safe validation guidance. No builds or tests were executed during this read-only review.
Current main and release still perform the unnecessary read: Inspected the activity loader at fetched main 33161af and v0.58.0: both request the report projection, which loads individual usage events. The merged predecessor #3250 optimized status and report reads but does not supersede this activity-specific change.
Read projection preserves the required inputs: The activity projection omits usageRows while retaining file/day aggregates, metadata, lineage, and retry presence. Conversion still runs identity reconciliation after the read transaction; detailed reports retain their existing event reads.
Findings None None.
Security None None.

How this fits together

CodexBar’s Usage & Spend dashboard reads locally cached Codex session activity from SQLite. The changed read path supplies daily token totals while detailed spending reports continue loading individual pricing events.

flowchart TD
  A[Usage and Spend dashboard] --> B[Account and date request]
  B --> C[Cached activity reader]
  D[SQLite session cache] --> C
  C --> E[Read daily aggregates]
  E --> F[Check scope and scan coverage]
  F --> G[Daily token totals or unavailable]
Loading

Before merge

None.

Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Reported activity-call CPU 1.385110 s → 0.234250 s median; approximately 83% lower The real-cache benchmark measures the changed read path and reports output parity, without claiming whole-app energy savings.
Production and test delta Production +7/−7 lines; tests +84/−5 lines The optimization adds no net production lines and includes focused coverage for preserved behavior.

Technical review

Best possible solution:

Keep activity reads aggregate-only while preserving scoped coverage checks and full event detail for spending reports.

Do we have a high-confidence way to reproduce the issue?

Yes, from source: current main requests event-bearing reports for an activity calculation that consumes daily aggregates only. Supplied before-fix counters corroborate the overhead; this review did not execute tests.

Is this the best way to solve the issue?

Yes. Extending the existing read-purpose projection avoids a parallel cache implementation and leaves report pricing, persistence, and account filtering intact.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against 33161afe7f2c.

Labels

Label justifications:

  • P2: This is a bounded, measured reduction in cached activity work, without evidence that it resolves the broader urgent resource-exhaustion report.
  • rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The supplied release-core benchmark exercises the changed cached-activity reader against independent real private-cache copies and reports lower CPU with identical output and unchanged stored records; focused fetcher tests supplement that evidence.
  • proof: sufficient: Contributor real behavior proof is sufficient. The supplied release-core benchmark exercises the changed cached-activity reader against independent real private-cache copies and reports lower CPU with identical output and unchanged stored records; focused fetcher tests supplement that evidence.

Evidence

What I checked:

  • Repository policy: Read the complete root AGENTS.md; no nested AGENTS.md or matching maintainer-note files were found. Applied provider isolation, focused coverage, and safe validation guidance. No builds or tests were executed during this read-only review. (AGENTS.md:1, 4b9ee5e081a9)
  • Current main and release still perform the unnecessary read: Inspected the activity loader at fetched main 33161af and v0.58.0: both request the report projection, which loads individual usage events. The merged predecessor perf: narrow cached Codex cost reads #3250 optimized status and report reads but does not supersede this activity-specific change. (Sources/CodexBarCore/CostUsageFetcher.swift:832, 33161afe7f2c)
  • Read projection preserves the required inputs: The activity projection omits usageRows while retaining file/day aggregates, metadata, lineage, and retry presence. Conversion still runs identity reconciliation after the read transaction; detailed reports retain their existing event reads. (Sources/CodexBarCore/Vendored/CostUsage/CostUsageStore+CodexCache.swift:70, 4b9ee5e081a9)
  • Account filtering and coverage remain intact: The dashboard supplies its selected account home to the activity loader. Scope filtering rebuilds daily totals solely from files within the selected roots; the loader retains time-zone, root-fingerprint, pending-scan, and date-coverage checks. The patch changes neither persisted schema nor authorization. (Sources/CodexBarCore/Vendored/CostUsage/CostUsageScanner+Projects.swift:4, 4b9ee5e081a9)
  • Focused regression coverage: Four new tests exercise the real activity fetcher for zero event decoding, identical totals and coverage, foreign-file exclusion, scope/time-zone rejection, retained retries, and incomplete scans. Shared projection-parity coverage now includes activity, including identity reconciliation cases; recorder tests belong to a serialized suite. (Tests/CodexBarTests/CostUsageStoreReadWorkTests+Activity.swift:7, 4b9ee5e081a9)
  • Real-cache performance evidence: The complete supplied PR body, captured under sourceRevision affc1ff23e937307d3e4a9a8610b656e2acb684de0fb4cbe8cadf8a684fff979, reports release-core activity calls on independent private-cache copies: median CPU 1.385110 seconds before and 0.234250 seconds after, identical output, and unchanged stored-record fingerprints. It separately reports 70 focused tests, make check, and all 1,057 full-suite selections passing. These are supplied execution results, not reviewer-run measurements.

Likely related people:

  • Peter Steinberger: Raw commit 27d8b5a adds Sources/CodexBarCore/Vendored/CostUsage/CostUsageStore+ReadView.swift:47 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: 27d8b5a75a5a; files: Sources/CodexBarCore/Vendored/CostUsage/CostUsageStore+ReadView.swift)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (3 earlier review cycles)
  • reviewed 2026-09-09T19:24:01.583Z sha 8729fc1 :: needs maintainer review before merge. :: none
  • reviewed 2026-09-09T19:30:52.605Z sha 8729fc1 :: needs maintainer review before merge. :: none
  • reviewed 2026-09-09T20:03:30.922Z sha 8729fc1 :: needs maintainer review before merge. :: none

steipete and others added 2 commits September 9, 2026 20:58
Read scoped daily aggregates while preserving coverage, retries, and account boundaries. Return typed optional cached results directly and document the lighter activity projection.

Co-authored-by: Nikolai Berezovskii <nikolai@brzv.sk>
@steipete
steipete force-pushed the perf/codex-activity-aggregate-read branch from 8729fc1 to 4b9ee5e Compare September 10, 2026 04:33
@steipete
steipete merged commit 5c0d7b4 into steipete:main Sep 10, 2026
9 checks passed
@steipete

Copy link
Copy Markdown
Owner

Landed as 5c0d7b4. Thanks @brzvsk for the focused projection fix and release-core measurements.

The activity reader now loads scoped daily aggregates without materializing usage-event payloads. Account/root, time-zone, coverage, incomplete-scan, and retry checks remain in place, and detailed reports retain their row-level history. The nearby optional-return cleanup preserves nil/error behavior.

Verification:

  • Before the fix, the real-fetcher regression failed on 1,024 event reads, 190,528 payload bytes, 1,024 decodes, and eight unnecessary reads in the retained-retry case. Afterward those work counts are zero while totals and coverage match.
  • swift test --filter 'CostUsageStoreReadWorkTests|CostUsageFetcherCacheSnapshotTests|CostUsageStoreReadViewTests': 70 tests passed across two suites.
  • make test: all 1,057 selections across 89 groups passed on the first attempt; zero retries/timeouts, 1,008.5 seconds total.
  • make check: passed, zero violations across 2,169 files.
  • Scripts/regenerate-codex-parser-hash.sh --check: unchanged 4969a789db679c93.
  • Independent P0–P2 review: no actionable findings.
  • Final-head CI: macOS shards, Linux x64/arm64/musl, lint, aggregate gate, and security checks passed on 4b9ee5e081a98e8345e3c41e7adc20869db27149.

Swift validation used Keychain suppression and Codex/session-file isolation through the test wrapper. Native UI/provider probing was unnecessary for this cache-read-only change. The contributor’s benchmark remains their measurement of this one read path; #3247 stays open for its broader CPU and WebKit symptoms. Documentation and the 0.58.1 Unreleased notes include the change and credit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants