feat: reindex builds the new vector index beside the old one (generation swap) - #185
Merged
Conversation
…m a bad key The two provider calls used a bare `fetch` with no timeout, no retry and no backoff. Two consequences, both on the path a full reindex depends on returning: A provider that accepted the connection and never answered hung the whole run indefinitely — a rebuild of several hundred entities with no way to finish and nothing said about why. And a 429 was indistinguishable from a 500 or a 401: every non-2xx became the same silent `null`, so hitting a rate limit looked exactly like a bad API key, and the run carried on spending entities that would all fail the same way. Now: 30 seconds per request; up to three attempts for a 429 or 5xx, honouring `Retry-After` when the server sends one rather than guessing a backoff against a server that already answered the question; and an immediate stop naming the status for a 401/403/404, which is configuration rather than weather and where retrying only spends the user's rate budget on a certainty. `embedAndStore` also gains an optional target, so a caller can write into a staging table at a width the live index does not yet have. It defaults to the current behaviour, so every other writer is unchanged.
…ly when complete
Rebuilding the vector index dropped every stored embedding first and refilled
afterwards. A run that died part way — rate limit, dropped connection,
Ctrl-C, a killed process — left the graph half unsearchable, and on a paid
API the embeddings it had already produced had to be bought again. The
`DROP` committed inside `openDatabase`, before the refill loop had even
started, so the loss happened whether or not anything could refill it.
The new index is now built in a staging generation (`entities_vec_next`)
while the live one keeps answering queries, and replaces it in a single
transaction only once every entity that should have a vector has one and
nothing failed. A failed run publishes nothing: the live index is what it
was, never a half-new mix whose distances no longer compare against each
other or against the dedup threshold. The embeddings already produced are
kept, so a second `memesh reindex` resumes and asks the provider only for
what the first run did not reach — measured in the test: two requests on the
failing run, one on the resume. A generation started by a different provider
or width is discarded rather than resumed, because vectors from two
embedding spaces must not share an index.
Three sqlite-vec behaviours were measured before this was designed, not
assumed:
- `ALTER TABLE ... RENAME` does NOT work on a vec0 table. It reports
success and leaves the table unreadable: vec0 keeps four shadow tables
(`_chunks`, `_info`, `_rowids`, `_vector_chunks00`) and the rename
touches none of them. Swapping by rename is unavailable, which is why
the swap copies rows instead.
- Two vec0 tables of different widths coexist. That is what makes
build-beside possible at all.
- DROP + CREATE + copy inside one transaction really does roll back. With
the swap forced to fail before COMMIT, a fresh connection still read the
original table at its original width with every row present. Checked on
the row data, not on `sqlite_master` — a table name returning proves
nothing about the vectors.
The destructive branch is gone from the open path entirely: a dimension
disagreement keeps the working index and records that a rebuild is owed. So
`--vectors` is retired rather than kept as a no-op — its only purpose was
consenting to that drop — and the consent machinery it needed
(`allowVectorIndexRebuild`, the path-scoped one-shot grant) is deleted with
it. What survives is the pre-flight probe, now for a different reason: a
provider that cannot produce a vector at the configured width fills nothing,
and finding that out one entity at a time wastes the run.
Namespace-scoped reindex keeps writing in place. A staging table holding one
namespace would drop every other namespace's vectors when swapped in, and
in-place is safe for the reason a full rebuild was not: each row's old
vector survives until its replacement exists.
Tests: the swap now has the crash-injection case this path never had — a
trigger refuses the metadata write at the last step, after the drop and the
copy, and the previous index is asserted intact by row width and dimension
stamp. Three suites moved from the old contract to the new one, and the
partial-run test states the stronger guarantee it now has: a partial run
leaves the live index untouched rather than merely not corrupting it row by
row.
…rting what it did not do
An eight-lane review of the generation swap found the mechanism sound — a
SIGKILL inside the swap transaction leaves the previous index intact, and DROP
on a vec0 table really does tear down all four shadow tables — but found the
swap's *semantics* wrong in one way with several faces: it installed exactly the
staging index, so any state that existed only in the live index was treated as
non-existent.
- A memory captured while the rebuild ran lost its vector, because every
writer except the rebuild targets the live index and the rebuild works from
an entity list snapshotted before it started. One whose text was EDITED
mid-rebuild was worse: its fresh vector was replaced by the older staged
one, and countMissingVectors cannot see a row that is present but stale.
Rows still active and absent from staging are now carried across — only at
the same width, since during a width change a concurrent write is refused as
a dimension mismatch and there is nothing comparable to carry.
- A memory deleted mid-rebuild could come back, because forget clears the live
row and knows nothing about a staging index. Staged rows for entities that
are no longer active are now pruned before the copy.
- pending_reindex had two owners and the destructive one won: the swap deleted
it inside its own transaction, before the post-rebuild measurement could
decide. A rebuild that finished while vectors were still missing printed
"the reindex-needed flag was left set" and then left nothing set, so doctor —
whose only vector check reads that row — reported a healthy install. The swap
no longer touches it; the measurement at the end of reindex() either clears
it or writes it.
Also fixed, each found by the same review:
- Two runtime messages told the user to run 'memesh reindex --vectors', the
flag this branch deleted, in the exact situation the mechanism exists to
handle. Measured: that command exits 1 with "unknown option". A new test
scans every source file that prints advice for a "memesh <cmd> --flag" the
CLI does not register; it immediately caught an older second instance,
'memesh doctor --verbose', a flag that never existed.
- A width-mismatched query cannot be matched against the index: sqlite-vec
raised, vectorSearch's catch turned that into an empty result, and empty is
indistinguishable from "searched and found nothing" — so recall reported
mode 'hybrid' / degraded false for a vector side that answered nothing, for
the whole window between switching provider and finishing the rebuild. It
now reports mode 'fts' / degraded true, and the message on open says
semantic search is off rather than that the index "still answers queries".
- BUSY_TIMEOUT_MS 5000 -> 30_000. The swap's row copy is the atomicity
guarantee (a vec0 table cannot be renamed) and is O(rows): measured 5.4s at
20,000 vectors and 9.1s at 30,000, so past roughly 16,500 vectors a rebuild
made concurrent writers fail instead of wait, losing hook captures.
- providerFetch used fetch's default redirect handling. A 307 forwards the
POST body — the user's memory text — and OLLAMA_HOST is an unvalidated env
var. Now redirect: 'error'; both providers answer 200 directly.
- generationRowIds swallowed every read error as "nothing staged". That number
decides whether a rebuild is promoted and what a resume re-buys, so it now
asks sqlite_master whether the table exists and lets real errors throw.
beginVectorGeneration keeps the generation's original startedAt across a
resume instead of overwriting it, and both it and swapVectorGeneration
reject a width that is not a positive integer before it reaches DDL.
- pending_reindex is written only when the need is new or changed, so a
mismatched database no longer takes the write lock on every hook invocation
and every MCP handshake, and its timestamp records when the need was first
noticed. The field is renamed from droppedAt, which named a deletion that no
longer happens.
Verification, run in this session against a throwaway HOME:
node scripts/run-tests-isolated.mjs exit=0
Test Files 153 passed (153) / Tests 2226 passed (2226); no "Errors" line
npm run typecheck exit=0
npm run build exit=0
Break-test of the three swap fixes, mutation applied then written back
(tests/vector-generation-swap-semantics.test.ts):
carry-forward removed -> KILLED
staging prune removed -> KILLED
swap deletes pending_reindex -> KILLED
Each restored byte-for-byte and re-verified.
The review also cleared, empirically rather than by reading: no SQL injection
path in the added interpolations, no secret in any new log line, Retry-After
bounded against every hostile form, no timer leak, and embedder.model cannot mix
two embedding spaces (embedWithProvider has exactly one caller and it hardcodes
model: undefined). Two review lanes reported that last one as critical; it is
not.
Not addressed here, and deliberately: a resumed run still trusts a staged row
without a content fingerprint, so an entity edited between an interrupted run
and its resume keeps the older vector; the reindex loop has no circuit breaker,
so a provider that fails at entity 50 of 20,000 still attempts the rest; and
res.json() is awaited outside providerFetch, so a body-phase timeout is not
retried and is not named as a timeout. Each needs its own change with its own
test.
…dit baseline
The swap-semantics tests asserted emptiness — "the forgotten row is gone", "the
staging table is empty" — without first proving the fixture had ever been
populated, so a setup that staged nothing would have satisfied them for the
wrong reason. Each now asserts the staged row exists before the swap, which is
also the spelling the C1 detector recognises as a size pin.
Audit baseline:
- Four C5 entries re-keyed +2 in src/core/operations.ts (51->53, 54->56,
57->59, 132->134). The two imports this branch added shifted them; each
statement was read against HEAD~1 and is character-for-character the same
metadata/provenance spread, so this is a re-key, not a re-triage.
- The C1 entry for tests/cli-reindex-vectors-guard.test.ts is PRUNED rather
than moved: the discriminating stderr assertion added in the previous commit
means the detector no longer flags the file. C1 hits went 24 -> 23.
- One new C6 entry for the same file, classified SAFE-DATA-EXTRACTION. The
added scanner reads src/ and text-matches on purpose: the text is the data
(which flags cli.ts registers), not the behaviour under test. It earned the
entry by catching two real defects on its first run.
Verification, this session:
node scripts/audit/verification-audit.mjs exit=0
C1 new=0 C3 new=0 C4 new=0 C5 new=0 C6 new=0 C7 new=0
no stale entries reported
node scripts/run-tests-isolated.mjs tests/vector-generation-swap-semantics.test.ts
exit=0, Tests 5 passed (5)
npm run verify:release exit=0
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.
Reindex stops deleting your embeddings before it has replacements
The last item on the UX arc, and the one with real user money in it.
Rebuilding the vector index dropped every stored embedding first and refilled
afterwards. The
DROPcommitted insideopenDatabase, before the refill loopstarted, so a run that died part way — rate limit, dropped connection,
Ctrl-C,a killed process — left the graph half unsearchable, and on a paid API the
embeddings already produced had to be bought a second time.
Now the new index is built beside the old one. A staging generation is filled
while the live index keeps answering every query, and replaces it in a single
transaction only once every entity that should have a vector has one and nothing
failed.
half-new, half-old mix whose distances no longer compare against each other or
against the dedup threshold.
memesh reindexresumesand asks the provider only for what the first run missed. Measured in the test:
two requests on the failing run, one on the resume.
than resumed. Vectors from two embedding spaces must not share an index.
memesh reindex.Three sqlite-vec facts, measured before the design
Not assumed — each was probed against sqlite-vec v0.1.9 first, and one of them
killed the obvious design:
ALTER TABLE ... RENAMEdoes not work on a vec0 table. It reports successand leaves the table unreadable: vec0 keeps four shadow tables (
_chunks,_info,_rowids,_vector_chunks00) and the rename touches none of them.The first read fails with
no such table: main.<new>_rowids. So the swapcopies rows; it cannot rename.
build-beside possible at all.
the swap forced to fail before COMMIT, a fresh connection still read the
original table at its original width with every row present.
Fact 3 is also where this review caught itself: the first probe checked
sqlite_masterfor the table name after a rollback and concluded "transactional".A table name returning proves nothing about the vectors. The real probe asserts
row count and vector width through a fresh connection on a file-backed database.
What was removed
The destructive branch is gone from the database-open path entirely — a dimension
disagreement now keeps the working index and records that a rebuild is owed. So
--vectorsis retired rather than kept as a no-op: its only purpose wasconsenting to that drop. The consent machinery it needed
(
allowVectorIndexRebuild, the path-scoped one-shot grant, ~55 lines) is deletedwith it.
The pre-flight probe survives for a different reason: a provider that cannot
produce a vector at the configured width fills nothing, and finding that out one
entity at a time wastes the whole run.
Namespace-scoped reindex still writes in place. A staging table holding one
namespace would drop every other namespace's vectors when swapped in, and
in-place is safe for the reason a full rebuild was not: each row's old vector
survives until its replacement exists.
Provider requests are bounded (first commit, stands alone)
The two provider calls used a bare
fetch— no timeout, no retry, no backoff. Aprovider that accepted the connection and never answered hung a whole rebuild
indefinitely, and a 429 was indistinguishable from a 500 or a 401, so a rate
limit produced the same silent
nullas a bad key while the run kept spendingentities that would all fail identically.
Now: 30s per request; up to three attempts for 429/5xx honouring
Retry-After;immediate stop naming the status for 401/403/404, which is configuration rather
than weather.
Tests
The swap now has the crash-injection case this path never had. A trigger
refuses the metadata write at the last step — after the drop and the copy, the
worst possible moment — and the previous index is asserted intact by row width
and dimension stamp, not by table existence. The technique already existed in
this repo (
migration-atomicity.test.ts) but had never been applied to thevector path, despite
db.ts's own comment describing the historical bug amid-transaction kill caused.
Three suites moved from the old contract to the new one. The partial-run test now
states the stronger guarantee: a partial run leaves the live index untouched,
rather than merely not corrupting it row by row.
One process note
Three times in this session I read output that could not have shown me
otherwise: a
grep | headwhose window was filled by unrelated matches (twice),and a CLI test suite that spawns from
dist/and therefore passed against thebuild from before the edit — printing the exact error message that had just been
deleted. Rebuilding turned 1 green into 3 red. Recorded so the next session
suspects the observation before the conclusion.
Second round: what an eight-lane review found, and what changed because of it
The first two commits were reviewed by eight independent lanes — a structured
pass, six checklist specialists, a red team, and two adversarial passes. The
mechanism held up; the semantics did not. Everything below is fixed in
23ae9434andda2412b8.The mechanism was confirmed, empirically
sleep, after
INSERT…SELECT) left the original index readable from a freshconnection with every row present and its original width.
DROP TABLEon avec0table tears down all four shadow tables plus bothautoindexes, and re-
CREATEat a different width leaves no residue — which iswhy rename was correctly rejected.
One root cause with several faces
The swap installed exactly the staging index, so any state that existed only
in the live index was treated as non-existent.
rebuild targets the live index, and the rebuild works from an entity list
snapshotted before it started. Worse silently: a memory edited mid-rebuild
had its fresh vector replaced by the older staged one, which the missing-vector
count cannot detect because the row is present. Now: still-active rows absent
from staging are carried across — same width only, since during a width change
a concurrent write is refused as a dimension mismatch.
forgetclears the live rowand knows nothing about a staging index. Now: staged rows for non-active
entities are pruned before the copy.
memesh doctorcould report health over a graph owed vectors. The swapdeleted
pending_reindexitself, pre-empting the post-rebuild measurement, soa rebuild that finished with vectors missing printed "the reindex-needed flag
was left set" and then left nothing set. The marker now has one owner.
Also fixed
--vectors, the flag this PRdeletes, in the exact situation the mechanism exists to handle. A new test
scans every advice-printing source file for a
memesh <cmd> --flagthe CLIdoes not register; it caught an older second instance on its first run,
memesh doctor --verbose, a flag that never existed.width-mismatched query raised inside sqlite-vec, the error was swallowed into
an empty result, and empty is indistinguishable from "found nothing" — so
recallclaimedmode: "hybrid",degraded: falsefor a vector side thatanswered nothing, for the entire window until a rebuild finished. Now
mode: "fts",degraded: true.BUSY_TIMEOUT_MS5000 → 30_000. The swap's row copy is the atomicityguarantee and is O(rows): measured 5.4s at 20,000 vectors, 9.1s at 30,000,
so past ~16,500 vectors a rebuild made concurrent writers fail rather than
wait — reproduced, a hook losing its capture after 5213ms.
providerFetchnow refuses redirects. A 307 forwards the POST body — theuser's memory text — and
OLLAMA_HOSTis an unvalidated env var.generationRowIdsno longer reports a failed read as "nothing staged", anumber that decides promotion and what a resume re-buys.
startedAtsurvives aresume. Both generation entry points reject a non-positive-integer width before
it reaches DDL.
Cleared by the review — stated so nobody re-spends the time
No SQL injection path in any added interpolation (all are a module constant or a
value from a fixed integer map); no secret in any new log line, and the URL is
never logged;
Retry-Afterbounded against every hostile form includingHTTP-date and negatives, worst-case sleep 60s; no timer or listener leak
(
AbortSignal.timeoutis unref'd); timeout and bad key are distinguishable. Andembedder.modelcannot mix two embedding spaces —embedWithProviderhasexactly one caller and it hardcodes
model: undefined. Two lanes reported thatlast one as critical; it is not.
Known and deliberately not in this PR
Each needs its own change and its own test:
edited between an interrupted run and its resume keeps the older vector.
20,000 still attempts the rest (worst case ~91.5s per entity), and the backoff
is linear rather than exponential.
res.json()is awaited outsideproviderFetch, so a body-phase timeout isnot retried and is not named as a timeout.
doctor.Verification
Break-test of the three swap fixes, mutation applied then written back and
re-verified —
carry-forward removed→ KILLED,staging prune removed→KILLED,
swap deletes pending_reindex→ KILLED.