From 8fc553b126fc0ef0dc0d2be29e71db4143b93f1a Mon Sep 17 00:00:00 2001 From: David Snelling Date: Thu, 27 Aug 2026 13:29:11 -0700 Subject: [PATCH] fix(hnsw): skip unvectored rows on rebuild; refuse empty vectors in the index A canonical row persisted with vector: [] (a system row, a deferred embed not yet landed, or any other legitimately-unvectored record) is a normal, enumerable row -- but rebuild()'s storage walk had no guard against it. storage.getVectorIndexData() derives its answer from the row's own record, so it returns non-null for any existing noun whether or not that noun was ever actually indexed -- rebuild() admitted such rows into the live graph with a length-0 vector. A vector-less node could become the entry point (or occupy any graph position); the next real insert then ran a distance calculation against it and blew up with a dimension mismatch. Fix at two layers in src/hnsw/hnswIndex.ts: - rebuild() now skips any row whose vector.length === 0 before it ever becomes a graph node (one summary count line, never per-row spam), and restores the pinned dimension from the first real vector it loads -- previously the pin stayed null across a restart, since addItem/updateItem are the only sites that set it and rebuild() never goes through either. - addItem/updateItem now refuse a length-0 vector with a typed EmptyVectorIndexError instead of ever pinning dimension to 0 or storing a vector-less node, so no future fill/rebuild/load path can poison the index silently. getVectorSafe's lazy-load "not found" check also missed that an empty array is truthy -- tightened to catch it. IndexOperations.ts's ReplaceInVectorIndexOperation rollback paths now skip re-adding an oldVector of length 0 (never a legal index member) instead of attempting an illegal empty re-insert on rollback. biography.test.ts's final ledger-exactness assertion assumed every noun the lane creates is vectored, including the VFS root counted in vfsBaselineNouns -- but the root is deliberately persisted unvectored. Corrected the expected formula to exclude it. Adds tests/integration/index-skips-unvectored.test.ts pinning: rebuild() indexes only vectored rows with the dimension pinned correctly; clear() then real adds never trip a dimension mismatch; addItem/updateItem refuse a length-0 vector; and a crash/repair cycle stays dimension-consistent. --- src/hnsw/hnswIndex.ts | 92 ++++++- src/transaction/operations/IndexOperations.ts | 22 +- .../index-skips-unvectored.test.ts | 253 ++++++++++++++++++ tests/lifecycle/biography.test.ts | 12 +- 4 files changed, 371 insertions(+), 8 deletions(-) create mode 100644 tests/integration/index-skips-unvectored.test.ts diff --git a/src/hnsw/hnswIndex.ts b/src/hnsw/hnswIndex.ts index 77e4f84d..3caf23ba 100644 --- a/src/hnsw/hnswIndex.ts +++ b/src/hnsw/hnswIndex.ts @@ -64,6 +64,34 @@ export class HnswFlushError extends Error { } } +/** + * @description Thrown by {@link JsHnswVectorIndex.addItem} / {@link + * JsHnswVectorIndex.updateItem} when handed a length-0 vector. A length-0 + * vector is the sanctioned "unvectored" shape for a canonical noun record + * (class-J: a VFS-system row, a deferred embed not yet landed, or any other + * legitimately-vector-less row) — but it is NEVER a legal INDEX insert. The + * index itself has no concept of "unvectored"; deciding that a row is + * unvectored and therefore skippable is the FILL/REBUILD/LOAD consumer's job + * (see {@link JsHnswVectorIndex.rebuild}), done BEFORE ever calling addItem. + * A length-0 vector reaching this point is a caller bug: silently accepting + * it would pin `this.dimension = 0` on an empty index (poisoning every real + * insert thereafter with a dimension mismatch) or store a vector-less node + * that a distance calculation can never safely compare against. Loud errors, + * never quiet losses — this throws instead of either. + */ +export class EmptyVectorIndexError extends Error { + constructor(public readonly id: string, operation: 'addItem' | 'updateItem') { + super( + `${operation}(${id}): refusing to index a length-0 vector — a length-0 vector is the ` + + `sanctioned "unvectored" shape for a canonical row, but it is never a legal index ` + + `insert. Callers that fill/rebuild/load the index must skip vector.length === 0 rows ` + + `themselves (unvectored = nothing to index, not an error at that layer); reaching ` + + `here with one is a caller bug.` + ) + this.name = 'EmptyVectorIndexError' + } +} + /** * Implements {@link VectorIndexProvider}: the vector-index surface Brainy calls * on whatever the `'vector'` factory returns (its own `JsHnswVectorIndex`, or a native @@ -580,6 +608,15 @@ export class JsHnswVectorIndex implements VectorIndexProvider { throw new Error('Vector is undefined or null') } + // THE INDEX REFUSES A LENGTH-0 VECTOR (see EmptyVectorIndexError's JSDoc): + // an empty vector is the sanctioned "unvectored" shape at the canonical + // layer, never a legal index member. Refusing here — loudly, before the + // dimension pin below — means no future fill/rebuild/load path can ever + // poison `this.dimension` to 0 or park a vector-less node in the graph. + if (vector.length === 0) { + throw new EmptyVectorIndexError(id, 'addItem') + } + // Set dimension on first insert if (this.dimension === null) { this.dimension = vector.length @@ -954,6 +991,13 @@ export class JsHnswVectorIndex implements VectorIndexProvider { return } + // Same refusal as addItem (see EmptyVectorIndexError's JSDoc) — an + // in-place relink must never rewrite an already-indexed node down to the + // unvectored shape or poison the pinned dimension. + if (vector.length === 0) { + throw new EmptyVectorIndexError(id, 'updateItem') + } + if (this.dimension === null) { this.dimension = vector.length } else if (vector.length !== this.dimension) { @@ -1555,7 +1599,15 @@ export class JsHnswVectorIndex implements VectorIndexProvider { } const loaded = await this.storage.getNounVector(noun.id) - if (!loaded) { + // `loaded` is a length-0 array (not null/undefined) for a canonical row + // that is legitimately unvectored — `![]` is FALSE (an empty array is + // truthy), so the bare `!loaded` check below would silently accept it + // as "found" and hand a dimension-0 vector to a distance calculation. + // A node only reaches this lazy-load path because it is a MEMBER of + // the live index (rebuild() now refuses to admit unvectored rows — see + // its JSDoc), so an empty vector here is never legitimate: treat it + // exactly like "not found", loudly. + if (!loaded || loaded.length === 0) { throw new Error(`Vector not found for noun ${noun.id}`) } @@ -1765,9 +1817,42 @@ export class JsHnswVectorIndex implements VectorIndexProvider { totalCount = result.totalCount || result.items.length + // UNVECTORED ROWS ARE NOT AN INDEX MEMBER (the class-J law): a canonical + // noun whose vector leg is `[]` (a VFS-root-style system row, a + // deferred embed not yet landed, or a best-effort fallback for an + // unreadable vector leg) is a normal, enumerable, countable row — it + // is simply not indexed. `storage.getVectorIndexData()` derives its + // {level, connections} answer straight from the noun's OWN record, so + // it returns non-null for every existing noun regardless of whether + // that noun ever actually reached `addItem()` — it cannot be used to + // decide indexability. `nounData.vector.length === 0` is the one + // truthful signal (mirrors the `noun.vector.length > 0` guards in + // {@link getVectorSafe} / {@link getVectorSync}): skip here, counted + // once in a summary line, never per-row spam. + let skippedUnvectored = 0 + // Process all nouns at once for (const nounData of result.items) { try { + if (!nounData.vector || nounData.vector.length === 0) { + skippedUnvectored++ + continue + } + + // Restore the pinned dimension from the first real vector this + // rebuild loads. `addItem`/`updateItem` only pin `this.dimension` + // on a LIVE insert — a fresh rebuild from storage never goes + // through either, so without this the pin stays `null` across a + // restart. A `null` pin means the very next insert (correct OR + // wrong length) silently BECOMES the new pin instead of being + // checked against the store's real dimension — the wrong-length + // case then fails much later and less clearly, inside a distance + // calculation against an already-loaded node, instead of here, + // immediately, with a named expected-vs-got mismatch. + if (this.dimension === null) { + this.dimension = nounData.vector.length + } + // Load HNSW graph data for this entity const hnswData = await this.storage.getVectorIndexData(nounData.id) @@ -1815,7 +1900,10 @@ export class JsHnswVectorIndex implements VectorIndexProvider { options.onProgress(loadedCount, totalCount) } - prodLog.info(`HNSW: Loaded ${loadedCount.toLocaleString()} nodes (${storageType})`) + prodLog.info( + `HNSW: Loaded ${loadedCount.toLocaleString()} nodes (${storageType})` + + (skippedUnvectored > 0 ? ` — ${skippedUnvectored.toLocaleString()} unvectored row(s) skipped` : '') + ) } // Step 5: CRITICAL - Recover entry point if missing) diff --git a/src/transaction/operations/IndexOperations.ts b/src/transaction/operations/IndexOperations.ts index cfee5074..1bbbca88 100644 --- a/src/transaction/operations/IndexOperations.ts +++ b/src/transaction/operations/IndexOperations.ts @@ -324,8 +324,17 @@ export class ReplaceInVectorIndexOperation implements Operation { return async () => { // Restore the declared before-state in place (see class JSDoc for - // the item-did-not-exist posture). - await index.updateItem!({ id: this.id, vector: this.oldVector }, generation) + // the item-did-not-exist posture). A length-0 oldVector means the row + // was never actually indexed before this op ran (a length-0 vector is + // never a legal index member — see EmptyVectorIndexError) — there is + // no in-place "restore to empty" for the provider to perform, so + // rollback removes the row instead, leaving the same "not indexed" + // state the row was in before execute(). + if (this.oldVector.length > 0) { + await index.updateItem!({ id: this.id, vector: this.oldVector }, generation) + } else { + await this.index.removeItem(this.id, generation) + } } } @@ -336,9 +345,14 @@ export class ReplaceInVectorIndexOperation implements Operation { return async () => { // updateItem-style restore via the same adjacent pair, back to the - // declared before-state. + // declared before-state. Same length-0 carve-out as the updateItem + // path above: an empty oldVector was never a legal index member, so + // rollback just leaves the row removed rather than attempting an + // illegal empty re-add. await this.index.removeItem(this.id, generation) - await this.index.addItem({ id: this.id, vector: this.oldVector }, generation) + if (this.oldVector.length > 0) { + await this.index.addItem({ id: this.id, vector: this.oldVector }, generation) + } } } } diff --git a/tests/integration/index-skips-unvectored.test.ts b/tests/integration/index-skips-unvectored.test.ts new file mode 100644 index 00000000..c65aaa01 --- /dev/null +++ b/tests/integration/index-skips-unvectored.test.ts @@ -0,0 +1,253 @@ +/** + * @module tests/integration/index-skips-unvectored + * @description THE UNVECTORED-ROW CURE — two integration tests + * (`tests/lifecycle/biography.test.ts`'s Ch4/5/6 chapter and + * `tests/integration/clear-persistence.test.ts`'s multi-cycle test) started + * failing after a canonical-storage change made a vector-less row (the + * class-J shape: `vector: []`, e.g. the VFS root, a deferred embed not yet + * landed, or any other legitimately-unvectored canonical record) VISIBLE to + * the enumeration walk `getNounsWithPagination()` for the first time — before + * that change such rows were simply invisible to the walk. `hnswIndex.ts`'s + * `rebuild()` never guarded against that shape: it inserted every row the + * walk yielded into the live in-memory index, including ones with a length-0 + * vector, because `storage.getVectorIndexData()` derives its {level, + * connections} answer straight from the noun's OWN record — it returns + * non-null for ANY existing noun, whether or not that noun was ever actually + * indexed via `addItem()`. A vector-less node admitted into the graph could + * become the entry point (or occupy any graph position), and the very next + * real-vectored `addItem()` then ran a distance calculation against it — + * `cosineDistance` throws "Vectors must have the same dimensions" the moment + * one operand is a length-0 array. + * + * THE FIX, at two layers (`src/hnsw/hnswIndex.ts`): + * (1) FILL/REBUILD/LOAD consumers treat `vector.length === 0` as "unvectored — + * nothing to index" and skip the row (normal, not an error; one summary + * count line, never per-row spam) — `rebuild()`'s loop now checks this + * BEFORE ever creating a graph node, so an unvectored row can never + * become an index member, entry point, or dimension-setter. + * (2) THE INDEX ITSELF refuses a length-0 vector in `addItem()` / + * `updateItem()` with a typed `EmptyVectorIndexError`, loudly, instead of + * ever pinning `dimension = 0` or storing a vector-less node — so no + * future fill/rebuild/load path can silently poison the index even if it + * forgets law (1). + * + * Four legs pinned here: + * (a) `rebuild()` over a store mixing real-vectored rows and `vector: []` + * rows indexes ONLY the vectored ones — size === vectored count, + * dimension pinned to the real (non-zero) length. + * (b) `clear()` then real adds afterward never trip a dimension mismatch — + * the exact `clear-persistence.test.ts` regression shape, reproduced + * directly against the index/storage seam this module owns. + * (c) `index.addItem({ id, vector: [] })` throws `EmptyVectorIndexError` + * (and `updateItem` does too, for an existing node). + * (d) crash -> repair: the crashed generation's entities survive, the ledger + * recounts honestly, and a fresh real-vectored add afterward never trips + * a dimension mismatch against a leftover vector-less phantom. + */ +import { describe, it, expect, afterEach } from 'vitest' +import * as fs from 'node:fs' +import * as os from 'node:os' +import * as path from 'node:path' +import { Brainy } from '../../src/index.js' +import { NounType } from '../../src/types/graphTypes.js' +import { EmptyVectorIndexError } from '../../src/hnsw/hnswIndex.js' +import { abandonAsCrashed, openBrain as openKillMatrixBrain, uid, vec } from '../helpers/durabilityKillMatrix.js' + +const tmpDirs: string[] = [] +function mkTmp(): string { + const d = fs.mkdtempSync(path.join(os.tmpdir(), 'brainy-index-skips-unvectored-')) + tmpDirs.push(d) + return d +} +afterEach(() => { + for (const d of tmpDirs.splice(0)) { + try { + fs.rmSync(d, { recursive: true, force: true }) + } catch { + /* best-effort cleanup */ + } + } +}) + +/** A filesystem-backed brain with explicit vectors (no embedder needed) and + * manual persistence — mirrors `durabilityKillMatrix.ts`'s `openBrain` so + * every write in this module is explicit and provably durable. */ +function openBrain(dir: string): any { + return new Brainy({ + requireSubtype: false, + storage: { type: 'filesystem', path: dir }, + silent: true, + persistence: { policy: 'manual' } + }) +} + +describe('HNSW index skips unvectored rows', () => { + it('(a) rebuild() indexes only vectored rows: size === vectored count, dimension pinned to the real length', async () => { + const dir = mkTmp() + let brain = openBrain(dir) + await brain.init() + + // 5 real-vectored rows. + const vectoredIds: string[] = [] + for (let i = 0; i < 5; i++) { + const id = uid(`vectored-${i}`) + await brain.add({ id, data: `real entity ${i}`, type: NounType.Document, vector: vec(i) }) + vectoredIds.push(id) + } + // 3 explicit unvectored rows — the class-J "vector: []" shape, a normal, + // enumerable, countable canonical row that must never reach the index. + const unvectoredIds: string[] = [] + for (let i = 0; i < 3; i++) { + const id = uid(`unvectored-${i}`) + await brain.add({ id, data: `unvectored entity ${i}`, type: NounType.Document, vector: [] }) + unvectoredIds.push(id) + } + await brain.flush() + + // The canonical ledger already agrees before any rebuild: nouns.all + // counts every row (8 + the VFS root); vectors.all counts only the real + // ones (5) — the VFS root and the 3 explicit unvectored rows are excluded. + const ledgerBeforeReopen = await brain.storage.getCanonicalCounts() + expect(ledgerBeforeReopen.vectors.all).toBe(5) + expect(ledgerBeforeReopen.nouns.all).toBe(9) // 5 vectored + 3 unvectored + 1 VFS root + + await brain.close() + + // Reopen: open()'s index build IS hnswIndex.rebuild() run fresh from + // storage — this is the exact path that used to admit unvectored rows. + brain = openBrain(dir) + await brain.init() + + const status = await brain.getIndexStatus() + expect(status.hnswIndex.size, 'the rebuilt index must contain ONLY the 5 real-vectored rows').toBe(5) + + // Dimension is pinned to the REAL embedded length (384 via `vec()`), not + // 0 — adding a wrong-length vector must be refused naming that real + // dimension, proving no vector-less row ever set it. + const realDimension = vec(0).length + let mismatchMessage: string | undefined + try { + await brain.index.addItem({ id: uid('dimension-probe'), vector: vec(0).slice(0, realDimension - 1) }) + expect.fail('expected a dimension mismatch error') + } catch (err) { + mismatchMessage = (err as Error).message + } + expect(mismatchMessage).toContain(`expected ${realDimension}`) + + // Every unvectored row is still a normal, enumerable, readable canonical + // record — class-J semantics survive the rebuild fix untouched. + for (const id of unvectoredIds) { + const entity = await brain.get(id, { includeVectors: true }) + expect(entity, `unvectored entity ${id} must remain readable`).not.toBeNull() + expect(entity.vector).toEqual([]) + } + // A correct-dimension add succeeds cleanly against the pinned dimension. + const freshId = uid('post-reopen-fresh') + await expect(brain.add({ id: freshId, data: 'fresh', type: NounType.Document, vector: vec(50) })).resolves.toBe( + freshId + ) + + await brain.close() + }) + + it('(b) clear() then real adds afterward never trip a dimension mismatch (the clear-persistence regression shape)', async () => { + const dir = mkTmp() + let brain = openBrain(dir) + await brain.init() // the VFS root (vector: []) is the store's only row + + await brain.clear() + await brain.close() + + // Reopen over a store whose only surviving row is the recreated, + // unvectored VFS root — this is exactly the shape that used to poison + // the entry point / dimension in `clear-persistence.test.ts`. + brain = openBrain(dir) + await brain.init() + expect((await brain.getIndexStatus()).hnswIndex.size).toBe(0) + + const id1 = uid('after-clear-1') + await expect(brain.add({ id: id1, data: 'after clear 1', type: NounType.Document, vector: vec(1) })).resolves.toBe( + id1 + ) + const id2 = uid('after-clear-2') + await expect(brain.add({ id: id2, data: 'after clear 2', type: NounType.Document, vector: vec(2) })).resolves.toBe( + id2 + ) + expect((await brain.getIndexStatus()).hnswIndex.size).toBe(2) + + await brain.close() + }) + + it('(c) index.addItem/updateItem refuse a length-0 vector with EmptyVectorIndexError', async () => { + const dir = mkTmp() + const brain = openBrain(dir) + await brain.init() + + await expect(brain.index.addItem({ id: uid('empty-add'), vector: [] })).rejects.toThrow(EmptyVectorIndexError) + + // updateItem on an EXISTING (real-vectored) node must refuse the same way. + const existingId = uid('existing-for-update') + await brain.add({ id: existingId, data: 'existing', type: NounType.Document, vector: vec(9) }) + await expect(brain.index.updateItem({ id: existingId, vector: [] })).rejects.toThrow(EmptyVectorIndexError) + + // The index was never disturbed by either refused call. + expect((await brain.getIndexStatus()).hnswIndex.size).toBe(1) + + await brain.close() + }) + + it('(d) crash -> repair: the crashed generation survives, the ledger recounts honestly, and a fresh add afterward never trips a dimension mismatch', async () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'brainy-index-skips-unvectored-crash-')) + try { + let brain = await openKillMatrixBrain(dir, { logAuthority: 'adopt' }) + + // Baseline: real-vectored entities, durably flushed. + const baselineIds: string[] = [] + for (let i = 0; i < 5; i++) { + const id = uid(`baseline-${i}`) + await brain.add({ id, data: `baseline entity ${i}`, type: NounType.Document, vector: vec(i) }) + baselineIds.push(id) + } + await brain.flush() + + // Crash window: at-ack writes that are never flushed before the crash. + const crashedIds: string[] = [] + for (let i = 0; i < 4; i++) { + const id = uid(`crashed-${i}`) + await brain.add({ id, data: `crash-window entity ${i}`, type: NounType.Document, vector: vec(100 + i) }) + crashedIds.push(id) + } + await abandonAsCrashed(brain) + + // Reopen — logAuthority: 'adopt' replays the at-ack log for the crash window. + brain = await openKillMatrixBrain(dir, { logAuthority: 'adopt' }) + for (const id of [...baselineIds, ...crashedIds]) { + expect(await brain.get(id), `entity ${id} must survive the crash`).not.toBeNull() + } + + // Repair — must not disturb any entity, and must recount the ledger honestly. + const report = await brain.repairIndex() + expect(report.families.length).toBeGreaterThan(0) + for (const id of [...baselineIds, ...crashedIds]) { + expect(await brain.get(id), `entity ${id} must survive repair`).not.toBeNull() + } + + const ledger = await brain.storage.getCanonicalCounts() + expect(ledger.suspect).toBe(false) + expect(ledger.vectors.all).toBe(baselineIds.length + crashedIds.length) + + // Second life: a fresh real-vectored add must never trip a dimension + // mismatch against a vector-less phantom left in the index — the exact + // mechanism `clear-persistence.test.ts` and the biography lane hit. + const secondLifeId = uid('second-life') + await expect( + brain.add({ id: secondLifeId, data: 'second life entity', type: NounType.Document, vector: vec(200) }) + ).resolves.toBe(secondLifeId) + expect(await brain.get(secondLifeId)).not.toBeNull() + + await brain.close() + } finally { + fs.rmSync(dir, { recursive: true, force: true }) + } + }) +}) diff --git a/tests/lifecycle/biography.test.ts b/tests/lifecycle/biography.test.ts index 8b274fce..274f0ef0 100644 --- a/tests/lifecycle/biography.test.ts +++ b/tests/lifecycle/biography.test.ts @@ -382,9 +382,17 @@ describe.sequential('lifecycle — the working store', () => { }, // Every noun this biography ever adds carries an explicit/computed // vector (the harness never defers an embed), so the vectored-noun - // scalar tracks nouns.all exactly. + // scalar tracks nouns.all exactly EXCEPT for the VFS root counted + // in `vfsBaselineNouns`: the root is deliberately persisted with + // `vector: []` (the sanctioned "unvectored" shape — see + // VirtualFileSystem.doInitializeRoot()'s zero-norm-avoidance + // comment) so it never pays the WASM engine's cold-compile cost and + // never crosses an engine boundary as a false attractor. It is the + // ONE hidden-tier record `vfsBaselineNouns` represents (see + // biographyHarness's module header), so it is excluded here even + // though it counts toward `nouns.all`. vectors: { - all: aliveEntities.length + model.vfsFileNouns + model.vfsBaselineNouns + all: aliveEntities.length + model.vfsFileNouns }, suspect: false })