Skip identical cost cache database rewrites - #2852
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. |
![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: ef0e5f33dd
ℹ️ 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".
| else { return false } | ||
| guard (cache.codexScanCatchUpPending ?? false) == restored.codexScanCatchUpPending | ||
| else { return false } | ||
| restored.lastScanUnixMs = cache.lastScanUnixMs |
There was a problem hiding this comment.
When an unchanged refresh supplies a newer lastScanUnixMs, this assignment deliberately masks the difference and the early return leaves the old timestamp in SQLite. On the next independently loaded request, makeCodexRefreshPlan compares that stale value against the 60-second refresh interval, so once it expires every subsequent request can rescan the session history instead of being debounced. Persist this freshness marker or change refresh planning so it does not depend on a timestamp discarded here.
Useful? React with 👍 / 👎.
| return CostUsageStoreBudgetResult( | ||
| deletedRows: 0, | ||
| rowCount: previous.files.count, | ||
| fileBytes: fileBytes, | ||
| catchUpRequired: false) |
There was a problem hiding this comment.
When the persisted content matches but the caller supplies a smaller rowBudget or fileBudgetBytes than was used for the preceding save, this return bypasses enforceBudgets entirely and reports success while leaving the database over the requested limit. This affects the exposed custom-budget path and would also prevent a future reduction of the defaults from pruning otherwise unchanged caches; only take the shortcut after confirming the existing row and byte counts satisfy the current limits.
Useful? React with 👍 / 👎.
ef0e5f3 to
f6b9fa4
Compare
clawsweeper
Bot
added
merge-risk: 🚨 availability
|
Codex review: needs maintainer review before merge. Reviewed August 11, 2026, 8:13 PM ET / August 12, 2026, 00:13 UTC. ClawSweeper reviewWhat this changesThe PR avoids full SQLite cost-cache rewrites for unchanged Codex scans while retaining budget enforcement, durable refresh timing, and concurrent-writer recovery. Merge readinessKeep open for normal merge review: the latest head resolves the prior writer-lock race finding and has focused regression coverage. Likely related people: Peter Steinberger (high confidence) and Robert (medium confidence). Priority: P2 Review scores
Verification
How this fits togetherCodexBar scans local Codex session data into a SQLite usage cache that feeds cost reports and refresh debouncing. The new decision path compares reconstructed scan content with the cache, then either advances freshness under a writer lock or performs the existing full save. flowchart LR
A[Codex session files] --> B[Cost scanner]
B --> C[Reconstructed usage cache]
C --> D[SQLite content comparison]
D --> E[Retention and writer lock]
E --> F[Freshness update]
E --> G[Full cache save]
F --> H[Usage reports]
G --> H
Before merge
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:
Technical reviewBest possible solution: Land the narrow scanner-only optimization with its explicit compatibility allowlist and keep unknown cache producers on the existing rebuild path. Do we have a high-confidence way to reproduce the issue? Yes: the focused store tests construct an unchanged cache, inject a competing writer at the former race boundary, and verify the retry and preservation behavior; source inspection confirms the path is scanner-only. Is this the best way to solve the issue? Yes: semantic comparison followed by a locked recheck is the narrowest approach because it avoids full rewrites without weakening retention, cache compatibility, or concurrent-writer safety. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 7b5510b22847. 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 (13 earlier review cycles; latest 8 shown)
|
clawsweeper
Bot
added
rating: 🧂 unranked krab
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
clawsweeper
Bot
added
merge-risk: 🚨 compatibility
|
@clawsweeper re-review |
clawsweeper
Bot
added
merge-risk: 🚨 compatibility
|
@clawsweeper re-review |
|
🦞🧹 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: 636888c0c9
ℹ️ 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 isPreviousReleasedGeneration = userVersion == Int64(Self.previousSchemaVersion) | ||
| && parserHash == Self.previousParserHash |
There was a problem hiding this comment.
When a user skips this release and later upgrades directly from b975eb705f905b9a to a build whose parser hash changed for an actual parsing change, this condition still accepts the old database and upgradePreviousReleasedGeneration relabels its rows with that future hash instead of rebuilding them. Those rows can then remain based on obsolete parsing behavior indefinitely; gate the exception on the known no-op target generation (15a5f81888046ea4) or model migrations as explicit source/target pairs.
Useful? React with 👍 / 👎.
|
@clawsweeper re-review |
1 similar comment
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
clawsweeper
Bot
removed
the
status: ⏳ waiting on author
f569db5 to
c735150
Compare
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
clawsweeper
Bot
added
rating: 🦞 diamond lobster
|
Final concurrency repair pushed at The optimistic no-op comparison is now only a preflight. The store acquires Local proof on this exact head:
The contributor commits and authorship remain preserved. This remains @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
clawsweeper
Bot
added
rating: 🐚 platinum hermit
|
Maintainer compatibility decision: retain Exact-head CI is green on |
|
Landed and verified.
The fix keeps unchanged Codex cost-cache content tables intact, advances only freshness under the SQLite writer transaction, preserves a concurrent winner, and requests catch-up rather than performing an unprotected or stale save. Contributor commits and authorship are preserved, and the GitHub auto-closed #2495 during merge despite the final |
2 participants
Summary
This branch fixes the Codex SQLite cost-cache write amplification without weakening cross-process correctness.
Refs #2495
This intentionally does not close #2495:
PlanUtilizationHistoryStorestill rewriteshistory/claude.jsonandhistory/codex.jsonbyte-identically. That remains a separate follow-up.Current-main integration
main, including contributor authorship and messages.PlanUtilizationHistoryStore, or changelog change.Persistence contract
lastScanUnixMsfreshness.BEGIN IMMEDIATEand rechecks the complete semantic snapshot under that writer lock.catchUpRequiredso the scanner reloads and rescans.Physical write behavior
An unchanged scan no longer rewrites files, snapshots, usage rows, aggregates, lineage, buffers, discovery/lookback state, or accumulators. It performs one bounded metadata-row update so refresh debounce survives reloads and app restarts. The regression test measures the real SQLite database and
-walpaths and does not claim zero physical writes.Regression coverage
Local proof
swift test --filter 'CostUsageStoreTests|CostUsagePerformanceGateTests': 99 tests in 2 suites passed.make check: 0 formatting or SwiftLint violations across 1,846 Swift files; generated parser hash current at47144baa8daccf52.make test: 840 selections in 70 groups, 70 first-pass successes, 0 retries, 0 timeouts.