fix(storage): stop rebuilding every FTS index on every store open - #1014
Merged
Conversation
Opening a store to READ it took SQLite's write lock and rebuilt three FTS5 indexes. Under a concurrent writer the open did not wait, it failed. All three schema-drift detectors compared the DDL stored in sqlite_master against the module constant that defines it. sqlite_master holds a statement without its trailing terminator; the constants are written as runnable script and end in `);`. strip() removes the newline after the semicolon and leaves the semicolon, so "differs" was true on every open of every store, forever. Each open dropped three FTS5 tables and nine triggers, recreated them, and reissued three full rebuilds over the entire corpus. Both sides of every comparison now go through one _normalize_ddl helper that puts a statement into the form sqlite_master stores it in. All three detectors route through it. Nothing in the code compels a fourth one to, so adding an FTS table means routing its detector there deliberately. The branch this unblocks has never executed in production. When the detector always reported drift, control never reached the else arm that verifies the three sync triggers survived a crash during save_chunks. That arm starts running on every open, everywhere, at once -- so it gets witnesses before it meets production rather than through it, one pair per table rather than one standing in for three. Mutation-verified: restoring the pre-fix comparison turns 14 of the 17 witnesses red, with the failure asserting on drift rather than erroring from another layer. Bypassing the helper on chunks_fts alone turns 4 red, and they are the chunks-parameterized witnesses plus the cross-table reopen, so the suite localizes which table regressed. Worth recording, because it nearly shipped as a passing witness that proved nothing: the obvious else-branch witness -- drop a trigger, reopen, assert it came back -- PASSES under the mutation. The migration arm reinstalls triggers too, so that test cannot tell which branch ran. Only the orphan-entry witness distinguishes them: an index entry written with no backing content row is erased by a rebuild and preserved by a no-op. Premium boundary: OSS. Index maintenance only, no identity or org semantics.
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Opening a store to read it took SQLite's write lock and rebuilt three FTS5
indexes. Under a concurrent writer the open did not wait — it failed.
The defect, and it is one character
All three schema-drift detectors compared the DDL in
sqlite_masteragainst themodule constant that defines it.
sqlite_master.sqlholds a statement withoutits trailing terminator; the constants are written as runnable script and end
in
);.strip()removes the newline after the semicolon and leaves thesemicolon itself, so the two could never compare equal.
"Differs" was therefore true on every open of every store, forever. Each open
dropped three FTS5 tables and nine triggers, recreated them, and reissued three
full rebuilds over the entire corpus.
Measured directly against a real SQLite database rather than argued from the
source:
The control is the part that makes this a diagnosis rather than a guess: removing
exactly the semicolon makes the comparison succeed, so nothing else about the two
strings differs.
A note on how this defect is easy to describe wrongly, because I received it
described that way. It is not that the comparison is unnormalized — it already
lowercases and collapses whitespace on both sides. What it fails to normalize is
the statement terminator. Anyone "fixing the normalization" by adding case or
whitespace handling would add what is already there and change nothing.
The fix
Both sides of every comparison now go through one
_normalize_ddlhelper thatputs a statement into the form
sqlite_masterstores it in. All three detectorsroute through it today, which
evidence.txtenumerates by line number.What this does not guarantee, stated because v1 claimed it did. Nothing in
the code compels a fourth FTS table's detector to use the helper — a new table
can still ship its own comparison and reintroduce the defect. v1's body, commit
message, and the helper's own docstring all said a fourth table "cannot
reintroduce it". That was a universal the code does not enforce, Atlas blocked it
at r2, and it is removed from all three surfaces rather than softened.
The thing that would make it true by construction is a table→DDL registry plus a
guard test enumerating every
CREATE VIRTUAL TABLE … fts5constant in the moduleand asserting each has a routed detector. That is a real mechanism and it is
filed as a follow-up rather than folded in here: a data-driven loop on its own
would still not earn the sentence, since a fourth table can bypass the registry
too, so it would change code without making the claim true.
The branch this unblocks has never executed in production
While the detector always reported drift, control never reached the
elsearmthat verifies the three sync triggers survived a crash during
save_chunks. Thatarm starts running on every open, everywhere, at once. So it gets witnesses
before it meets production rather than through it — one pair per table rather
than one standing in for three.
Witnesses: 17, and mutation-proven
tests/recall/test_fts_schema_detector.py.rstrip(";")— restore the defectchunks_ftsonlyThe second row is the one worth reading. The suite does not merely notice that
something regressed; it localizes which table did, because the drift and
trigger witnesses are parameterized per table. A single-table regression cannot
hide behind two healthy siblings.
Recorded because it nearly shipped as a passing witness that proved nothing:
the obvious else-branch test — drop a trigger, reopen, assert it came back —
passes under the mutation. The migration arm reinstalls triggers too, so that
test cannot tell which branch ran. Only the orphan-entry witness distinguishes
them: an index entry written with no backing content row is erased by a rebuild
and preserved by a no-op.
Verification at this head
pytest tests/recall/test_fts_schema_detector.py: 17 passed.checkout:
synapt.recall.storage -> .../synapt-dev/synapt/src/synapt/recall/storage.py,interpreter
.venv/bin/python, Python 3.13.2.origin/dev;git patch-id --stableis identical beforeand after the rebase, so this is the same change replayed, not a new one.
fingerprint taken before the first mutation, under a
trap … EXIT INT TERM.Why this is being frozen now
The change was authored two weeks ago and its gate request went unserviced while
both reviewers were on another chain. It is re-frozen rather than re-submitted
because the intervening 49 commits on
devrequired a rebase, and a verdict mustbind bytes that exist. Everything above was re-measured at this head; nothing is
carried from the original freeze.
Premium boundary
recall is OSS. This is index maintenance only: no identity resolution, no org
context, no workspace-identity derivation.