Repository navigation
fix(vectorstore): fresh index builds raced startup validation; id counter survives a missing metadata record - #491
Merged
Merged
Conversation
Contributor
Greptile SummaryThis PR prevents interrupted or startup-raced vector index builds from reusing persisted HNSW node IDs and coordinates concurrent requests for the same full build.
Confidence Score: 5/5The PR appears safe to merge; no actionable correctness, security, or repository-rule issue remains. Both previous findings are fully addressed in the current code: ID restoration includes persisted graph keys, and metadata repair runs for every nonempty metadata-less store. The new build promise also makes the startup validator and Important Files Changed
Sequence DiagramsequenceDiagram
participant E as ensureIndex
participant V as Startup validation
participant B as buildFullIndex
participant S as Vector store
V->>S: Inspect rows and metadata
E->>S: Inspect metadata and count
V->>B: Request full build for empty store
B->>B: Store buildPromise
E->>B: Request same full build
B-->>E: Return existing buildPromise
B->>S: Embed rows, flush graph, write metadata
B-->>V: Build completes
B-->>E: Shared build completes
Reviews (3): Last reviewed commit: "fix(vectorstore): count persisted graph ..." | Re-trigger Greptile |
Leo310
added a commit
that referenced
this pull request
Sep 13, 2026
…r mark; repair metadata unconditionally Review follow-ups for #491: - `remove()` drops mappings before rows in separate transactions, so an interrupted removal leaves a row and a persisted graph node with no mapping, and `loadGraph` keeps a node whose row exists. The counter now also clears the graph store's largest key; each read is one reverse key-cursor step. `setMetadata` probes the row width when the store already holds rows, so a repair record restores dimensions on the next open. - The metadata repair runs whenever a non-empty store lacks the record, not only when something needs embedding.
…nter survives a missing metadata record A newly added index could be built by the startup validation instead of the full build: "Add index" runs `ensureIndex`, whose opening of the instance schedules validation on a zero-delay timer, and that timer wins against `ensureIndex`'s two worker round-trips. The validation path never writes the store's metadata record, so nothing persisted the id counter or dimensions, and `lastBuiltAt` was never stamped. After a cancel and a reload the store reopened with the counter at 0 and every new chunk hit "Node with id N already exists" — with the id mapping of the note that owned N already overwritten. Observed live: 450 graph nodes, no metadata record, 87 rows written over live ids. - `HNSWVectorStore.open` takes the counter as the max of the metadata high-water mark and the loaded id mappings, so no persisted state can reset it below an id in use. - Validation on an empty store runs the full build (metadata, build date, report) instead of a repair; `buildFullIndex` keeps one build per instance and a second caller joins it, so `ensureIndex` no longer reports "already in progress" against its own twin. - Validation writes the metadata record when a store has rows but none, repairing indexes built through the old path.
…r mark; repair metadata unconditionally Review follow-ups for #491: - `remove()` drops mappings before rows in separate transactions, so an interrupted removal leaves a row and a persisted graph node with no mapping, and `loadGraph` keeps a node whose row exists. The counter now also clears the graph store's largest key; each read is one reverse key-cursor step. `setMetadata` probes the row width when the store already holds rows, so a repair record restores dimensions on the next open. - The metadata repair runs whenever a non-empty store lacks the record, not only when something needs embedding.
Leo310
force-pushed
the
fix/fresh-index-build-race
branch
from
September 13, 2026 15:11
0a17d2f to
73170f2
Compare
Leo310
added a commit
that referenced
this pull request
Sep 14, 2026
Stability release for the Obsidian 1.13 installer (Electron 43 / Node 24): the renderer crash on the first chat send (#492) and the transport connection error (#501), plus the streaming render coalescing (#484), the checkpoint callbacks strip (#483), and the indexing fixes since 2.1.0 (#486, #487, #490, #491, #493, #495, #497, #498, #499, #500). Co-Authored-By: Claude <noreply@anthropic.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
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.
Found during the live test of #490: after cancelling a build on a newly added index and reloading Obsidian, every note in the resumed run failed with
Node with id N already exists.Root cause
The live database had 450 graph nodes, 537 rows, a graph header, and no
metadatarecord. Without itHNSWVectorStore.openleft the id counter at 0, so the resumed run reallocated ids into the existing graph. Each failed insert had already overwritten the id mapping of the note that owned that id, so the index is corrupted beyond what a resume can repair.Which run writes rows without metadata? The startup validation. "Add index" calls
ensureIndex, which opens the instance; opening schedules validation on a zero-delay timer, and that timer fires beforeensureIndexgets through itsgetMetadataandcountworker round-trips. Validation sees an empty store, embeds everything throughembedFilesInBatches, and the full build thatensureIndexthen attempts bails with "Indexing already in progress". Only the full build callssetMetadata, andupdateLastUpdatedhas no record to persist the counter into. The index also never gets alastBuiltAt.What changed
opensets the counter to the max of the metadata high-water mark and the loaded id mappings, so a missing or stale record can never reset it below an id in use.buildFullIndexinstead of a repair.buildFullIndexkeeps one build promise per instance; a second caller joins it, so the modal'sensureIndexand the scheduled validation no longer race or notice against each other.Tests
HNSWVectorStore.resume.test.ts: rows and graph written with no metadata record, reopen, upsert succeeds and both old and new notes resolve to their own rows.VectorStoreService.lazyOpen.test.ts: with the store's reads parked on a macrotask so validation fires first, a fresh index gets exactly one build, the metadata record exists, andlastBuiltAtis stamped.Both fail on
mainand pass here.bun run check,format,lint,test(1677 passing) and a dev build are clean; the bundle loads in a live vault without errors.Note for anyone who hit this: an index that already collided needs Rebuild index; the overwritten mappings cannot be reconstructed.