diff --git a/src/brainy.ts b/src/brainy.ts index a97bdde2..5fd6c627 100644 --- a/src/brainy.ts +++ b/src/brainy.ts @@ -20834,34 +20834,45 @@ export class Brainy implements BrainyInterface { // Phase 1: Flush ALL components in parallel to persist buffered data // This is critical when cor native providers buffer data in Rust memory + // + // READ-ONLY GUARD, applied to EVERY flush here. A flush is a write by + // definition, and a reader has nothing of its own to persist — but these + // calls were not conditional, so a read-only open → read → close REWROTE + // four files under `_system/`: the metadata field registry (whose flush() + // saves it unconditionally, "even with no dirty fields"), and the three + // type/subtype statistics files the storage adapter's count flush stamps. + // Every one of them was re-stamped on a session that committed nothing. + // A reader must leave `_system/` exactly as it found it — the same law the + // clean-shutdown marker already lives under (see the generation-store + // guard below and `Brainy.openReadOnly`). await Promise.all([ // Flush HNSW dirty nodes (deferred persistence mode) (async () => { - if (this.index && typeof this.index.flush === 'function') { + if (this.index && !this.isReadOnly && typeof this.index.flush === 'function') { await this.index.flush() } })(), // Flush metadata index (field indexes + EntityIdMapper) (async () => { - if (this.metadataIndex && typeof this.metadataIndex.flush === 'function') { + if (this.metadataIndex && !this.isReadOnly && typeof this.metadataIndex.flush === 'function') { await this.metadataIndex.flush() } })(), // Flush graph adjacency index (LSM trees) (async () => { - if (this.graphIndex && typeof this.graphIndex.flush === 'function') { + if (this.graphIndex && !this.isReadOnly && typeof this.graphIndex.flush === 'function') { await this.graphIndex.flush() } })(), // Flush storage adapter counts (async () => { - if (this.storage && typeof this.storage.flushCounts === 'function') { + if (this.storage && !this.isReadOnly && typeof this.storage.flushCounts === 'function') { await this.storage.flushCounts() } })(), // Flush aggregation index state (async () => { - if (this._aggregationIndex) { + if (this._aggregationIndex && !this.isReadOnly) { await this._aggregationIndex.flush() } })(), @@ -20910,21 +20921,37 @@ export class Brainy implements BrainyInterface { // Phase 2: Close components to release resources (timers, file handles) // Data is already safe on disk from Phase 1 + // + // READ-ONLY GUARD, same law as Phase 1. Each of these closes is a WRITER: + // the graph index drains both LSM MemTables to SSTables and stamps its + // watermark, and the vector/metadata `close` hooks — optional doors the + // reference engine leaves unimplemented, but which a native provider fills + // in — persist their buffered state. None of that is a reader's to write. + // + // A reader still has to RELEASE what it holds, which is why this is a + // branch rather than a skip: `stopBackgroundFlush()` is the non-writing + // half of the graph index's close, clearing the auto-flush interval that + // would otherwise outlive the session. The optional hooks have no + // non-writing counterpart to call, and a provider that buffers nothing on + // a read-only open has nothing to release. await Promise.all([ (async () => { - if (this.graphIndex && typeof this.graphIndex.close === 'function') { + if (!this.graphIndex) return + if (this.isReadOnly) { + this.graphIndex.stopBackgroundFlush() + } else if (typeof this.graphIndex.close === 'function') { await this.graphIndex.close() } })(), (async () => { const index = this.index as JsHnswVectorIndex & VectorIndexOptionalHooks - if (index && typeof index.close === 'function') { + if (index && !this.isReadOnly && typeof index.close === 'function') { await index.close() } })(), (async () => { const metadataIndex = this.metadataIndex as MetadataIndexManager & MetadataIndexOptionalHooks - if (metadataIndex && typeof metadataIndex.close === 'function') { + if (metadataIndex && !this.isReadOnly && typeof metadataIndex.close === 'function') { await metadataIndex.close() } })(), diff --git a/src/graph/graphAdjacencyIndex.ts b/src/graph/graphAdjacencyIndex.ts index ebd3b90c..2c131a30 100644 --- a/src/graph/graphAdjacencyIndex.ts +++ b/src/graph/graphAdjacencyIndex.ts @@ -1105,13 +1105,31 @@ export class GraphAdjacencyIndex implements GraphIndexProvider { } /** - * Clean shutdown + * Stop the auto-flush interval WITHOUT writing anything. + * + * The non-writing half of {@link close}, for a shutdown that must leave the + * store byte-identical — a read-only brain's close. `close()` itself is a + * writer: it drains both LSM MemTables to SSTables and stamps the watermark, + * which is exactly right for a writer and forbidden for a reader. A reader + * still has to release this interval, though: it is the one piece of this + * index that outlives the close and could fire against a store the session no + * longer owns. + * + * @returns Nothing. */ - async close(): Promise { + stopBackgroundFlush(): void { if (this.flushTimer) { clearInterval(this.flushTimer) this.flushTimer = undefined } + } + + /** + * Clean shutdown — drains both trees and stamps the watermark. THIS WRITES; + * a read-only brain must call {@link stopBackgroundFlush} instead. + */ + async close(): Promise { + this.stopBackgroundFlush() // Close both LSM-trees (will flush MemTables to SSTables) if (this.initialized) { diff --git a/tests/integration/readonly-close-no-marker.test.ts b/tests/integration/readonly-close-no-marker.test.ts index 7bcf99df..ad9357db 100644 --- a/tests/integration/readonly-close-no-marker.test.ts +++ b/tests/integration/readonly-close-no-marker.test.ts @@ -149,12 +149,12 @@ describe('a read-only brain writes no clean-shutdown evidence', () => { brain = null // The FILE SET under `_system/` is unchanged — a reader creates and - // removes nothing. (Other files under `_system/` — e.g. the metadata - // field registry, which stamps its own `lastUpdated` on every persist — - // are a pre-existing, separate concern outside this fix's scope: this - // pin is specifically about the generation store's clean-shutdown - // evidence, not about every subsystem's close() being a true no-op for - // a reader.) + // removes nothing. This pin is specifically about the generation store's + // clean-shutdown evidence. The wider law — that a reader leaves EVERY + // file under `_system/` byte-identical, which this fix left open as a + // known residual (the metadata field registry and the three statistics + // files were still re-stamped by a reader's close) — is closed and pinned + // in `readonly-close-writes-nothing.test.ts`. const after = snapshotDir(systemDir()) expect([...after.keys()].sort()).toEqual([...before.keys()].sort()) diff --git a/tests/integration/readonly-close-writes-nothing.test.ts b/tests/integration/readonly-close-writes-nothing.test.ts new file mode 100644 index 00000000..701a1974 --- /dev/null +++ b/tests/integration/readonly-close-writes-nothing.test.ts @@ -0,0 +1,261 @@ +/** + * @module tests/integration/readonly-close-writes-nothing + * @description A READ-ONLY BRAIN LEAVES `_system/` BYTE-IDENTICAL — the WHOLE + * directory, not just the clean-shutdown marker. + * + * `readonly-close-no-marker` closed the marker half of this law and named the + * rest as a known, out-of-scope residual: + * + * "Other files under `_system/` — e.g. the metadata field registry, which + * stamps its own `lastUpdated` on every persist — are a pre-existing, + * separate concern outside this fix's scope." + * + * This is that residual, closed. MEASURED on the base before the fix, a + * read-only open → read → close rewrote FOUR files: + * + * _system/__metadata_field_registry__.json.gz + * _system/type-statistics.json.gz + * _system/subtype-statistics.json.gz + * _system/verb-subtype-statistics.json.gz + * + * THE CAUSE was not the closes the marker fix guarded — it was Phase 1 of + * `closeDurableSteps`, where every component flush ran unconditionally. A flush + * is a write by definition: `MetadataIndexManager#flush()` saves the field + * registry "even with no dirty fields" (its own comment), and the storage + * adapter's count flush re-stamps the three statistics files. A session that + * committed nothing re-stamped all four. Phase 2's closes were ungated too — + * the graph index's close drains both LSM MemTables and stamps a watermark, + * and the optional vector/metadata `close` hooks (unimplemented in the + * reference engine, filled in by a native provider) persist buffered state. + * + * THE LAW. A reader writes nothing, anywhere under `_system/`, at open or at + * close. It still RELEASES what it holds: the graph index's auto-flush interval + * is cleared through `stopBackgroundFlush()`, the non-writing half of its + * close, so nothing outlives the session. + * + * WHY IT MATTERS beyond tidiness: `_system/` is where a store keeps its + * evidence about itself — what the writer committed, what the projections have + * seen. A reader that rewrites any of it is vouching for a state it only + * observed, and on shared or snapshot storage it mutates bytes another process + * owns. + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest' +import { mkdtempSync, rmSync, readdirSync, readFileSync, statSync } from 'node:fs' +import { createHash } from 'node:crypto' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { Brainy } from '../../src/brainy.js' +import { NounType, VerbType } from '../../src/types/graphTypes.js' + +/** Recursively hash every regular file under `dir`, keyed by its path relative to `dir`. */ +function snapshotDir(dir: string): Map { + const out = new Map() + const walk = (rel: string): void => { + const abs = rel ? join(dir, rel) : dir + let entries: string[] + try { + entries = readdirSync(abs) + } catch { + return + } + for (const name of entries) { + const childRel = rel ? join(rel, name) : name + const childAbs = join(dir, childRel) + const st = statSync(childAbs) + if (st.isDirectory()) { + walk(childRel) + } else if (st.isFile()) { + out.set(childRel, createHash('sha256').update(readFileSync(childAbs)).digest('hex')) + } + } + } + walk('') + return out +} + +/** Every path where `after` differs from `before`, labelled — the failure message. */ +function diff(before: Map, after: Map): string[] { + const lines: string[] = [] + for (const [path, hash] of after) { + if (!before.has(path)) lines.push(`ADDED ${path}`) + else if (before.get(path) !== hash) lines.push(`CHANGED ${path}`) + } + for (const path of before.keys()) if (!after.has(path)) lines.push(`REMOVED ${path}`) + return lines.sort() +} + +describe('a read-only brain writes nothing under `_system/`', () => { + let dir: string + let brain: Brainy | null = null + + beforeEach(() => { + dir = mkdtempSync(join(tmpdir(), 'brainy-readonly-writes-')) + }) + + afterEach(async () => { + if (brain) { + try { + await brain.close() + } catch { + /* already closed */ + } + brain = null + } + try { + rmSync(dir, { recursive: true, force: true }) + } catch { + /* ignore */ + } + }) + + const systemDir = () => join(dir, '_system') + + /** + * A writer seeds a store with nouns, verbs and queryable metadata — enough + * that the field registry, the statistics files and the graph index all hold + * real content — then closes cleanly. + */ + async function seedStore(): Promise { + const writer = new Brainy({ + requireSubtype: false, + storage: { type: 'filesystem', path: dir } + }) + await writer.init() + for (let i = 0; i < 6; i++) { + await writer.add({ + id: `seed-${i}`, + data: `seed entity ${i}`, + type: i % 2 === 0 ? NounType.Concept : NounType.Document, + metadata: { lane: i % 2 === 0 ? 'alpha' : 'beta', rank: i, tags: [`t${i}`, 'shared'] }, + vector: [] + }) + } + for (let i = 1; i < 6; i++) { + await writer.relate({ from: 'seed-0', to: `seed-${i}`, type: VerbType.RelatedTo }) + } + await writer.flush() + await writer.close() + } + + it('open → read → close leaves every file under `_system/` byte-identical', async () => { + await seedStore() + + const before = snapshotDir(systemDir()) + expect(before.size, 'the writer left a populated `_system/`').toBeGreaterThan(0) + + brain = await Brainy.openReadOnly({ storage: { type: 'filesystem', path: dir } }) + expect(brain.isReadOnly).toBe(true) + + // Exercise the read surface that drives each subsystem: statistics (counts), + // a metadata filter (field index + registry), a graph walk (adjacency), a + // vector search, and a direct get. + await brain.stats() + await brain.find({ where: { lane: 'alpha' }, limit: 10 } as any) + await brain.find({ where: { tags: 'shared' }, limit: 10 } as any) + await brain.find({ connected: { from: 'seed-0', direction: 'out' }, limit: 10 } as any) + await brain.get('seed-1') + + await brain.close() + brain = null + + const after = snapshotDir(systemDir()) + const changes = diff(before, after) + expect(changes, `a reader modified \`_system/\`:\n${changes.join('\n')}`).toEqual([]) + }, 120_000) + + it('names the four files that used to change — the measured shape of the defect', async () => { + await seedStore() + const before = snapshotDir(systemDir()) + + // These are the exact paths the base rewrote. Naming them keeps the pin + // honest about what it caught: if a future change reintroduces the write, + // the test above fails and this one says which subsystem did it. + const previouslyRewritten = [ + '__metadata_field_registry__.json.gz', + 'type-statistics.json.gz', + 'subtype-statistics.json.gz', + 'verb-subtype-statistics.json.gz' + ] + for (const name of previouslyRewritten) { + expect(before.has(name), `fixture must contain ${name}`).toBe(true) + } + + brain = await Brainy.openReadOnly({ storage: { type: 'filesystem', path: dir } }) + await brain.stats() + await brain.find({ where: { lane: 'alpha' }, limit: 10 } as any) + await brain.close() + brain = null + + const after = snapshotDir(systemDir()) + for (const name of previouslyRewritten) { + expect(after.get(name), `${name} was rewritten by a reader`).toBe(before.get(name)) + } + }, 120_000) + + it('a reader that only opens and closes — touching nothing — writes nothing', async () => { + await seedStore() + const before = snapshotDir(systemDir()) + + brain = await Brainy.openReadOnly({ storage: { type: 'filesystem', path: dir } }) + await brain.close() + brain = null + + const changes = diff(before, snapshotDir(systemDir())) + expect(changes, `an idle reader modified \`_system/\`:\n${changes.join('\n')}`).toEqual([]) + }, 120_000) + + it('two readers in sequence each leave the store exactly as they found it', async () => { + await seedStore() + const before = snapshotDir(systemDir()) + + for (let i = 0; i < 2; i++) { + const reader = await Brainy.openReadOnly({ storage: { type: 'filesystem', path: dir } }) + await reader.find({ where: { lane: 'beta' }, limit: 10 } as any) + await reader.close() + const changes = diff(before, snapshotDir(systemDir())) + expect(changes, `reader ${i + 1} modified \`_system/\`:\n${changes.join('\n')}`).toEqual([]) + } + }, 120_000) + + it('the store outside `_system/` is untouched too — a reader writes nowhere', async () => { + await seedStore() + const before = snapshotDir(dir) + + brain = await Brainy.openReadOnly({ storage: { type: 'filesystem', path: dir } }) + await brain.stats() + await brain.find({ where: { lane: 'alpha' }, limit: 10 } as any) + await brain.close() + brain = null + + const changes = diff(before, snapshotDir(dir)) + expect(changes, `a reader modified the store:\n${changes.join('\n')}`).toEqual([]) + }, 120_000) + + it('a WRITER still persists on close — the guard did not disarm the write path', async () => { + await seedStore() + const before = snapshotDir(systemDir()) + + const writer = new Brainy({ requireSubtype: false, storage: { type: 'filesystem', path: dir } }) + await writer.init() + await writer.add({ + id: 'after-reader', + data: 'a new row', + type: NounType.Concept, + metadata: { lane: 'gamma', rank: 99 }, + vector: [] + }) + await writer.close() + + // The writer's close DID move `_system/` — that is the whole point of the + // asymmetry, and the guard must not have flattened it. + expect(diff(before, snapshotDir(systemDir())).length).toBeGreaterThan(0) + + // And the row is really there on the next open. + const reopened = new Brainy({ requireSubtype: false, storage: { type: 'filesystem', path: dir } }) + await reopened.init() + brain = reopened + const hits = await reopened.find({ where: { lane: 'gamma' }, limit: 10 } as any) + expect(hits.length).toBe(1) + }, 120_000) +})