From a17540649723e4c001f535720b26fa6519aad977 Mon Sep 17 00:00:00 2001 From: David Snelling Date: Fri, 10 Jul 2026 18:06:37 -0700 Subject: [PATCH] fix: transact forward references resolve graph endpoint ints at execute time MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit transact() promises atomic forward references — add an entity and relate to it in one batch — but the planner resolved relationship endpoint ints at PLAN time, before the batch's add operations had applied. Asking the id mapper about an entity that does not exist yet made the (correctly strict) native mapper throw in EntityIdMapper.getOrAssign, so transact([{op:'add', id:X}, {op:'relate', to:X}]) failed on native deployments; the permissive JS mapper masked the bug and silently leaked an id assignment whenever a batch was later rejected by a commit precondition (normal control flow since the conditional-commit CAS landed). Make endpoint resolution lazy: AddToGraphIndexOperation and RemoveFromGraphIndexOperation take VerbEndpointInts — an eager {sourceInt, targetInt} or a thunk evaluated when the operation EXECUTES, mirroring the constructor's already-lazy generationFn. The four transact-planner sites (relate, its bidirectional reverse edge, remove's relationship cascade, unrelate) pass thunks, so resolution happens inside the commit after same-batch adds have applied; the seven single-operation sites keep eager objects (their endpoints provably pre-exist — relate validates both, unrelate/updateRelation/remove read the existing verb). The remove operation's rollback captures the ints resolved at execute so its re-add uses the same mappings. Byproduct fix: a rejected batch no longer pollutes the id mapper — the regression suite pins this (mapper has no assignment for the phantom entity after a precondition-rejected forward-ref batch), alongside the exact reported shape, both-endpoints-in-batch, bidirectional, add+relate+remove in one batch, and the split-transact control (tests/integration/transact-forward-ref-graph.test.ts). --- src/brainy.ts | 42 +++--- src/transaction/operations/IndexOperations.ts | 49 ++++-- .../transact-forward-ref-graph.test.ts | 140 ++++++++++++++++++ 3 files changed, 202 insertions(+), 29 deletions(-) create mode 100644 tests/integration/transact-forward-ref-graph.test.ts diff --git a/src/brainy.ts b/src/brainy.ts index d54cfe1c..422cb2e6 100644 --- a/src/brainy.ts +++ b/src/brainy.ts @@ -3024,7 +3024,7 @@ export class Brainy implements BrainyInterface { // rollback can re-add through the BigInt addVerb contract) const { sourceInt, targetInt } = this.resolveVerbEndpointInts(verb) tx.addOperation( - new RemoveFromGraphIndexOperation(this.graphIndex, verb, sourceInt, targetInt, this.graphWriteGeneration) + new RemoveFromGraphIndexOperation(this.graphIndex, verb, { sourceInt, targetInt }, this.graphWriteGeneration) ) // Delete verb metadata tx.addOperation( @@ -3734,7 +3734,7 @@ export class Brainy implements BrainyInterface { // Operation 3: Add to graph index for O(1) lookups tx.addOperation( new AddToGraphIndexOperation( - this.graphIndex, verb, sourceInt, targetInt, + this.graphIndex, verb, { sourceInt, targetInt }, this.graphWriteGeneration, (verbInt) => this.cacheVerbInt(verbInt, id) ) @@ -3772,7 +3772,7 @@ export class Brainy implements BrainyInterface { // Operation 6: Add reverse relationship to graph index tx.addOperation( new AddToGraphIndexOperation( - this.graphIndex, reverseVerb, targetInt, sourceInt, + this.graphIndex, reverseVerb, { sourceInt: targetInt, targetInt: sourceInt }, this.graphWriteGeneration, (verbInt) => this.cacheVerbInt(verbInt, reverseId) ) @@ -3854,7 +3854,7 @@ export class Brainy implements BrainyInterface { if (verb && endpointInts) { tx.addOperation( new RemoveFromGraphIndexOperation( - this.graphIndex, verb, endpointInts.sourceInt, endpointInts.targetInt, this.graphWriteGeneration + this.graphIndex, verb, endpointInts, this.graphWriteGeneration ) ) } @@ -4006,12 +4006,12 @@ export class Brainy implements BrainyInterface { if (typeChanged && reindexInts) { tx.addOperation( new RemoveFromGraphIndexOperation( - this.graphIndex, existing, reindexInts.sourceInt, reindexInts.targetInt, this.graphWriteGeneration + this.graphIndex, existing, reindexInts, this.graphWriteGeneration ) ) tx.addOperation( new AddToGraphIndexOperation( - this.graphIndex, verbForIndex, reindexInts.sourceInt, reindexInts.targetInt, + this.graphIndex, verbForIndex, reindexInts, this.graphWriteGeneration, (verbInt) => this.cacheVerbInt(verbInt, params.id) ) @@ -6634,7 +6634,7 @@ export class Brainy implements BrainyInterface { for (const verb of allVerbs) { const { sourceInt, targetInt } = this.resolveVerbEndpointInts(verb) tx.addOperation( - new RemoveFromGraphIndexOperation(this.graphIndex, verb, sourceInt, targetInt, this.graphWriteGeneration) + new RemoveFromGraphIndexOperation(this.graphIndex, verb, { sourceInt, targetInt }, this.graphWriteGeneration) ) tx.addOperation( new DeleteVerbMetadataOperation(this.storage, verb.id) @@ -9014,9 +9014,12 @@ export class Brainy implements BrainyInterface { } plan.operations.push(new DeleteNounMetadataOperation(this.storage, id)) for (const verb of cascade.values()) { - const { sourceInt, targetInt } = this.resolveVerbEndpointInts(verb) plan.operations.push( - new RemoveFromGraphIndexOperation(this.graphIndex, verb, sourceInt, targetInt, this.graphWriteGeneration), + // Endpoint ints resolve at EXECUTE time — a cascade verb (or its + // endpoints) may have been created earlier in this same batch, so a + // plan-time resolution would ask the id mapper about entities that do + // not exist yet (the native mapper rightly refuses). + new RemoveFromGraphIndexOperation(this.graphIndex, verb, () => this.resolveVerbEndpointInts(verb), this.graphWriteGeneration), new DeleteVerbMetadataOperation(this.storage, verb.id) ) plan.touchedVerbs.push(verb.id) @@ -9163,8 +9166,6 @@ export class Brainy implements BrainyInterface { data: params.data, createdAt: now } - const { sourceInt, targetInt } = this.resolveVerbEndpointInts(verb) - plan.operations.push( new SaveVerbOperation(this.storage, { id, @@ -9175,7 +9176,11 @@ export class Brainy implements BrainyInterface { targetId: params.to }), new SaveVerbMetadataOperation(this.storage, id, verbMetadata), - new AddToGraphIndexOperation(this.graphIndex, verb, sourceInt, targetInt, this.graphWriteGeneration, (verbInt) => + // Endpoint ints resolve at EXECUTE time — after any same-batch add of + // an endpoint has applied. Plan-time resolution was a consumer-reported + // native/JS parity bug: transact([add X, relate →X]) asked the native + // id mapper to assign an int for an entity that did not exist yet. + new AddToGraphIndexOperation(this.graphIndex, verb, () => this.resolveVerbEndpointInts(verb), this.graphWriteGeneration, (verbInt) => this.cacheVerbInt(verbInt, id) ) ) @@ -9203,9 +9208,9 @@ export class Brainy implements BrainyInterface { ...verb, id: reverseId, sourceId: params.to, - targetId: params.from, - sourceInt: targetInt, - targetInt: sourceInt + targetId: params.from + // sourceInt/targetInt are mirrored onto this object when the graph + // operation resolves endpoints at execute time. } plan.operations.push( new SaveVerbOperation(this.storage, { @@ -9217,7 +9222,7 @@ export class Brainy implements BrainyInterface { targetId: params.from }), new SaveVerbMetadataOperation(this.storage, reverseId, verbMetadata), - new AddToGraphIndexOperation(this.graphIndex, reverseVerb, targetInt, sourceInt, this.graphWriteGeneration, (verbInt) => + new AddToGraphIndexOperation(this.graphIndex, reverseVerb, () => this.resolveVerbEndpointInts(reverseVerb), this.graphWriteGeneration, (verbInt) => this.cacheVerbInt(verbInt, reverseId) ) ) @@ -9259,9 +9264,10 @@ export class Brainy implements BrainyInterface { : (state.verbs.get(id) ?? (await this.storage.getVerb(id))) if (verb) { - const { sourceInt, targetInt } = this.resolveVerbEndpointInts(verb) plan.operations.push( - new RemoveFromGraphIndexOperation(this.graphIndex, verb, sourceInt, targetInt, this.graphWriteGeneration) + // Endpoint ints resolve at EXECUTE time — the verb (or its endpoints) + // may have been created earlier in this same batch (forward refs). + new RemoveFromGraphIndexOperation(this.graphIndex, verb, () => this.resolveVerbEndpointInts(verb), this.graphWriteGeneration) ) } plan.operations.push(new DeleteVerbMetadataOperation(this.storage, id)) diff --git a/src/transaction/operations/IndexOperations.ts b/src/transaction/operations/IndexOperations.ts index f0bede05..97c39270 100644 --- a/src/transaction/operations/IndexOperations.ts +++ b/src/transaction/operations/IndexOperations.ts @@ -176,14 +176,36 @@ export class RemoveFromMetadataIndexOperation implements Operation { * generation is reused for the rollback removal, so an add and its undo * reference one watermark in a provider's per-generation edge chain. */ +/** + * A verb's interned endpoint ints — eager (already resolved), or a thunk + * evaluated when the operation EXECUTES. The lazy form exists for + * `transact()` forward references: a relate whose endpoint is added in the + * SAME batch cannot resolve ints at plan time (the entity does not exist yet + * — a strict native id mapper rightly refuses to assign, and even a permissive + * one would leak the assignment if the batch is rejected at precommit). + * Deferring to execute time resolves after the batch's add operations have + * applied, mirroring how `generationFn` is already evaluated lazily. + */ +export type VerbEndpointInts = + | { sourceInt: bigint; targetInt: bigint } + | (() => { sourceInt: bigint; targetInt: bigint }) + +/** Resolve a {@link VerbEndpointInts} at execution time. */ +function resolveEndpointInts( + endpoints: VerbEndpointInts +): { sourceInt: bigint; targetInt: bigint } { + return typeof endpoints === 'function' ? endpoints() : endpoints +} + export class AddToGraphIndexOperation implements Operation { readonly name = 'AddToGraphIndex' /** * @param index - The graph-index provider (JS baseline or native). * @param verb - The verb to index (`sourceInt`/`targetInt` mirrored on it). - * @param sourceInt - The source entity's interned int. - * @param targetInt - The target entity's interned int. + * @param endpointInts - The endpoints' interned ints — eager, or a thunk + * evaluated at execute time (REQUIRED for transact forward references; + * see {@link VerbEndpointInts}). * @param generationFn - Resolves the commit generation to stamp this edge at, * evaluated when the operation executes (see class note). * @param onVerbInt - Optional hook invoked with the interned verb int @@ -192,17 +214,18 @@ export class AddToGraphIndexOperation implements Operation { constructor( private readonly index: GraphIndexProvider, private readonly verb: GraphVerb, - private readonly sourceInt: bigint, - private readonly targetInt: bigint, + private readonly endpointInts: VerbEndpointInts, private readonly generationFn: () => bigint, private readonly onVerbInt?: (verbInt: bigint) => void ) {} async execute(): Promise { // Stamp this edge at the in-flight commit generation; reuse it for the - // rollback so add + undo reference the same watermark. + // rollback so add + undo reference the same watermark. Endpoint ints + // resolve HERE — after any same-batch adds have applied. const generation = this.generationFn() - const verbInt = await this.index.addVerb(this.verb, this.sourceInt, this.targetInt, generation) + const { sourceInt, targetInt } = resolveEndpointInts(this.endpointInts) + const verbInt = await this.index.addVerb(this.verb, sourceInt, targetInt, generation) this.onVerbInt?.(verbInt) // Return rollback action @@ -231,28 +254,32 @@ export class RemoveFromGraphIndexOperation implements Operation { /** * @param index - The graph-index provider (JS baseline or native). * @param verb - The verb being removed (required for rollback re-add). - * @param sourceInt - The source entity's interned int (rollback re-add). - * @param targetInt - The target entity's interned int (rollback re-add). + * @param endpointInts - The endpoints' interned ints for the rollback + * re-add — eager, or a thunk evaluated at execute time (required when the + * verb or its endpoints were created in the SAME transact batch; see + * {@link VerbEndpointInts}). * @param generationFn - Resolves the commit generation for this removal, * evaluated when the operation executes. */ constructor( private readonly index: GraphIndexProvider, private readonly verb: GraphVerb, // Required for rollback - private readonly sourceInt: bigint, - private readonly targetInt: bigint, + private readonly endpointInts: VerbEndpointInts, private readonly generationFn: () => bigint ) {} async execute(): Promise { // Resolve the removal generation once; reuse it for the rollback re-add. + // Endpoint ints resolve HERE (after any same-batch adds applied) and are + // captured for the rollback, whose re-add must use the same mappings. const generation = this.generationFn() + const { sourceInt, targetInt } = resolveEndpointInts(this.endpointInts) await this.index.removeVerb(this.verb.id, generation) // Return rollback action return async () => { // Re-add verb with original data - await this.index.addVerb(this.verb, this.sourceInt, this.targetInt, generation) + await this.index.addVerb(this.verb, sourceInt, targetInt, generation) } } } diff --git a/tests/integration/transact-forward-ref-graph.test.ts b/tests/integration/transact-forward-ref-graph.test.ts new file mode 100644 index 00000000..a9619424 --- /dev/null +++ b/tests/integration/transact-forward-ref-graph.test.ts @@ -0,0 +1,140 @@ +/** + * @module tests/integration/transact-forward-ref-graph + * @description Regression for a consumer-reported native/JS parity bug: + * `transact([{op:'add', id:X}, {op:'relate', to:X}])` — the platform's first + * atomic add+relate consumer — threw in `EntityIdMapper.getOrAssign` at PLAN + * time on the native id mapper (an entity added in the same batch does not + * exist when the planner runs, so a strict mapper rightly refuses to assign; + * the permissive JS mapper masked the bug — and silently leaked an int + * assignment whenever the batch was later rejected at precommit). + * + * Fix: graph-index operations resolve endpoint ints at EXECUTE time (a lazy + * thunk, mirroring the existing lazy `generationFn`), after the batch's add + * operations have applied. These tests pin every forward-ref shape on the JS + * path, including the JS-observable deferral proof: a REJECTED batch leaves + * the id mapper untouched (pre-fix, plan-time resolution assigned ints for + * entities that never came to exist). The native side is locked in by the + * accelerator's own gate running this same shape. + */ +import { describe, it, expect, beforeEach, 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/brainy.js' +import { NounType, VerbType } from '../../src/types/graphTypes.js' + +let seq = 0 +const freshId = (): string => + `00000000-0000-4000-8000-${(++seq).toString(16).padStart(12, '0')}` + +describe('transact() forward references into the graph index (native/JS parity)', () => { + let dir: string + let brain: any + + beforeEach(async () => { + process.env.BRAINY_DETERMINISTIC_EMBEDDINGS = 'true' + dir = fs.mkdtempSync(path.join(os.tmpdir(), 'brainy-fwd-ref-')) + brain = new Brainy({ + requireSubtype: false, + storage: { type: 'filesystem', path: dir }, + dimensions: 384, + silent: true + }) + await brain.init() + }) + + afterEach(async () => { + await brain.close() + fs.rmSync(dir, { recursive: true, force: true }) + }) + + it('CASE 1 (the exact repro): add + relate-to-that-add in ONE transact', async () => { + const agent = await brain.add({ id: freshId(), data: 'agent probe', type: NounType.Person }) + const thread = freshId() + + const db = await brain.transact([ + { op: 'add', id: thread, data: 'thread PROBE-ONE', type: NounType.Thing }, + { op: 'relate', from: agent, to: thread, type: VerbType.RelatedTo } + ]) + + // The batch committed as one generation and the edge traverses. + expect(db.generation).toBeGreaterThan(0) + const related = await brain.related(agent) + expect(related.map((r: any) => r.to)).toContain(thread) + }) + + it('both endpoints created in-batch: add A + add B + relate A→B', async () => { + const a = freshId() + const b = freshId() + await brain.transact([ + { op: 'add', id: a, data: 'node A', type: NounType.Thing }, + { op: 'add', id: b, data: 'node B', type: NounType.Thing }, + { op: 'relate', from: a, to: b, type: VerbType.Contains } + ]) + expect((await brain.related(a)).map((r: any) => r.to)).toContain(b) + }) + + it('bidirectional relate to an in-batch add: both edges traverse', async () => { + const hub = await brain.add({ id: freshId(), data: 'hub', type: NounType.Thing }) + const spoke = freshId() + await brain.transact([ + { op: 'add', id: spoke, data: 'spoke', type: NounType.Thing }, + { op: 'relate', from: hub, to: spoke, type: VerbType.RelatedTo, bidirectional: true } + ]) + expect((await brain.related(hub)).map((r: any) => r.to)).toContain(spoke) + expect((await brain.related(spoke)).map((r: any) => r.to)).toContain(hub) + }) + + it('add + relate + remove in ONE transact: the cascade covers the in-batch verb', async () => { + const keeper = await brain.add({ id: freshId(), data: 'keeper', type: NounType.Thing }) + const doomed = freshId() + await brain.transact([ + { op: 'add', id: doomed, data: 'doomed', type: NounType.Thing }, + { op: 'relate', from: keeper, to: doomed, type: VerbType.Contains }, + { op: 'remove', id: doomed } + ]) + expect(await brain.get(doomed)).toBeNull() + expect((await brain.related(keeper)).length).toBe(0) + }) + + it('CASE 2 control: the same two ops split across two transacts still work', async () => { + const agent = await brain.add({ id: freshId(), data: 'agent two', type: NounType.Person }) + const thread = freshId() + await brain.transact([{ op: 'add', id: thread, data: 'thread two', type: NounType.Thing }]) + await brain.transact([{ op: 'relate', from: agent, to: thread, type: VerbType.RelatedTo }]) + expect((await brain.related(agent)).map((r: any) => r.to)).toContain(thread) + }) + + it('a REJECTED forward-ref batch leaves the id mapper untouched (the deferral proof)', async () => { + const existing = await brain.add({ + id: freshId(), + data: 'cas anchor', + type: NounType.Thing + }) + const never = freshId() + + // Reject the batch via a stale per-op CAS — planning completes, the + // commit precondition throws, nothing applies. + await expect( + brain.transact([ + { op: 'add', id: never, data: 'never exists', type: NounType.Thing }, + { op: 'relate', from: existing, to: never, type: VerbType.RelatedTo }, + { op: 'update', id: existing, metadata: { poke: 1 }, ifRev: 999 } + ]) + ).rejects.toMatchObject({ name: 'RevisionConflictError' }) + + // Nothing applied… + expect(await brain.get(never)).toBeNull() + // …and the id mapper was never asked to assign for the phantom entity — + // pre-fix, plan-time resolution leaked an int here on every rejected batch. + const idMapper = brain['metadataIndex'].getIdMapper() + expect(idMapper.getInt(never)).toBeUndefined() + + // The same shape then succeeds cleanly on retry. + await brain.transact([ + { op: 'add', id: never, data: 'exists now', type: NounType.Thing }, + { op: 'relate', from: existing, to: never, type: VerbType.RelatedTo } + ]) + expect((await brain.related(existing)).map((r: any) => r.to)).toContain(never) + }) +})