fix: honest counters — removal never re-reads the removed record + repairIndex recounts and persists all rollups
Persisted entity totals inflated permanently: a delete's count decrement was sourced from re-reading the record being removed, so a null read (replace race, or a ghost left by a pre-8.3.1 partial delete) silently skipped the decrement while the paired add had counted — and Math.max(persistedTotal, scanned) meant no disk cleanup could ever lower the reported total. Reported by a production deployment with a write→delete→re-create repro minting drift within hours. - The caller's pre-delete read now rides through the entire delete path (remove()/removeMany()/plan → DeleteNoun/DeleteVerb operations → deleteNoun/deleteVerb → deleteNounMetadata/deleteVerbMetadata): a null internal read falls back to the known prior record instead of skipping the decrement. StorageAdapter.deleteNoun/deleteVerb gain an optional priorMetadata parameter (additive). - rebuildTypeCounts() rebuilt only the type-statistics arrays and computed the total just to LOG it — the persisted scalar (counts.json) survived every rebuild. One canonical walk now rebuilds every counter rollup (scalar totals + per-type maps + type statistics) and persists them; repairIndex() runs the recount unconditionally, since counters can be inflated over clean shelves.
This commit is contained in:
parent
d9fa3be648
commit
2e2ba9c9aa
5 changed files with 237 additions and 28 deletions
|
|
@ -3093,9 +3093,11 @@ export class Brainy<T = any> implements BrainyInterface<T> {
|
|||
)
|
||||
}
|
||||
|
||||
// Operation 3: Delete noun metadata
|
||||
// Operation 3: Delete noun (full removal). The pre-read metadata rides
|
||||
// along so the count decrement never depends on re-reading the record
|
||||
// being removed (a null re-read must not silently skip it).
|
||||
tx.addOperation(
|
||||
new DeleteNounMetadataOperation(this.storage, id)
|
||||
new DeleteNounMetadataOperation(this.storage, id, metadata)
|
||||
)
|
||||
|
||||
// Operations 4+: Delete all related verbs atomically
|
||||
|
|
@ -6871,8 +6873,10 @@ export class Brainy<T = any> implements BrainyInterface<T> {
|
|||
)
|
||||
}
|
||||
|
||||
// Pre-read metadata rides along: the count decrement must not
|
||||
// depend on re-reading the record being removed (see remove()).
|
||||
tx.addOperation(
|
||||
new DeleteNounMetadataOperation(this.storage, id)
|
||||
new DeleteNounMetadataOperation(this.storage, id, metadata)
|
||||
)
|
||||
|
||||
for (const verb of allVerbs) {
|
||||
|
|
@ -9265,7 +9269,9 @@ export class Brainy<T = any> implements BrainyInterface<T> {
|
|||
if (metadata) {
|
||||
plan.operations.push(new RemoveFromMetadataIndexOperation(this.metadataIndex, id, metadata))
|
||||
}
|
||||
plan.operations.push(new DeleteNounMetadataOperation(this.storage, id))
|
||||
// Pre-read metadata rides along: the count decrement must not depend on
|
||||
// re-reading the record being removed (see remove()).
|
||||
plan.operations.push(new DeleteNounMetadataOperation(this.storage, id, metadata))
|
||||
for (const verb of cascade.values()) {
|
||||
plan.operations.push(
|
||||
// Endpoint ints resolve at EXECUTE time — a cascade verb (or its
|
||||
|
|
@ -15278,15 +15284,22 @@ export class Brainy<T = any> implements BrainyInterface<T> {
|
|||
if (typeof pruner.pruneOrphanedEntities === 'function') {
|
||||
const orphans = await pruner.pruneOrphanedEntities()
|
||||
if (orphans.nouns.length + orphans.verbs.length > 0) {
|
||||
await pruner.rebuildTypeCounts?.()
|
||||
await pruner.rebuildSubtypeCounts?.()
|
||||
prodLog.warn(
|
||||
`[Brainy] repairIndex() pruned ${orphans.nouns.length} orphaned noun + ` +
|
||||
`${orphans.verbs.length} orphaned verb container(s) left by a pre-8.3.1 ` +
|
||||
`partial delete, and recomputed counts.`
|
||||
`partial delete.`
|
||||
)
|
||||
}
|
||||
}
|
||||
// SANCTIONED RECOUNT — unconditional, not gated on orphans found: the
|
||||
// persisted counters can be inflated over perfectly clean shelves (deletes
|
||||
// whose decrement was skipped by the removed-record re-read), and
|
||||
// Math.max(totalNounCount, scanned) means an inflated scalar can never
|
||||
// correct itself. rebuildTypeCounts() recomputes EVERY counter rollup
|
||||
// (scalar totals + per-type maps + type-statistics arrays) from one
|
||||
// canonical walk and persists them.
|
||||
await pruner.rebuildTypeCounts?.()
|
||||
await pruner.rebuildSubtypeCounts?.()
|
||||
|
||||
await this.metadataIndex.detectAndRepairCorruption()
|
||||
// Lift a failed-rollback write-quarantine: force a full rebuild so the
|
||||
|
|
|
|||
|
|
@ -848,7 +848,17 @@ export interface StorageAdapter {
|
|||
*/
|
||||
getNounsByNounType(nounType: string): Promise<HNSWNounWithMetadata[]>
|
||||
|
||||
deleteNoun(id: string): Promise<void>
|
||||
/**
|
||||
* Delete a noun — FULL canonical removal (both legs + the entity container).
|
||||
*
|
||||
* @param id The entity id.
|
||||
* @param priorMetadata OPTIONAL already-known metadata of the entity being
|
||||
* removed (the caller's pre-delete read). The count decrement must never
|
||||
* REQUIRE re-reading the record being removed: when the internal read
|
||||
* returns `null` (replace race, or a ghost left by an earlier version) the
|
||||
* decrement falls back to this record instead of being silently skipped.
|
||||
*/
|
||||
deleteNoun(id: string, priorMetadata?: NounMetadata | null): Promise<void>
|
||||
|
||||
/**
|
||||
* Save verb - Pure HNSW verb with core fields only
|
||||
|
|
@ -905,7 +915,15 @@ export interface StorageAdapter {
|
|||
*/
|
||||
getVerbsByType(type: string): Promise<HNSWVerbWithMetadata[]>
|
||||
|
||||
deleteVerb(id: string): Promise<void>
|
||||
/**
|
||||
* Delete a verb — FULL canonical removal (both legs + the container).
|
||||
*
|
||||
* @param id The relationship id.
|
||||
* @param priorMetadata OPTIONAL already-known metadata of the edge being
|
||||
* removed (the caller's pre-delete read); keeps the count decrement honest
|
||||
* when the internal read returns `null` (see `deleteNoun`).
|
||||
*/
|
||||
deleteVerb(id: string, priorMetadata?: VerbMetadata | null): Promise<void>
|
||||
|
||||
/**
|
||||
* Save metadata
|
||||
|
|
|
|||
|
|
@ -1616,7 +1616,7 @@ export abstract class BaseStorage extends BaseStorageAdapter {
|
|||
/**
|
||||
* Delete a noun from storage
|
||||
*/
|
||||
public async deleteNoun(id: string): Promise<void> {
|
||||
public async deleteNoun(id: string, priorMetadata?: NounMetadata | null): Promise<void> {
|
||||
await this.ensureInitialized()
|
||||
|
||||
// FULL removal (live-HEAD hygiene): remove BOTH canonical legs AND the
|
||||
|
|
@ -1631,7 +1631,9 @@ export abstract class BaseStorage extends BaseStorageAdapter {
|
|||
// success); a REAL fault must surface loudly (loud errors, never quiet
|
||||
// losses) and must never silently skip the count decrement — so this is NO
|
||||
// LONGER wrapped in a blind catch that masked faults as "file didn't exist".
|
||||
await this.deleteNounMetadata(id)
|
||||
// `priorMetadata` (the caller's pre-delete read) keeps the decrement honest
|
||||
// even when the canonical read inside returns null (replace race / ghost).
|
||||
await this.deleteNounMetadata(id, priorMetadata)
|
||||
|
||||
// Remove the now-empty entity container (a no-op for key/prefix stores).
|
||||
await this.removeCanonicalContainer(getNounVectorPath(id))
|
||||
|
|
@ -2936,14 +2938,15 @@ export abstract class BaseStorage extends BaseStorageAdapter {
|
|||
/**
|
||||
* Delete a verb from storage
|
||||
*/
|
||||
public async deleteVerb(id: string): Promise<void> {
|
||||
public async deleteVerb(id: string, priorMetadata?: VerbMetadata | null): Promise<void> {
|
||||
await this.ensureInitialized()
|
||||
|
||||
// FULL removal (see deleteNoun): both canonical legs + the verb container.
|
||||
// The blind catch that masked real faults as "no metadata file" is gone — a
|
||||
// genuine absence is already a no-op downstream; a real fault surfaces loudly.
|
||||
// `priorMetadata` keeps the count decrement honest on a null internal read.
|
||||
await this.deleteVerb_internal(id) // vectors.json leg
|
||||
await this.deleteVerbMetadata(id) // metadata leg + count decrement
|
||||
await this.deleteVerbMetadata(id, priorMetadata) // metadata leg + count decrement
|
||||
await this.removeCanonicalContainer(getVerbVectorPath(id))
|
||||
}
|
||||
/**
|
||||
|
|
@ -3596,19 +3599,32 @@ export abstract class BaseStorage extends BaseStorageAdapter {
|
|||
}
|
||||
|
||||
/**
|
||||
* Delete noun metadata from storage (ID-first, O(1) delete)
|
||||
* Delete noun metadata from storage (ID-first, O(1) delete).
|
||||
*
|
||||
* @param id - The entity id.
|
||||
* @param priorRecord - OPTIONAL already-known metadata of the entity being
|
||||
* removed (e.g. the pre-delete read `remove()` performs, or a captured
|
||||
* before-image). The count decrement must never REQUIRE re-reading the
|
||||
* thing being removed: when the canonical read here returns `null` (a
|
||||
* replace race, or a partial-delete ghost from an earlier version) the
|
||||
* decrement falls back to this record instead of being silently skipped —
|
||||
* the skip permanently inflated the persisted totals (adds counted, paired
|
||||
* removals not decremented), and `Math.max(totalNounCount, scanned)` made
|
||||
* the inflation unfixable by any disk cleanup.
|
||||
*/
|
||||
public async deleteNounMetadata(id: string): Promise<void> {
|
||||
public async deleteNounMetadata(id: string, priorRecord?: NounMetadata | null): Promise<void> {
|
||||
await this.ensureInitialized()
|
||||
|
||||
// Direct O(1) delete with ID-first path. Read the canonical record BEFORE
|
||||
// removing it: the per-type and subtype decrements are sourced from the
|
||||
// entity's own metadata (`noun` type, `subtype`, `visibility`) rather than an
|
||||
// id-keyed cache, keeping type-statistics honest across deletes — symmetric
|
||||
// with the increments in `saveNounMetadata_internal()`.
|
||||
// with the increments in `saveNounMetadata_internal()`. A null read falls
|
||||
// back to the caller-provided prior record (see @param priorRecord).
|
||||
const path = getNounMetadataPath(id)
|
||||
const record = await this.readCanonicalObject(path)
|
||||
const read = await this.readCanonicalObject(path)
|
||||
await this.deleteCanonicalObject(path)
|
||||
const record = read ?? priorRecord
|
||||
|
||||
const priorType = record?.noun as NounType | undefined
|
||||
// 8.0 visibility: an internal/system entity was never added to `nounCountsByType`
|
||||
|
|
@ -3782,16 +3798,20 @@ export abstract class BaseStorage extends BaseStorageAdapter {
|
|||
/**
|
||||
* Delete verb metadata from storage (ID-first, O(1) delete)
|
||||
*/
|
||||
public async deleteVerbMetadata(id: string): Promise<void> {
|
||||
public async deleteVerbMetadata(id: string, priorRecord?: VerbMetadata | null): Promise<void> {
|
||||
await this.ensureInitialized()
|
||||
|
||||
// Direct O(1) delete with ID-first path. Read the canonical record BEFORE
|
||||
// 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()`.
|
||||
// symmetric with the increments in `saveVerbMetadata_internal()`. A null
|
||||
// read falls back to the caller-provided prior record: the decrement must
|
||||
// never REQUIRE re-reading the thing being removed (a silent skip minted
|
||||
// permanent counter inflation — see deleteNounMetadata).
|
||||
const path = getVerbMetadataPath(id)
|
||||
const record = await this.readCanonicalObject(path)
|
||||
const read = await this.readCanonicalObject(path)
|
||||
await this.deleteCanonicalObject(path)
|
||||
const record = read ?? priorRecord
|
||||
|
||||
const priorVerb = record?.verb as VerbType | undefined
|
||||
// Symmetric count decrement (previously OMITTED — verb deletes touched neither the
|
||||
|
|
@ -4228,6 +4248,17 @@ export abstract class BaseStorage extends BaseStorageAdapter {
|
|||
this.nounCountsByType = new Uint32Array(NOUN_TYPE_COUNT)
|
||||
this.verbCountsByType = new Uint32Array(VERB_TYPE_COUNT)
|
||||
|
||||
// The SAME walk also rebuilds the user-facing scalar totals + per-type maps
|
||||
// persisted in counts.json (totalNounCount / totalVerbCount / entityCounts /
|
||||
// verbCounts). Previously only the type-statistics arrays were rebuilt and
|
||||
// the total was computed just to LOG it — so a drifted persisted scalar
|
||||
// (deletes whose decrement was skipped) survived every "rebuild" forever,
|
||||
// and Math.max(totalNounCount, scanned) made the inflation unfixable by any
|
||||
// disk cleanup. This method is now the SANCTIONED RECOUNT: one canonical
|
||||
// walk, every counter rollup rebuilt and persisted from it.
|
||||
const countedNouns = new Map<string, number>()
|
||||
const countedVerbs = new Map<string, number>()
|
||||
|
||||
// Scan noun shards
|
||||
for (let shard = 0; shard < 256; shard++) {
|
||||
const shardHex = shard.toString(16).padStart(2, '0')
|
||||
|
|
@ -4248,6 +4279,7 @@ export abstract class BaseStorage extends BaseStorageAdapter {
|
|||
if (typeIndex >= 0 && typeIndex < NOUN_TYPE_COUNT) {
|
||||
this.nounCountsByType[typeIndex]++
|
||||
}
|
||||
countedNouns.set(metadata.noun, (countedNouns.get(metadata.noun) || 0) + 1)
|
||||
}
|
||||
}
|
||||
} catch (error) {
|
||||
|
|
@ -4279,6 +4311,7 @@ export abstract class BaseStorage extends BaseStorageAdapter {
|
|||
if (typeIndex >= 0 && typeIndex < VERB_TYPE_COUNT) {
|
||||
this.verbCountsByType[typeIndex]++
|
||||
}
|
||||
countedVerbs.set(metadata.verb, (countedVerbs.get(metadata.verb) || 0) + 1)
|
||||
}
|
||||
}
|
||||
} catch (error) {
|
||||
|
|
@ -4295,7 +4328,19 @@ export abstract class BaseStorage extends BaseStorageAdapter {
|
|||
|
||||
const totalVerbs = this.verbCountsByType.reduce((sum, count) => sum + count, 0)
|
||||
const totalNouns = this.nounCountsByType.reduce((sum, count) => sum + count, 0)
|
||||
prodLog.info(`[BaseStorage] Rebuilt counts: ${totalNouns} nouns, ${totalVerbs} verbs`)
|
||||
|
||||
// The sanctioned recount half: replace the user-facing scalar totals + the
|
||||
// per-type maps with the walk's truth and PERSIST them (counts.json), so an
|
||||
// inflated persisted counter is actually corrected — not merely out-voted
|
||||
// in memory until the next reopen rehydrates the stale file.
|
||||
this.entityCounts = countedNouns
|
||||
this.verbCounts = countedVerbs
|
||||
this.totalNounCount = totalNouns
|
||||
this.totalVerbCount = totalVerbs
|
||||
this.countCache.clear()
|
||||
await this.persistCounts()
|
||||
|
||||
prodLog.info(`[BaseStorage] Rebuilt counts: ${totalNouns} nouns, ${totalVerbs} verbs (scalar + per-type persisted)`)
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
|
|||
|
|
@ -127,22 +127,33 @@ export class DeleteNounMetadataOperation implements Operation {
|
|||
|
||||
constructor(
|
||||
private readonly storage: StorageAdapter,
|
||||
private readonly id: string
|
||||
private readonly id: string,
|
||||
/**
|
||||
* OPTIONAL already-known metadata of the entity being removed (the caller's
|
||||
* pre-delete read). Removal must never REQUIRE re-reading the thing being
|
||||
* removed: if the reads here return null (replace race, or a ghost left by
|
||||
* an earlier version), the count decrement downstream falls back to this
|
||||
* record instead of being silently skipped — the skip minted permanent
|
||||
* counter inflation (adds counted, paired removals not decremented).
|
||||
*/
|
||||
private readonly priorMetadata?: NounMetadata | null
|
||||
) {}
|
||||
|
||||
async execute(): Promise<RollbackAction> {
|
||||
// Capture the FULL before-image (both legs) so the undo restores the whole
|
||||
// entity — a metadata-only rollback would leave the vector leg unrestored.
|
||||
// A null metadata read falls back to the caller's pre-delete read.
|
||||
const previousNoun = await this.storage.getNoun(this.id)
|
||||
const previousMetadata = await this.storage.getNounMetadata(this.id)
|
||||
const previousMetadata = (await this.storage.getNounMetadata(this.id)) ?? this.priorMetadata ?? null
|
||||
|
||||
if (!previousNoun && !previousMetadata) {
|
||||
// Nothing to delete - no rollback needed
|
||||
return async () => {}
|
||||
}
|
||||
|
||||
// Full removal: both canonical legs + the entity container + count decrement.
|
||||
await this.storage.deleteNoun(this.id)
|
||||
// Full removal: both canonical legs + the entity container + count decrement
|
||||
// (the prior record keeps the decrement honest on a null canonical read).
|
||||
await this.storage.deleteNoun(this.id, previousMetadata)
|
||||
|
||||
// Return rollback action
|
||||
return async () => {
|
||||
|
|
@ -268,9 +279,9 @@ export class DeleteVerbMetadataOperation implements Operation {
|
|||
return async () => {}
|
||||
}
|
||||
|
||||
// Delete verb (metadata + vector)
|
||||
// Note: StorageAdapter has deleteVerb but not deleteVerbMetadata
|
||||
await this.storage.deleteVerb(this.id)
|
||||
// Delete verb (metadata + vector). The pre-read rides along so the count
|
||||
// decrement never depends on re-reading the record being removed.
|
||||
await this.storage.deleteVerb(this.id, previousMetadata)
|
||||
|
||||
// Return rollback action
|
||||
return async () => {
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue