From 1a09be0628f49978369ad2a1b7a7862f6e965d9d Mon Sep 17 00:00:00 2001 From: David Snelling Date: Mon, 3 Aug 2026 11:57:32 -0700 Subject: [PATCH] =?UTF-8?q?fix:=20user=20metadata=20named=20'level'=20is?= =?UTF-8?q?=20a=20real=20field=20everywhere=20=E2=80=94=20the=20engine-int?= =?UTF-8?q?ernal=20node=20layer=20no=20longer=20shadows=20it=20in=20sort/f?= =?UTF-8?q?ilter/aggregation,=20and=20the=20indexing=20views=20stop=20stam?= =?UTF-8?q?ping=20a=20phantom=200=20into=20its=20column;=20index=20epoch?= =?UTF-8?q?=202=20rebuilds=20existing=20brains=20at=20first=20open?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Also completes the v8.10.2 write-granularity law for the transact() plan path: a metadata-only batch update never rewrites the vector-bearing noun record (planUpdate staged the unconditional save the update() fix removed). Seven pins in tests/integration/level-field-shadow.test.ts including the reporting consumer's exact repro rows; orderBy JSDoc documents the ordering contract and the announced field-addressing law. --- RELEASES.md | 55 +++++++ src/brainy.ts | 33 ++-- src/coreTypes.ts | 7 +- src/storage/brainFormat.ts | 7 +- src/types/brainy.types.ts | 18 ++- tests/integration/level-field-shadow.test.ts | 147 ++++++++++++++++++ tests/integration/orderby-sort-bug.test.ts | 5 +- tests/unit/brainy/migration-deference.test.ts | 4 +- 8 files changed, 259 insertions(+), 17 deletions(-) create mode 100644 tests/integration/level-field-shadow.test.ts diff --git a/RELEASES.md b/RELEASES.md index 2e137e4f..41d99dd9 100644 --- a/RELEASES.md +++ b/RELEASES.md @@ -74,6 +74,61 @@ to the caller today. on CI**, triggered by the release tag, instead of PUTting the tarball from the laptop over WAN — no change to what gets published or how a consumer installs it. +## Unreleased (natural field names stop colliding with engine internals) + +From a production report: sorting by a user metadata field named `level` silently +returned insertion order — the engine's internal HNSW node layer (also called +`level`) shadowed the user's field in every by-name read, and the indexing path +stamped a hardcoded `0` into the same index column (multi-valued poison). `level` +is a perfectly natural field name (game characters, priorities, floors); the +engine was wrong, not the caller. + +- **`level` is user data now, everywhere.** Engine plumbing no longer resolves by + name, never shadows metadata, and never enters the indexed views. `orderBy: + 'level'`, `where: { level: 9 }`, `groupBy: ['level']` all read YOUR field. + Regression pins: `tests/integration/level-field-shadow.test.ts` (the reporting + consumer's exact repro rows). +- **Index epoch 2.** The derived posting set changed, so every existing brain + rebuilds its metadata index from canonical at first open — poisoned columns + heal automatically; no manual step. First open after upgrade pays one rebuild + (observable via `getIndexStatus()`); pair this release with the same-day + native-accelerator release, which makes `level` indexable on the native path. +- **`transact()` metadata-only updates stop rewriting the vector record** — the + v8.10.2 write-granularity law now covers the batch/plan path too (it was + fixed for `update()` but the transact plan builder still staged the + unconditional save). If you batch stat touches through `transact()`, this is + your write-amplification fix. +- Coming next (announced so parsers and call sites can prepare): one + field-addressing law — bare names = user metadata, `system.` for + engine fields, typed refusals for unresolvable names. Ships as its own + release with a migration advisory; nothing changes in this release. + +--- + +## v8.10.2 — 2026-07-29 (metadata-only updates stop rewriting the vector record) + +From a production incident on a large deployment: a read-heavy sweep that bumped +per-entity stats (metadata-only `update()` calls) saturated the disk — 5.8GB written +in 40 minutes — because every `update()` unconditionally re-persisted the WHOLE noun +record, unchanged vector included, fsynced. + +- **`update()` write granularity fixed at the core.** A metadata-only update (no new + `data`, `vector`, or `type`) now writes the metadata leg and index deltas ONLY — + the vector-bearing noun record is never rewritten. Vector-side writes and HNSW + reindexing still happen exactly when the vector side actually changed. Regression + pins: `tests/integration/update-write-granularity.test.ts`. +- **Consumer guidance:** per-entity stat touches are now cheap, but batch them anyway + (one `transact()` instead of N `update()` calls) — granularity fixes the cost per + touch; batching fixes the count. +- Idle VFS `PathResolver` no longer logs `NaN% hit rate` once a minute (stats log + only on new traffic, at debug level). +- Native graph providers' `graph-lsm-*` storage keys are recognized as system + resources — the per-boot `Unknown key format` warning for them is gone. + +Pairs with the native accelerator's same-day patch release; adopt as one bump. + +--- + ## v8.10.1 — 2026-07-24 (the no-hot-retry contract + warm()'s metadata surface under native providers) From a production incident: a native-provider op ground 38-40s inside a transaction, diff --git a/src/brainy.ts b/src/brainy.ts index 5bb77d05..d8eca08b 100644 --- a/src/brainy.ts +++ b/src/brainy.ts @@ -2123,11 +2123,13 @@ export class Brainy implements BrainyInterface { // If undefined values are included as explicit keys, extractIndexableFields indexes // them as '__NULL__' entries that removeFromIndex can never clean up (storageMetadata // omits those keys entirely via conditional spreading, so the fields don't match). + // No `level` here: engine plumbing never enters the indexing view — a + // hardcoded level:0 landed in the SAME flattened index column as user + // metadata named `level`, poisoning it multi-valued ([0, real]). const entityForIndexing = { id, vector, connections: new Map(), - level: 0, type: params.type, ...(params.subtype !== undefined && { subtype: params.subtype }), ...(params.visibility !== undefined && @@ -3102,12 +3104,13 @@ export class Brainy implements BrainyInterface { }) } - // Build entity structure for metadata index (with top-level fields) + // Build entity structure for metadata index (with top-level fields). + // No `level`: engine plumbing never enters the indexing view (it + // poisoned the flattened user `level` column — VENUE-BRAINY-ORDERBY-NOOP). const entityForIndexing = { id: params.id, vector, connections: new Map(), - level: 0, type: params.type || existing.type, subtype: params.subtype !== undefined ? params.subtype : existing.subtype, ...(((params.visibility ?? existing.visibility) ?? 'public') !== 'public' && { @@ -9377,7 +9380,7 @@ export class Brainy implements BrainyInterface { id, vector, connections: new Map(), - level: 0, + // no `level` — plumbing never enters the indexing view type: params.type, ...(params.subtype !== undefined && { subtype: params.subtype }), ...(params.visibility !== undefined && @@ -9528,7 +9531,7 @@ export class Brainy implements BrainyInterface { id: params.id, vector, connections: new Map(), - level: 0, + // no `level` — plumbing never enters the indexing view type: params.type || existing.type, subtype: params.subtype !== undefined ? params.subtype : existing.subtype, ...(((params.visibility ?? existing.visibility) ?? 'public') !== 'public' && { @@ -9556,16 +9559,22 @@ export class Brainy implements BrainyInterface { } plan.operations.push( - new UpdateNounMetadataOperation(this.storage, params.id, updatedMetadata), - new SaveNounOperation(this.storage, { - id: params.id, - vector, - connections: new Map(), - level: 0 - }) + new UpdateNounMetadataOperation(this.storage, params.id, updatedMetadata) ) + // Noun-record write + HNSW reindex ONLY when the vector side actually + // changed — the same write-granularity law as update(): a metadata-only + // patch must never rewrite the whole vector record. This plan path is the + // one transact() updates ride, so an unconditional save here would + // re-open the read-sweep disk-saturation amplifier for exactly the + // consumers batching their stat touches through transact(). if (needsReindexing) { plan.operations.push( + new SaveNounOperation(this.storage, { + id: params.id, + vector, + connections: new Map(), + level: 0 + }), new RemoveFromVectorIndexOperation(this.index, params.id, existing.vector), new AddToVectorIndexOperation(this.index, params.id, vector) ) diff --git a/src/coreTypes.ts b/src/coreTypes.ts index e0248d17..90fc4462 100644 --- a/src/coreTypes.ts +++ b/src/coreTypes.ts @@ -284,7 +284,12 @@ export const STANDARD_ENTITY_FIELDS: ReadonlySet = new Set([ 'id', 'vector', 'connections', - 'level', + // 'level' is deliberately ABSENT: it is HNSW plumbing, not an entity field. + // Listing it here made every by-name read of a user metadata field called + // `level` resolve to the engine's internal node layer instead — a silent + // shadow that broke sort/filter/aggregation on a perfectly natural field + // name (VENUE-BRAINY-ORDERBY-NOOP). Engine plumbing is invisible to the + // query surface; a bare `level` reads `entity.metadata.level`. 'type', 'subtype', 'visibility', diff --git a/src/storage/brainFormat.ts b/src/storage/brainFormat.ts index 2e6488e9..a1241fe0 100644 --- a/src/storage/brainFormat.ts +++ b/src/storage/brainFormat.ts @@ -69,7 +69,12 @@ export const BRAIN_FORMAT_PATH = '_system/brain-format.json' * (the 8.0 GA baseline). An on-disk `indexEpoch` that differs from this — or an * absent marker — triggers a full derived-index rebuild on open. */ -export const EXPECTED_INDEX_EPOCH = 1 +// Epoch 2 (2026-08-03, paired with the native accelerator's same-day release): +// user metadata fields named `level` become indexable on both engines — the +// derived posting set changed, so every pre-fix brain must rebuild its +// metadata index from canonical at first open (poisoned multi-valued `level` +// columns heal through this rebuild; no bespoke heal path). +export const EXPECTED_INDEX_EPOCH = 2 /** * @description The data-layer format string this build writes and runs as. diff --git a/src/types/brainy.types.ts b/src/types/brainy.types.ts index b5f286ed..89be78b9 100644 --- a/src/types/brainy.types.ts +++ b/src/types/brainy.types.ts @@ -551,7 +551,23 @@ export interface FindParams { cursor?: string // Cursor-based pagination // Sorting - orderBy?: string // Field to sort by (e.g., 'createdAt', 'title', 'metadata.priority') + /** + * Field to sort by. User metadata fields sort by their stored values — + * including natural names like `level`, `rank`, or `score` (an engine-internal + * field can never shadow your metadata; fixed 2026-08 after a production + * report). System timestamps (`createdAt`, `updatedAt`) sort by entity age. + * + * Ordering contract (identical on the pure-JS engine and the native + * accelerator): entities missing the field sort LAST in both directions — + * they are never dropped from the result; ties break deterministically. + * + * NOTE — the field-addressing law is changing (announced 2026-08): bare + * names will mean user metadata ALWAYS, and system fields will be reached + * explicitly as `system.` (e.g. `system.createdAt`), with typed + * refusals for unresolvable names. Until that release, bare `createdAt` + * and friends keep resolving to the system fields as documented above. + */ + orderBy?: string order?: 'asc' | 'desc' // Sort direction: 'asc' (default) or 'desc' // Advanced options diff --git a/tests/integration/level-field-shadow.test.ts b/tests/integration/level-field-shadow.test.ts new file mode 100644 index 00000000..d50593ff --- /dev/null +++ b/tests/integration/level-field-shadow.test.ts @@ -0,0 +1,147 @@ +/** + * @module tests/integration/level-field-shadow + * @description The reserved-name shadow fix (VENUE-BRAINY-ORDERBY-NOOP, + * 2026-08-03): `level` is HNSW plumbing, not an entity field — it must never + * shadow user metadata of the same name. Pre-fix, STANDARD_ENTITY_FIELDS + * listed `level`, so every by-name read returned the engine's internal 0 + * (all-equal → stable sort → insertion order, silently), and the indexing + * views stamped level:0 into the same flattened column as user values + * (multi-valued [0, real] poison). Laws: + * (1) venue's exact repro sorts: three adds with metadata.level 3/9/6 → + * find({orderBy:'level'}) returns 9,6,3 desc and 3,6,9 asc; + * (2) where {level: N} matches through filter AND egress guard; + * (3) the index column carries the user value only (no 0 poison); + * (4) update() keeps `level` readable (the update indexing view is clean too); + * (5) the transact() update path never rewrites the noun record on a + * metadata-only patch (the planUpdate granularity completion). + */ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest' +import { Brainy } from '../../src/brainy.js' +import { NounType } from '../../src/types/graphTypes.js' +import { EXPECTED_INDEX_EPOCH } from '../../src/storage/brainFormat.js' + +const stubEmbedding = async (text: string): Promise => { + const hash = text.split('').reduce((acc, char) => acc + char.charCodeAt(0), 0) + return new Array(384).fill(0).map((_, i) => Math.sin(hash + i)) +} + +describe('level field shadow — user metadata named level is a real field', () => { + let brain: Brainy + + beforeEach(async () => { + brain = new Brainy({ + requireSubtype: false, + storage: { type: 'memory' as const }, + embeddingFunction: stubEmbedding + }) + await brain.init() + }) + + afterEach(async () => { + await brain.close() + }) + + async function addProbeRows(): Promise { + const ids: string[] = [] + for (const level of [3, 9, 6]) { + ids.push( + await brain.add({ + data: `probe character level ${level}`, + type: NounType.Person, + subtype: 'probe-char', + metadata: { name: `char-${level}`, level } + }) + ) + } + return ids + } + + it("venue's exact repro: orderBy 'level' sorts desc and asc", async () => { + await addProbeRows() + + const desc = await brain.find({ + type: NounType.Person, + subtype: 'probe-char', + orderBy: 'level', + order: 'desc', + limit: 100 + }) + expect(desc.map((r: any) => r.metadata?.level)).toEqual([9, 6, 3]) + + const asc = await brain.find({ + type: NounType.Person, + subtype: 'probe-char', + orderBy: 'level', + order: 'asc', + limit: 100 + }) + expect(asc.map((r: any) => r.metadata?.level)).toEqual([3, 6, 9]) + }) + + it('ordered reads are COMPLETE — no row dropped (the 2-of-3 face)', async () => { + const ids = await addProbeRows() + const desc = await brain.find({ + type: NounType.Person, + subtype: 'probe-char', + orderBy: 'level', + order: 'desc', + limit: 100 + }) + expect(desc).toHaveLength(3) + expect(new Set(desc.map((r: any) => r.id))).toEqual(new Set(ids)) + }) + + it('where {level: N} matches through the filter and the egress guard', async () => { + const ids = await addProbeRows() + const hit = await brain.find({ where: { level: 9 } }) + expect(hit).toHaveLength(1) + expect(hit[0].id).toBe(ids[1]) + expect(hit[0].metadata?.level).toBe(9) + }) + + it('the index column carries ONLY the user value (no 0 poison)', async () => { + const ids = await addProbeRows() + const metadataIndex = (brain as any).metadataIndex + const value = await metadataIndex.getFieldValueForEntity(ids[1], 'level') + expect(value).toBe(9) + + // Zero must not match anything — pre-fix every entity carried a phantom 0. + const phantom = await brain.find({ where: { level: 0 } }) + expect(phantom).toHaveLength(0) + }) + + it('update() keeps level readable (the update indexing view is clean)', async () => { + const ids = await addProbeRows() + await brain.update({ id: ids[0], metadata: { level: 12 } }) + const desc = await brain.find({ + type: NounType.Person, + subtype: 'probe-char', + orderBy: 'level', + order: 'desc', + limit: 100 + }) + expect(desc.map((r: any) => r.metadata?.level)).toEqual([12, 9, 6]) + }) + + it('transact() metadata-only update never rewrites the noun record', async () => { + const ids = await addProbeRows() + const storage = (brain as any).storage + const saveNounSpy = vi.spyOn(storage, 'saveNoun') + + await brain.transact([ + { op: 'update', id: ids[0], metadata: { level: 4 } }, + { op: 'update', id: ids[2], metadata: { level: 7 } } + ]) + + expect(saveNounSpy).not.toHaveBeenCalled() + saveNounSpy.mockRestore() + + const after = await brain.get(ids[0], { includeVectors: true }) + expect(after?.metadata?.level).toBe(4) + expect(Array.isArray(after?.vector) && after!.vector!.length).toBe(384) + }) + + it('this build runs index epoch 2 (the paired level-indexability rebuild)', () => { + expect(EXPECTED_INDEX_EPOCH).toBe(2) + }) +}) diff --git a/tests/integration/orderby-sort-bug.test.ts b/tests/integration/orderby-sort-bug.test.ts index 9c28b1c9..db40fe12 100644 --- a/tests/integration/orderby-sort-bug.test.ts +++ b/tests/integration/orderby-sort-bug.test.ts @@ -215,7 +215,6 @@ describe('resolveEntityField helper', () => { 'id', 'vector', 'connections', - 'level', 'type', 'confidence', 'weight', @@ -228,5 +227,9 @@ describe('resolveEntityField helper', () => { for (const field of expected) { expect(STANDARD_ENTITY_FIELDS.has(field)).toBe(true) } + // `level` is deliberately NOT resolvable: it is HNSW plumbing, and listing + // it here shadowed user metadata named `level` in every by-name read + // (the reserved-name shadow bug). Plumbing stays out of the resolver. + expect(STANDARD_ENTITY_FIELDS.has('level')).toBe(false) }) }) diff --git a/tests/unit/brainy/migration-deference.test.ts b/tests/unit/brainy/migration-deference.test.ts index 6471b4ef..f03bba9c 100644 --- a/tests/unit/brainy/migration-deference.test.ts +++ b/tests/unit/brainy/migration-deference.test.ts @@ -245,7 +245,9 @@ describe('rc.8 no-freeze migration deference (isMigrating / stampBrainFormat / b it('the brain-format marker module exports the compiled epoch + data-format constants', () => { // cor imports these from '@soulcraft/brainy/brain-format' (Hook 3) so both // sides share ONE source of truth — no duplicated constant to drift. - expect(EXPECTED_INDEX_EPOCH).toBe(1) + // Epoch 2: user metadata named `level` became indexable (the reserved-name + // shadow fix, 2026-08-03) — pre-fix brains rebuild derived indexes at open. + expect(EXPECTED_INDEX_EPOCH).toBe(2) expect(CURRENT_DATA_FORMAT).toBe('8.0') }) })