soulcraft/brainy
1
0
Fork 0
Code Issues 1 Pull requests Projects Releases 335 Packages Wiki Activity Actions

fix: spine hardening pass 1 (part) — count symmetry, honest partial-load, flush durability, read-fault propagation

Browse source 
Write/index-spine hardening, first batch of Pass 1. Each fix restores an
invariant the surrounding code already intended; every one has a
fail-before/pass-after test.

- Pattern C, finding 5 (baseStorage): delete now decrements the user-facing
  scalar total symmetrically — deleteNounMetadata was decrementing only the
  per-type bucket, deleteVerbMetadata neither the bucket nor the scalar, so
  getNounCount()/getVerbCount() inflated permanently (the stale scalar wins
  pagination via Math.max and is persisted). Invariant now holds:
  scalar total === Σ per-type across add/update/delete and reopen.

- Pattern A, finding 3 (graph/lsm/LSMTree): a partial SSTable-load failure no
  longer publishes the manifest's full relationship count as healthy. Any
  per-SSTable load failure throws after the batch, which resets to honest-empty
  and lets the existing size()===0 self-heal rebuild run — size()/isHealthy()
  can no longer lie about a partial load.

- Pattern B, finding 6 (hnsw/hnswIndex): deferred flush() no longer clears
  dirty nodes whose connections failed to persist — failed nodes stay in the
  retry set, and flush() throws HnswFlushError instead of returning a lying
  node count. The immediate-mode first-noun saveHNSWSystem is un-swallowed, so
  addItem() rejects rather than returning an id for a rootless index.

- Pattern B, finding 11 (part — storage reads): new shared isAbsentError()
  helper (utils/errorClassification, ENOENT-only absence) applied to
  loadBinaryBlob and readObjectFromPath — a real IO fault (EIO/EACCES/EMFILE)
  now propagates loudly instead of masquerading as "absent", which had driven
  needless rebuilds / empty reads (loadBinaryBlob feeds the native provider).

Regression: 78 green across the 3 new suites + db-mvcc, generationStore,
temporal-vfs, rollback-trapdoor, restore-nondestructive. Full gate runs before
the Pass-1 release (David-gated). Remaining Pass 1: finding 11 getNoun/getVerb
legs, finding 4 (ColumnStore), finding 8 (pending-flush), finding 10 (degraded),
finding 7 (clear). Pattern A guards (1,2,9) as a follow-up release.
This commit is contained in:
David Snelling 2026-07-13 08:50:07 -07:00
parent eb9c4eb963
commit 119087a75c
8 changed files with 518 additions and 30 deletions
Download patch file Download diff file Expand all files Collapse all files

20
src/graph/lsm/LSMTree.ts
View file

@ -574,6 +574,7 @@ export class LSMTree {
* Load SSTables from storage based on manifest
*/
private async loadSSTables(): Promise<void> {
const failures: string[] = []
const loadPromises: Promise<void>[] = []
this.manifest.sstables.forEach((level, sstableId) => {
@ -598,7 +599,12 @@ export class LSMTree {
}
}
} catch (error) {
// A per-SSTable load failure means the persisted adjacency is INCOMPLETE.
// Record it and fail the whole load closed (below): a partially-loaded
// tree that still publishes its full manifest count via size() would
// serve silent-empty traversals as truth (the cold-load swallow class).
prodLog.warn(`LSMTree: Failed to load SSTable ${sstableId}`, error)
failures.push(sstableId)
}
})()
@ -606,6 +612,20 @@ export class LSMTree {
})
await Promise.all(loadPromises)
if (failures.length > 0) {
// Fail closed. loadManifest()'s catch resets sstables/totalRelationships/
// sstablesByLevel to honest-empty, so size() reports 0 and the graph
// self-heal (_initializeGraphIndex size()===0 → rebuild) restores the index
// from the canonical records. Honest-partial is never published.
throw new Error(
`LSMTree(${this.config.storagePrefix}): ${failures.length} of ` +
`${this.manifest.sstables.size} SSTable(s) failed to load ` +
`(${failures.join(', ')}) — failing the load closed so size() reports 0 ` +
`and the graph self-heal rebuilds from canonical.`
)
}
prodLog.info(`LSMTree: Loaded ${this.manifest.sstables.size} SSTables`)
}

94
src/hnsw/hnswIndex.ts
View file

@ -25,6 +25,29 @@ const DEFAULT_CONFIG: HNSWConfig = {
ml: 16 // Max level
}
/**
* @description Thrown by {@link JsHnswVectorIndex.flush} when one or more dirty
* nodes (or the system record) could not be persisted. The failed nodes remain
* in the dirty set for the next flush; this error tells the caller the flush did
* NOT achieve durability instead of a node-count that lies. Mandate: loud
* errors, never quiet losses.
*/
export class HnswFlushError extends Error {
constructor(
public readonly failedNodeCount: number,
public readonly systemFailed: boolean,
public override readonly cause?: Error
) {
super(
`HNSW flush did not achieve durability: ${failedNodeCount} node(s) failed to ` +
`persist${systemFailed ? ' and the system record (entryPoint/maxLevel) failed' : ''}. ` +
`Failed nodes remain dirty for retry.` +
(cause ? ` First error: ${cause.message}` : '')
)
this.name = 'HnswFlushError'
}
}
/**
* Implements {@link VectorIndexProvider}: the vector-index surface Brainy calls
* on whatever the `'vector'` factory returns (its own `JsHnswVectorIndex`, or a native
@ -150,41 +173,69 @@ export class JsHnswVectorIndex implements VectorIndexProvider {
const startTime = Date.now()
const nodeCount = this.dirtyNodes.size
// Batch persist all dirty nodes concurrently
// Batch persist all dirty nodes concurrently. A node whose connections FAIL
// to persist must stay dirty (retried on the next flush) — clearing it would
// silently drop the write forever. Track failures; only successfully-
// persisted (or deleted) nodes leave the dirty set, so nodes added to it
// during this flush are preserved.
const failedNodes = new Set<string>()
let firstError: Error | null = null
if (this.dirtyNodes.size > 0) {
const batchSize = 50 // Reasonable batch size for cloud storage
const nodeIds = Array.from(this.dirtyNodes)
for (let i = 0; i < nodeIds.length; i += batchSize) {
const batch = nodeIds.slice(i, i + batchSize)
const promises = batch.map(nodeId => {
const promises = batch.map(async nodeId => {
const noun = this.nouns.get(nodeId)
if (!noun) return Promise.resolve() // Node was deleted
return this.persistNodeConnections(nodeId, noun).catch(error => {
console.error(`[HNSW flush] Failed to persist node ${nodeId}:`, error)
})
if (!noun) return // Node was deleted — drop it from the dirty set.
try {
await this.persistNodeConnections(nodeId, noun)
} catch (error) {
failedNodes.add(nodeId)
if (firstError === null) firstError = error as Error
prodLog.error(`[HNSW flush] Failed to persist node ${nodeId}: ${(error as Error).message}`)
}
})
await Promise.allSettled(promises)
await Promise.all(promises)
}
this.dirtyNodes.clear()
// Remove only nodes that were persisted (or deleted mid-flush); keep the
// failed ones dirty for the next attempt.
for (const nodeId of nodeIds) {
if (!failedNodes.has(nodeId)) this.dirtyNodes.delete(nodeId)
}
}
// Persist system data if dirty
// Persist system data if dirty — keep it dirty on failure so the next flush
// retries rather than losing the entry-point/maxLevel update.
let systemFailed = false
if (this.dirtySystem) {
await this.storage.saveHNSWSystem({
entryPointId: this.entryPointId,
maxLevel: this.maxLevel
}).catch(error => {
console.error('[HNSW flush] Failed to persist system data:', error)
})
this.dirtySystem = false
try {
await this.storage.saveHNSWSystem({
entryPointId: this.entryPointId,
maxLevel: this.maxLevel
})
this.dirtySystem = false
} catch (error) {
systemFailed = true
if (firstError === null) firstError = error as Error
prodLog.error(`[HNSW flush] Failed to persist system data: ${(error as Error).message}`)
}
}
const duration = Date.now() - startTime
// Loud failure: if ANY node or the system record could not be persisted the
// flush did not achieve durability. Throw so callers (close(), explicit
// flush(), the flush-request watcher) see the failure instead of a success
// count that lies. The failed nodes/system stay dirty for retry.
if (failedNodes.size > 0 || systemFailed) {
throw new HnswFlushError(failedNodes.size, systemFailed, firstError ?? undefined)
}
if (nodeCount > 0) {
prodLog.info(`[HNSW] Flushed ${nodeCount} dirty nodes in ${duration}ms`)
}
@ -396,13 +447,14 @@ export class JsHnswVectorIndex implements VectorIndexProvider {
this.maxLevel = nounLevel
this.nouns.set(id, noun)
// Persist system data for first noun (previously skipped)
// Persist system data for first noun (previously skipped). Surface a
// persist failure loudly — the entry point is the root of the whole index;
// silently dropping it while addItem() returns the id would strand every
// future search on a rootless index. Mandate: never a quiet loss.
if (this.storage && this.persistMode === 'immediate') {
await this.storage.saveHNSWSystem({
entryPointId: this.entryPointId,
maxLevel: this.maxLevel
}).catch(error => {
console.error('Failed to persist initial HNSW system data:', error)
})
} else if (this.persistMode === 'deferred') {
this.dirtySystem = true

19
src/storage/adapters/fileSystemStorage.ts
View file

@ -17,6 +17,7 @@ import {
WriterLockInfo
} from '../baseStorage.js'
import { getBrainyVersion } from '../../utils/index.js'
import { isAbsentError } from '../../utils/errorClassification.js'
// Node.js modules - dynamically imported to avoid issues in browser environments
let fs: any
@ -446,8 +447,12 @@ export class FileSystemStorage extends BaseStorage {
return null
}
console.error(`Error reading object from ${pathStr}:`, error)
return null
// A real storage fault (EIO/EACCES/EMFILE/…) is NOT "object absent". The
// ENOENT branch (above) already returns null, and the corrupted-JSON
// branch (above) is a deliberate concurrent-write tolerance; a genuine
// fault reaching here must propagate loudly rather than masquerade as a
// missing object — which would corrupt reads and drive needless rebuilds.
throw error
}
}
@ -1207,8 +1212,14 @@ export class FileSystemStorage extends BaseStorage {
await this.ensureInitialized()
try {
return await fs.promises.readFile(this.blobPath(key))
} catch {
return null
} catch (err) {
// Absent blob → null (the documented contract). A real fault
// (EIO/EACCES/EMFILE/…) must NOT be masked as "absent": doing so makes a
// present-but-unreadable blob look missing and drives a needless rebuild
// or an empty read (the native provider consumes this). Mandate: loud
// errors, never quiet losses.
if (isAbsentError(err)) return null
throw err
}
}

36
src/storage/baseStorage.ts
View file

@ -3234,10 +3234,22 @@ export abstract class BaseStorage extends BaseStorageAdapter {
const priorCounted = isCountedVisibility(record?.visibility)
if (priorType) {
if (priorCounted) {
// Symmetric with the counted-add increment in saveNounMetadata_internal():
// decrement BOTH the user-facing scalar total (getNounCount / counts.json) AND
// the per-type bucket, then persist. The scalar decrement was previously
// omitted here, so deletes permanently inflated getNounCount() — the stale
// scalar wins pagination via Math.max(totalNounCount, collected.length) and is
// persisted by scheduleCountPersist(). With this, the invariant
// `totalNounCount === Σ nounCountsByType` holds across add / update-flip / delete.
this.decrementEntityCount(priorType)
const idx = TypeUtils.getNounIndex(priorType)
if (this.nounCountsByType[idx] > 0) {
this.nounCountsByType[idx]--
}
this.scheduleCountPersist().catch(() => {
// Ignore persist errors — the in-memory count is authoritative; a later
// operation retries the persist.
})
}
// Symmetric subtype decrement — same non-empty-string guard as the write path.
@ -3392,16 +3404,30 @@ export abstract class BaseStorage extends BaseStorageAdapter {
await this.ensureInitialized()
// Direct O(1) delete with ID-first path. Read the canonical record BEFORE
// removing it so the verb-subtype decrement is sourced from the edge's own
// metadata (`verb` type + `subtype`) rather than an id-keyed cache — symmetric
// with the increment in `saveVerbMetadata_internal()`. Verb deletes do not
// touch `verbCountsByType` in this path (matching prior behavior), so no
// visibility read is needed here.
// removing it so every decrement is sourced from the edge's own metadata
// (`verb` type, `subtype`, `visibility`) rather than an id-keyed cache —
// symmetric with the increments in `saveVerbMetadata_internal()`.
const path = getVerbMetadataPath(id)
const record = await this.readCanonicalObject(path)
await this.deleteCanonicalObject(path)
const priorVerb = record?.verb as VerbType | undefined
// Symmetric count decrement (previously OMITTED — verb deletes touched neither the
// scalar total nor the per-type bucket, so both inflated permanently). A COUNTED
// edge bumped BOTH the scalar (incrementVerbCount in saveVerbMetadata_internal) and
// the per-type bucket (the unconditional bump in saveVerb_internal that the metadata
// path keeps for counted edges). Delete must undo both, gated on the SAME visibility
// as the add, so `totalVerbCount === Σ verbCountsByType` holds across delete.
const priorCounted = isCountedVisibility(record?.visibility)
if (priorVerb && priorCounted) {
this.decrementVerbCount(priorVerb)
const idx = TypeUtils.getVerbIndex(priorVerb)
if (this.verbCountsByType[idx] > 0) this.verbCountsByType[idx]--
this.scheduleCountPersist().catch(() => {
// Ignore persist errors — in-memory count is authoritative; a later op retries.
})
}
const priorSubtype = typeof record?.subtype === 'string' && (record.subtype as string).length > 0
? (record.subtype as string)
: undefined

37
src/utils/errorClassification.ts Normal file
View file

@ -0,0 +1,37 @@
/**
* @module utils/errorClassification
* @description Shared classification of caught errors into "genuine absence" vs
* "real fault" — the antidote to blind `catch { return null }` handlers that
* cannot tell ENOENT (the object is legitimately not on disk) from EIO / EACCES
* / EMFILE / … (a transient or permission fault on data that IS on disk).
* Masking a fault as absence yields wrong results (a present record read as
* "not found") or a needless rebuild. Mandate: loud errors, never quiet losses.
*/
/**
* The error `code`s that denote GENUINE absence of a file/object. Only `ENOENT`
* ("no such file or directory") qualifies — on every platform Node maps a
* missing file/directory to ENOENT, and no other errno means "simply not
* there". Every other errno (EIO, EACCES, EPERM, EMFILE, ENFILE, EBUSY,
* ENOTDIR, EISDIR, ELOOP) is a real fault and must propagate, as must any error
* without an errno `code` (parse/decompress failures, generic Errors).
* `ENOTDIR`/`EISDIR` are deliberately faults: a path component of the wrong
* type is corruption, not benign absence. Named constant so a future
* genuine-absence code can be added in one reviewed place.
*/
const ABSENCE_CODES: ReadonlySet<string> = new Set(['ENOENT'])
/**
* @description True IFF `e` represents genuine absence (an ENOENT-class errno),
* for which returning `null`/`[]`/`undefined` is the correct answer. Returns
* `false` for every real fault, so the canonical call site is:
* `catch (e) { if (isAbsentError(e)) return null; throw e }`.
*
* @param e - The caught value (typed `unknown`; non-objects are never absence).
* @returns Whether the error means "the thing is simply not there".
*/
export function isAbsentError(e: unknown): boolean {
if (e === null || typeof e !== 'object') return false
const code = (e as { code?: unknown }).code
return typeof code === 'string' && ABSENCE_CODES.has(code)
}