fix: preserve fork accuracy during bounded Codex cost catch-up - #2525
Conversation
|
Codex review: needs maintainer review before merge. Reviewed August 2, 2026, 6:40 PM ET / 22:40 UTC. ClawSweeper reviewWhat this changesThis PR makes bounded Codex cost scans persist and resume fork-parent discovery, preserves stale reports during cache rebuilds, and adds background or accelerated catch-up controls to the Spend Dashboard. Merge readinessKeep this PR open for maintainer review. Its current head contains the maintainer-authored missing-parent discovery follow-up, but that head is not on current Priority: P2 Review scores
Verification
How this fits togetherCodexBar scans local Codex session JSONL files into a persisted cost cache that feeds token and spending reports. The changed path decides which session files are scanned, resolves fork-parent baselines, persists partial progress, and publishes progress to the native Spend Dashboard. flowchart LR
A[Local Codex session JSONL] --> B[Bounded cost scanner]
B --> C[Persistent scan and fork cache]
C --> D[Fork-parent discovery]
C --> E[Catch-up scheduler]
D --> F[Cost and token reports]
E --> G[Spend Dashboard progress]
F --> G
Decision needed
Why: The contributor proof and owner follow-up are substantial, but the branch changes persisted state, accounting visibility during unresolved forks, background scheduling, and native UI controls. The remaining decision is release-risk acceptance, not a narrow mechanical repair. Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Merge the reviewed head only after a maintainer confirms that a cache created by the latest shipped version resumes through the new parent-discovery and catch-up paths, reaches stable final totals, and does not regress the bounded CPU behavior on a many-file corpus. Do we have a high-confidence way to reproduce the issue? Yes, source and the supplied real-corpus validation establish the bounded parent-scan path: current main has byte-budgeted scanning, while this branch adds resumable parent discovery and regression coverage for the unresolved-parent behavior. This read-only review did not execute the corpus or tests. Is this the best way to solve the issue? Yes, the branch’s persistent scanner path is the maintainable direction because it avoids a separate full-parent resolver read and keeps parent work subject to the same bounded cache path. The final merge decision still needs upgrade-safety review because the implementation changes durable cache and visibility semantics. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 5b0b9fa3e37c. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (8 earlier review cycles)
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab34bd0cca
ℹ️ 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".
| ScrollView { | ||
| VStack(alignment: .leading, spacing: 18) { | ||
| self.header | ||
| self.codexCostCatchUpPanel |
There was a problem hiding this comment.
Bind catch-up controls to the dashboard account cache
When an account-scoped dashboard scan remains pending—for example, a Codex archive exceeding the 512 MiB per-refresh limit—this panel reports and controls UsageStore.codexCostCatchUpActivity, but dashboard rows are loaded from separate caches under accounts/<cacheIdentity> in SpendDashboardSource.load (SpendDashboardController.swift:273-283). The buttons therefore advance the store's default selected-account cache rather than the cache supplying the displayed row, and completing that task does not invalidate Codex dashboard data because Codex is excluded from sourceRevisions; users can click “Finish now” yet keep seeing the stale/partial dashboard result. Track catch-up per dashboard account cache or avoid presenting these controls as dashboard progress.
Useful? React with 👍 / 👎.
|
Completed the #2538 CPU-spin catch-up on top of @xx205's PR in The cost cache now owns a persisted, resumable session-discovery index. Missing parent IDs are stored with an inventory generation derived from the relevant roots/partitions, so a cached unresolved child reuses the same negative dependency key and remains idle until that inventory changes. Root traversal and session-head reads reserve and consume the existing I also included the bounded mechanism-1 follow-up from #2538/#2558: newest-first sorting snapshots each file's metadata once, eliminating Regression proof:
The 0.46.1 changelog credits @xx205 and the #2538 investigation by @Helmi and @kiranmagic7. This now covers both identified refresh-amplification mechanisms without merging the PR. |
|
Refreshed PR #2525 onto current
No merge of the PR was performed. |
After rebasing onto main (including steipete#2525 catch-up), only prime the dashboard from caches without staleSnapshotUpdatedAt so previous-report and incompatible-producer placeholders stay hidden until live validation. Also keep live-system ambient cache-root routing in the single public codexCacheRoot helper.
Summary
Problem
#2452 correctly replaced a permanent giant-file skip with resumable bounded parsing, but fork baseline resolution still has a separate whole-parent admission path.
When a child references a parent larger than the 256 MiB per-file slice, the resolver cannot admit the full parent and returns an unresolved baseline. The unresolved fallback skips the first cumulative total and only counts later positive deltas, which can permanently omit child usage already present in that first total. Re-reading a smaller parent directly inside the resolver also bypasses the ordinary persistent catch-up path and can extend a foreground refresh.
Implementation
Accurate, resumable fork baselines
CodexScanBudgetand cache path as ordinary rollout files.Bounded scanner work
Adaptive catch-up and UI
Real-corpus validation
A privacy-safe validation selected a stable child/parent fork pair from a real local corpus. The parent was 488,985,173 bytes, so it could not fit in one 256 MiB slice. The copied files used anonymous names and the report emitted no paths, session IDs, dates, model names, token totals, prompts, responses, or session content.
CostUsageDailyReport.dataCostUsageDailyReport.summaryThe bounded passes ran back-to-back without automatic scheduler sleeps. The 2-second deadline covers JSONL reading/parsing; cache load/save and aggregation remain outside that parser deadline.
Upgrade-cache convergence validation
A separate privacy-safe synthetic fixture seeded a populated cache under an incompatible parser producer, then rebuilt an oversized parent/child fork corpus with 1 KiB per-file and per-refresh slices.
Verification
make testmake checkFinish nowandCancel;Continue in backgroundandCancel.Screenshots
Privacy-safe Spend Dashboard screenshots for automatic/background and accelerated indexing were captured from the freshly built app. They contain no local paths, account identity, session IDs, prompts, responses, or session content.
Automatic/background catch-up:
Accelerated catch-up:
Scope and limitations