HarperFast / HarperFast/harper

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

Open
#1,413 2 comments 0 reactions 0 assignees View on GitHub
bug
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.