HarperFast / HarperFast/harper

Table.deleteHistory() returns a non-zero count on RocksDB, where RocksTransactionLogStore.remove() is a no-op and nothing is deleted

Open
#2,469 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
JavaScript
Stars
89
Forks
10
Avg merge
2d 6h
Merged PRs (30d)
200

Description

`Table.deleteHistory()` returns a count of audit entries it did not delete when the table is on RocksDB.

`RocksTransactionLogStore.remove()` is a no-op — an `async remove() { /* TODO */ }` stub (`resources/RocksTransactionLogStore.ts`) — so `removeAuditEntry()` resolves without removing anything. `deleteHistory` increments `entriesDeleted` in its per-removal `onSuccess` callback either way, so a component author calling it on a RocksDB table gets a non-zero return for work that did not happen.

Found while reviewing #2458 (the audit staleness floor), which is why the floor deliberately is NOT raised on the RocksDB path there: nothing is actually pruned, so claiming it was would be worse than the misleading count.

## Why the obvious fix is wrong

An early `if (isRocksDB) return 0;` at the top of `deleteHistory` looks right and is not: the `cleanupDeletedRecords` branch calls `primaryStore.remove(key, version)`, which is real work on RocksDB and has a RocksDB-only regression test — `unitTests/resources/auditLog.test.js`, "RocksDB versioned removal preserves records recreated before or during removal", which `this.skip()`s *unless* `isPrimaryRocksDatabase` and calls `deleteHistory(Date.now() + 60_000, true)`. An early return would silently make that test dead and disable tombstone cleanup on the engine that needs it.

## Options

- Count only removals that actually removed something — needs `remove()` to report that, which it currently cannot.
- Return a shaped result that separates audit entries from tombstones (`{ auditEntriesDeleted, recordsDeleted }`), which is a breaking change to a public return value.
- Implement `RocksTransactionLogStore.remove()` so the count becomes true. The `TODO` there suggests this was always the intent; note that RocksDB prunes by whole log file, so per-entry removal may not be expressible.
- Document the return as "entries considered" and leave it.

Worth deciding which, since the return value is public API surface on a table.

Contributor guide

Open the contributing guide

Research direction

Start by reading resources/RocksTransactionLogStore.ts and the Table.deleteHistory/removeAuditEntry implementation to understand the no-op removal and success counting. Run unitTests/resources/auditLog.test.js, including the RocksDB versioned-removal case. Done means the chosen public return behavior is implemented and tested without disabling cleanupDeletedRecords or its RocksDB regression coverage.

Written by the indexing model from the issue text.

Assessment

Tech stack
javascript, nodejs
Domain
backend-api-design, databases
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Needs clarification
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.