Bound Codex cost cache persistence size (#2637) - #2646
Conversation
![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: dd2fa7dffd
ℹ️ 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".
|
|
||
| let outOfWindowKeys = cache.files.keys.filter { key in | ||
| guard let usage = cache.files[key] else { return false } | ||
| return !usage.touchesCodexScanWindow(sinceKey: sinceKey, untilKey: untilKey) |
There was a problem hiding this comment.
When the cache is over budget, this removes every file with no usage day in the current window, including an incomplete old parent rollout that was discovered while resolving an in-window fork. Production scans read at most 256 MiB per file per refresh and rely on the cached offset to resume; pruning that entry on every save makes a larger parent restart from byte zero indefinitely, so the child baseline never resolves and catch-up cannot complete. Exclude incomplete files and active fork dependencies from this pruning.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 2fb4d4c: pruning now preserves entries that are still resuming (incomplete scans, resume state, in-progress scan ids, buffered fork retries) and out-of-window fork parents referenced by in-window children; the token-snapshot stripping step was removed entirely, so incremental-resume state is never invalidated.
| for (path, var usage) in cache.files { | ||
| let newestDay = usage.days.keys.max() | ||
| guard let newestDay, newestDay < cutoffKey else { continue } | ||
| usage.codexTokenSnapshots = nil |
There was a problem hiding this comment.
For an older cached parent rollout that is later appended, clearing its snapshots does not invalidate parsedBytes, size, or the incremental-resume state. The next scan therefore reads only the appended tail and rebuilds codexTokenSnapshots from that tail, yet marks the resulting index as covering the whole file; a child fork whose cutoff predates the append then receives a missing or incorrect inherited baseline and can overcount replayed tokens. Retain these snapshots or invalidate the cached parse so the next change forces a full reread.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 2fb4d4c: pruning now preserves entries that are still resuming (incomplete scans, resume state, in-progress scan ids, buffered fork retries) and out-of-window fork parents referenced by in-window children; the token-snapshot stripping step was removed entirely, so incremental-resume state is never invalidated.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2fb4d4ca6b
ℹ️ 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 usage.touchesCodexScanWindow(sinceKey: sinceKey, untilKey: untilKey) { return false } | ||
| if usage.codexScanComplete == false { return false } | ||
| if usage.codexJSONLResumeState != nil { return false } | ||
| if usage.codexScanFileId != nil { return false } |
There was a problem hiding this comment.
Every current Codex normalization path assigns codexScanFileId to completed entries as well as partial ones (CostUsageScanner+CacheHelpers.swift:1270-1272 and 1427-1429), so this unconditional check preserves virtually every session produced by the current parser. Once the cache exceeds either budget, completed out-of-window files therefore remain persisted and the memory-growth regression this change is intended to fix continues; restrict this exemption to genuinely incomplete/resumable entries.
Useful? React with 👍 / 👎.
| let neededParentSessionIDs = Set(cache.files.values.compactMap(\.forkedFromId)) | ||
| let outOfWindowKeys = cache.files.keys.filter { key in | ||
| guard let usage = cache.files[key] else { return false } | ||
| if usage.touchesCodexScanWindow(sinceKey: sinceKey, untilKey: untilKey) { return false } |
There was a problem hiding this comment.
On normal rolling-window refreshes, CostUsageScanner.swift:4646-4655 keeps scanSinceKey at the minimum historical start and scanUntilKey at the maximum end. Testing entries against those retained bounds means a session that was once requested remains “in window” forever, even after it falls outside the current report, so a long-running installation still accumulates entries without limit. Pass the actual requested range into persistence pruning and adjust the cache coverage bounds when older entries are discarded.
Useful? React with 👍 / 👎.
| #expect(loaded.days["2026-04-11"] == nil) | ||
| } | ||
|
|
||
| func `save preserves out-of-window entries that are still resuming`() throws { |
There was a problem hiding this comment.
This newly added Swift Testing function lacks @Test, so swift test silently ignores it; the reported 15-test run therefore does not exercise the preservation behavior this case was added to cover. Add the attribute so future pruning changes are actually checked.
AGENTS.md reference: AGENTS.md:L5-L5
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
rating: 🧂 unranked krab
|
Codex review: needs changes before merge. Reviewed August 4, 2026, 11:40 PM ET / August 5, 2026, 03:40 UTC. ClawSweeper reviewWhat this changesBounds the persisted Codex cost-usage cache and adds recovery-state compaction and regression tests to prevent unbounded memory use during local-session scans. Regression provenancePossible regression — probable (reproduction; failure trace; reviewed change). No predecessor PR is attributed. Merge readinessThis PR remains necessary, but one cache-recovery defect can replace a previously complete report with partial data after a repeated budget trim. Priority: P1 Review scores
Verification
How this fits togetherCodexBar scans local Codex session files into a persisted usage cache that supplies background refreshes and menu cost reports. The cache is loaded before scanning and saved afterward, so its size and recovery state affect memory use, refresh time, and displayed totals. flowchart LR
A[Local Codex sessions] --> B[Usage scanner]
B --> C[In-memory usage cache]
C --> D{Within cache budget?}
D -- Yes --> E[Persisted cache]
D -- No --> F[Compact and mark catch-up]
F --> E
E --> G[Next refresh and menu report]
Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Root-cause clusterRelationship: Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge-risk optionsMaintainer options:
Copy recommended automerge instructionTechnical reviewBest possible solution: Retain an existing matching complete previous report during later trims unless a replacement is proven to cover at least the same report window, with a regression test covering partial catch-up, another trim, and restart. Do we have a high-confidence way to reproduce the issue? Yes, at source level. The linked production report and the contributor’s real-corpus run establish the cache path, while the remaining partial-report case follows directly from the persisted catch-up control flow. Is this the best way to solve the issue? No, not yet. The bounded cache design is appropriate, but it must preserve an already complete catch-up report rather than replacing it with a report built from a partial scan. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 9cc993f310cb. LabelsLabel justifications:
EvidenceAcceptance criteria:
What 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 (38 earlier review cycles; latest 8 shown)
|
|
@clawsweeper re-review - all three findings are addressed in 88aa138:
Real-behavior proof is being collected: I will run the fixed build's |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 88aa138a29
ℹ️ 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".
| let overBudget = cache.files.count > maxCacheEntries | ||
| || (previousArtifactBytes ?? 0) > Int64(maxCacheBytes) |
There was a problem hiding this comment.
When the existing cache is below 256 MiB but one refresh adds enough rows or token snapshots to cross that limit, this condition remains false as long as there are at most 25,000 entries because it checks only previousArtifactBytes. A production refresh can admit up to 512 MiB of new session data, so save can write a 256 MiB–1 GiB document that the next load will decode, recreating the memory spike this change is intended to prevent. Base pruning on the candidate artifact being written, or otherwise trigger it before the write when the current cache crosses the byte budget.
Useful? React with 👍 / 👎.
clawsweeper
Bot
removed
the
merge-risk: 🚨 compatibility
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7883c72aa5
ℹ️ 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".
| || (previousArtifactBytes ?? 0) > Int64(maxCacheBytes) | ||
| guard overBudget else { return false } | ||
|
|
||
| let neededParentSessionIDs = Set(cache.files.values.compactMap(\.forkedFromId)) |
There was a problem hiding this comment.
The revised dependency calculation includes forkedFromId from every cached child, including out-of-window children removed by this same pass. In a fork-heavy stale corpus, pruning can therefore delete the children while retaining tens of thousands of now-unreferenced parents, leaving the resulting artifact above both budgets; because pruned is then true, the candidate-size fallback at line 158 is skipped, and an artifact above the 1 GiB load limit can be written and discarded on the next refresh. Build the dependency set from in-window/resumable children that will actually remain, or recompute it after candidate removal.
Useful? React with 👍 / 👎.
| let outOfWindowKeys = cache.files.keys.filter { key in | ||
| guard let usage = cache.files[key] else { return false } | ||
| if usage.touchesCodexScanWindow(sinceKey: sinceKey, untilKey: untilKey) { return false } |
There was a problem hiding this comment.
When the cache is over budget, a completed session in the currently requested partition that produces no usage rows—such as a suppressed subagent or idless copied prefix—fails touchesCodexScanWindow because days is empty and is therefore removed. The next refresh rediscovers and fully parses that same file; with enough zero-day files, every bounded pass can spend its budget reparsing them and repeatedly defer usage-bearing files. Pass the active scan paths into pruning or otherwise distinguish active zero-day entries from stale out-of-window entries.
Useful? React with 👍 / 👎.
| private static func decodeCache(at url: URL, maxBytes: Int) -> CostUsageCache? { | ||
| let fileSize = (try? FileManager.default.attributesOfItem(atPath: url.path)[.size] as? NSNumber)? | ||
| .int64Value ?? 0 | ||
| guard fileSize <= maxBytes else { return nil } |
There was a problem hiding this comment.
The size guard is shared by every CostUsageCacheIO.load call, but the inspected Claude path in loadClaudeDaily uses an unbounded recursive scanner and save prunes only when provider == .codex. Once a Claude or Vertex cache exceeds 1 GiB, each load now returns an empty cache, the provider performs a full corpus rebuild, and then writes another oversized artifact that will be rejected again on the next refresh. Either scope this refusal to Codex or add equivalent bounded persistence/rebuild behavior for the other providers.
Useful? React with 👍 / 👎.
Real local-corpus proof (redacted)Ran the fixed CLI ( Same machine, same corpus, same command: peak memory drops ~16x and wall time ~6x once the artifact is bounded, and the oversized artifact is never left on disk for the next refresh to decode. Cache and session paths were not touched beyond the cost-usage cache file; private session IDs/paths omitted. |
clawsweeper
Bot
added
proof: sufficient
|
@clawsweeper re-review - all three findings are addressed in 2ad965d:
Added focused regression tests for all three (suite is now 35 tests), |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
clawsweeper
Bot
added
rating: 🦐 gold shrimp
Resolves the CodexParserHash conflict by regenerating from the merged parser sources (74cc412adaee297d) rather than picking a side. Keeps main's compatibleCodexProducerKeys entry for the shipped p843ca061 hash (steipete#2648): legacy caches now load compatibly and are bounded by the new load cap and save-side pruning instead of a forced rebuild.
Parser hash regenerated from the fully merged sources (e6cd0b92f60a0380).
|
@clawsweeper re-review - the P1 from the 03:20 review is addressed in 9fdc1a7:
Rebased onto the current head (including @steipete's load-cap commit and the main merges). Suite is 50 tests, |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
clawsweeper
Bot
added
rating: 🐚 platinum hermit
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9fdc1a7008
ℹ️ 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".
| cache.codexScanCatchUpPending = true | ||
| cache.lastScanUnixMs = 0 | ||
| cache.codexPreviousReport = previousReport |
There was a problem hiding this comment.
When a refresh is already in catch-up, cache.codexPreviousReport may be the last complete user-visible report, but this save-time trim always replaces it with a report rebuilt from preTrimCache. If that refresh is still partial (for example after a force/incompatible rebuild or a prior save-time trim) and the cache remains over budget, the rebuilt report can be missing deferred files; after restart, codexPreviousReport will serve that partial data until catch-up finishes. Prefer the existing previous report when present, or only overwrite it with a report known to cover at least the same window.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
rating: 🦐 gold shrimp
|
@clawsweeper re-review - the P2 from the 03:40 review is addressed in c6004b3: Both trim paths now only create a catch-up previous report when the cache does not already carry one, so a repeated budget trim during an in-progress catch-up pass can no longer overwrite a complete report with partial data. The catch-up report is only computed when it will actually be stored (avoids the wasted build). Regression test: Suite is 52 tests, |
|
🦞🧹 Reason: re-review requires an open issue or PR. |
|
Landed. Verification before merge (independent maintainer-agent review, distrust-first):
This closes the traced allocation path for #2637. Suggested closing condition there: reporter validation on the next build (cold-launch |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c6004b3fa0
ℹ️ 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".
| } | ||
| var seen = Set(discovery?.filePaths ?? []) | ||
| for path in pendingPaths where !seen.contains(path) { | ||
| discovery?.filePaths.append(path) |
There was a problem hiding this comment.
Fresh evidence versus the earlier lookback comment is that this now copies pending lookback paths only into codexSessionDiscovery.filePaths before clearing codexActiveLookbackState.pendingFilePaths. When the byte cap trips with older recently modified sessions queued, the normal refresh path still only appends active-lookback pendingFilePaths to the scan list (CostUsageScanner.swift:4539-4543); discovery filePaths are consumed only during fork-parent lookup, so any queued session that is not needed as a parent disappears from ordinary scanning and its current-window usage stays missing until a force rescan. Keep these paths in the active lookback queue or also feed them into the normal scan list.
Useful? React with 👍 / 👎.
|
@clawsweeper re-review |
2 participants
Summary
CostUsageCacheIO.saveprunes per-file entries whose usage days fall outside the active requested scan window when the cache exceeds an entry count (25,000), the previous artifact exceeded the byte budget (256 MiB), or the candidate artifact encoded in this refresh exceeds the byte budget, mirroring the scanner's drop semantics including day-aggregate subtraction, and narrows persisted coverage after removals.CostUsageCacheIO.loadrefuses artifacts above 1 GiB so a pathological cache is rebuilt by the existing bounded scanner instead of being decoded in one shot.Real local-corpus proof
Ran the fixed CLI against the real local Codex corpus (~290 MB session files) with the cache seeded to 600,273 entries / 80.6 MB (simulating all-time accumulation):
Peak memory drops ~16x and wall time ~6x once the artifact is bounded. Details in the comment thread.
Why
Issue #2637 reports the main process growing past 7 GiB. The reporter's allocation capture showed
MALLOC_LARGEdominating duringJSONDecoder.decodeon thecost-usage-scanqueue. The cache keeps oneCostUsageFileUsageentry per scanned session file (with rows, turn IDs, token snapshots) and retains out-of-window entries forever, so an all-time corpus can grow the single JSON artifact to multiple gigabytes; every scan then decodes and re-encodes the whole document.Commands run
swift buildpassed;make checkpassed (0 lint violations)swift test --filter CostUsageCacheTests|CostUsageFetcherTests— 32 tests passed (current revision)swift test --filter 'CostUsage|CodexLocalProjectUsage|CodexSubagentAccounting'— 398 tests passedKept open for reporter validation on a 0.47.0+ build (re-run
vmmap -summary+sampleon cold launch).