fix(db): stop rebuilding the FTS index on every start, and page the rebuild - #40
Open
ankit-thebigred wants to merge 1 commit into
Open
fix(db): stop rebuilding the FTS index on every start, and page the rebuild#40ankit-thebigred wants to merge 1 commit into
ankit-thebigred wants to merge 1 commit into
Conversation
…ebuild Closes suitedaces#37. Two compounding bugs made the gateway unable to start on a large database. 1. The "is the index already populated?" probe could never return true. messages_fts is created as fts5(text_content, content=''), a contentless table, so SQLite stores no column values and reading one back always returns NULL regardless of how many rows are indexed. `!sample?.text_content` was therefore always true, the full-rebuild branch ran on every launch, and the fts_version check directly above it never had any effect. The first thing that branch does is DROP TABLE, so a healthy index was destroyed each start. Reproduction, no app required: CREATE VIRTUAL TABLE t USING fts5(text_content, content=''); INSERT INTO t(rowid, text_content) VALUES (1, 'hello world'); SELECT text_content FROM t LIMIT 1; -- NULL, looks empty SELECT COUNT(*) FROM t; -- 1 Fixed by counting rows. 2. The rebuild materialised the entire history in one .all(). Every user and assistant message, content JSON included, was loaded into a single array before indexing. On a large history that exhausts the V8 heap, and because the table was already dropped, each respawn restarted from an empty index and OOMed again. The incremental path had the same shape. Fixed by paging both paths through a shared helper with a keyset cursor, so peak memory is flat regardless of history size. Paging rather than .iterate() is deliberate: better-sqlite3 refuses to run a write while an iterator is open on the same connection ("This database connection is busy executing a query"), so an open iterator with batched inserts throws. Verified against the bundled better-sqlite3. Verified on a scratch database with the same schema: a 5,000 message rebuild spanning three pages indexes 5,000 rows, non user/assistant types stay excluded, a second call re-indexes nothing, ten newly inserted messages are picked up incrementally, and MATCH queries still return the expected rowids.
|
@ankit-thebigred is attempting to deploy a commit to the DevApe Team on Vercel. A member of the Team first needs to authorize it. |
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.
Closes #37.
That issue has been open since 9 July with a correct diagnosis and no fix, so here is the fix. It turned out to need one change beyond what the issue suggested, explained below.
Two compounding bugs
1. The "is the index already populated?" probe can never return true.
messages_ftsis created asfts5(text_content, content=''), a contentless table. SQLite stores no column values for contentless FTS5, so reading one back always returnsNULLno matter how many rows are indexed.!sample?.text_contentwas therefore always true, the full rebuild branch ran on every launch, and thefts_versioncheck directly above it never had any effect. The first thing that branch does isDROP TABLE, so a perfectly good index was destroyed on every start.Reproduction, no app required:
2. The rebuild materialises the entire history in one
.all().Every user and assistant message, content JSON included, was loaded into a single array before indexing. On a large history that exhausts the V8 heap. Because the table was already dropped at the top of the branch, each respawn restarted from an empty index and ran out of memory again, which is the crash loop described in #37. The incremental path had the same shape.
Bug 1 is what makes it fire on every launch. Bug 2 is what turns an unnecessary rebuild into a hard crash.
The fix
Probe
COUNT(*), and page both the full rebuild and the incremental backfill through one shared helper using a keyset cursor, so peak memory stays flat regardless of history size.One correction to the fix suggested in #37: it proposes
.iterate()with batched inserts, and that does not work. better-sqlite3 refuses to run a write while an iterator is open on the same connection:I hit this on the bundled better-sqlite3 before settling on paging. Keyset pagination gives the same bounded memory without holding an iterator open, so that is what this PR does. The reasoning is left as a comment on the helper so it does not get "simplified" back into an iterator later.
Verification
Root
npm run typecheckis clean.Behaviour checked against a scratch database using the same schema:
COUNT(*)probe after rebuildMATCHqueries after paged insertImpact
On the database that prompted #37 this was re-reading and re-indexing roughly 600,000 messages on every single launch. Beyond the crash, the repeated drop and rebuild of a ~1 GB index leaves a large amount of free space behind:
VACUUMon that database reclaimed 2.6 GiB.