fix: surface segment/entity read faults loudly instead of masking as absent
ColumnStore silently skipped a manifest-listed segment it could not load: a corrupt or missing segment dropped every one of its entities out of filter/rangeQuery/sortTopK with no error, so an inconsistent index read as a merely short result. It now throws a typed ColumnSegmentLoadError when a listed segment yields undecodable or no bytes, and lets a genuine storage IO fault propagate verbatim. Only a field with no manifest at all stays benign (nothing was ever written for it). baseStorage.getNoun_internal / getVerb_internal likewise caught every error and returned null, reporting a present-but-unreadable entity as "not found". They now return null only for genuine ENOENT-class absence (via isAbsentError) and rethrow real faults and deserialize errors. Pattern-B (blind catch) hardening: absence -> null, fault -> loud.
This commit is contained in:
parent
119087a75c
commit
af5d2f389b
3 changed files with 216 additions and 33 deletions
|
|
@ -69,6 +69,34 @@ interface HeapEntry {
|
|||
* await store.flush()
|
||||
* const newest = await store.sortTopK('createdAt', 'desc', 10) // bigint[]
|
||||
*/
|
||||
/**
|
||||
* @description Thrown when a segment the manifest lists cannot be loaded —
|
||||
* either it yields no bytes (missing/empty on disk) or its bytes are
|
||||
* undecodable (corruption). Previously such a segment was silently skipped,
|
||||
* dropping ALL of its entities from every `filter`/`rangeQuery`/`sortTopK` with
|
||||
* no error — a manifest↔segments divergence that looked like a short result.
|
||||
* Surfacing it loudly makes the divergence visible and repairable. (A genuine
|
||||
* storage IO fault is a different class — it propagates as the underlying error,
|
||||
* not wrapped in this.)
|
||||
*/
|
||||
export class ColumnSegmentLoadError extends Error {
|
||||
/** The indexed field whose segment failed to load. */
|
||||
public readonly field: string
|
||||
/** The manifest-listed segment id that could not be loaded. */
|
||||
public readonly segmentId: number | string
|
||||
constructor(field: string, segmentId: number | string, reason: string) {
|
||||
super(
|
||||
`ColumnStore segment ${field}:${segmentId} is listed in the manifest but ` +
|
||||
`could not be loaded (${reason}). The index is inconsistent with its ` +
|
||||
`manifest — rebuild/repair the metadata index rather than trusting a ` +
|
||||
`short query result.`
|
||||
)
|
||||
this.name = 'ColumnSegmentLoadError'
|
||||
this.field = field
|
||||
this.segmentId = segmentId
|
||||
}
|
||||
}
|
||||
|
||||
export class ColumnStore implements ColumnStoreProvider {
|
||||
private storage!: StorageAdapter
|
||||
private idMapper!: EntityIdMapper
|
||||
|
|
@ -594,14 +622,15 @@ export class ColumnStore implements ColumnStoreProvider {
|
|||
let cursor = this.segmentCache.get(cacheKey)
|
||||
|
||||
if (!cursor) {
|
||||
const loaded = await this.loadSegmentCursor(field, seg)
|
||||
if (loaded) {
|
||||
cursor = loaded
|
||||
// loadSegmentCursor either returns a cursor or THROWS — a corrupt /
|
||||
// missing manifest-listed segment raises ColumnSegmentLoadError and a
|
||||
// real storage fault propagates, so a listed segment is never silently
|
||||
// dropped from the result set.
|
||||
cursor = await this.loadSegmentCursor(field, seg)
|
||||
this.segmentCache.set(cacheKey, cursor)
|
||||
}
|
||||
}
|
||||
|
||||
if (cursor) cursors.push(cursor)
|
||||
cursors.push(cursor)
|
||||
}
|
||||
|
||||
return cursors
|
||||
|
|
@ -615,11 +644,13 @@ export class ColumnStore implements ColumnStoreProvider {
|
|||
* `.cidx` object-path so indexes written before the format unification keep
|
||||
* loading correctly. Mirror of cortex's 2.3.1 read-side fallback.
|
||||
*/
|
||||
private async loadSegmentCursor(field: string, seg: SegmentMeta): Promise<ColumnSegmentCursor | null> {
|
||||
private async loadSegmentCursor(field: string, seg: SegmentMeta): Promise<ColumnSegmentCursor> {
|
||||
const manifest = this.manifests.get(field)
|
||||
if (!manifest) return null
|
||||
if (!manifest) {
|
||||
// Defensive: getSegmentCursors guards on the manifest before calling.
|
||||
throw new ColumnSegmentLoadError(field, seg.id, 'no manifest for field')
|
||||
}
|
||||
|
||||
try {
|
||||
let buf: Buffer | null = null
|
||||
|
||||
const storage = this.storage as unknown as {
|
||||
|
|
@ -627,7 +658,9 @@ export class ColumnStore implements ColumnStoreProvider {
|
|||
readObjectFromPath: (path: string) => Promise<any>
|
||||
}
|
||||
|
||||
// Preferred: raw blob at the cor-shared key.
|
||||
// Preferred: raw blob at the cor-shared key. A real IO fault PROPAGATES —
|
||||
// loadBinaryBlob throws on a fault and returns null only for genuine absence
|
||||
// (a present-but-unreadable segment must not read as "missing").
|
||||
if (typeof storage.loadBinaryBlob === 'function') {
|
||||
const key = `${this.basePath}/${field}/L${seg.level}-${String(seg.id).padStart(6, '0')}`
|
||||
const blob = await storage.loadBinaryBlob(key)
|
||||
|
|
@ -638,16 +671,24 @@ export class ColumnStore implements ColumnStoreProvider {
|
|||
if (!buf) {
|
||||
const segPath = manifest.segmentPath(seg.level, seg.id)
|
||||
const stored = await storage.readObjectFromPath(segPath)
|
||||
if (!stored) return null
|
||||
if (stored) {
|
||||
if (stored._binary && stored.data) {
|
||||
buf = Buffer.from(stored.data, 'base64')
|
||||
} else if (Buffer.isBuffer(stored)) {
|
||||
buf = stored
|
||||
} else {
|
||||
return null
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// A manifest-listed segment that yields NO loadable bytes is corruption, not
|
||||
// benign absence: swallowing it silently dropped all of the segment's
|
||||
// entities from every filter/rangeQuery/sortTopK with no error. Surface it
|
||||
// loudly so the manifest↔segments divergence is visible (and repairable).
|
||||
if (!buf) {
|
||||
throw new ColumnSegmentLoadError(field, seg.id, 'manifest-listed segment has no loadable bytes')
|
||||
}
|
||||
|
||||
try {
|
||||
const parsed = readSegmentFromBuffer(buf)
|
||||
return new ColumnSegmentCursor(
|
||||
parsed.header,
|
||||
|
|
@ -655,8 +696,13 @@ export class ColumnStore implements ColumnStoreProvider {
|
|||
parsed.entityIds,
|
||||
parsed.tombstones
|
||||
)
|
||||
} catch {
|
||||
return null
|
||||
} catch (err) {
|
||||
// Undecodable bytes for a listed segment — corruption, not a short result.
|
||||
throw new ColumnSegmentLoadError(
|
||||
field,
|
||||
seg.id,
|
||||
`segment decode failed: ${(err as Error).message}`
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -29,6 +29,7 @@ import { getShardId } from './sharding.js'
|
|||
import { BlobStorage, type BlobStoreAdapter } from './blobStorage.js'
|
||||
import { unwrapBinaryData } from './binaryDataCodec.js'
|
||||
import { prodLog } from '../utils/logger.js'
|
||||
import { isAbsentError } from '../utils/errorClassification.js'
|
||||
import { BrainyError } from '../errors/brainyError.js'
|
||||
import { MetadataWriteBuffer } from '../utils/metadataWriteBuffer.js'
|
||||
import {
|
||||
|
|
@ -4064,8 +4065,12 @@ export abstract class BaseStorage extends BaseStorageAdapter {
|
|||
return this.deserializeNoun(noun)
|
||||
}
|
||||
} catch (error) {
|
||||
// Entity not found
|
||||
return null
|
||||
// A real storage/deserialize fault is NOT "entity not found":
|
||||
// readCanonicalObject returns null for genuine absence and only throws on
|
||||
// a real fault, so masking it here reports a present-but-unreadable entity
|
||||
// as missing. Absence → null, fault → propagate loudly.
|
||||
if (isAbsentError(error)) return null
|
||||
throw error
|
||||
}
|
||||
|
||||
return null
|
||||
|
|
@ -4175,8 +4180,12 @@ export abstract class BaseStorage extends BaseStorageAdapter {
|
|||
return this.deserializeVerb(verb)
|
||||
}
|
||||
} catch (error) {
|
||||
// Entity not found
|
||||
return null
|
||||
// A real storage/deserialize fault is NOT "relationship not found":
|
||||
// readCanonicalObject returns null for genuine absence and only throws on
|
||||
// a real fault, so masking it here reports a present-but-unreadable edge
|
||||
// as missing. Absence → null, fault → propagate loudly.
|
||||
if (isAbsentError(error)) return null
|
||||
throw error
|
||||
}
|
||||
|
||||
return null
|
||||
|
|
|
|||
128
tests/unit/indexes/columnStore/segment-load-fault.test.ts
Normal file
128
tests/unit/indexes/columnStore/segment-load-fault.test.ts
Normal file
|
|
@ -0,0 +1,128 @@
|
|||
/**
|
||||
* @module tests/unit/indexes/columnStore/segment-load-fault
|
||||
* @description Pattern-B acceptance for the ColumnStore (finding 4): a segment
|
||||
* the manifest LISTS but that cannot be loaded must never be silently skipped —
|
||||
* doing so dropped every entity in that segment out of `filter`/`rangeQuery`/
|
||||
* `sortTopK` with no error, so a corrupt index looked like a merely short result.
|
||||
*
|
||||
* The three failure classes and their required behaviour:
|
||||
* - a real storage IO fault (EIO) PROPAGATES verbatim — a present-but-unreadable
|
||||
* segment is not "absent", so it must not read as an empty result;
|
||||
* - a manifest-listed segment with undecodable bytes throws `ColumnSegmentLoadError`;
|
||||
* - a manifest-listed segment with NO bytes (gone on disk) throws `ColumnSegmentLoadError`.
|
||||
* Only genuine absence stays benign: querying a field that has no manifest at all
|
||||
* returns empty (nothing was ever written for it) — that is not a fault.
|
||||
*/
|
||||
import { describe, it, expect, beforeEach } from 'vitest'
|
||||
import {
|
||||
ColumnStore,
|
||||
ColumnSegmentLoadError
|
||||
} from '../../../../src/indexes/columnStore/ColumnStore.js'
|
||||
import { MemoryStorage } from '../../../../src/storage/adapters/memoryStorage.js'
|
||||
import { EntityIdMapper } from '../../../../src/utils/entityIdMapper.js'
|
||||
|
||||
type FaultMode = 'none' | 'io' | 'corrupt' | 'missing'
|
||||
|
||||
/**
|
||||
* A MemoryStorage that can fault reads of persisted column SEGMENTS only
|
||||
* (keys/paths containing the `L0-` segment marker). Manifest and DELETED-bitmap
|
||||
* reads pass through untouched so a fresh store still initialises normally — the
|
||||
* fault is isolated to the exact seam finding 4 hardened.
|
||||
*/
|
||||
class FaultInjectingStorage extends MemoryStorage {
|
||||
public faultMode: FaultMode = 'none'
|
||||
|
||||
private eio(): Error {
|
||||
const e = new Error('simulated disk read fault') as Error & { code: string }
|
||||
e.code = 'EIO'
|
||||
return e
|
||||
}
|
||||
|
||||
public async loadBinaryBlob(key: string): Promise<Buffer | null> {
|
||||
if (this.faultMode !== 'none' && key.includes('/L0-')) {
|
||||
if (this.faultMode === 'io') throw this.eio()
|
||||
// Too small to hold even a header → readSegmentFromBuffer throws → wrapped.
|
||||
if (this.faultMode === 'corrupt') return Buffer.from([1, 2, 3, 4, 5])
|
||||
if (this.faultMode === 'missing') return null
|
||||
}
|
||||
return super.loadBinaryBlob(key)
|
||||
}
|
||||
}
|
||||
|
||||
describe('ColumnStore segment-load faults surface loudly, absence stays benign (finding 4)', () => {
|
||||
let storage: FaultInjectingStorage
|
||||
let idMapper: EntityIdMapper
|
||||
|
||||
beforeEach(async () => {
|
||||
storage = new FaultInjectingStorage()
|
||||
await storage.init()
|
||||
idMapper = new EntityIdMapper({ storage, storageKey: 'test:idMapper' })
|
||||
await idMapper.init()
|
||||
|
||||
// Write one persisted L0 segment for `createdAt`, then close the writer.
|
||||
const writer = new ColumnStore({ flushThreshold: 10 })
|
||||
await writer.init(storage, idMapper)
|
||||
for (let i = 0; i < 5; i++) {
|
||||
writer.addEntity(BigInt(idMapper.getOrAssign(`e${i}`)), {
|
||||
createdAt: (i + 1) * 100
|
||||
})
|
||||
}
|
||||
await writer.flush()
|
||||
await writer.close()
|
||||
storage.faultMode = 'none'
|
||||
})
|
||||
|
||||
// Fresh reader over the same storage: empty segment cache, so every query is
|
||||
// forced to actually load the persisted segment (that is the seam under test).
|
||||
const reopen = async (): Promise<ColumnStore> => {
|
||||
const s = new ColumnStore({ flushThreshold: 10 })
|
||||
await s.init(storage, idMapper)
|
||||
return s
|
||||
}
|
||||
|
||||
it('propagates a storage IO fault verbatim — not [] and not a ColumnSegmentLoadError', async () => {
|
||||
storage.faultMode = 'io'
|
||||
const store = await reopen()
|
||||
await expect(store.filter('createdAt', 300)).rejects.toMatchObject({
|
||||
code: 'EIO'
|
||||
})
|
||||
await store.close()
|
||||
})
|
||||
|
||||
it('throws ColumnSegmentLoadError when a manifest-listed segment is undecodable', async () => {
|
||||
storage.faultMode = 'corrupt'
|
||||
const store = await reopen()
|
||||
await expect(
|
||||
store.sortTopK('createdAt', 'desc', 10)
|
||||
).rejects.toBeInstanceOf(ColumnSegmentLoadError)
|
||||
await store.close()
|
||||
})
|
||||
|
||||
it('throws ColumnSegmentLoadError when a manifest-listed segment has no loadable bytes', async () => {
|
||||
storage.faultMode = 'missing'
|
||||
const store = await reopen()
|
||||
await expect(
|
||||
store.rangeQuery('createdAt', 100, 500)
|
||||
).rejects.toBeInstanceOf(ColumnSegmentLoadError)
|
||||
await store.close()
|
||||
})
|
||||
|
||||
it('a field with no manifest is genuine absence — returns empty, never throws', async () => {
|
||||
storage.faultMode = 'none'
|
||||
const store = await reopen()
|
||||
const bitmap = await store.filter('no_such_field', 'x')
|
||||
expect(bitmap.size).toBe(0)
|
||||
const sorted = await store.sortTopK('no_such_field', 'asc', 10)
|
||||
expect(sorted).toEqual([])
|
||||
await store.close()
|
||||
})
|
||||
|
||||
it('with no fault, the persisted segment still loads and answers queries', async () => {
|
||||
storage.faultMode = 'none'
|
||||
const store = await reopen()
|
||||
const sorted = await store.sortTopK('createdAt', 'desc', 10)
|
||||
const uuids = sorted.map((id) => idMapper.getUuid(Number(id)))
|
||||
expect(uuids).toEqual(['e4', 'e3', 'e2', 'e1', 'e0'])
|
||||
await store.close()
|
||||
})
|
||||
})
|
||||
Loading…
Add table
Add a link
Reference in a new issue