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.
Table.deleteHistory()returns a count of audit entries it did not delete when the table is on RocksDB.RocksTransactionLogStore.remove()is a no-op — anasync remove() { /* TODO */ }stub (resources/RocksTransactionLogStore.ts) — soremoveAuditEntry()resolves without removing anything.deleteHistoryincrementsentriesDeletedin its per-removalonSuccesscallback 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 ofdeleteHistorylooks right and is not: thecleanupDeletedRecordsbranch callsprimaryStore.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", whichthis.skip()s unlessisPrimaryRocksDatabaseand callsdeleteHistory(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
remove()to report that, which it currently cannot.{ auditEntriesDeleted, recordsDeleted }), which is a breaking change to a public return value.RocksTransactionLogStore.remove()so the count becomes true. TheTODOthere suggests this was always the intent; note that RocksDB prunes by whole log file, so per-entry removal may not be expressible.Worth deciding which, since the return value is public API surface on a table.