You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(db): index request_cost_ledger for quota and session-tag reads - #19081
A key with a monthly USD cap makes every request run aggregateLedgerThisMonth (via checkKeyQuota) synchronously before it is routed. The query filters request_cost_ledger on (api_key_id, timestamp) and also reads success and amount_usd. The only index for it, idx_rcl_api_key_timestamp(api_key_id, timestamp) from migration 182, does not hold those columns, so SQLite reads every month-to-date row of that key from the table. sumSearchLedgerSpendSince (daily/weekly limits) has the same shape and also reads service_tier.
getCostBySessionTag (GET /api/usage/by-run) joins call_logs.correlation_id to request_cost_ledger.request_id. There is no index on request_id, so the plan walks the whole ledger (SCAN r USING INDEX idx_rcl_timestamp).
Changes
Migration 226_request_cost_ledger_read_indexes.sql, in this order:
drops idx_rcl_api_key_timestamp. 182 is the only place that creates it, so it does not come back on startup;
adds idx_rcl_api_key_ts_cover ON request_cost_ledger(api_key_id, timestamp, success, service_tier, amount_usd). It starts with the old index's two columns, so listLedgerEntries and other lookups that used the old index still get an index;
adds idx_rcl_request_id ON request_cost_ledger(request_id).
The drop comes first so the new indexes reuse the old index's pages. The runner applies the file in one transaction, so no reader sees the table without an index. Every statement uses IF EXISTS / IF NOT EXISTS.
No application code changes. README.md, AGENTS.md, llm.txt and their 66 docs/i18n/<locale>/ mirrors now say 223 migrations instead of 222.
Plans (EXPLAIN QUERY PLAN, database built by the real runner)
read
before
after
aggregateLedger*
SEARCH ... USING INDEX idx_rcl_api_key_timestamp (api_key_id=? AND timestamp>?)
SEARCH ... USING COVERING INDEX idx_rcl_api_key_ts_cover (api_key_id=? AND timestamp>?)
sumSearchLedgerSpendSince
same as above, table lookups
USING COVERING INDEX idx_rcl_api_key_ts_cover
listLedgerEntries
USING INDEX idx_rcl_api_key_timestamp
USING INDEX idx_rcl_api_key_ts_cover
getCostBySessionTag
SCAN r USING INDEX idx_rcl_timestamp
SEARCH c USING INDEX idx_cl_session_tag, then SEARCH r USING INDEX idx_rcl_request_id (request_id=?)
Cost
Measured with node:sqlite and the sqlite3 CLI 3.46.1:
500k-row ledger with 36-character UUID key and request ids, as the app writes them (138 MB file): the file grows by 29.1 MB and nothing is left on the freelist. With the earlier order (covering index, then the drop, then idx_rcl_request_id) it grew by 44.9 MB and left 15.7 MB on the freelist.
1.4 GB database (500k ledger rows, 500k call_logs rows): the file grows by 33.3 MB.
With short ids (91 MB file) the growth is 18.6 MB, or 30.0 MB with that earlier order.
Dropping the old index last is worse. On a 100k-row UUID ledger (25.6 MB file) the growth is 6.4 MB with this order, 8.9 MB with the earlier one and 13.4 MB with the drop last, which leaves the whole old index (7.1 MB) on the freelist.
While the migration commits, the WAL briefly grows to 42–73 MB on these databases.
The upgrade also takes the runner's pre-migration VACUUM INTO backup, which scales with the database size (about 1.36 GB for the 1.4 GB database). The index build holds the write lock for its whole transaction (about 7 s on the 1.4 GB database on a loaded machine).
Each ledger insert now updates one more index with a wider key. One row is written per completed request.
Speed
SQL-layer timings, before and after, on a 1.08 GB test database with a 500k-row ledger (median of interleaved runs on a shared machine):
query
rows in range
before
after
monthly SUM, large key
256,654
248.0 ms
17.0 ms
monthly SUM, small key
6,665
4.04 ms
0.31 ms
search spend, 1 week
59,168
59.1 ms
2.78 ms
by-run, real tag (10 rows)
—
2744.7 ms
0.027 ms
On a smaller database that fits in the page cache the gains are smaller (about 1.8× to 4.5× for the SUMs). checkKeyQuota and the by-run route were not timed end to end.
Tests
tests/unit/request-cost-ledger-index-plans.test.ts (3 tests) runs the application code and checks the EXPLAIN QUERY PLAN of the exact SQL and parameters it executes: the monthly cap check in enforceApiKeyPolicy (one ledger SUM, covering-index seek, still 429 at the cap), sumSearchLedgerSpendSince (right sum, covering-index seek) and getCostBySessionTag (right rows, request_id probe, no SCAN r).
tests/unit/db/migration-226-request-cost-ledger-read-indexes.test.ts (3 tests). Each one also checks both new indexes column by column (PRAGMA index_list and index_xinfo: key columns in order, sort order, collation, unique and partial flags):
fresh database through the runner: exactly the cover, request_id and timestamp indexes; a second runner pass applies nothing;
the 226 SQL executed twice more on a database that already has it: no error, same indexes;
upgrade of a database with 2,000 ledger rows (duplicate request_ids included): old index gone, counts and sums unchanged, integrity_check ok, freelist empty.
Without the migration file all 6 fail; with it all 6 pass.
Mutation run (before this rebase; the SQL statements have not changed since): 37 mutants on the migration, the ledger SQL and the file number; 35 are caught. The two survivors build the same indexes: one creates idx_rcl_request_id before the drop, the other repeats the IF EXISTS / IF NOT EXISTS statements.
117 related test files (ledger, key quota, usage limits, by-run, analytics, migrations), run on this branch and on c84f0551a6: 29 files fail on both because they cannot load better-sqlite3, which is not installed here. Each side also has one other failing test, and it is a different test on each side. On this branch it was R3b in api-key-allow-auto-combos.test.ts, which fails intermittently when run alone (1 of 7 in a later check; earlier runs saw 3 of 3), with or without the migration file. On the base it was a cache hit records no cost in search-provider-reported-cost.test.ts, which passed 3 of 3 times alone on this branch. readme-migration-count-14574.test.ts passes.
check:migration-numbering (223 files, no new gap; 026, 055 and 121 are allowlisted), check:db-rules, check:docs-sync, check:docs-counts, check:docs-frontmatter, prettier (on the files it has a parser for; it skips .sql and .txt), eslint, typecheck:core, check:changelog-integrity, check:ai-attribution, check:test-masking, check:test-runner-api, check:file-size, check:tracked-artifacts and check:boundary:ratchet against c84f0551a6 (753 edges, same as the base) pass. check:cycles reports 14 cycles; this PR adds no TypeScript under src/.
Not tested: better-sqlite3 (everything ran on node:sqlite), Postgres or other backends.
Notes
Migration number: 226 is the next free number on release/v3.8.53 (c84f0551a6, last file 225_token_ledger_balance_indexes.sql); a gap would fail check:migration-numbering. As of 2026-11-04, open PRs feat(usage): retain cache-write tiers in diagnostics #17307, fix(codex): fence stale credential, quota and catalog writes #18347 and Publish closed usage days into their own summary #18488 also add a 226_ file, and 223–232 are claimed by other open PRs as well. If two 226_ files end up on disk, startup throws Migration version collision detected. If another 226 is applied first, the runner skips this one with a CRITICAL log and the indexes are never built. Whichever PR merges later renumbers. I will check again before this merges and move the file and the test constant with it.
Migration counts: check:docs-counts checks README.md, AGENTS.md, llm.txt and the 66 docs/i18n/<locale>/README.md mirrors; check:docs-sync checks the 66 llm.txt mirrors. This PR sets all of them to 223. If another migration PR merges first, I will renumber and update the same files in that rebase.
checkKeyQuota runs aggregateLedgerThisMonth on every request that has
a key with a cost cap. It filters request_cost_ledger on api_key_id and
timestamp and reads success and amount_usd, so with the (api_key_id,
timestamp) index from migration 182 SQLite still fetched every matching
row from the table. sumSearchLedgerSpendSince has the same shape and
also filters on service_tier. getCostBySessionTag joins the ledger on
request_id, which had no index, so it walked the whole ledger.
Migration 226 replaces idx_rcl_api_key_timestamp with a covering index
on (api_key_id, timestamp, success, service_tier, amount_usd) and adds
idx_rcl_request_id. The covering index has the old one as its prefix,
so lookups that used it keep an index. The old index is dropped first
so the new ones reuse its pages; the runner applies the file in one
transaction.
README.md, AGENTS.md, llm.txt and their 66 docs/i18n mirrors now say
223 migrations, which is what check:docs-counts expects with this file
added.
Auto incremental reviews are disabled on this repository.
Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.
You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.
Use the checkbox below for a quick retry:
Trigger review
No actionable comments were generated in the recent review.
Summary
Summary by CodeRabbit
Performance
API-key quota checks, search-spend calculations, and session-tag cost lookups can complete more efficiently as the request-cost ledger grows.
Database
The update adds indexes to support these lookups. The database may use more storage, and the migration’s backup step can take longer on larger databases.
Migration 226 replaces one request-cost-ledger index and adds two indexes. Tests check the migration and the query plans for quota, search-spend, and session-tag cost reads. Project references update the documented SQLite migration count to 223.
Migration 226 drops idx_rcl_api_key_timestamp, creates a covering index on (api_key_id, timestamp, success, service_tier, amount_usd), and adds an index on request_id. The changelog describes the indexed reads and migration characteristics. Project references update the migration count from 222 to 223.
Tests check the index definitions, repeated migration application, migration-runner behavior, aggregate results, and SQLite integrity after upgrading existing ledger data.
Tests inspect SQLite query plans for quota, search-spend, and session-tag cost reads. They also check quota responses, search-spend totals, and returned request IDs.
No actionable issue remains from this review; the change is mergeable after normal checks.
Security architecture risk: Low · up to 65c91
The indexes preserve existing ledger data, quota predicates and management-access checks. Remaining uncertainty concerns upgrade resource requirements and concurrent startup against a shared database, rather than a demonstrated new security weakness.
Retained concerns
No architecture-level concerns identified.
Security review details
Security Blast Radius
inferred — The direct change affects the request-cost ledger in each upgraded SQLite database, including records for all API keys stored there. Its operational exposure is database-wide index construction and ongoing index storage, not additional caller authority. The changelog reports approximately 29 MB of growth for its 500,000-row example and backup time proportional to database size; these figures were not independently measured.
Trust Boundaries and Controls
observed — The existing usage handler invokes requireManagementAuth before reading run_id or accessing ledger results. The supplied run identifier is trimmed, bounded to 256 characters and passed as a bound SQL value. The migration changes neither that control sequence nor the lookup's caller reachability.
Resilience and Maintainability Implications
inferred — The planning transaction ends before per-file migration execution, whose marker insertion is not conflict-tolerant. Competing migration callers could therefore encounter locking or duplicate-marker failures while transaction rollback protects the index changes. This behavior predates the PR; deployment-level migration serialization remains unknown and is not treated as an introduced security finding.
Hardening Proposals
proposed — For deployments sharing a SQLite file, use a single migration owner during upgrade and reserve disk and maintenance time for the backup and index construction. This is an operational precaution, not evidence of a verified vulnerability.
Pre-merge checks: 6 · 1
Failed checks: 1 warning
Check name
Status
Explanation
Resolution
Description check
The description follows the repository template closely. It has a Summary, a Changes section, measured Plans, Cost and Speed tables, a Tests section that lists both added test files and their coverage…
1. Add a Related Issues section. List the closed or related issue numbers, or state that none exist. 2. Add the Target Branch section. Confirm that the base is the active release/v3.8.x branch. Note that the author uses release/v3.8.53, whi…
Passed checks: 6 passed
Check name
Status
Explanation
Title check
The title 'fix(db): index request_cost_ledger for quota and session-tag reads' follows Conventional Commits with type 'fix' and scope 'db'. It matches the main change: migration 226 adds covering and …
Docstring Coverage
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Linked Issues check
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
Check skipped because no linked issues were found for this pull request.
Tests For Production Code
The PR changes production code under src/ by adding src/lib/db/migrations/226_request_cost_ledger_read_indexes.sql. It also adds two targeted test files: `tests/unit/db/migration-226-request-cost-…
No Ai Attribution
No AI attribution found. Both reviewed commits have empty trailer fields and human author/committer metadata. Their subjects and bodies contain no Co-Authored-By or AI-generation footer. The PR body…
Full details: Title check
Explanation
The title 'fix(db): index request_cost_ledger for quota and session-tag reads' follows Conventional Commits with type 'fix' and scope 'db'. It matches the main change: migration 226 adds covering and request_id indexes on request_cost_ledger for quota and session-tag reads.
Full details: Docstring Coverage
Explanation
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 1 functions across 2 files. (5 skipped: 5 unsupported.)
Full details: Tests For Production Code
Explanation
The PR changes production code under src/ by adding src/lib/db/migrations/226_request_cost_ledger_read_indexes.sql. It also adds two targeted test files: tests/unit/db/migration-226-request-cost-ledger-read-indexes.test.ts validates the migration and index definitions, and tests/unit/request-cost-ledger-index-plans.test.ts checks the affected query plans.
Full details: No Ai Attribution
Explanation
No AI attribution found. Both reviewed commits have empty trailer fields and human author/committer metadata. Their subjects and bodies contain no Co-Authored-By or AI-generation footer. The PR body also contains no AI/bot co-author trailer or Generated with <AI tool> footer. The AI matches in changed documentation refer to the product domain, not attribution.
Full details: Description check
Explanation
The description follows the repository template closely. It has a Summary, a Changes section, measured Plans, Cost and Speed tables, a Tests section that lists both added test files and their coverage, and Notes on the migration number collision risk. The Related Issues section is missing. The Target Branch checklist is also missing, and the author does not say whether the base is the active release/v3.8.x branch. The author does state the base as release/v3.8.53 (c84f055), which is not the template's stated active base. The Validation checklist items are not ticked, and the description does not name the change type. The Reviewer Notes content appears under Cost and Notes rather than under a Reviewer Notes heading. The migration number may collide with other open PRs, which the author flags.
Resolution
Add a Related Issues section. List the closed or related issue numbers, or state that none exist. 2. Add the Target Branch section. Confirm that the base is the active release/v3.8.x branch. Note that the author uses release/v3.8.53, which the template does not list as the active base. Explain the choice or rebase onto the active base. 3. Tick or annotate the Validation checklist. Name the change type (DB). State that npm run lint passed, and state whether the change was reconciled with the current active release base. 4. Confirm that the Tests Added Or Updated section lists every changed test file. The current list covers the two new test files. 5. Move the migration-collision and rebase risks, and the manual validation notes, under a Reviewer Notes heading. 6. Use the template headings exactly: Summary, Related Issues, Target Branch, Validation, Tests Added Or Updated, Coverage Notes, Reviewer Notes.
Finishing Touches
Generate unit tests (beta)
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review.
Re-homed to release/v3.8.54: v3.8.53 entered its release freeze, so the branch now belongs to the release captain and development continues on the next cycle. Nothing is wrong with this PR — it just needed a live base. No action needed from you; CI will re-run against the new base.
This branch has not been deployed
No deployments
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
deferred-v3.8.54Feat PRs held for v3.8.54. Keep open. Do not merge into v3.8.53.
2 participants
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.
diegosouzapw
added
the
deferred-v3.8.54
Summary
A key with a monthly USD cap makes every request run
aggregateLedgerThisMonth(viacheckKeyQuota) synchronously before it is routed. The query filtersrequest_cost_ledgeron(api_key_id, timestamp)and also readssuccessandamount_usd. The only index for it,idx_rcl_api_key_timestamp(api_key_id, timestamp)from migration 182, does not hold those columns, so SQLite reads every month-to-date row of that key from the table.sumSearchLedgerSpendSince(daily/weekly limits) has the same shape and also readsservice_tier.getCostBySessionTag(GET /api/usage/by-run) joinscall_logs.correlation_idtorequest_cost_ledger.request_id. There is no index onrequest_id, so the plan walks the whole ledger (SCAN r USING INDEX idx_rcl_timestamp).Changes
Migration
226_request_cost_ledger_read_indexes.sql, in this order:idx_rcl_api_key_timestamp. 182 is the only place that creates it, so it does not come back on startup;idx_rcl_api_key_ts_cover ON request_cost_ledger(api_key_id, timestamp, success, service_tier, amount_usd). It starts with the old index's two columns, solistLedgerEntriesand other lookups that used the old index still get an index;idx_rcl_request_id ON request_cost_ledger(request_id).The drop comes first so the new indexes reuse the old index's pages. The runner applies the file in one transaction, so no reader sees the table without an index. Every statement uses
IF EXISTS/IF NOT EXISTS.No application code changes. README.md, AGENTS.md, llm.txt and their 66
docs/i18n/<locale>/mirrors now say 223 migrations instead of 222.Plans (
EXPLAIN QUERY PLAN, database built by the real runner)aggregateLedger*SEARCH ... USING INDEX idx_rcl_api_key_timestamp (api_key_id=? AND timestamp>?)SEARCH ... USING COVERING INDEX idx_rcl_api_key_ts_cover (api_key_id=? AND timestamp>?)sumSearchLedgerSpendSinceUSING COVERING INDEX idx_rcl_api_key_ts_coverlistLedgerEntriesUSING INDEX idx_rcl_api_key_timestampUSING INDEX idx_rcl_api_key_ts_covergetCostBySessionTagSCAN r USING INDEX idx_rcl_timestampSEARCH c USING INDEX idx_cl_session_tag, thenSEARCH r USING INDEX idx_rcl_request_id (request_id=?)Cost
Measured with
node:sqliteand the sqlite3 CLI 3.46.1:idx_rcl_request_id) it grew by 44.9 MB and left 15.7 MB on the freelist.call_logsrows): the file grows by 33.3 MB.The upgrade also takes the runner's pre-migration
VACUUM INTObackup, which scales with the database size (about 1.36 GB for the 1.4 GB database). The index build holds the write lock for its whole transaction (about 7 s on the 1.4 GB database on a loaded machine).Each ledger insert now updates one more index with a wider key. One row is written per completed request.
Speed
SQL-layer timings, before and after, on a 1.08 GB test database with a 500k-row ledger (median of interleaved runs on a shared machine):
On a smaller database that fits in the page cache the gains are smaller (about 1.8× to 4.5× for the SUMs).
checkKeyQuotaand the by-run route were not timed end to end.Tests
tests/unit/request-cost-ledger-index-plans.test.ts(3 tests) runs the application code and checks theEXPLAIN QUERY PLANof the exact SQL and parameters it executes: the monthly cap check inenforceApiKeyPolicy(one ledgerSUM, covering-index seek, still 429 at the cap),sumSearchLedgerSpendSince(right sum, covering-index seek) andgetCostBySessionTag(right rows,request_idprobe, noSCAN r).tests/unit/db/migration-226-request-cost-ledger-read-indexes.test.ts(3 tests). Each one also checks both new indexes column by column (PRAGMA index_listandindex_xinfo: key columns in order, sort order, collation, unique and partial flags):request_idandtimestampindexes; a second runner pass applies nothing;request_ids included): old index gone, counts and sums unchanged,integrity_checkok, freelist empty.idx_rcl_request_idbefore the drop, the other repeats theIF EXISTS/IF NOT EXISTSstatements.c84f0551a6: 29 files fail on both because they cannot loadbetter-sqlite3, which is not installed here. Each side also has one other failing test, and it is a different test on each side. On this branch it wasR3binapi-key-allow-auto-combos.test.ts, which fails intermittently when run alone (1 of 7 in a later check; earlier runs saw 3 of 3), with or without the migration file. On the base it wasa cache hit records no costinsearch-provider-reported-cost.test.ts, which passed 3 of 3 times alone on this branch.readme-migration-count-14574.test.tspasses.check:migration-numbering(223 files, no new gap; 026, 055 and 121 are allowlisted),check:db-rules,check:docs-sync,check:docs-counts,check:docs-frontmatter,prettier(on the files it has a parser for; it skips.sqland.txt),eslint,typecheck:core,check:changelog-integrity,check:ai-attribution,check:test-masking,check:test-runner-api,check:file-size,check:tracked-artifactsandcheck:boundary:ratchetagainstc84f0551a6(753 edges, same as the base) pass.check:cyclesreports 14 cycles; this PR adds no TypeScript undersrc/.Not tested:
better-sqlite3(everything ran onnode:sqlite), Postgres or other backends.Notes
release/v3.8.53(c84f0551a6, last file225_token_ledger_balance_indexes.sql); a gap would failcheck:migration-numbering. As of 2026-11-04, open PRs feat(usage): retain cache-write tiers in diagnostics #17307, fix(codex): fence stale credential, quota and catalog writes #18347 and Publish closed usage days into their own summary #18488 also add a226_file, and 223–232 are claimed by other open PRs as well. If two226_files end up on disk, startup throwsMigration version collision detected. If another 226 is applied first, the runner skips this one with a CRITICAL log and the indexes are never built. Whichever PR merges later renumbers. I will check again before this merges and move the file and the test constant with it.check:docs-countschecks README.md, AGENTS.md, llm.txt and the 66docs/i18n/<locale>/README.mdmirrors;check:docs-syncchecks the 66llm.txtmirrors. This PR sets all of them to 223. If another migration PR merges first, I will renumber and update the same files in that rebase.