fix: keep recently modified files out of cost-store window pruning (refs #2760) - #2764
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: 6ef23bc53e
ℹ️ 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 activeSinceMs = CostUsageScanner.parseDayKey(sinceDay, calendar: calendar) | ||
| .map { Int64($0.timeIntervalSince1970 * 1000) } |
There was a problem hiding this comment.
When a file was modified during the morning of sinceDay, this cutoff still classifies it as stale: CostUsageScanner.parseDayKey constructs the date at 12:00, so f.mtime_ms < ? deletes files from midnight through noon even though their modification day is inside the retained window. Those files can therefore re-enter the expensive prune/rediscover/full-parse cycle this change is intended to prevent; derive the cutoff with calendar.startOfDay(for:) or compare day keys instead.
Useful? React with 👍 / 👎.
| if activeSinceMs != nil { | ||
| sql += "\n AND f.mtime_ms < ?" | ||
| } |
There was a problem hiding this comment.
When pruning a historical or otherwise bounded window, this one-sided predicate preserves every file modified after sinceDay, including files modified after untilDay. For example, an Aug 4 file is treated as active for an Aug 1–3 window, unlike the former day-key isInRange check; this can leave out-of-window rows behind and force hard-budget deletion to remove retained rows instead. Add an upper boundary for the end of untilDay (or compare the mtime day key against both bounds).
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
rating: 🦪 silver shellfish
|
Codex review: needs maintainer review before merge. Reviewed August 8, 2026, 2:22 PM ET / 18:22 UTC. ClawSweeper reviewWhat this changesThe PR retains recently modified Codex session-cache rows during SQLite window pruning only within the requested inclusive local calendar-day range. Regression provenancePossible regression — probable (reviewed change; reproduction). No predecessor PR is attributed. Merge readinessThe earlier calendar-boundary defects are fixed on the current PR head. This remains a focused, proof-backed correction to current-main cache retention and should stay open for routine maintainer merge review. Priority: P2 Review scores
Verification
How this fits togetherCodexBar scans Codex session files and stores parsed usage in a SQLite cost cache. Window and budget pruning decide which cached rows survive for reuse during subsequent refreshes. flowchart LR
A[Codex session files] --> B[Cost usage scanner]
B --> C[SQLite cost store]
C --> D[Window and budget pruning]
D --> E{Modified within scan days?}
E -->|Yes| F[Keep cached row]
E -->|No| G[Prune stale row]
F --> H[Reuse on refresh]
G --> H
Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Merge the day-bounded retention correction after routine confirmation that preserving active cache rows matches the SQLite retention contract. Do we have a high-confidence way to reproduce the issue? Yes. Current main’s noon-anchored parser excludes first-day morning modifications; the PR adds deterministic boundary cases and a production refresh-path scenario. Is this the best way to solve the issue? Yes. Local day starts plus the next local day as the exclusive upper bound is the narrowest fix for inclusive calendar-day retention without altering hard-budget escape behavior. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 985546901bb9. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (7 earlier review cycles)
|
clawsweeper
Bot
added
rating: 🧂 unranked krab
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
clawsweeper
Bot
added
proof: sufficient
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
Landed. Verified before merge: complementary to #2767, not duplicative — the shared activeWindowMs boundary was noon-anchored via parseDayKey, leaving first-day-morning files unprotected and bleeding into the morning after the final day; this pins protection to local midnight boundaries with tests on both edges plus an over-budget refresh proving cached-row reuse. Thanks @Yuxin-Qiao! |
2 participants
Summary
retentionCandidates(the window prune, also reached fromenforceBudgetswhen the store is over budget) now mirrors the JSON path'sisRecentlyActiveexemption: a session file whose mtime falls inside the retained window is not pruned, even when all its usage coverage days are out of window.sinceDaythrough 24:00 ofuntilDay(computed viaCostUsageScanner.CostUsageDayRange.localGregorianCalendar, then passed to SQL asAND (f.mtime_ms < ? OR f.mtime_ms >= ?)). This addresses the review findings:parseDayKeyanchors at noon, which would have pruned files modified during the morning ofsinceDay. The cutoff now starts at day start.untilDayare candidates again instead of being retained indefinitely.deleteOldestRetainedFile) keeps its own narrowerzeroDayRecentlyActiveguard on purpose: hard caps still need an escape valve when every remaining file is recently active.Real behavior proof (production refresh path, synthetic corpus)
Drives the production refresh path (
CostUsageScanner.loadDailyReportfor.codexwith a syntheticCODEX_HOME+ injected cache root, no credentials, no real user data): one still-active session whose usage rows are all out of window (coverage 2026-05-10, mtime 2026-08-02) plus 30 idle stale sessions, then an over-budgetenforceBudgets(1-file row budget, the same functionsaveCodexCachecalls on every refresh).The 30 idle files are pruned; the active stale-coverage file survives the over-budget pass, and the warm refresh performs zero head parses (the row's validation anchor is reused instead of re-parsing the file).
Test
retention keeps recently modified file with stale coverage(mtime = 06:00 onsinceDay)retention prunes stale file modified before the windowretention prunes stale file modified after the windowretention keeps stale file modified at the window edges(00:00 ofsinceDay, 23:59:59 ofuntilDay)over budget prune retains stale coverage file modified inside the window(inCostUsagePerformanceGateTests)swift test --filter "CostUsageStoreTests|CostUsagePerformanceGateTests"(73 tests, green)make check(0 violations)