Repository navigation
fix(usage): page filtered index reads in offset order - #8479
Merged
Merged
Conversation
- Close the candidate reader on the unchanged and generation-mismatch branches of refreshFilteredAggregate; only the paging paths closed it, so repeated unchanged reads leaked one SQLite connection each. - Wrap index-side candidate-page failures in UsageFilterIndexPageError and answer from the streaming ledger scan instead of rejecting the request; generation changes keep the rebuild retry. - Give the write-contention degradation test a 20s timeout: the extended busy-retry window takes several seconds by design. Co-Authored-By: Epinephrine <luvs01@hanmail.net>
The extended busy-retry backoff plus a 30k-row double scan pushes this test past the default 5s timeout on CI (observed 5002ms). Co-Authored-By: Epinephrine <luvs01@hanmail.net>
Keep bounded retry handling for ledger and aggregation-input changes after the index pager becomes unavailable. Cover ledger replacement, overlay changes, and real SQLite pager rollback/error wrapping in active and passive reads.
Leftover '+' diff-application artifacts between test blocks; they parse as unary-plus expressions but carry no meaning. Co-Authored-By: Epinephrine <luvs01@hanmail.net>
…dule - nextPage() requests an index rebuild when a page fails with SQLITE_CORRUPT/NOTADB, so a damaged projection heals instead of degrading every query to streaming\n- UsageLedgerRebuildRequiredError retries on the original short schedule; the 9-delay backoff stays for SQLITE_BUSY contention\n- document per-page transactions on close(), name the fallback retry contract in the structure doc, and fix the stale one-transaction claim\n- cover the per-page generation recheck against real SQLite with a rebuild between pages Co-Authored-By: Epinephrine <luvs01@hanmail.net>
Publish a new generation between the first and second candidate pages of a live aggregate, asserting the restarted paging matches the full-scan oracle with fresh totals. Co-Authored-By: Epinephrine <luvs01@hanmail.net>
Document candidatePager.nextPage, readActive, accumulateCandidates and refreshFilteredAggregate. Co-Authored-By: Epinephrine <luvs01@hanmail.net>
…ading Two racing writers can observe the projection mid-creation: classifyDatabase sees the schema's statements commit one by one, and a journal sidecar can vanish mid-stat in preparePrivateSqliteDatabase. Both produced an instant foreign/unsafe degrade to streaming even though the index was healthy a moment later; the two-process rebuild test observed exactly that as candidates:null. Re-check both on the short rebuild schedule; only a home change still degrades immediately. Co-Authored-By: Epinephrine <luvs01@hanmail.net>
- transient foreign file clears mid-retry and a later attempt builds the index - transient unsafe sidecar clears mid-retry and recovers the same way - permanent unsafe sidecar still falls back to streaming, unmodified Co-Authored-By: Epinephrine <luvs01@hanmail.net>
The schema now commits in one BEGIN IMMEDIATE transaction, so the stale partial-schema justification for the foreign/unsafe re-check is replaced; the structure note names the classifications that share the short schedule. Co-Authored-By: Epinephrine <luvs01@hanmail.net>
The outer WHERE byteOffset > ? on the compound select could not reach the UNION branches, so every page materialized the whole candidate set before dropping the rows below the cursor. Each branch now carries the cursor bound and its own LIMIT, and the wrapped UNION still merges and deduplicates the bounded branch pages. Adds a >2,000-candidate provider-only test and a paged plan assertion on the branch seeks. Co-Authored-By: Epinephrine <luvs01@hanmail.net>
The candidate pager wrapped every branch in a compound UNION and sorted the merged range with an outer ORDER BY byteOffset. Each branch index was ordered (generation, timestamp, byte_offset) or (generation, provider|model, ...), so SQLite could not deliver byte order from it and materialized a temp B-tree over the whole matching set on every page. A deep page on a 400k-row ledger cost seconds against a sub-second streaming scan. Each branch is now one keyset statement whose index is ordered (generation, <equality prefix>, byte_offset, ...), and the pager merges the branch heads in JS. Per branch it keeps a fetch cursor and the surplus rows the last page did not need, so a candidate is fetched about once instead of being re-read on every page. Index changes behind schema version 5 (recreated, then rebuilt from the ledger): - idx_usage_rows_window replaces idx_usage_rows_ts; the window is a residual predicate on a byte-order index rather than a sort-inducing timestamp seek. - key, client, admission and unattributable indexes carry byte_offset before any other varying column. - lone provider/model filters page from new offset-ordered attribution indexes; the unused idx_usage_attr_model is dropped. - the Codex roster expands to one equality per label, since client IN (...) spans key values and forces a sort. Tests: new tests/usage/usage-filter-paging.test.ts pins the no-temp-B-tree plan for every branch shape, multi-branch cursor order and page bounds, the corrupt projection rebuild, and the version 5 recreation. The transient-open retry tests replace their real timers with a deterministic beforeRetryDelay hook; the page-yield test observes the scheduled macrotask instead of racing it. The 400k-row benchmark runs on macmini-cf over SSH, not the interactive machine. Co-authored-by: Epinephrine <luvs01@hanmail.net> Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Contributor
Pre-merge checks:
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Title check |  |
This assessment has not completed. The check will be retried on the next pre-merge check run. |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled.
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Contributor
|
✅ Deterministic PR hygiene checks passed. |
2 of 3 tasks
The indexed offset is where the next unindexed row starts. Each page runs in its own transaction, so a concurrent refresh can index a row at exactly that offset while a read is paging; with a <= bound the read returned it, the row reader rejected it as content_changed, and the whole read retried until it failed. Bound candidates with <, and start an append read's branch cursors at afterOffset instead of adding a second lower bound SQLite seeks from on every page. Co-authored-by: Epinephrine <luvs01@hanmail.net>
Owner
Author
Maintainer judgmentValue: Filtered usage reports on large ledgers stop materializing every matching offset and stop sorting the whole candidate range per page. Pages are bounded, readers are released on every path, and an index failure falls back to the authoritative ledger scan. This carries #8435 (luvs01) with every commit preserved, plus two maintainer fixes: offset-ordered keyset paging (c5e5d37) and a strict UX change: none visible in totals; large filtered dashboard/API usage views respond without long synchronous stalls. Existing users: the derived filter index moves to schema v5 and is recreated and rebuilt from the ledger once on first filtered read; the ledger itself is never written. While another process holds the index lock, a filtered read may now wait up to about 9 s before streaming. A v4 and a v5 binary sharing one Windows: SQLite file locking is unchanged; no transaction is held between pages, so WAL checkpoints are not blocked and no index file is unlinked while open. |
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
3 tasks done
lidge-jun
added a commit
that referenced
this pull request
Nov 12, 2026
#8479 lengthened the active index read's SQLITE_BUSY schedule from [10, 25, 50] ms to nine delays totaling about 6.4 s, plus a synchronous 250 ms busy timeout per attempt. A filtered request that meets a held index write lock therefore waited 9-13 s before degrading to the streaming read (2.7 s before #8479), and the macOS dev batch timed out the contention test at 20 s. The streaming read answers the same request, so the long wait only held dashboard requests and the event loop. Restore the original schedule, assert the scheduled busy delays stay bounded (deterministic, through the retry-delay hook), and return the contention test to the default timeout. The two-process rebuild test, which passed on the short schedule before #8479, still passes. Co-authored-by: lidge-jun <243035832+lidge-jun@users.noreply.github.com>
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.
Summary
Filtered usage reports (provider, model, API key, client or device filters) read candidate offsets from the derived filter index in bounded pages instead of materializing every matching offset, yield between pages, release SQLite readers on every path, and fall back to the authoritative ledger when an index page fails. Each candidate branch is a keyset statement over an index ordered
(generation, <equality prefix>, byte_offset), merged in JS, so a deep page no longer sorts the whole matching range (schema version 5; the index is recreated and rebuilt from the ledger, never migrated).Carries contributor PR #8435 (all of its commits are here, rebased onto dev) plus two maintainer fixes:
ORDER BY byteOffsetbuilt a temp B-tree over the full match set on every page.<=the read returned it, the row reader rejected it ascontent_changed, and the request retried until it failed. Append reads also start each branch cursor atafterOffsetinstead of adding a second lower bound that SQLite seeks from on every page.Behavior to know: while another process holds the index lock, a filtered read now waits up to about 9 s (longer busy-retry schedule) before falling back to the streaming scan. A v4 and a v5 binary sharing one
OPENCODEX_HOMErebuild the derived index alternately, as with every earlier schema bump; the ledger is never modified.Co-authored-by: Epinephrine luvs01@hanmail.net
Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Verification
tests/usage/usage-filter-paging.test.ts"a read never pages past its snapshot while a concurrent refresh appends rows": fails before 994a4c5 (6 pass / 1 fail, offsets beyond the snapshot), passes after.bun scripts/test.ts tests/usage/ tests/server/api-usage.test.ts tests/test-layout.test.ts tests/test-layout-tooling.test.ts tests/ci-workflows/file-size-ratchet.test.ts: 1697 pass, 0 fail.bun run typecheck: no errors in 2,418 files.bun run structure:check: pass.Checklist