fix: restore JSON-cache retention semantics lost in the SQLite cutover (refs #2760) - #2767
Conversation
…r (refs #2760) Semantics-parity audit of the CostUsageCache -> CostUsageStore migration found four behaviors the cutover dropped: - Retention pruned only the typed discovery columns while the scanner round-trips discovery through the opaque payload, so deleted files resurfaced on the next load; prune now updates both in lockstep and resets cursors/isComplete like the old cache did. - The row budget deleted the oldest files regardless of window; the old entry budget never sacrificed in-window or recently active files (the byte budget remains the only authority that may). - Fork-parent protection ignored the lineage-only dependency key and a stale parent survived when its only referencing child was pruned in the same pass; candidates now honor the dependency key and iterate to a fixpoint. - Out-of-window files with an in-window mtime (active sessions with unscanned rows) were deletable; they are protected again. saveCodexCache budgets are injectable for tests, restoring pins for previous-report preservation across trims and the non-Gregorian catch-up report (#2703), plus strip/compaction coverage.
![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: afd374639a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let lower = Int64(since.timeIntervalSince1970 * 1000) | ||
| let upper = Int64(end.timeIntervalSince1970 * 1000) |
There was a problem hiding this comment.
CostUsageScanner.parseDayKey creates the date at noon for a day key, but these timestamps are now used as the active mtime window boundaries. With a window like 2026-08-01...2026-08-03, an out-of-window session modified at 08:00 on Aug 1 is treated as inactive and can be pruned even though it falls inside the requested local day, while one modified before noon on Aug 4 is incorrectly protected. That defeats the new protection for files modified inside the scan window; derive these bounds from local start-of-day values instead.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
rating: 🦪 silver shellfish
|
Codex review: needs changes before merge. Reviewed August 8, 2026, 11:51 AM ET / 15:51 UTC. ClawSweeper reviewWhat this changesThe PR restores SQLite cost-cache retention semantics for discovery metadata, row budgets, fork-parent cleanup, and recently modified files. Regression provenancePossible regression — probable (reviewed change; known regression link). No predecessor PR is attributed. Merge readiness⛔ Blocked by patch quality or review findings - 5 items remain Keep this owner-authored PR open: it addresses real SQLite retention regressions, but the active-file protection still uses noon-based day parsing and can prune files modified during the first morning of the requested window. Priority: P2 Review scores
Verification
How this fits togetherCodexBar scans local Codex session files and persists derived usage data in a SQLite cache. Retention and budget enforcement decide which cached file and discovery records survive before the scanner uses them to resume or revalidate coverage. flowchart LR
A[Session files] --> B[Usage scanner]
B --> C[SQLite usage cache]
C --> D[Retention and budgets]
D --> E[Discovery state]
E --> B
C --> F[Usage reports]
Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Represent the active mtime interval as local start-of-day for the first requested day through local start-of-day after the final requested day, with boundary regressions for both sides. Do we have a high-confidence way to reproduce the issue? Yes, from source: a file modified at 08:00 on the first requested local day falls before parseDayKey's noon lower bound and is treated as stale. The existing test covers an interior-day timestamp, not this boundary. Is this the best way to solve the issue? No. The proposed active-file protection needs local day-start bounds; preserving the noon parser for date-key normalization is fine, but it is not a day-range boundary primitive. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 5205dd1e003c. LabelsLabel changes:
Label justifications:
EvidenceAcceptance criteria:
What I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
|
1 participant
Adversarial semantics-parity audit of the SQLite cutover (#2761/#2762, refs #2760): every behavior pinned by the deleted
CostUsageCacheTests.swift(39 tests, 1,677 lines) was mapped against the current suite and production code. Most behaviors survived the migration or became legitimately obsolete with the JSON artifact, but four real regressions did not — this PR restores them, with regression tests for each.Gaps found and fixed
Discovery pruning never reached the scanner.
CostUsageStoreretention pruned only the typed columns ofdiscovery_state, while the scanner round-trips discovery exclusively through the opaque JSON payload (CostUsageStore+CodexCache.swift,discovery(from:)). After retention deleted files, the reloaded cache still listed them (stalefilePaths,fileStamps,filePathBySessionId,missingSessionIds) withisComplete == true— the entire pruning path was dead code end-to-end. Prune now updates both representations in lockstep and restores the old cache's cursor resets and incomplete marking so the next discovery pass revalidates instead of trusting stale coverage.The row budget deleted in-window files. The old entry budget never sacrificed in-window data — only the byte budget could (old pin: "save never drops in-window files even when over the entry budget"). The cutover's row-budget loop deleted the oldest files regardless of window, which for a >25k-session in-window corpus means delete → rediscover → full rescan → delete again, a permanent catch-up churn loop the old design explicitly avoided. The row loop now protects files touching the requested window (and recently active ones); the byte budget (256 MiB) remains the sole authority that may drop in-window data, with the same catch-up marking and previous-report preservation.
Fork-parent protection diverged from the old rules. Retention protected parents referenced by lineage-only children (dependency key "not required"), which the old cache deliberately did not, and a stale parent whose only referencing child was pruned in the same pass survived as an unreferenced row (old pin: "save drops stale parents referenced only by stale children"). Candidates now honor the dependency key and iterate to a fixpoint.
Recently active out-of-window files were deletable. The old
isRecentlyActiveprotected any file whose mtime fell inside the scan window — its scanned coverage may be out-of-window solely because new rows have not been scanned yet, and deleting it forces rediscovery plus a full reparse every refresh. Retention and the row budget now honor the mtime window again.Restored test pins
saveCodexCachebudgets are injectable for tests, which made the old cache-level pins portable: previous-report preservation across repeated trims, the non-Gregorian catch-up report calendar fix (#2703), detail stripping of protected/incomplete files instead of stalling, and fork-parent compaction (strip, not delete) when a surviving child still needs the baseline.The full coverage map (behavior → ported / obsolete / gap) is in the review session; obsolete classifications are all JSON-encoding mechanics (producer keys and predecessor acceptance lists → schema/parser-hash
user_version, load caps and estimate-underestimation loops → SQLite byte budget with incremental vacuum).Proof
swift test --filter CostUsage: 398 tests, 25 suites, all passmake check: clean (0 violations in 1,804 files)Known pre-existing flakes (present on the base commit, unrelated to this change): "app refresh bypasses scanner debounce" and "cached codex token hydration populates startup token snapshot" fail under specific parallel suite combinations, pass in isolation and on rerun.