fix(repair): check the FTS5 content table before rebuilding from it (#2278) - #2279
Conversation
…emPalace#2278) An isolated `malformed inverted index for FTS5 table ...` says the inverted index and `embedding_fulltext_search_content` disagree; it does not say which of them is wrong. `maybe_autoheal_fts5_index` rebuilt the index from that content table regardless and reported "rebuilt from intact content". Damaging the content table produces that same wording on SQLite 3.45.1, 3.47.1 and 3.51.2 against chroma's trigram table, and the rebuild then overwrites the index that still held the drawers' own terms. Measured on a mined 30-drawer palace with 12 drawers carrying one distinctive word: `lexical_search` returns 12 before the damage, 12 while quick_check is dirty, and 0 after the heal, with `embedding_metadata` still holding the word in all 12 rows. On the `mine` path nothing re-files afterwards and the heal prints nothing, so the loss is silent and permanent. Chroma writes each document twice -- into `embedding_metadata` under `chroma:document` and into the FTS5 table at `rowid = embeddings.id` -- and every read path returns the metadata copy. The heal now checks the content table against it, restores the rows that disagree, and rebuilds, in one transaction under the mine lock. It declines when the comparison raises and when no content row has a document to check it against. Rows the authority cannot speak for keep their content and are counted in the output; the rebuild still indexes them as they stand, which this does not fix. `typeof(m.id) = 'integer'` guards the restore: `embedding_metadata.id` is nullable, and NULL into the content table's `INTEGER PRIMARY KEY` auto-assigns rather than conflicts, so such a row would never reconcile and every later mine would fail. The test fixture now builds the three tables chroma writes rather than a bare FTS5 table, and indexes them with `trigram`: `unicode61` reports content-side damage as `fts5: checksum mismatch` from SQLite 3.51.2 on, which the classifier deliberately does not match, so the tests would otherwise pass or fail by which SQLite the runner links. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
igorls
left a comment
There was a problem hiding this comment.
Verified against current develop (merges cleanly; full suite 4423 passed, 31 skipped, ruff clean).
Approving. The framing is what makes this one land: malformed inverted index says the index and the content table disagree, not which side is wrong — and rebuilding from the content table anyway assumes the answer. When the content side is the damaged one, the current heal destroys lexical reach and reports success. Your measurement of that is the part worth quoting: lexical_search 12 → 12 → 0, called "rebuilt from intact content", with the word still sitting in embedding_metadata the whole time. Silent loss reported as a repair is the worst failure mode a repair path can have.
Checking against the copy that reads actually return is the right authority to pick, and declining when the check cannot conclude is the right default — an inconclusive repair that does nothing beats a confident one that deletes.
Two things I appreciated in the writeup:
You were explicit about what this does not fix: rows the authority cannot speak for keep their content, are named, and are still indexed unchecked. Scoping that honestly rather than implying a full guarantee is more useful than a bigger claim would have been.
And the cost is stated with its shape, not just a number — 4.9s and 3.1s against a 232s rebuild at 325k documents, one run, ratio moving with corpus size and page cache. Two extra scans in front of a repair that only runs on damage is an easy trade.
Leaving _vacuum_and_rebuild_fts5 alone because both its call sites run after the collection was re-filed is the right call too; widening the blast radius of a repair fix is how repair fixes turn into incidents.
Fixes #2278.
What does this PR do?
An isolated
malformed inverted index for FTS5 table …says the index andembedding_fulltext_search_contentdisagree, not which one is wrong.maybe_autoheal_fts5_indexrebuilds the index from that content table anyway and calls it "rebuilt from intact content".Chroma stores each document twice — in
embedding_metadataunderchroma:document, and in the FTS5 table atrowid = embeddings.id— and reads return the metadata copy. The heal now checks the content table against it, restores the rows that disagree and rebuilds, in one transaction under the mine lock, and declines when the check cannot conclude. Rows the authority cannot speak for keep their content and are named; the rebuild still indexes them unchecked, which this PR does not fix. The ABORT banner it stands in front of already said to "Recreate the FTS5 virtual table from intact embedding_metadata rows".On a mined 30-drawer palace with 12 drawers carrying one word, those 12 content rows rewritten:
lexical_searchreturns 12, still 12 while quick_check is dirty, then 0 afterdevelop's heal — reported as success, the word still inembedding_metadata. Here it stays 12.mineis where that is permanent, and there the heal prints nothing.Cost: two scans before a full reindex — 4.9s and 3.1s against a 232s rebuild at 325,451 documents, one run, and the ratio moves with corpus size and page cache.
_vacuum_and_rebuild_fts5rebuilds unchecked too and is left alone — both its call sites run after the collection was re-filed.How to test
uv run pytest tests/test_repair.py -q -k "fts5 or autoheal" uv run pytest tests/test_cli.py tests/test_miner_fts5_validation.py -qVerification
trigramevery content case is accepted on 3.45.1/3.47.1/3.51.2 and reports rows to restore where index damage reports nonelexical_search12 → 12 → 0 ondevelop, 12 → 12 → 12 here; a realmineprints no FTS5 line either way, exit 0Connectionsubclass: errors unchanged, file byte-identical each timeINTEGER PRIMARY KEYauto-assigns instead of conflicting, so the row never reconciles and every laterminefails;typeof(m.id) = 'integer'skips ittrigrambecauseunicode61calls content damagefts5: checksum mismatchfrom 3.51.2 on, which the classifier does not match4289 passed, 31 skipped; six spawn-based lock and timing tests flake here under load, anddevelopfails the same suites at the same rate.ruff checkandruff format --checkclean on 0.16.1 and 0.15.14.Checklist
python -m pytest tests/ -v)ruff check .)