From b2408cb5a6f2eb004b7bf7dbdd0d3dec22a48cdf Mon Sep 17 00:00:00 2001 From: David Snelling Date: Thu, 28 May 2026 09:45:22 -0700 Subject: [PATCH] =?UTF-8?q?feat:=20stable=20EntityIdMapper=20=E2=80=94=20r?= =?UTF-8?q?ebuild()=20no=20longer=20renumbers=20UUID=E2=86=92int?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously metadataIndex.rebuild() called idMapper.clear() which reset nextId to 1 and renumbered every UUID by re-insertion order. Any consumer that had persisted int-keyed data against the old map was silently invalidated — and 2.4.0's vector mmap store (#20), graph link compression (#21), and column-store JS↔native interchange all need persisted int indices that survive a rebuild. Remove the unconditional clear() in rebuild(). The rebuild already re-iterates every entity via idMapper.getOrAssign(uuid), which returns the existing int unchanged for known UUIDs. Stale UUID→int entries for entities no longer in storage persist as harmless memory overhead; a dedicated prune step can be added if it ever matters. clearAllIndexData() — the explicit nuclear recovery path — keeps its existing idMapper.clear() call (renumbering is intentional there), and now logs a prodLog.warn making it explicit that any persisted int-keyed data is invalidated and must be rebuilt from canonical sources. Strengthened the EntityIdMapper class JSDoc to document the stability guarantee as a contract — append-only getOrAssign, monotonic nextId, remove() leaves permanent holes, rebuild() never renumbers, only clear() does. Added tests/regression/entity-id-mapper-stability.test.ts pinning down the five-point contract: (1) single-rebuild stability; (2) many-rebuild stability; (3) post-rebuild adds get fresh monotonic ints; (4) removes leave permanent holes — new entities never recycle; (5) clearAllIndexData() explicitly renumbers (the documented destructive path). Foundation for 2.4.0 #2-#4. Full test suite (62 files, 1417 tests) green. --- src/utils/entityIdMapper.ts | 34 +++- src/utils/metadataIndex.ts | 24 ++- .../entity-id-mapper-stability.test.ts | 161 ++++++++++++++++++ 3 files changed, 209 insertions(+), 10 deletions(-) create mode 100644 tests/regression/entity-id-mapper-stability.test.ts diff --git a/src/utils/entityIdMapper.ts b/src/utils/entityIdMapper.ts index ff67a4e9..5333c025 100644 --- a/src/utils/entityIdMapper.ts +++ b/src/utils/entityIdMapper.ts @@ -1,14 +1,34 @@ /** - * EntityIdMapper - Bidirectional mapping between UUID strings and integer IDs for roaring bitmaps + * EntityIdMapper - Bidirectional mapping between UUID strings and integer IDs. * - * Roaring bitmaps require 32-bit unsigned integers, but Brainy uses UUID strings as entity IDs. - * This class provides efficient bidirectional mapping with persistence support. + * Roaring bitmaps require 32-bit unsigned integers, but Brainy uses UUID strings + * as canonical entity IDs. This class provides efficient O(1) bidirectional + * mapping with persistence — and, importantly, a stability guarantee that any + * persisted int-keyed data can rely on. + * + * **Stability guarantee (the foundation 2.4.0 vector-mmap, graph-link-compression, + * and column-store interchange all key off):** + * + * - `getOrAssign(uuid)` is **append-only**: once a UUID is assigned an int, the + * mapping never changes. Subsequent `getOrAssign` calls for the same UUID + * return the same int. + * - `nextId` is **monotonically increasing**. New UUIDs always get an int greater + * than any previously assigned, so a removed-then-re-added UUID is treated as + * a fresh entity (and gets a fresh int — there is no automatic "revive"). + * - `remove(uuid)` removes the mapping but does **not** decrement `nextId` or + * recycle the int. The removed int becomes a permanent hole in `intToUuid` — + * downstream consumers seeing `getUuid(int) === undefined` know the entity + * was deleted. + * - A metadata-index `rebuild()` does **not** clear the mapper (the rebuild path + * re-iterates entities via `getOrAssign`, which returns existing ints unchanged). + * Only the explicit `clear()` method renumbers — used by `clearAllIndexData()` + * as the nuclear recovery path with a documented warning. * * Features: - * - O(1) lookup in both directions - * - Persistent storage via storage adapter - * - Atomic counter for next ID - * - Serialization/deserialization support + * - O(1) lookup in both directions. + * - Persistent storage via storage adapter. + * - Atomic, monotonic, append-only int counter. + * - Serialization/deserialization support. * * @module utils/entityIdMapper */ diff --git a/src/utils/metadataIndex.ts b/src/utils/metadataIndex.ts index a83d2e2a..5be89566 100644 --- a/src/utils/metadataIndex.ts +++ b/src/utils/metadataIndex.ts @@ -2635,13 +2635,20 @@ export class MetadataIndexManager implements MetadataIndexProvider { this.verbCountsByTypeFixed.fill(0) this.typeFieldAffinity.clear() - // Clear EntityIdMapper + // Clear EntityIdMapper. This is the explicit destructive path: the caller + // asked for nuclear recovery of a corrupted index, so renumbering UUIDs is + // intentional. Persisted int-keyed data (vector-mmap slots, graph + // link-compression encodings) is invalidated by this op — the warning + // below makes that explicit. Rebuild on its own does NOT clear the mapper. await this.idMapper.clear() // Clear chunk manager cache this.chunkManager.clearCache() prodLog.info(`✅ Cleared ${deletedCount} field indexes and all in-memory state`) + prodLog.warn('⚠️ EntityIdMapper was cleared — any persisted int-keyed data ' + + '(vector mmap slots, graph link-compression encodings, etc.) is now stale ' + + 'and must be rebuilt from canonical sources.') prodLog.info('⚠️ Run brain.index.rebuild() to recreate the index from entity data') } @@ -3032,8 +3039,19 @@ export class MetadataIndexManager implements MetadataIndexProvider { prodLog.info(`Cleared ${existingFields.length} field indexes from storage`) } - // Clear EntityIdMapper to start fresh - await this.idMapper.clear() + // EntityIdMapper is intentionally NOT cleared here. Rebuild re-iterates + // every entity in storage and calls idMapper.getOrAssign(uuid), which + // returns the existing int for known UUIDs (no renumbering). This is the + // foundational stability guarantee — vector-mmap slot indices, graph + // link-compression encodings, and any other persisted int-keyed data + // remain valid across a rebuild. Previously this line reset nextId to 1 + // and renumbered every UUID by re-insertion order, silently breaking + // any consumer that had persisted int-keyed data against the old map. + // Stale entries for UUIDs no longer in storage persist (harmless memory + // overhead); a dedicated prune step can be added if it ever matters. + // The destructive wipe is still available via clearAllIndexData() → + // idMapper.clear(), which is the explicit "recovery" path with the + // appropriate warning about invalidating persisted int-keyed data. // Clear chunk manager cache this.chunkManager.clearCache() diff --git a/tests/regression/entity-id-mapper-stability.test.ts b/tests/regression/entity-id-mapper-stability.test.ts new file mode 100644 index 00000000..4ba2bda6 --- /dev/null +++ b/tests/regression/entity-id-mapper-stability.test.ts @@ -0,0 +1,161 @@ +/** + * Regression test: EntityIdMapper stability across rebuild. + * + * The foundation 2.4.0 (vector mmap store, graph link compression, column-store + * JS↔native interchange) all key off UUID→int mappings that **must not change** + * across a metadata-index rebuild. Previously `metadataIndex.rebuild()` called + * `idMapper.clear()` which reset `nextId` to 1 and renumbered every UUID by + * re-insertion order, silently invalidating any consumer that had persisted + * int-keyed data against the old map. + * + * This test pins down the stability contract: + * + * 1. UUID→int mappings persist across a single rebuild. + * 2. Mappings persist across many consecutive rebuilds. + * 3. New entities added after rebuild get fresh ints greater than any prior + * assignment — no collisions with existing UUIDs' ints. + * 4. Removed entities leave a permanent hole — new entities don't recycle the + * gap, even across a rebuild. + * 5. `clearAllIndexData()` is the explicit, intentional nuclear path — it DOES + * renumber. This is the only documented way to invalidate the int space, and + * a warning is logged so consumers know persisted int-keyed data is now stale. + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest' +import { Brainy } from '../../src/brainy.js' + +const DIM = 384 +const makeVec = (seed = 1) => + new Float32Array(DIM).map((_, i) => ((i + seed) % DIM) / DIM) + +describe('EntityIdMapper stability (foundation for 2.4.0)', () => { + let brain: Brainy + + beforeEach(async () => { + brain = new Brainy({ storage: { type: 'memory' }, silent: true }) + await brain.init() + }) + + afterEach(async () => { + await brain.close() + }) + + async function addEntity(name: string, seed: number): Promise { + return brain.add({ + data: name, + vector: makeVec(seed), + type: 'thing' as any, + metadata: { name } + }) + } + + function getInt(uuid: string): number | undefined { + return (brain as any).metadataIndex.idMapper.getInt(uuid) + } + + async function rebuild(): Promise { + await (brain as any).metadataIndex.rebuild() + } + + it('UUID→int mappings persist across a single metadata-index rebuild', async () => { + const ids = [ + await addEntity('a', 1), + await addEntity('b', 2), + await addEntity('c', 3), + await addEntity('d', 4), + await addEntity('e', 5) + ] + const before = ids.map(id => getInt(id)) + expect(before.every(i => typeof i === 'number' && (i as number) > 0)).toBe(true) + + await rebuild() + + const after = ids.map(id => getInt(id)) + expect(after).toEqual(before) + }) + + it('mappings stay byte-for-byte stable across many consecutive rebuilds', async () => { + const ids = [ + await addEntity('a', 1), + await addEntity('b', 2), + await addEntity('c', 3) + ] + const before = ids.map(id => getInt(id)) + + for (let i = 0; i < 5; i++) { + await rebuild() + const after = ids.map(id => getInt(id)) + expect(after).toEqual(before) + } + }) + + it('entities added after rebuild get fresh monotonic ints (no collision with existing)', async () => { + const priorIds = [ + await addEntity('a', 1), + await addEntity('b', 2), + await addEntity('c', 3) + ] + const priorInts = priorIds.map(id => getInt(id) as number) + const maxPrior = Math.max(...priorInts) + + await rebuild() + + const newId = await addEntity('d', 4) + const newInt = getInt(newId) as number + expect(newInt).toBeGreaterThan(maxPrior) + // Prior entities' ints didn't drift. + expect(priorIds.map(id => getInt(id))).toEqual(priorInts) + }) + + it('removed entities leave a permanent hole — new entities never recycle the gap', async () => { + const ids = [ + await addEntity('a', 1), + await addEntity('b', 2), + await addEntity('c', 3), + await addEntity('d', 4), + await addEntity('e', 5) + ] + const beforeInts = ids.map(id => getInt(id) as number) + const deletedId = ids[2] + const deletedInt = beforeInts[2] + const maxBefore = Math.max(...beforeInts) + + await brain.delete(deletedId) + expect(getInt(deletedId)).toBeUndefined() + + const newId = await addEntity('f', 6) + const newInt = getInt(newId) as number + expect(newInt).not.toBe(deletedInt) + expect(newInt).toBeGreaterThan(maxBefore) + + // Surviving ids keep their ints across the deletion + the add. + const survivors = ids.filter((_, i) => i !== 2) + const survivorIntsBefore = beforeInts.filter((_, i) => i !== 2) + expect(survivors.map(id => getInt(id))).toEqual(survivorIntsBefore) + + // Survivors' ints also survive a rebuild after the delete. + await rebuild() + expect(survivors.map(id => getInt(id))).toEqual(survivorIntsBefore) + // The deleted id is still gone after rebuild (no resurrection). + expect(getInt(deletedId)).toBeUndefined() + }) + + it('clearAllIndexData() is the explicit nuclear path that DOES renumber', async () => { + const id1 = await addEntity('a', 1) + const id2 = await addEntity('b', 2) + const priorInts = [getInt(id1) as number, getInt(id2) as number] + expect(priorInts.every(i => i >= 1)).toBe(true) + + // Nuclear recovery: explicit destructive op. The warning logged here is + // the only documented way to invalidate the canonical int space. + await (brain as any).metadataIndex.clearAllIndexData() + + // Both UUIDs are gone from the mapper. + expect(getInt(id1)).toBeUndefined() + expect(getInt(id2)).toBeUndefined() + + // The int counter restarted from 1: the next add() gets int 1. + const idAfter = await addEntity('c', 3) + expect(getInt(idAfter)).toBe(1) + }) +})