Skip to content

audit: getRecordAtTime crash family (3 triggers) + silent-wrong reverse-reconstruction (4 mechanisms) #1413

Description

@kriszyp

Summary

resources/crdt.ts:getRecordAtTime has two distinct categories of defect, both in the same reverse-reconstruction function. A thorough fix pass covers all of them.

Part 1 — Crash family: null/undefined dereference in getRecordAtTime (three independent triggers)

read_audit_log search_type:hash_value returns HTTP 500 with a null/undefined dereference. Three triggers, each at a different unguarded site:

Trigger A — Window-spanning updated+deleted history (crdt.ts:~137 PK-stamp, QA-148, reframes #1330)
Table.getHistoryOfRecord walks the audit chain in fixed 100ms auditWindow windows. For a key with history insert + ≥2 updates + delete spanning >100ms, the delete reconstructs to null (case 'delete': record = null, crdt.ts:97) across a window boundary, then the downstream PK-stamp does record['id'] = … on null → TypeError: Cannot set properties of null. The "delete-then-reinsert" framing in the original #1330 title is a red herring — reinsert sequences read faithfully; the real trigger is the window-span.

Trigger B — Prune-induced missing entry (crdt.ts:107 second loop, QA-161, LMDB only)
After per-entry audit pruning (retention) removes an early entry that a still-live key's reconstruction needs, read_audit_log hash_value → 500 Cannot read properties of undefined (reading 'type') at line 107. The FIRST reverse loop has a guard (if (!auditEntry) break, line 88); the SECOND "fill-in-the-blanks" loop (lines 104-122) reads auditEntry.type (107) and auditEntry.previousVersion (121) with no null guard. RocksDB's coarse whole-file prune doesn't strip entries in practice; LMDB per-entry prune does.

Trigger C — Stamp on a reinsert-after-delete (crdt.ts:~137, the original #1330 trigger)
The original filed crash — read_audit_log on a delete-then-reinsert key whose history spans a window boundary — is the same mechanism as Trigger A (the delete window-span); reinsert just changes the conditions under which it surfaces.

Fix for all three: null-guard every record['id'] = … / record[key] = … assignment (so a null record from a delete entry is handled gracefully), AND add the missing null guard in the second fill-in-the-blanks loop (lines 104-122) to mirror the first loop's if (!auditEntry) break.


Part 2 — Silent-wrong reconstruction: reverse-walk returns incorrect historical state (QA-155/QA-158)

Beyond the crashes, the reverse-walk silently returns wrong historical data with HTTP 200 for PATCH-built or mixed-op history. Three root mechanisms plus an off-by-one:

(a) Plain-field bleedapplyReverse (crdt.ts:60-71) does not revert plain (non-__op__) values; it only marks them unknown. The fill-unknowns loop restores each key once from the nearest prior partial that contains it — so a field not covered by any prior partial keeps the HEAD (latest) value. Result: earlier versions bleed later field values. Observed: overlapping PATCH sequences return HEAD values for fields not touched by a prior patch.

(b) addTo/atomic-delta never reversed — wrong cumulativegetRecordAtTime fetches a patch entry via getValue(store) with no fullRecord/auditTime args (crdt.ts:94), so it hits the plain-partial branch and arrives without the {__op__:'add'} marker. It falls into the plain-unknowns path and is never reverse-added. A counter reconstructed at any non-HEAD point shows the final cumulative HEAD value regardless of the target instant.

(c) Delete-gap backward bleed — the reverse walk stops once auditTime ≤ timestamp and never crosses a delete boundary to reset. A reinserted record's fields bleed backward into pre-delete versions.

(d) Off-by-one at "one-before-HEAD" — the strict > comparison (crdt.ts:86) stops one entry too early, returning the next version's value when querying the instant just before the final version.

Control: a full PUT-replace reconstructs correctly (snaps the whole record); the delete instant itself correctly yields null. Everything else reachable via the patch reverse-walk is wrong.

Fix: the reverse reconstruction needs to (a) properly revert plain values per-version rather than single-pass filling unknowns from the nearest partial, (b) preserve and reverse the __op__:add marker (fix the partial-read path at crdt.ts:94 so delta entries carry their operator), (c) cross the delete boundary correctly so reinserted state doesn't bleed into pre-delete versions, and (d) change the strict > to >= (or equivalent) to fix the off-by-one.


Repro commands

# Crash — trigger A (window-span)
npm run test:integration -- "integrationTests/qa-scratch/qa148-audit-trigger.test.ts"

# Crash — trigger B (prune-induced, LMDB)
HARPER_STORAGE_ENGINE=lmdb npm run test:integration -- "integrationTests/qa-scratch/qa161-audit-prune.test.ts"

# Silent-wrong — overlapping PATCH
npm run test:integration -- "integrationTests/qa-scratch/qa155-time-travel.test.ts"

# Silent-wrong — mixed ops (PUT/addTo/delete-reinsert)
npm run test:integration -- "integrationTests/qa-scratch/qa158-timetravel-mixed.test.ts"

Harper: 7aaa5a152 (branch kris/repl-no-revalidate-1302-core). All cited crdt.ts / Table.ts code is byte-identical to main @6797f091d — reproduces on main.


Surfaced by the QA-explorer exploratory campaign for @kris. Consolidates the F-015/#1330 crash family (three triggers, two unguarded loops) with the F-032 silent-wrong reconstruction (four mechanisms). Filed by Claude (Sonnet 4.6) for @kris.

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    No type

    Fields

    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions