fix: bound Codex cost scans on giant session corpora - #2452
Conversation
Local token-cost history walks Codex rollout JSONL under ~/.codex/sessions. On machines with multi-GB rollouts this can peg a core for hours on the cost-usage-scan queue while quitting/relaunching only restarts the same backlog. Rate-limit/account usage probes are a separate path and are unaffected. - Cap per-file cold/full scan work (default 256 MiB) - Cap newly-read bytes per refresh (default 512 MiB) and defer the rest - Prefer newest session files first so recent usage lands before catch-up - Add performance gates for oversized skip, budget deferral, and fork work sizing
![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: e2ffbf5807
ℹ️ 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 cached.mtimeUnixMs == metadata.mtimeUnixMs, cached.size == metadata.size { | ||
| return 0 |
There was a problem hiding this comment.
When an unchanged cached file must nevertheless be rescanned—such as after a history-window expansion, pricing/project-metadata change, or a priority-turn invalidation—keepCachedCodexFileIfFresh rejects it, but this equality branch reports zero pending bytes. The budget therefore admits even a multi-GB file and records zero bytes after rescanCodexFile reads it completely, bypassing both new limits and recreating the CPU-thrashing behavior this change is intended to prevent.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed — valid hole. Fixed in 49d0966.
pendingCodexScanWorkBytes is only used after keepCachedCodexFileIfFresh fails. The old size/mtime-equality branch could still report 0 for forced full rescans (window/pricing/project-metadata/priority invalidation), so the budget admitted multi-GB files and then charged nothing after rescanCodexFile.
That zero branch is gone. Unchanged-but-must-rescan files now charge full file size; only true grow+non-fork incremental tails charge the delta.
| let pendingWorkBytes = Self.pendingCodexScanWorkBytes(metadata: metadata, cached: cached) | ||
| if let budget = context.scanBudget { | ||
| switch budget.admit(workBytes: pendingWorkBytes) { |
There was a problem hiding this comment.
When an admitted small fork/subagent rollout references an oversized parent, its rescan calls CodexInheritedTotalsResolver, whose snapshotResolution reads the entire parent through parseCodexTokenSnapshots. This outer-file admission does not charge or limit that dependency read, so a parent that was skipped by its own top-level scan can still be parsed in full while processing the child, allowing the same multi-GB CPU-bound scan on a cold corpus.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed — also valid for fork/subagent corpora. Fixed in 49d0966.
CodexInheritedTotalsResolver now shares the same CodexScanBudget. Before parseCodexTokenSnapshots on a parent, we admit the parent file size; oversized/deferred parents return an unresolved baseline (existing fork path) instead of a full multi-GB dependency parse. Successful parent reads consume budget so they count toward the per-refresh cap.
Added a performance gate covering small-child + oversized-parent.
Address Codex review P1s on steipete#2452: - pendingCodexScanWorkBytes no longer reports zero for unchanged size/mtime entries after keepCached fails. Forced full rescans (window/pricing/priority invalidation) now charge the full file. - CodexInheritedTotalsResolver shares the scan budget and skips or defers oversized parent baseline snapshot reads instead of parsing multi-GB parents while processing small fork/subagent children. - Add gates for forced-rescan work sizing and oversized-parent skip.
Follow-up: addressed Codex P1 review commentsPushed P1 — forced full rescans not charged
P1 — fork parent snapshot reads unbounded
Tests added
Local verification
Thanks for the sharp catches — both were real bypasses of the original protection. |
clawsweeper
Bot
added
rating: 🧂 unranked krab
|
Codex review: needs maintainer review before merge. Reviewed July 26, 2026, 12:22 PM ET / 16:22 UTC. ClawSweeper reviewWhat this changesThe PR bounds local Codex JSONL token-cost scans with a 256 MiB per-file limit, a 512 MiB per-refresh budget, newest-first ordering, and focused performance coverage for deferred and fork-parent scans. Merge readinessKeep this PR open for maintainer review. The bounded scanner and its real-corpus proof credibly address the CPU-saturation failure, but the default 256 MiB hard skip can permanently omit newly created large rollout files from local token-cost history; that intentional compatibility tradeoff needs an explicit product decision before merge. Priority: P1 Review scores
Verification
How this fits togetherCodexBar’s local cost-usage subsystem scans Codex rollout JSONL files and stores daily token-cost totals for the menu bar. It uses cached file results and fork-parent resolution while account rate-limit polling follows a separate provider-RPC path. flowchart LR
A[Codex rollout JSONL files] --> B[Cost scan refresh]
B --> C[Cache freshness and fork lookup]
C --> D[Per-file and refresh budgets]
D --> E[JSONL parsing and daily totals]
E --> F[Cost-usage cache]
F --> G[Menu bar cost history]
H[Account rate-limit RPCs] --> I[Rate-limit display]
Decision needed
Why: The implementation makes a deliberate user-visible accuracy tradeoff rather than a purely mechanical bug fix. Repository evidence proves the availability benefit, but cannot determine whether incomplete cost history is acceptable for existing users after upgrade. Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Preserve bounded background work while guaranteeing eventual accounting for oversized rollouts through resumable/chunked parsing with persisted progress, or explicitly adopt permanent omission with a visible incomplete-history state and documented recovery path. Do we have a high-confidence way to reproduce the issue? No independent live reproduction was run in this read-only environment, but the source path and the contributor’s redacted current-head large-corpus run provide a high-confidence source-reproducible path: enable local token-cost history and scan a corpus containing multi-hundred-MiB rollout JSONL files. Is this the best way to solve the issue? Unclear. Per-refresh budgeting and newest-first ordering are a good solution for the CPU problem, but permanently skipping completed files above 256 MiB is not the best default unless maintainers explicitly accept incomplete local cost history and make that state discoverable. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against cc8da27cec92. 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
History |
Real behavior proof added (ClawSweeper request)Updated the PR body with redacted after-fix live runtime evidence on head What was runPublic Corpus (aggregate / redacted)
After-fix cold catch-up (forceRefresh, historyDays=30, empty temp cache)CodexBarCore work-limit log (path-free): Second refresh (same cache)Synthetic oversized siblingIntentional tradeoff (explicit for merge review)Local token-cost history can lag for deferred/oversized multi-hundred-MB and multi-GB rollouts. That is the availability tradeoff. Account/rate-limit remaining + reset remain on a separate path and are unaffected by these scan budgets. Not runnable on author hostFull @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
clawsweeper
Bot
added
proof: sufficient
2 participants
Summary
Resumable parsing
The cache now records the rollout file identity, target size, byte offset, reducer state, and any partial JSONL record needed to continue safely. Each refresh consumes at most the configured per-file slice and remaining global byte budget. A changed identity or size invalidates incompatible progress instead of mixing histories.
Ordinary rollouts publish accumulated complete records as they advance. Subagent rollouts retain compact relevant events until the final chunk establishes their copied-prefix ownership boundary, then publish the same result as an unbounded parse. Completed files follow the existing warm-cache path.
Contributor evidence
@D4ilyHub diagnosed the original scan storm and validated the bounded/newest-first machinery against a real 276 GiB corpus containing 1,322 rollouts. Before the change, the scanner could pin a core for hours and immediately resume after relaunch. The contributor's cold proof completed in about 16 seconds wall time and 6 seconds CPU under a 512 MiB refresh budget, while account and rate-limit display continued independently.
The maintainer follow-up replaces the initial permanent 256 MiB file skip with resumable bounded parsing while preserving that bounded-refresh behavior.
Verification