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
perf(db): index token_ledger and collapse the balance read to one statement - #17908
token_ledger has had no index since 060_create_gamification.sql created it. This adds the two indexes the balance and history reads need, and collapses getBalance from two full scans into one statement.
Why
Read
Before
After
getBalance
SCAN token_ledger ×2
SEARCH ... USING INDEX idx_token_ledger_to_created / ..._from_created
getHistory
MULTI-INDEX OR (already fine)
unchanged
Measured with sqlite 3.46.1 via EXPLAIN QUERY PLAN.
transferTokens calls getBalanceinside its own write transaction (src/lib/db/gamification.ts), and the ledger is append-only, so the scan cost was unbounded write-lock hold time — a correctness-adjacent problem, not only a latency one.
Why not UNION ALL
The obvious rewrite of getHistory into a UNION ALL of two indexed branches measures worse: MERGE plus two USE TEMP B-TREE FOR LAST TERM OF ORDER BY. The existing WHERE from = ? OR to = ? ORDER BY created_at DESC already plans as MULTI-INDEX OR over both new indexes. So getHistory is untouched.
getBalance is different: a single SUM(...) WHERE from = ? OR to = ? cannot use either index (it has to scan and filter). Splitting it into two scalar subqueries is what makes the indexes reachable, and it is also one round trip instead of two.
Test
tests/unit/migration-213-token-ledger-indexes.test.ts — 5 tests that pin both the plan and the behaviour:
index names exist after migration
getBalance plan uses each index and does not SCAN token_ledger
getHistory plan still resolves MULTI-INDEX OR (guards against a future UNION ALL regression)
net balance across credit and debit
overdraft rejected, then accepted at exactly the balance
check-open-sse-typecheck.mjs reports 11 errors (browser-worker.ts TS2353 ×7, chatgpt-session.ts TS2353 ×3, token-estimate.ts TS2307 tiktoken). They reproduce on a clean HEAD worktree containing none of this branch's files, and they vanish when packages/browser-pool is removed — inherited tip drift in the signal member of the browser-pool page type.
Docs
Migration count 209 → 210 in README.md, AGENTS.md, llm.txt, and the 69 translated README rows. The 66 docs/i18n/<locale>/llm.txt files are byte-exact mirrors of root llm.txt (check-docs-sync.mjs compares the body), so they carry the same four sites. Each mirror is exactly 4 4 in --numstat and every removed line contains 209 — verified, not assumed.
docs/audit/AUDIT-2026-10-22.md carries the full audit this came out of, with §6 item 2 corrected to the index-only fix.
…tement
token_ledger was created in 060 with no index. getBalance summed one column and
then the other (two full scans), and getHistory filtered on one of two columns
and sorted by created_at (scan + temp B-tree). transferTokens calls getBalance
inside its own write transaction, so the scan was write-lock hold time on an
append-only table.
Two indexes, one per direction. getBalance becomes a single row with two scalar
subqueries: a single SUM over cannot use either index, but
each correlated subquery can. getHistory is untouched - EXPLAIN QUERY PLAN shows
the existing OR already plans as MULTI-INDEX OR over both new indexes, and a
UNION ALL rewrite measured slower (MERGE plus two temp B-trees).
Measured with sqlite 3.46.1 via EXPLAIN QUERY PLAN:
before: SCAN token_ledger / SCAN token_ledger
after: SEARCH token_ledger USING INDEX idx_token_ledger_to_created
SEARCH token_ledger USING INDEX idx_token_ledger_from_created
tests/unit/migration-213-token-ledger-indexes.test.ts pins the plan and the
behaviour (net balance, newest-first history, overdraft rejected then accepted
at the exact balance).
…and mirrors
Migration 213 lands in this branch, so the four English surfaces that state the
count (README.md, AGENTS.md, llm.txt) and the 69 translated README rows move from
209 to 210. check-docs-counts-sync.mjs fails without this; it passes after.
The 66 docs/i18n/<locale>/llm.txt files are exact-copy mirrors of root llm.txt
(check-docs-sync.mjs compares the body byte-for-byte), so they carry the same
four substitution sites. Rewritten from the root body, not by a blind regex: each
mirror is 4 lines added, 4 removed, and no removed line lacks "209".
…ecord
Eleven findings across security, performance, orchestration and structure, every
one citing path:line plus a verbatim snippet. Numbers are re-measured in-session
and the commands live in section 9, which separates the read-only audit against
the pristine tip from the remediation run on the tree that carries migration 213.
Section 6 item 2 records what the fix actually is: indexes only. The UNION ALL
rewrite that the finding originally implied measured slower (MERGE plus two temp
B-trees per branch) because the existing OR already plans as MULTI-INDEX OR over
both new indexes. What does need to change is getBalance, whose single SUM over
cannot reach either index.
Known-red gates are recorded rather than papered over: the open-sse typecheck
gate fails with eleven errors that reproduce on a clean HEAD worktree with none
of this branch's files, so they are inherited tip drift.
Take the tip's README/AGENTS/llm.txt and i18n mirrors (migration-count bumps are left to the cycle bump), drop the dangling poolOrdering.ts reference from the audit doc, add the changelog fragment.
Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>
…e audit report
Migration 213 is taken on release/v3.8.53 by 213_api_key_preferred_connections, and the runner refuses version collisions, so every DB test failed. Move the token_ledger indexes to the next free slot (218), rename the test and changelog reference to match, drop docs/audit/AUDIT-2026-10-22.md (out of scope for this PR) and bump the README migration count to 215.
Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>
…nted migration count to 221
Tip has 220 migrations and gaps at 226/227 for 228; next free slot is 224. Count-only bumps (220 to 221) in README, AGENTS.md, llm.txt and their i18n mirrors.
Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>
…ollision
The tip gained 224_proxy_logs_egress_ip_families.sql (cheaper-inference#17326). Rename this PR's migration, its test and changelog text to 225 and bump the documented migration count 221 to 222 (digits only).
Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>
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
Token balance lookups and transfers now use indexed ledger data, reducing unnecessary full-table scans.
Bug Fixes
Balance calculations remain unchanged, and transfers that exceed the available balance continue to be rejected without altering the ledger. A transfer equal to the available balance succeeds.
Migration 225 adds two indexes for token-ledger lookups. The getBalance function calculates received minus sent tokens in one query. Unit tests cover query plans, balance and history results, and transfer outcomes.
Changes: Token Ledger Balance
Layer / File(s)
Summary
Indexes and balance lookup src/lib/db/migrations/225_token_ledger_balance_indexes.sql, src/lib/db/gamification.ts, changelog.d/fixes/17908-token-ledger-balance-indexes.md, AGENTS.md, README.md, llm.txt
Migration 225 adds indexes for incoming and outgoing ledger entries. getBalance calculates the net balance in one query. The changelog describes these changes, and the documentation updates the migration count to 222.
Index and transfer tests tests/unit/migration-225-token-ledger-indexes.test.ts
Tests check index creation, query plans, balance and history results, and transfers that exceed or equal the available balance.
No merge-blocking issue is established by the supplied evidence.
Security architecture risk: Minimal · up to 1ece0
The indexes and consolidated balance query preserve the existing transfer controls and ledger structure. No material security risk introduced or worsened by this change was identified.
Retained concerns
No architecture-level concerns identified.
Security review details
Security Blast Radius
inferred — The changed behavior affects balance reads and transfers backed by the existing token-ledger database. The inspected changes do not expand the set of caller-selectable accounts, introduce another service or datastore, or grant additional authority.
Trust Boundaries and Controls
observed — Both transfer-route handlers retain their management-authorization gate. POST validates source and destination identifiers and a positive amount; the unchanged sharing layer additionally rejects self-transfers and non-finite amounts before the database transaction. The query rewrite does not bypass these controls.
Resilience and Maintainability Implications
observed — Database retries with the same explicit idempotency key remain deduplicated. End-to-end HTTP retry idempotency is a pre-existing limitation: the route does not accept a reusable key, so the sharing layer generates a fresh key for each call. Neither this behavior nor its exposure changes in this PR.
Pre-merge checks: 7
Check name
Status
Explanation
Title check
The title uses the Conventional Commits form with type perf and scope db. It matches the main change: a new index set on token_ledger and a single-statement getBalance read. The changeset contains bot…
Description check
The description covers the What, Why, Test, and Gates sections, and it explains the migration count and docs updates. It omits several template items: the Related Issues section, the Target Branch che…
Docstring Coverage
No eligible functions found in the changed files. Skipping docstring coverage check. 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/lib/db/gamification.ts and adds src/lib/db/migrations/225_token_ledger_balance_indexes.sql. It also adds `tests/unit/migration-225-token-ledger-indexes.tes…
No Ai Attribution
No prohibited AI attribution found. The reviewed commit range contains six standard Co-authored-by: diegosouzapw trailers, which identify a human contributor. No trailer names an AI assistant or bot…
Full details: Title check
Explanation
The title uses the Conventional Commits form with type perf and scope db. It matches the main change: a new index set on token_ledger and a single-statement getBalance read. The changeset contains both parts, so the title is accurate and specific.
Full details: Description check
Explanation
The description covers the What, Why, Test, and Gates sections, and it explains the migration count and docs updates. It omits several template items: the Related Issues section, the Target Branch checklist (base branch release/v3.8.x and v4-feature label), the Validation change-type selection, the Tests Added Or Updated list, the Coverage Notes for src/ changes, and the Reviewer Notes on the migration risk. The Test section also names tests/unit/migration-213-token-ledger-indexes.test.ts, but the changed file in the summary is tests/unit/migration-225-token-ledger-indexes.test.ts, and the migration is 225 in the summary, not 213. Because most of the template is present, the description is mostly complete, but the missing sections and the file-name mismatch need fixing.
Resolution
Add or complete these items: Related Issues (or state none); Target Branch checkbox confirmation; the change type under Validation; the list of changed test files under Tests Added Or Updated, with the correct name tests/unit/migration-225-token-ledger-indexes.test.ts; Coverage Notes for the src/lib/db/gamification.ts change; and Reviewer Notes on the migration 225 risk and the transferTokens write-lock behavior. Correct the test file name and migration number references from 213 to 225.
Full details: Docstring Coverage
Explanation
No eligible functions found in the changed files. Skipping docstring coverage check. 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 0 functions across 2 files. (5 skipped: 5 unsupported.)
Full details: Tests For Production Code
Explanation
The PR changes production code under src/lib/db/gamification.ts and adds src/lib/db/migrations/225_token_ledger_balance_indexes.sql. It also adds tests/unit/migration-225-token-ledger-indexes.test.ts, covering the new indexes, query plans, balance/history behavior, and transfers.
Full details: No Ai Attribution
Explanation
No prohibited AI attribution found. The reviewed commit range contains six standard Co-authored-by: diegosouzapw trailers, which identify a human contributor. No trailer names an AI assistant or bot, and no commit contains an AI-generation footer. The authored PR body also contains neither prohibited form.
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; 6 remain after this review.
diegosouzapw
added a commit
to shevar0/OmniRoute
that referenced
this pull request
Nov 3, 2026
…ip collision
The tip gained 224_proxy_logs_egress_ip_families.sql (cheaper-inference#17326) and cheaper-inference#17908 takes 225. Move the fence migration, its rehome rows and tests to 226 and bump the documented migration count to 223 (digits only; assumes cheaper-inference#17908 lands first).
Co-authored-by: diegosouzapw <8016841+diegosouzapw@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
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.
What
token_ledgerhas had no index since060_create_gamification.sqlcreated it. This adds the two indexes the balance and history reads need, and collapsesgetBalancefrom two full scans into one statement.Why
getBalanceSCAN token_ledger×2SEARCH ... USING INDEX idx_token_ledger_to_created/..._from_createdgetHistoryMULTI-INDEX OR(already fine)Measured with sqlite 3.46.1 via
EXPLAIN QUERY PLAN.transferTokenscallsgetBalanceinside its own write transaction (src/lib/db/gamification.ts), and the ledger is append-only, so the scan cost was unbounded write-lock hold time — a correctness-adjacent problem, not only a latency one.Why not
UNION ALLThe obvious rewrite of
getHistoryinto aUNION ALLof two indexed branches measures worse:MERGEplus twoUSE TEMP B-TREE FOR LAST TERM OF ORDER BY. The existingWHERE from = ? OR to = ? ORDER BY created_at DESCalready plans asMULTI-INDEX ORover both new indexes. SogetHistoryis untouched.getBalanceis different: a singleSUM(...) WHERE from = ? OR to = ?cannot use either index (it has to scan and filter). Splitting it into two scalar subqueries is what makes the indexes reachable, and it is also one round trip instead of two.Test
tests/unit/migration-213-token-ledger-indexes.test.ts— 5 tests that pin both the plan and the behaviour:getBalanceplan uses each index and does notSCAN token_ledgergetHistoryplan still resolvesMULTI-INDEX OR(guards against a futureUNION ALLregression)Gates
Known red, not from this branch
check-open-sse-typecheck.mjsreports 11 errors (browser-worker.tsTS2353 ×7,chatgpt-session.tsTS2353 ×3,token-estimate.tsTS2307tiktoken). They reproduce on a cleanHEADworktree containing none of this branch's files, and they vanish whenpackages/browser-poolis removed — inherited tip drift in thesignalmember of thebrowser-poolpage type.Docs
Migration count 209 → 210 in
README.md,AGENTS.md,llm.txt, and the 69 translated README rows. The 66docs/i18n/<locale>/llm.txtfiles are byte-exact mirrors of rootllm.txt(check-docs-sync.mjscompares the body), so they carry the same four sites. Each mirror is exactly4 4in--numstatand every removed line contains209— verified, not assumed.docs/audit/AUDIT-2026-10-22.mdcarries the full audit this came out of, with §6 item 2 corrected to the index-only fix.