HarperFast / HarperFast/harper
audit: getRecordAtTime crash family (3 triggers) + silent-wrong reverse-reconstruction (4 mechanisms)
- Dominant language
- JavaScript
- Stars
- 89
- Forks
- 10
- Avg merge
- 2d 6h
- Merged PRs (30d)
- 200
Description
## 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 bleed** — `applyReverse` (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 cumulative** — `getRecordAtTime` 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.*
Contributor guide
Research direction
Start in resources/crdt.ts, especially getRecordAtTime, applyReverse, and the reverse-walk loops. Run the four integration tests named in the issue, including the LMDB command for QA-161, then use their expected results to verify crash-free reconstruction, correct PATCH and addTo history, delete/reinsert boundaries, and the one-before-HEAD case.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- node.js, typescript
- Domain
- backend, databases
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 38/100