HarperFast / HarperFast/harper
Table.deleteHistory() returns a non-zero count on RocksDB, where RocksTransactionLogStore.remove() is a no-op and nothing is deleted
- 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
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