Persist Codex priority-turn scan cursor across relaunches - #3130
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![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: 3d780b8a94
ℹ️ 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 databasePath: String | ||
| var coverageSinceEpoch: Int64 | ||
| var lastRowID: Int64 | ||
| var fileIdentity: UInt64? |
There was a problem hiding this comment.
If logs_2.sqlite is deleted and recreated between launches and the filesystem reuses its inode, this persisted identity still matches; when the replacement has already reached maxRowID >= lastRowID, the validation at lines 266–269 accepts the old cursor and skips every replacement row through lastRowID, retaining stale priority classifications. Inodes are only unique while a file exists, so the durable cursor needs a replacement-resistant database generation or equivalent validation before resuming.
Useful? React with 👍 / 👎.
After every relaunch, CodexBar's first Codex refresh re-scans the whole codex CLI trace DB. Profiling (release CodexBarCLI, real ~/.codex, sample(1)) pinned it on CostUsageScanner.codexPriorityTurns: three leading-wildcard `feedback_log_body LIKE '%...%'` predicates over `logs_2.sqlite` (1.4 GB / 130k rows on the reference machine) — a full body-column scan. With a cold OS page cache that is ~2 minutes of I/O-bound wall time (~20 s CPU); with a warm page cache it is ~2.5-3 s of CPU on every relaunch. The function already scans incrementally (`rowid > lastRowID`) through a process-global memo, but that memo was never persisted, and app refreshes always inspect priority turns (`bypassScannerDebounce` → `refreshMinIntervalSeconds = 0`), so every process launch paid the cold scan again. Persist the memo cursor (lastRowID, sqlite file identity, coverage epoch, accumulated turns) in the codex cost cache metadata next to the existing StoredPriorityState, and seed the memo from it before the cold path runs. All existing invalidation rules still apply to a seeded cursor (file identity change, rowid regression, window expanded earlier than coverage), and `forceRescan` drops the memo instead of seeding it. Two steady-state guards: the cursor is excluded from the identical-content save comparison (its lastRowID advances on nearly every refresh) and is written as a metadata-only update on the skip path, so the cheap no-op save stays cheap; and the cursor decodes leniently so a malformed cursor can never take `turnKeys` down with it (which would trigger cache-wide reprocessing). CodexParserHash is regenerated because hashed scanner sources changed; the previous hash (2d17f4981b78d07f) is added to compatiblePredecessorParserHashes since parsing and the persisted row shape are unchanged, so existing cost-usage.sqlite stores are adopted on upgrade instead of rebuilt (an old priority payload without a cursor simply yields one cold trace-DB scan). Measured (isolated cache root, same real data, same machine): the first refresh after a relaunch drops from ~5.4-5.9 s / ~5.0-5.5 s CPU (main) to ~2.9-3.3 s / ~2.6-3.1 s CPU (this change); token and cost totals are identical at every step; the within-interval cached fast path is unchanged (~0.8 s). Tests: 9 new cases in CostUsageScannerCodexPriorityCursorTests covering relaunch reuse with incremental-only scanning, inode change, window expansion, old payload compatibility, live-memo-wins seeding, stale-cursor re-accumulation idempotency, skip-path cursor persistence, malformed cursor decode, and force rescan; plus a predecessor-adoption case in CostUsageStoreTests for a cursorless payload. Implemented by grok-4.6 (xhigh) via implementation-loop; reviewed hunk by hunk plus an independent deep review; one iterate round. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
c1b9d25 to
a37fa55
Compare
clawsweeper
Bot
added
merge-risk: 🚨 compatibility
|
Codex review: needs maintainer review before merge. Reviewed August 21, 2026, 5:33 PM ET / 21:33 UTC. ClawSweeper reviewWhat this changesThis PR persists Codex trace-scan progress in the local cost cache so relaunches scan only newly appended SQLite rows instead of the full trace database. Merge readinessKeep open for normal merge review: the prior inode-reuse concern is addressed with content-anchor validation, and no blocking defect was found in the updated patch. Priority: P2 Review scores
Verification
Live VerificationCommand: Result: FAIL (failed) — execution before step 1 Assertions:
How this fits togetherCodexBar scans the local Codex CLI trace database to derive priority-turn cost metadata. This change saves that scanner state in the cost cache and validates it before restoring it on a later launch. flowchart LR
A[Codex trace SQLite database] --> B[Priority-turn scanner]
B --> C[Cursor validation]
C --> D[Usage summary]
D --> E[Local cost cache]
E --> F[Next-launch restore]
Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Merge the durable cursor with its conservative invalidation and cache-adoption coverage once the pending platform checks complete. Do we have a high-confidence way to reproduce the issue? Yes—current source shows each eligible refresh scans the trace database, and the PR supplies a concrete real-database CLI benchmark for the relaunch path. Is this the best way to solve the issue? Yes—the durable cursor reuses the existing incremental scanner while retaining forced-rescan, coverage, identity, and content-anchor invalidation paths. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against e85543ecf0f1. LabelsLabel changes:
Label 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 (1 earlier review cycle)
|
Review feedback on steipete#3130: the persisted cursor's `fileIdentity` is only the sqlite inode. If `logs_2.sqlite` is deleted and recreated between launches and the filesystem reuses the inode, and the replacement already has `maxRowID >= lastRowID`, the inode check accepts the stale cursor and the `rowid > lastRowID` query skips every replacement row up to lastRowID while stale priority classifications are retained. In-process that window was short; with persistence it is arbitrarily long. Add a content anchor to both the in-memory memo and the persisted cursor: the rowid the accumulation ended on plus the SHA-256 of that row's `"<ts>\n<feedback_log_body>"`. Capture it with one primary-key lookup after each successful accumulation (if the lookup fails the turns are still returned but nothing is memoized or persisted), and validate it before resuming: a missing row or a digest mismatch forces a full rescan. The inode check stays as a cheap pre-check. A missing anchor row can also be Codex pruning old rows in place; the conservative full rescan is intended there. SHA-256 uses the repo's CryptoKit/swift-crypto pattern so the Linux CLI still builds. Tests: replaced database with a reused inode (anchor mismatch -> full rescan; matching anchor -> incremental control), deleted anchor row -> full rescan, payload without anchor fields -> nil cursor / cold scan; the relaunch test now also asserts the anchor survives and advances. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Addressed the P2 from the Codex review (and ClawSweeper's merge-risk item): the persisted cursor no longer trusts the inode as its only identity. What changed (commit on this branch): both the in-memory memo and the persisted cursor now carry a content anchor — the rowid the accumulation ended on plus the SHA-256 of that row's Tests added: replaced database with a reused inode (stale anchor → full rescan; matching anchor → incremental control, with a mutated old row that must not reappear), deleted anchor row → full rescan, payload without anchor fields → nil cursor / cold scan; the relaunch test now also asserts the anchor survives and advances. The "Live Verification FAIL" in the ClawSweeper report is its own environment failing at |
clawsweeper
Bot
added
rating: 🐚 platinum hermit
2 participants
Summary
After every relaunch, CodexBar's first Codex refresh re-scans the whole codex CLI trace database. This PR makes the priority-turn scan cursor durable across launches so a relaunch resumes incrementally instead of re-scanning.
Root cause (profiled).
CostUsageScanner.codexPriorityTurnsruns three leading-wildcardfeedback_log_body LIKE '%…%'predicates over<codexHome>/logs_2.sqlite. Leading%defeats every index, so it is a full body-column scan — 1.4 GB / 130k rows on the reference machine (releaseCodexBarCLI,sample(1):pread+strcspn/patternCompareundercodexPriorityTurns). With a cold OS page cache that is ~2 minutes of I/O-bound wall time (~20 s CPU); with a warm page cache it is ~2.5–3 s of CPU. The function already scans incrementally (rowid > lastRowID) through a process-global memo, but that memo was never persisted, and app refreshes always inspect priority turns (bypassScannerDebounce→refreshMinIntervalSeconds = 0), so every process launch paid the scan again.Measured (isolated cache root, same real data, same machine,
codexbar cost --provider codex --days 30; M1/M2 = a fresh process after the 60 s refresh interval, i.e. the relaunch path):main(f74117a)Token and cost totals are identical between the two binaries at every step; the cached fast path is unchanged. The saved ~2.5 s CPU per relaunch is the LIKE scan with a warm page cache; the cold-page-cache case (~2 min I/O wait) is removed for the same reason but was not re-measured (no
purgewithout sudo).What changes
lastRowID, sqlite file identity, coverage epoch, accumulated turns) as an optionalturnsCursorinside the existingStoredPriorityStatepayload in the codex cost cache, and seed the in-memory memo from it before the cold path runs.forceRescandrops the memo instead of seeding it.lastRowIDadvances on nearly every refresh because the codex CLI appends trace rows continuously) and is written as a metadata-only update on the skip path, so the cheap no-op save stays cheap.StoredPriorityStatedecodes the cursor leniently (try? decodeIfPresent): a malformed cursor yieldsniland one cold trace scan, never a droppedturnKeys(which would trigger cache-wide reprocessing).ts\nbody), captured with one primary-key lookup and checked before resuming; a missing row or digest mismatch forces a full rescan, so a recreatedlogs_2.sqlitethat reuses an inode cannot resume from a stale cursor (review feedback). The inode check remains as a cheap pre-check.CodexParserHashis regenerated (hashed scanner sources changed). The previous hash2d17f4981b78d07fis added tocompatiblePredecessorParserHashes— parsing and the persisted row shape are unchanged — so existingcost-usage.sqlitestores are adopted on upgrade, not rebuilt; an old priority payload without a cursor loads withcodexPriorityTurnsCursor == niland intactturnKeys.Out of scope (deliberately unchanged): the LIKE predicates / query plans / parsers, pricing, the JSONL corpus scan and its 2 s budget, catch-up policy. No new dependencies.
Tests
New
CostUsageScannerCodexPriorityCursorTests(9 cases): relaunch reuses the persisted cursor and scans only appended rows (a mutated old row must not reappear); inode change still full-scans (withmaxRowID == lastRowIDso only identity can trigger it); window expanded earlier still full-scans; old payload without a cursor cold-scans; live memo wins over a stale seed; stale-cursor re-accumulation is idempotent (turns, source maps, insertion order, no duplicates); identical-content skip still persists an advanced cursor with zero file writes; malformed cursor still loads turn keys; force rescan drops the cursor and cold-scans; replaced database with a reused inode (anchor mismatch → full rescan, matching anchor → incremental); deleted anchor row → full rescan; payload without anchor fields → cold scan. PlusCostUsageStoreTests: predecessor-hash adoption of a cursorless payload without rebuild.Note: the skip-path test uses the repo's process-global
saveCycleCheckpointForTestingseam like other store tests; it is safe under the repo runner (swift test --no-parallel) and can interfere only in ad-hoc parallelswift testruns.Verification
make check: parser hash current (3c984b655688593f),0/1974 files require formatting, swiftlint0 violations, 0 serious in 1973 files.make test: green onc529ac35c(treecc21c5df; exit 0, 1116 s, ~9.2k tests) and again on51845ab7dafter the content-anchor follow-up (exit 0, 852 s, 9231 tests; all cursor-suite cases incl. the reused-inode / deleted-anchor / missing-anchor-fields cases executed inside the gate) — exit 0, 1116.6 s, ~9.2k tests passed across all shards; all 9CostUsageScannerCodexPriorityCursorTestscases and the new predecessor-adoption case executed inside the gate.make testonmain(f74117a): green (1174 s).Memory note
The ~400 MB steady RSS some users see is a separate story (GUI framework residency + transient multi-provider scan buffers + malloc fragmentation; the CLI runs this same scan in ~107 MB). This PR targets the CPU burst, not the resident-memory number.
Implemented by grok-4.6 (xhigh) via implementation-loop; reviewed hunk by hunk plus an independent deep review; one iterate round (write-amplification and lenient-decode fixes) and two follow-ups (parser hash regeneration, predecessor adoption).
🤖 Generated with Claude Code