Skip unchanged cost snapshot conversions - #2832
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. |
clawsweeper
Bot
added
P1
|
Codex review: needs maintainer review before merge. Reviewed August 10, 2026, 3:05 AM ET / 07:05 UTC. ClawSweeper reviewWhat this changesThe PR postpones Codex token-snapshot conversion and usage-row encoding until the SQLite cost-usage cache must append or replace rows, avoiding repeated work for unchanged files. Merge readinessKeep this PR open for normal merge review: the patch is a focused repair for the reported cost-store hot path, includes a targeted regression test, and now provides credible after-fix real-corpus proof. No blocking patch defect was found. Priority: P1 Review scores
Verification
How this fits togetherCodexBar scans local Codex session files and persists usage data in a SQLite cost-usage cache. On each refresh, the store decides whether existing cached rows can be reused, appended to, or replaced before reports consume that cache. flowchart LR
A[Codex session files] --> B[Usage scanner]
B --> C[Cost usage cache store]
C --> D{Cursor comparison}
D -->|unchanged| E[Reuse stored rows]
D -->|append or replace| F[Convert needed rows]
F --> G[SQLite cache]
G --> H[Usage reports]
Before merge
Agent review detailsSecurityNone. Review metrics
Technical reviewBest possible solution: Land the narrow lazy-conversion change with its stable-cursor regression coverage, preserving the existing append and replacement semantics. Do we have a high-confidence way to reproduce the issue? Yes at high confidence from source: current main performs conversion before the stable-cursor decision, and the focused test exercises append then unchanged-EOF refreshes. The PR also supplies redacted real-corpus output showing stable conversions drop from 1,866 to zero. Is this the best way to solve the issue? Yes. Deferring conversion until the existing persistence decision is reached is the narrowest maintainable repair because it preserves the schema and all existing write branches. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 930b1c4d0d43. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (1 earlier review cycle)
|
clawsweeper
Bot
added
proof: sufficient
|
Thanks for the careful diagnosis and real-corpus proof. This independently found the same eager-conversion hot path that was merged shortly afterward in #2834 / I compared the implementations against current
Your 125.8 MB before/after run remains useful corroboration of the root cause, and the unchanged cold-run timing is a good comparability check. No criticism of the patch itself—this was simply a parallel fix race. Thanks again for the strong evidence. |
|
That makes sense, thanks for checking the overlap. I agree #2834 covers the stable, append, and replacement cases, and the planner shape is the better place for the shared decision logic. I will leave this closed. |
2 participants
Refs #2824.
persistFileconverted every retained Codex token timestamp and JSON-encoded every usage row before deciding whether the SQLite cursor was unchanged. During bounded catch-up, that repeated the hottest ISO-8601 work across the full historical prefix even when the stable-cursor branch wrote no rows.This moves conversion and encoding behind the persistence decision. Stable files do none of that work, append-safe files convert only the new suffix, and replacement behavior stays the same. The database schema, metadata, aggregates, event indexes, and row indexes are unchanged.
Tests:
CI=1 CODEXBAR_SUPPRESS_TEST_KEYCHAIN_ACCESS=1 swift test --disable-keychain --disable-netrc --filter 'CostUsageStore|CostUsagePerformanceGateTests'(103 tests passed)CI=1 CODEXBAR_SUPPRESS_TEST_KEYCHAIN_ACCESS=1 make check(passed)CI=1 CODEXBAR_SUPPRESS_TEST_KEYCHAIN_ACCESS=1 make testdoes not complete locally because two unrelatedAdaptiveRefreshTimerTeststime out withCancellationError(). The exact clean base,1ba79680364dddbc611d710fbf59dddf76062627, reproduces both failures at lines 39 and 78.Real-corpus proof (redacted):
I ran the exact pre-fix base and this head against the same completed 125.8 MB local Codex session-day subset, using a fresh isolated cache for each run. The base carried only the same counter hook around token-snapshot conversion; it did not change persistence decisions.
The patched run asserted that stable conversions were zero and passed. The near-identical cold times are a useful comparability check; the conversion counts are the deterministic signal. No filenames, paths, session contents, report output, project names, or account/model/cost data were captured.
This is a narrow reduction in the repeated store work reported in #2824, not the broader delta-persistence redesign. Compatibility risk should be low because the write branches and schema are unchanged. Review focus: the append-only suffix indexes and the stable-cursor no-conversion invariant.