diff --git a/src/vectorstore/HNSWVectorStore.ts b/src/vectorstore/HNSWVectorStore.ts index 454ed8c3..a8bf9215 100644 --- a/src/vectorstore/HNSWVectorStore.ts +++ b/src/vectorstore/HNSWVectorStore.ts @@ -286,8 +286,47 @@ export class HNSWVectorStore implements VectorStore { this.dimensions = meta.dimensions; this._providerId = meta.providerId; this._modelId = meta.modelId; - this.nextHnswId = meta.nextHnswId ?? this.idToNumeric.size; } + + // The id counter must clear every numeric id that is or was in use, and the + // metadata record is not a safe sole source for it: a store can hold rows and + // a persisted graph without that record at all (a bulk run that never called + // `setMetadata` — `updateLastUpdated` then has nothing to write the counter + // into). Restoring 0 in that state reassigned live ids: `addPoint` threw + // "Node with id N already exists" for every new chunk, and each attempt had + // already overwritten the id mapping of the note that owned N. Observed on a + // resumed build: 450 graph nodes, no metadata record, 87 rows written over + // live ids. + // + // So take the high-water mark of everything that can hold an id. The + // mappings alone are not enough: `remove()` deletes a note's mappings before + // its 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. Each read is one reverse key-cursor step. + const [maxMapped, maxGraphNode] = await Promise.all([this.maxKey(ID_MAPPING_STORE), this.maxKey(GRAPH_STORE)]); + this.nextHnswId = Math.max(meta?.nextHnswId ?? 0, maxMapped + 1, maxGraphNode + 1); + } + + /** Largest numeric primary key in a store, or -1 when it is empty. */ + private async maxKey(storeName: string): Promise { + const db = this.requireDb(); + const tx = db.transaction(storeName, "readonly"); + const request = tx.objectStore(storeName).openKeyCursor(null, "prev"); + return awaitCursor(tx, request, (cursor) => ({ + done: true, + value: cursor && typeof cursor.key === "number" ? cursor.key : -1, + })); + } + + /** Vector width of the first stored row, or null for an empty store. */ + private async probeDimensions(): Promise { + const db = this.requireDb(); + const tx = db.transaction(DOCUMENTS_STORE, "readonly"); + const request = tx.objectStore(DOCUMENTS_STORE).openCursor(); + return awaitCursor(tx, request, (cursor) => ({ + done: true, + value: cursor ? ((cursor as IDBCursorWithValue).value as StoredDocument).vector.length : null, + })); } /** @@ -679,6 +718,11 @@ export class HNSWVectorStore implements VectorStore { this._providerId = providerId; this._modelId = modelId; + // A record written to repair a store that already holds rows must carry + // their width, or the next open leaves `dimensions` unset and the graph is + // never loaded until something is written. An empty store stays at 0. + if (!this.dimensions) this.dimensions = await this.probeDimensions(); + const meta: StoredMetadata = { key: "metadata", version, diff --git a/src/vectorstore/VectorStoreService.ts b/src/vectorstore/VectorStoreService.ts index 02c5e838..27887230 100644 --- a/src/vectorstore/VectorStoreService.ts +++ b/src/vectorstore/VectorStoreService.ts @@ -245,6 +245,8 @@ interface IndexInstance { validationScheduled: boolean; /** A full build (`buildFullIndex`) is in progress. Validation runs set only `progress.isIndexing`. */ isIndexing: boolean; + /** The full build in flight, so a second caller joins it instead of racing it. */ + buildPromise: Promise | null; progress: IndexingProgress; /** Timestamp (ms) when the current indexing run started, for ETA estimation */ indexingStartedAt: number | null; @@ -432,6 +434,7 @@ export class VectorStoreService { hasValidatedThisSession: false, validationScheduled: false, isIndexing: false, + buildPromise: null, progress: { isIndexing: false, total: 0, @@ -845,6 +848,28 @@ export class VectorStoreService { indexedMap.set(note.path, { mtime: note.mtime }); } + // An empty store is not a repair job, it is a first build, and the two paths + // differ in what they leave behind: only the full build writes the store's + // metadata record (the version, dimensions and the id counter — see + // `HNSWVectorStore.open`) and stamps `lastBuiltAt`. This was reachable for + // every newly added index: "Add index" runs `ensureIndex`, whose opening of + // the instance schedules this validation on a zero-delay timer, and the + // timer wins the race against `ensureIndex`'s two worker round-trips. + if (indexedMap.size === 0 && vaultFiles.length > 0) { + Logger.log(`[VectorStore] ${inst.indexId} is empty — running a full build instead of a repair`); + await this.buildFullIndex(inst, embeddings, defaultModel); + return; + } + + // A store built through the old validation path has rows but no metadata + // record, so nothing persists its dimensions or id counter. Repair that + // here, whether or not any note needs embedding, so the next open restores + // both the normal way. + if (indexedMap.size > 0 && (await inst.store.getMetadata()) === null) { + Logger.log(`[VectorStore] ${inst.indexId} has rows but no metadata record — writing it`); + await inst.store.setMetadata(defaultModel.provider, defaultModel.model, INDEX_VERSION); + } + const missingFiles: TFile[] = []; const staleFiles: TFile[] = []; const vaultPaths = new Set(); @@ -1283,16 +1308,30 @@ export class VectorStoreService { * Build the full index for a specific instance. The store has been cleared * (or is empty) when this is called, so chunks are written without purging. */ - private async buildFullIndex( + private buildFullIndex( inst: IndexInstance, embeddings: EmbeddingsInterface, model: DefaultEmbedModel, ): Promise { + // One build per instance. The startup validation and `ensureIndex` can both + // decide an empty store needs building; the second joins the first rather + // than reporting "already in progress" against its own twin. + if (inst.buildPromise) return inst.buildPromise; if (inst.isIndexing || inst.progress.isIndexing) { new Notice("Indexing already in progress..."); - return; + return Promise.resolve(); } + inst.buildPromise = this.runFullBuild(inst, embeddings, model).finally(() => { + inst.buildPromise = null; + }); + return inst.buildPromise; + } + private async runFullBuild( + inst: IndexInstance, + embeddings: EmbeddingsInterface, + model: DefaultEmbedModel, + ): Promise { inst.isIndexing = true; inst.abortController = new AbortController(); const { vault } = this.plugin.app; diff --git a/test/vectorstore/HNSWVectorStore.open.test.ts b/test/vectorstore/HNSWVectorStore.open.test.ts index 93249f0d..01d6bddb 100644 --- a/test/vectorstore/HNSWVectorStore.open.test.ts +++ b/test/vectorstore/HNSWVectorStore.open.test.ts @@ -42,6 +42,8 @@ function makeConnection() { objectStore: () => ({ getAll: () => makeReq([]), get: () => makeReq(undefined), + // `open()` reads each id store's high-water mark; null = empty. + openKeyCursor: () => makeReq(null), }), onerror: null as null | (() => void), onabort: null as null | (() => void), diff --git a/test/vectorstore/HNSWVectorStore.resume.test.ts b/test/vectorstore/HNSWVectorStore.resume.test.ts index 64a709b6..1f03761b 100644 --- a/test/vectorstore/HNSWVectorStore.resume.test.ts +++ b/test/vectorstore/HNSWVectorStore.resume.test.ts @@ -109,6 +109,60 @@ describe("HNSWVectorStore — resuming after an interrupted build", () => { await store.close(); }); + it("keeps allocating fresh ids after a reopen when no metadata record was ever written", async () => { + // The old validation path embedded into a fresh store without `setMetadata`, + // so nothing persisted the id counter. Reopening restored 0 and every new + // chunk collided with a live graph node ("Node with id N already exists"). + const first = await openStore(); // no setMetadata: rows and graph only + await first.upsert(doc("a.md", [1, 0, 0])); + await first.upsert(doc("b.md", [0, 1, 0])); + await first.flush(); + + const second = await openStore(); + await expect(second.upsert(doc("c.md", [0, 0, 1]))).resolves.toBeUndefined(); + expect(await second.count()).toBe(3); + expect(graphNodeCount(second)).toBe(3); + const hits = await second.search(new Float32Array([0, 0, 1]), 1); + expect(hits.map((h) => h.doc.path)).toEqual(["c.md"]); + // a.md still resolves to its own row: no mapping was overwritten. + const older = await second.search(new Float32Array([1, 0, 0]), 1); + expect(older.map((h) => h.doc.path)).toEqual(["a.md"]); + await second.close(); + }); + + it("clears a graph node whose mapping was lost to an interrupted removal", async () => { + // `remove()` drops mappings before rows in separate transactions; a kill in + // between leaves c's row and graph node with no mapping. Its id must still + // be off limits on the next open, or the next upsert collides with it. + const first = await openStore(); + await first.upsert(doc("a.md", [1, 0, 0])); + await first.upsert(doc("b.md", [0, 1, 0])); + await first.upsert(doc("c.md", [0, 0, 1])); // numeric id 2, the high-water mark + await first.flush(); + const dbName = (first as unknown as { dbName: string }).dbName; + await new Promise((resolve, reject) => { + const open = indexedDB.open(dbName); + open.onerror = () => reject(open.error); + open.onsuccess = () => { + const db = open.result; + const tx = db.transaction("id_mapping", "readwrite"); + tx.objectStore("id_mapping").delete(2); + tx.oncomplete = () => { + db.close(); + resolve(); + }; + tx.onerror = () => reject(tx.error); + }; + }); + + const second = await openStore(); + await expect(second.upsert(doc("d.md", [1, 1, 0]))).resolves.toBeUndefined(); + expect(await second.count()).toBe(4); + const hits = await second.search(new Float32Array([1, 1, 0]), 1); + expect(hits.map((h) => h.doc.path)).toEqual(["d.md"]); + await second.close(); + }); + it("flush() persists the pending graph immediately instead of on the debounce", async () => { const first = await openForBuild(); await first.upsert(doc("a.md", [1, 0])); diff --git a/test/vectorstore/VectorStoreService.lazyOpen.test.ts b/test/vectorstore/VectorStoreService.lazyOpen.test.ts index 9b490973..1f78aa07 100644 --- a/test/vectorstore/VectorStoreService.lazyOpen.test.ts +++ b/test/vectorstore/VectorStoreService.lazyOpen.test.ts @@ -418,4 +418,58 @@ describe("bulk embed run", () => { await startService(); expect(indexStats.documentCount).toBe(1); }); + + it("a fresh index gets exactly one full build when startup validation fires before ensureIndex", async () => { + platform.isMobile = false; + vaultFiles = [file("a.md"), file("b.md"), file("c.md")]; + const svc = await startService(); + const store = stores.get(INDEX); + if (!store) throw new Error("store not opened"); + // The real store answers over a worker round-trip, so `ensureIndex`'s reads + // land after the zero-delay validation timer registered at open. Model that + // by parking the two reads on a (fake) macrotask. + for (const method of ["getMetadata", "count"] as const) { + const original = store[method].bind(store); + (store as unknown as Record)[method] = async () => { + await new Promise((resolve) => setTimeout(resolve, 0)); + return original(); + }; + } + + const run = svc.ensureIndex(INDEX); + await vi.advanceTimersByTimeAsync(2_000); + expect(await run).toBe(true); + + // Every note embedded once — one build, not a validation run racing a build. + const embedded = embedDocuments.mock.calls.flatMap(([texts]) => texts); + expect(embedded).toHaveLength(3); + expect(await store.countNotes()).toBe(3); + // And it was the *full build*: the metadata record and the build date exist. + expect(store.meta).not.toBeNull(); + expect(indexStats.lastBuiltAt).toEqual(expect.any(Number)); + }); + + it("writes the missing metadata record of a store that has rows, even with nothing to embed", async () => { + platform.isMobile = false; + vaultFiles = [file("a.md", 1_000)]; + const svc = await startService(); + const store = stores.get(INDEX); + if (!store) throw new Error("store not opened"); + // Built through the old validation path: a row, no record. + await store.upsert({ + id: "a.md#0", + path: "a.md", + mtime: 1_000, + checksum: "x", + chunkIndex: 0, + vector: new Float32Array(3), + }); + expect(store.meta).toBeNull(); + + expect(await svc.ensureIndex(INDEX)).toBe(true); + await vi.advanceTimersByTimeAsync(1_000); + + expect(embedDocuments).not.toHaveBeenCalled(); + expect(store.meta).toMatchObject({ providerId: "fake", modelId: "embed-model" }); + }); });