Repository navigation
fix(summary): never touch the history cache inside a transaction - #18
Merged
Merged
Conversation
Every scanner 'update' makes the dashboard refetch /api/summary, and the menu-bar app polls it every 30 s. Several queries in it scanned all history each time: 6.5 s per call at 1M events (synthetic), the likely source of luwill#13's sustained CPU. src/historyCache.js caches aggregates over ts < local midnight per connection; callers query only today's rows and merge. Invalidation: - TEMP triggers on main.events flag inserts/updates/deletes touching rows before the boundary. They cover every write path (supersede, model backfill, Codex rebuilds), roll back with the write's transaction, and do not change the database schema. - PRAGMA data_version catches commits from other connections (`token-watcher scan` while serve runs). - the boundary moving past midnight. Raw SQL group rows are cached, not priced results, so pricing.json and FX changes still apply immediately. Cached: all-history per-day totals, all-time cost groups (aggregateCosts split into costRows + priceCostRows; merged rows are sorted in GROUP BY key order so results match a single query), the pre-today part of the 30-day per-day tool and cost series, and per-tool health counts. Also `COUNT(DISTINCT +session_id)` for today's sessions: without it SQLite walks idx_events_session over all history (440 ms -> 15 ms). 1M events: warm 6.5 s -> ~90 ms; writing today's rows keeps the cache; a history write rebuilds once (~7 s, same as before). test/history-cache.mjs: first call equals direct full-history SQL; warm == cold recompute after today inserts, history insert / UPDATE / DELETE, a row moved into history, another connection's write, a rolled back write, and crossing midnight, with recompute counts asserted. Removing the UPDATE trigger or the data_version check fails the test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The guard's dirty flag rolls back with the transaction but the JS entries do not, so a cache rebuilt inside a transaction that later rolls back would keep serving the undone rows. cachedHistory now computes directly when the connection is in a transaction and leaves the cache and guard alone. db.isTransaction needs Node 22.16; older versions fall back to probing with BEGIN. Adds the rollback repro from the luwill#17 review as a regression test, covering both detection paths. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Owner
|
Reviewed and verified in a clean worktree — LGTM.
Checking the connection instead of a scanner-set flag (option 1, generalized) is the right call — it covers any future transaction, not just |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to the #17 review. Stacked on #17: this branch contains its commit (0aa1ce3), so please merge #17 first. After that, only 5f8a92f remains in this diff.
Problem (from your review)
The
tw_history_guard.dirtyflag rolls back with the enclosing transaction, but the JSentriesdo not. IfcachedHistoryrebuilt the cache inside a write transaction that later rolled back, the cache would keep serving the undone rows, and nothing would invalidate it. The current code never reaches this path, because summaries are not built inside transactions. This change makes that invariant enforced instead of implicit.Fix
I went with your option 1, but
cachedHistorychecks the connection itself instead of relying on a flag set by the scanner. That way any future transaction, not justreplay(), is covered.cachedHistoryreturnscompute(boundary)directly and does not read or write the cache or the guard table. The result is correct for whatever the transaction can see, and callers get no new exception path.db.isTransaction, which exists from Node 22.16 on.enginesallows 22.13, so on older versions it probes withBEGIN: if that throws, we are already in a transaction; otherwise it doesCOMMITright away. A deferredBEGINtakes no locks.historyCache.jsalso gets a header comment stating the invariant (your option 2).Tests
test/history-cache.mjsincludes your repro:BEGIN, write another history row, read inside the transaction (it must see its own row).ROLLBACK, then compare against a cold recompute on a fresh connection.The case runs twice: once through
db.isTransaction, and once with that property hidden behind a Proxy so theBEGIN-probe path is used. Both cases fail without the fix (stale cache after rollback) and pass with it. Fullnpm testis green on Node 22.22.🤖 Generated with Claude Code