Parse the OpenCodex usage log incrementally instead of re-reading it every refresh - #3140
Conversation
…ay reloads
The OpenCodex spend source (`~/.opencodex/usage.jsonl` → `OpenCodexUsageFanOut`
→ `OpenCodexUsageAggregator.snapshot`) re-resolved pricing context per entry:
`listPriceUSD` called `CostUsagePricing.codexCostUSD` without a pre-resolved
models.dev catalog, so every call went through `ModelsDevCache.load` →
`FileManager.attributesOfItem` (a stat plus an extended-attribute read), and
without a pre-resolved custom-pricing overlay, so every call also re-read the
overlay file location. Each windowed entry was priced three times (day, session
and hour accumulators), and day keys / hour buckets were recomputed through
Calendar per entry. On a 35k-entry log (all inside the 30-day window) that is
~100k stat+xattr syscalls and ~70k Calendar interval computations per refresh —
in the running app this was the 25–35 s CPU spike on every adaptive refresh
(sampled: `snapshotsBySubscription` → `attributesOfItem` → `getxattr`/`listxattr`).
Changes (snapshot output is byte-identical; verified against a reference
implementation in tests and by diffing CLI JSON on frozen inputs):
- Resolve the models.dev catalog and the custom-pricing overlay once per
fan-out / snapshot and pass them down; price each windowed entry once and
reuse the value for the day/session/hour/model merges. A missing catalog is
substituted with an empty catalog so the degraded path never falls back to
per-call loads.
- Memoize the local-day key and hour-bucket start per calendar interval using
the calendar's own `[start, end)` intervals (DST-correct; no 86400/3600
arithmetic).
- `ModelsDevCache.load` reads (mtime, size) via POSIX `stat` instead of
`attributesOfItem` (which also reads xattrs); memo/invalidation semantics
unchanged. This helps every caller repo-wide.
CodexParserHash is regenerated because ModelsDevPricing.swift is in the hashed
set; the previous hash (3c984b655688593f) 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.
Measured (release CodexBarCLI, isolated cache root, real 41.7 MB / ~35k-entry
usage.jsonl, same machine, `cost --provider codex --days 30`), OpenCodex path
isolated with identical frozen inputs:
- OpenCodex path alone (empty codex home, identical frozen inputs, CLI JSON
output identical apart from `updatedAt`):
cold 14.3 s real / 9.1 s user / 4.9 s sys / 193 G instructions
→ 2.6 s / 2.4 s / 0.1 s / 40 G
warm (store cache hit) 13.8 s / 8.3 s / 5.3 s / 166 G
→ 1.2 s / 1.1 s / 0.04 s / 13 G
- Full `cost --provider codex` CLI run on live data, steady state after the log
grew (the app's per-refresh case): ~11 s → ~3.5 s real (7.3–9.2 s → 3.2 s user);
cold 26 s → 14 s. Peak footprint unchanged (~430 MB cold/grown, ~120–140 MB
warm).
Peak memory is unchanged — the remaining transient is the append-only log
re-parse (`OpenCodexUsageStore` identity = path|size|mtime), left for a
follow-up.
Tests: equivalence against an independent reference implementation (mixed
providers, estimated/reported/unreported/unsupported, custom overlay, duplicate
request IDs, DST transitions in America/Los_Angeles and America/Santiago),
metadata-read counting proving one catalog load per snapshot (zero with an
injected catalog), day/hour memo boundary cases, and ModelsDevCache memo
invalidation on size/mtime change after the stat switch.
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>
|
🦞👀 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: 8a5e56efdf
ℹ️ 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".
| private func insertCachedEntries(_ entries: [OpenCodexUsageEntry], cursor: ParseCursor) { | ||
| self.writeEntries(entries, cursor: cursor, replaceAll: false) |
There was a problem hiding this comment.
When the app and CLI refresh this shared cache concurrently, a loader that parsed from an older cursor can acquire the write lock after another loader has already committed a later cursor, yet this insert-only write blindly moves the cursor backward. Usually the next refresh repairs that state, but if the log is then truncated back to the older offset, the cache-hit path sees size == parsedOffset and returns rows retained from beyond the truncation indefinitely. Compare the persisted cursor inside the transaction before inserting, or otherwise prevent an older incremental operation from overwriting a newer cursor.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
merge-risk: 🚨 other
|
Codex review: needs changes before merge. Reviewed August 25, 2026, 3:46 AM ET / 07:46 UTC. ClawSweeper reviewWhat this changesThe PR replaces whole-file OpenCodex usage-log reparsing with cursor-based tail parsing, a versioned SQLite cache, and dedicated parser/cache regression tests. Merge readinessNeeds changes before merge: full reloads can publish and cache records from a same-inode log that was truncated after parsing, and the branch still edits the release-owned changelog. Priority: P2 Review scores
Verification
Live VerificationCommand: Result: FAIL (failed) — execution before step 1 Assertions:
How this fits togetherCodexBar parses the OpenCodex usage log into a local SQLite cache, which feeds spend aggregation for the dashboard and CLI. The changed subsystem decides when cached rows remain valid and when the log must be reread. flowchart LR
A[OpenCodex usage log] --> B[Parser reads snapshot or tail]
C[File identity and cursor] --> D{Cache reuse decision}
B --> D
D --> E[SQLite usage cache]
E --> F[Spend aggregation]
F --> G[Dashboard and CLI]
Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Copy recommended automerge instructionTechnical reviewBest possible solution: Revalidate the full parse against the post-read log size before publishing or caching it, add a deterministic truncation-after-read regression test, and leave release-note edits to the release owner. Do we have a high-confidence way to reproduce the issue? Yes, from source: truncate the same file after full parsing completes but before its post-read stat; the current full-reload check accepts the unchanged inode without checking the captured size. Is this the best way to solve the issue? No: the incremental path revalidates after reading, but full reload must apply the same size-aware validation before its rows and cursor are persisted. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 31cb6dfb9b93. LabelsLabel changes:
Label justifications:
EvidenceAcceptance criteria:
What 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 (4 earlier review cycles)
|
Explain why the models.dev catalog and the custom-pricing overlay are resolved once per snapshot / fan-out, why a missing catalog is substituted with an empty one (so the degraded path never falls back to per-call ModelsDevCache.load), the two-level overlay precedence in listPriceUSD, why the day-key memo cannot disagree with CostUsageLocalDay.key, and that the metadata-read recorder is task-local test-only instrumentation. Comments only; CodexParserHash is regenerated because ModelsDevPricing.swift is in the hashed set (no shipped hash is affected; the predecessor list is unchanged). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…every refresh
`~/.opencodex/usage.jsonl` is append-only, but `OpenCodexUsageStore` keyed its
sqlite cache on `path|size|mtime`, so the identity changed on every append and
the cache never hit in the steady state. Each refresh therefore re-read the
whole log (42 MB / ~35k entries on the reference machine), rebuilt a 42 MB
`String`, ran one `JSONSerialization` per line, and then did
`DELETE FROM entries` plus 35k inserts. Even a cache hit re-decoded a stored
JSON payload for every row.
The store now keeps a parse cursor in its metadata — log path, file identity
(`st_dev`/`st_ino`, not mtime), the byte offset just past the last consumed
record, and a SHA256 of the first 64 KiB — and on load either serves the cached
rows unchanged, or reads only `[parsedOffset, size)` and merges those rows with
`INSERT OR REPLACE`. A changed file identity, a shrunken file, or a prefix-digest
mismatch falls back to the full re-parse, so rotation, truncation and in-place
rewrites are still handled. A trailing record with no newline is returned but
not committed and does not advance the cursor, so a later append that glues
bytes onto it cannot desync the cache from a full parse.
Two supporting changes: token fields become typed sqlite columns (schema v2), so
reading the cache no longer decodes JSON per row, and the parser slices lines out
of the file's bytes (`mappedIfSafe` plus `memchr`) instead of materializing the
whole log as a `String`. `parseLines` now uses the same splitter, so there is one
line-splitting rule instead of two.
Measured (release CodexBarCLI, isolated cache root, real 42 MB / 35k-entry log,
`cost --provider codex --format json --days 30`, OpenCodex path isolated with
identical frozen inputs; CLI JSON output identical apart from `updatedAt`):
- cold, no cache: 2.85 s / 422 MB peak -> 2.74 s / 187 MB
- warm cache hit: 1.06 s / 112 MB -> 0.52 s / 75 MB
- after the log grew (the app's per-refresh case, full run on live data):
4.58 s / 422 MB -> 2.14 s / 110 MB
- cache database: 18 MB -> 5.1 MB
Baselines are the parent commit (Codex 0.54.2 branch state); against upstream
main before the OpenCodex work the same cold case was 19-21 s / ~450 MB.
Tests: incremental result equals a full re-parse (append, partial trailing line,
newline-less record later glued to more bytes, duplicate request IDs, truncation,
rotation with a matching 64 KiB prefix, in-place rewrite), tail-only reads with a
byte/line recorder (zero bytes when nothing changed), schema-v2 round trip for a
missing `usage`, a single zero-valued field and mismatched totals, a v1 database
rebuild, and a busy-writer case proving a failed `BEGIN IMMEDIATE` leaves both the
rows and the cursor untouched.
Implemented by grok-4.6 (xhigh) via implementation-loop; reviewed hunk by hunk
plus an independent deep review whose blocker (an ignored `BEGIN IMMEDIATE`
result that could empty the cache under a concurrent writer) and seven further
findings were fixed in one iterate round.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The app and the CLI open the same OpenCodex usage cache, so two loaders can race: A reads cursor C1 and parses from it, B reads C1, parses further, takes the write lock and commits C2 > C1, and A then takes the lock and commits its snapshot — moving the durable cursor backwards and re-inserting rows B already stored. The same bytes are then parsed again on the next refresh, and after a later truncation the `size == parsedOffset` cache-hit path can serve rows for content no longer in the log. `writeEntries` now re-reads the durable cursor after `BEGIN IMMEDIATE` succeeds and before any mutation. For an incremental append (`replaceAll == false`), a durable cursor with the same path and file identity whose `parsedOffset` is at least the proposed one means this work is stale: the transaction rolls back without touching the cursor or the rows. Full reloads re-derived the whole file and still replace even a newer cursor, which is what truncation, rotation and a schema rebuild need. `loadEntries` re-runs its cursor path once against the freshly committed durable state and falls back to a full reload if that write is stale too, so a caller never sees a partial result and never loops. Test: `stale incremental write does not regress a newer durable cursor` is ordered rather than racy — load once to capture a cursor, append and load again to commit a newer one, then drive the write path with the first snapshot through a test-only seam and assert the newer cursor survives, no duplicate rows exist, and a subsequent load still equals a full re-parse. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
f456d6c to
7b57ab1
Compare
|
Both items are addressed on the pushed head ( P2 — stale cursor commits after acquiring the write lockThe race is real and I reproduced the mechanism as described: the write decided what to persist from a cursor snapshot taken before the lock, so a delayed loader could commit an older
Regression test P3 — release-owned changelogRemoved.
Note that the stacked predecessor #3136 still carries its own changelog commit; say the word and I will drop it there too rather than force-push a PR that is otherwise sitting quietly. Checks
|
clawsweeper
Bot
added
merge-risk: 🚨 compatibility
…al result `loadEntries` validates the file identity and prefix digest before parsing, but the tail read happens after that check. A rotation or replacement in that window made the parse start at the old `parsedOffset` inside the new file, and those bytes were then merged with cached rows belonging to the old file and persisted under the old identity — a refresh could publish a mixed old/new snapshot and store a cursor for a file that no longer exists at that path. `incrementalReload` now re-stats the log after the parse and before anything is inserted or returned, and requires the same path and `st_dev`/`st_ino` plus a cursor that still validates against the post-read stat (size and prefix digest included). On mismatch it discards the parsed tail, writes nothing, and runs a single `fullReload` against the file as it now exists — the same bounded fallback the stale-cursor path uses. A replacement that preserves path, device, inode, size and the first 64 KiB is still undetectable, which is the digest threat model already documented at `canReuseCursor`. Tests drive the window deterministically through a task-local post-parse hook (unset and free in production, with a case asserting it is unset by default): the log is replaced with a different inode while the parse result is in hand, once with a replacement longer than the old offset and once shorter. Both assert the returned entries equal a full re-parse of the replacement, that no request ID from the old file survives, that the persisted cursor describes the replacement, that no duplicate rows exist, and that the next load is a cache hit reading zero bytes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Addressed on P2 — revalidate the file after tail parsingCorrect, and thank you for pushing on it: the identity check ran before the tail read, so a rotation in that window made the parse start at the old
Still undetectable, and commented at the check: a replacement that preserves path, device, inode, size and the first 64 KiB. That is the pre-existing digest threat model documented at Deterministic coverage — no sleeps, no real threads. A task-local post-parse hook (unset and free in production; one test asserts it is unset by default) lets a test replace the log while the parse result is in hand:
Both assert that the returned entries equal a full re-parse of the replacement, that no request ID from the old file survives, that the persisted cursor describes the replacement, that no duplicate rows exist, and that the next load is a cache hit reading zero bytes. Merge order with #3136Noted — this branch is stacked on #3136 deliberately (both touch the same OpenCodex path and #3140 builds on the price-once work). Whenever #3136 lands I will rebase this onto Checks
|
clawsweeper
Bot
added
rating: 🐚 platinum hermit
…nd I/O errors An independent side-effect review found three ways this cache could misbehave at runtime. None are theoretical; two are regressions this branch introduced. Mixed ledgers in one cache. Every OpenCodex home shares one cache database while the log path differs per home, and this branch had replaced the per-read identity check with a cursor check made from a separate connection. A loader could read its own cursor, have another home's CLI replace the cache, then read that home's rows as its baseline — and the write-lock check only rejected same-file older offsets, so it kept those rows, reset the cursor and appended its own tail. The cursor and the cached rows are now read in one transaction, the write carries the exact base cursor the parse was derived from, and an incremental write is rejected whenever the durable cursor differs from that base in any field. A crash class. The full parse mapped the whole log and then walked it; a truncation between mapping and touching those pages raises SIGBUS, which Swift cannot catch — the app dies. The parser now opens one descriptor, snapshots its size with fstat, and reads exactly the range it intends to parse, treating a short read as "changed under us". That also bounds a read that would otherwise chase a growing file, and gives the full-reload path the same post-read identity check the incremental path already had. The cost is that a full parse now holds the file's bytes: on a 42 MB log the cold rebuild peaks at ~290 MB instead of ~187 MB, still well under the ~485 MB of the current implementation, and it only runs on a cold cache or a schema rebuild — the per-refresh path reads only the appended tail and is unchanged at ~118 MB. Transient failures shown as "no spend". Any stat failure returned an empty array, and the dashboard turns that into a confirmed-empty state that removes the OpenCodex source. Only ENOENT/ENOTDIR now mean "absent"; every other errno throws so the source is marked unavailable instead. Schema v2 also moves to its own database filename, so downgrading to an older build leaves it a usable v1 cache rather than one it can read but never write. Tests: an incremental load whose cache was replaced by another home keeps only its own entries; a circular symlink and an unreadable directory throw instead of returning empty; a missing log still returns empty without throwing; the legacy v1 database survives a v2 rebuild. The cached-state accessor is `parseCursor`, not `cursor`: the provider-architecture gate reads a bare `.cursor` as a reference to the Cursor provider, and this one is a parse position, matching the persisted metadata key. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Pushed 1. Two OpenCodex homes could be mixed in one cache. Now: the cursor and the cached rows are read in one transaction; the write carries the exact base cursor the parse was derived from; and an incremental write is rejected whenever the durable cursor differs from that base in any field, not just when it is a newer same-file offset. Regression test: seed the cache for log B, drive an incremental load for log A, assert both the returned entries and the sqlite rows contain only A's request IDs. 2. A crash class from The parser now opens one The cost is memory on the cold path: the full parse now holds the file's bytes, so a rebuild on a 42 MB log peaks at ~290 MB instead of ~187 MB. That is still well under the ~452 MB of the parent branch, and it runs only on a cold cache or a schema rebuild — the per-refresh path reads only the appended tail and is unchanged (~118 MB). I took the trade deliberately; say the word if you would rather have the mapping back with a documented caveat, or a chunked reader to claw the memory back. 3. Transient I/O failures were published as "no spend". Any Also: schema v2 moved to What the review could not break, for the record: the retry state machine is bounded at two iterations; there is no cyclic SQLite lock acquisition between the app and the CLI; the cursor cannot point past EOF; and no descriptor, statement or connection leaks on any error path. One thing I am disclosing rather than fixing here. The forced-refresh reconciliation tail calls the synchronous OpenCodex merge from a
|
|
Heads-up: #3136 just landed on main, touching the same aggregator/fan-out files plus a regenerated parser hash. This one needs a rebase onto latest main; after resolving, re-run |
Co-authored-by: olddonkey <olddonkeyblog@gmail.com>
Co-authored-by: olddonkey <olddonkeyblog@gmail.com>
clawsweeper
Bot
added
the
rating: 🦐 gold shrimp
clawsweeper
Bot
added
status: ⏳ waiting on author
|
Maintainer verification: the combined focused OpenCodex parser, incremental store, fan-out, pricing, native cost-store, and provider-architecture suites passed 199 tests; the prior parallel SQLite fixture race reproduced before the database-scoped checkpoint fix and passed afterward; make check passed with zero formatting/lint violations; parser hash remained cfd84d13ad7d4cfa; exact-head GitHub CI including both macOS shards and Linux builds passed https://github.com/steipete/CodexBar/actions/runs/32822882427. The initial full local suite reached group 58/78 before the architecture scanner exceeded its 180-second timeout under host load above 90; exact-head CI completed the full platform coverage successfully. Landed as 17ade2b. |
2 participants
Summary
~/.opencodex/usage.jsonlis append-only, butOpenCodexUsageStorekeyed its sqlite cache onpath|size|mtime— so the identity changed on every append and the cache never hit in the steady state. Every refresh re-read the whole log (42 MB / ~35k entries on my machine), rebuilt it as one 42 MBString, ran aJSONSerializationper line, then didDELETE FROM entriesplus ~35k inserts. Even a cache hit re-decoded a stored JSON payload for every row.#3136 removed the per-entry pricing work on this path; this removes the per-refresh re-read.
Changes
st_dev/st_ino, not mtime), the byte offset just past the last consumed record, and a SHA256 of the first 64 KiB. On load it either serves the cached rows unchanged (size == parsedOffset), or reads only[parsedOffset, size)and merges those rows withINSERT OR REPLACE— which keeps the existing "last occurrence in file order wins" dedupe. A changed file identity, a shrunken file, or a prefix-digest mismatch falls back to the full re-parse, so rotation, truncation and in-place rewrites are still handled.mappedIfSafe+memchr) instead of materializing the log as aString.parseLinesnow uses the same splitter, so there is one line-splitting rule instead of two (\nonly; a lone\ror a form feed is no longer a record separator, and a raw U+2028/U+0085 inside a JSON string no longer destroys the record).Ordering is still produced by Swift comparisons, not SQLite collation, and the returned array is byte-identical to a full re-parse.
Measured
Release
CodexBarCLI, isolated cache root, real 42 MB / 35k-entry log,cost --provider codex --format json --days 30. The OpenCodex path is isolated by pointing the run at an empty Codex home and giving both binaries an identical frozen copy of the log; CLI JSON output is identical apart fromupdatedAt.The cold figure is higher than an earlier revision of this PR (187 MB) because the full parse no longer memory-maps the log — see the SIGBUS note under "Side-effect review" below. That path runs on a cold cache or a schema rebuild; the per-refresh path reads only the appended tail and is unaffected.
For scale: before #3136 the same cold case was 19–21 s / ~450 MB.
Tests
OpenCodexUsageStoreIncrementalTests(new): incremental result equals a full re-parse for append, partial trailing line, a newline-less record later glued to more bytes, duplicate request IDs, truncation, rotation with a matching 64 KiB prefix and in-place rewrite; tail-only reads verified with a byte/line recorder (zero bytes read when nothing changed); schema-v2 round trip for a missingusage, a single zero-valued field and mismatched totals; a v1 database rebuild; and a busy-writer case proving a failedBEGIN IMMEDIATEleaves both the rows and the cursor untouched.OpenCodexUsageParserTestsgained cases for the separators whose handling changed.Known limitation, deliberately accepted and commented in the code: the prefix digest covers only the first 64 KiB, so an in-place rewrite past 64 KiB that preserves both size and inode is not detected. The log is append-only; rotation changes the inode and truncation trips the size check.
Side-effect review
An independent review focused purely on runtime side effects (hangs, crashes, data loss) found three things worth fixing, all now addressed:
fstatand reads exactly the range it intends to parse, treating a short read as "changed under us". That also bounds a read that would otherwise chase a growing file, and gives the full-reload path the same post-read identity check the incremental path had. The cost is the cold-path memory noted above.statfailure returned an empty array, which the dashboard turns into a confirmed-empty state that removes the OpenCodex source. OnlyENOENT/ENOTDIRnow mean "absent"; every other errno throws so the source is marked unavailable instead.Schema v2 also moved to its own database filename, so downgrading to an older build leaves it a usable v1 cache rather than one it can read but never write.
The same review confirmed what it could not break: the retry state machine is bounded at two iterations, there is no cyclic SQLite lock acquisition, the cursor cannot point past EOF, and no file descriptor, statement or connection leaks on any error path.
Disclosed, not fixed here (pre-existing, and outside this PR's files): the forced-refresh reconciliation tail calls the synchronous OpenCodex merge from a
@MainActormethod (SpendDashboardController.merge(outcome:capture:)→SpendDashboardSource+OpenCodex.swift), so that read happens on the main thread. It is reached only fromSpendDashboardController.refresh(), i.e. the refresh button in the spend dashboard preferences pane — not the menu bar and not the periodic refresh. This PR makes that path roughly an order of magnitude cheaper (a cache hit or a tail read instead of a full 42 MB re-parse), but the synchronous call remains. Happy to move it off the main actor in a separate PR if you want it.Process
Implemented by grok-4.6 (xhigh) via implementation-loop; reviewed hunk by hunk plus an independent deep review. That review found a merge blocker —
writeEntriesignored the result ofBEGIN IMMEDIATE, so under a concurrent writer (the CLI opens the same database as the app) theDELETEand the cursor write would land in autocommit and the rollback would be a no-op, leaving an empty cache with a valid cursor — plus seven smaller findings; all were fixed in one iterate round. Fullmake checkandmake test(77/77 sharded groups) pass locally on this head.🤖 Generated with Claude Code