Apply audit retention continuously to RocksDB transaction logs

[kriszyp]

RocksDB audit retention now runs on the existing self-rearming cleanup cadence instead of depending on startup or disk-pressure signals, so eligible transaction-log segments are reclaimed continuously according to logging.auditRetention. The Rocks delay is a pure function of the pressure-adjusted retention window (a tenth of it, floored at DEFAULT_AUDIT_CLEANUP_DELAY) rather than LMDB's adaptive backoff: segment eligibility only changes on rotation/flush, so a per-entry delete count would only make it rescan the same files.

Fixes #846 — Transaction-log retention has no config-driven time/size control (and the storageReclamation hardening isn't on v5.1). Also fixes #2140, closed as a duplicate.

stopAuditCleanup() retires the loop from the database close, drop, and branch-close paths, before any table store closes so an in-flight LMDB pass cannot fire a tombstone callback against a closed primary store. The legacy per-table drop branch takes the same pair of calls; it closed and unlinked its primaryStore with neither. closeDatabase and dropDatabase also deregister storage reclamation now — only closeBranchHandles did, so a closed or dropped store left a handler, and the closure pinning the store, registered for the life of the process, with every re-open of the same path appending another.

The pin moves to @harperfast/rocksdb-js 2.8.0, which is a prerequisite rather than hygiene. The previously pinned 2.7.1 already refuses to purge above the durable flush floor, but at the boundary segment it compares logFile->size without resolving the extent: a lazily-registered segment reads 0, so 0 > positionInLogFile is false, the guard passes, and the segment is deleted with a tail that never reached RocksDB. 2.8.0 calls ensureExtent() there and refuses to purge when the extent cannot be resolved; its should not purge a log file whose tail is past the flushed position regression covers exactly that case and does not exist in 2.7.1. One purge per boot rarely lands on whichever segment sits at the floor — a retention-derived cadence takes a fresh shot at it every pass, which is what turns a latent hole into a prerequisite. This supersedes the earlier claim that the change was blocked on rocksdb-js #799; that PR remains a safety improvement on top, restricting deletion to a contiguous oldest prefix so a non-contiguous purge cannot leave a hole that silently truncates a reader, but it is not required here.

Root msgpackr moves to 2.0.6 alongside it. rocksdb-js 2.8.0 declares that version exactly, which an exact 2.0.5 root pin cannot satisfy, so the install nested a second msgpackr under node_modules/@harperfast/rocksdb-js — two module instances, each with its own extension registry and its own msgpackr-extract native addon. That release is a single commit, "Reject array and map lengths that exceed the remaining source data".

For the human reviewer

1. Retention window doubles as the replication safety window, and this is the one finding rated major. Rocks segment purge is driven by logging.auditRetention with no floor for the slowest consumer. A peer offline longer than that window resumes, reaches the purged prefix, and gets done from TransactionLog.query() — indistinguishable from being caught up, so it is recorded as caught up while permanently missing that range. txnlogReplayGapBytes is an analytics gauge with no consumer that forces a base copy. Continuous retention is what moves this from unreachable-in-steady-state to routine. The escalation signal belongs in the reader, filed as HarperFast/rocksdb-js#805; the alternative is gating purge here on the oldest live consumer position, a semantics change operators would feel on a knob they tune for disk. Decide whether Create withHarper() Next.js config utility #805 lands before this ships enabled-by-default, and confirm whether harper-pro has an escalation path core cannot see — if it does, the DESIGN.md paragraph overstates the risk and should say so.

2. The 2.8.0 bump carries far more than the retention fix — 186 commits, 55 files, +4606/-593 in src/, including transaction-lifecycle work (Hash long URLs into primary key for cache table storage efficiency #741 close-pending-txns-with-owner, HTTP Logs showing up in system (non external) logs #744 bounding the coordinated-retry park, txn-registry weakptr, deferred orphan cleanup). Local suites are clean across that surface; CI is the authoritative gate. The narrower alternative is asking upstream for a 2.7.2 carrying only the ensureExtent commit.

3. purgeLogs() is synchronous and unbounded on the last worker, which also serves requests. The LMDB path four lines below caps itself at MAX_DELETES_PER_CLEANUP and yields through setImmediate; the Rocks branch has no analogue. After an operator lowers logging.auditRetention from 30d to 1d, the next pass stats and unlinks the whole accumulated backlog in one blocking call. Accepted here because a steady-state pass has a handful of eligible segments, but the retention-lowering case is real and unbounded.

4. Cadence formula: retention/10 floored at 10s, pressure shortening cutoff and cadence together, versus keying the backoff on the purge counters rocksdb-js now exposes. One expression to change.

5. The last-worker conjunct on the re-arm is unreachable through both arming paths and kept deliberately as a backstop for a direct caller of the exported scheduleAuditCleanup: onStorageReclamation registers only on the last worker (it takes no skipThreadCheck) and the store-open arm gates on the same index. A reviewer may prefer it deleted as dead code or promoted to the sole ownership check. Both re-arm guards are Rocks-only; the LMDB arm keeps main's unconditional re-arm.

6. stopAuditCleanup() is irreversible — it latches per audit-store instance, relying on reopen constructing a fresh store. Correct for today's four callers; a future caller reusing a store object would get silent permanent retention loss.

Verification

test:unit:resources 1796 passing / 22 pending, test:unit:dataLayer 242 passing / 111 pending, test:unit:backup 74 passing, all against the 2.8.0 and msgpackr 2.0.6 pins.

The end-to-end route is a new live integration regression, audit-retention-rocks.test.ts: 1 passing in ~25s, of which ~20s is two cadence passes. It fails on origin/main with RocksDB transaction-log cleanup did not advance past purgeRuns=0, which is #2140's exact symptom. purgeRuns counts scans rather than deletions — transaction_log_store.cpp increments it before any eligibility check, marked observability-only — so an unproductive pass still advances it and the oracle is valid for the re-arm claim. Deletion itself is proved by the real-RocksDatabase unit test asserting only 3.txnlog survives.

The pressure-cadence regression drives a real reclamation signal through a path-scoped ratio getter and pins the shortened delay; dropping the priority term from the formula fails it (100 vs 20). The drop-path deregistration test and its closeDatabase sibling each fail without their fix. The LMDB audit-retention integration suite passed 4/4, preserving existing engine behavior. Prettier, oxlint and tsc --noEmit clean.

Three gaps stated rather than papered over: the last-worker gate has no multi-worker test, because asserting "no second loop exists" needs timing-based assertions; the live regression proves re-arm rather than on-disk deletion, which rocksdb-js's own suite covers; and it skips win32, where deleting a mapped segment raises a sharing violation instead of unlinking, so a Windows purge can warn-log and make no progress while a consumer holds a mapping.

Complexity: complicated

_{Review-Coverage: authored=claude; ran=gemini,codex; adjudicated=domain; declined=cursor-grok,cursor-composer; rounds=4 @ f258332}

_{Human-Review-Need: 4 (decisions: continuous-purge-before-reader-escalation, rocks-cadence-formula, purge-on-the-request-thread, irreversible-stop-semantics, rearm-guards-rocks-only, integration-oracle) @ f258332}

[gemini-code-assist]

Code Review

This pull request unifies the audit retention cleanup lifecycle for both LMDB and RocksDB storage engines, establishing a self-rearming, engine-independent process. RocksDB now shares the same scheduling, error containment, and backoff logic as LMDB, purging native transaction-log segments during its cleanup passes. Feedback on the changes points out a high-severity issue where the backoff delay for RocksDB is never decreased or reset when logs are actively being purged, which could cause the cleanup loop to run at an extremely slow frequency even under active write traffic.

[kriszyp]

Reviewer routing: @cb1kenobi is the RocksDB responsible expert and highest-affinity reviewer; @heskew reported #846 and can validate the original retention failure mode.

— Codex

[claude]

Reviewed; no blockers found.

[claude]

Re-raise: the draft-status gate from the prior thread has been undone

Thread #2338 (comment) was marked resolved on the basis of "Converted the PR back to Draft" (2026-08-26T22:46). Verified just now via gh api repos/HarperFast/harper/pulls/2338: draft: false, updated_at: 2026-08-27T15:13:23Z — the PR is currently ready-for-review again, not draft. The two facts that motivated the original gate are unchanged: rocksdb-js#799 is still open/unmerged (gh api repos/HarperFast/rocksdb-js/pulls/799 → merged: false), and package.json:182 still pins "@harperfast/rocksdb-js": "2.7.1".

Why it matters: this line turns purgeLogs() into a continuously self-rearming call against a native binding the PR description itself says lacks the repeated-purge watermark repair. With the PR back in ready-for-review state, mergeable_state: "blocked" is gated only on a required review — exactly the scenario the original finding warned about: an approval is what unblocks a merge the author says must not happen yet, and GitHub's UI gives a reviewer no signal that a merge here is unsafe.

Suggested fix: convert back to Draft until rocksdb-js#799 ships and the pin here moves past 2.7.1, per the PR's own stated blocking condition.

[claude]

Re-raise: draft-status gate still unaddressed

Thread #2338 (comment) (unresolved) flagged that this PR is ready-for-review (draft: false) while its own description says it must stay blocked until rocksdb-js#799 merges and the pin here moves past 2.7.1. Re-verified just now: the PR is still draft: false (mergeable_state: blocked), HarperFast/rocksdb-js#799 is still open/unmerged, and package.json:182 still pins "@harperfast/rocksdb-js": "2.7.1". Nothing has changed since the re-raise — this line still turns purgeLogs() into a continuous cadence against a binding the author says lacks the repeated-purge watermark repair, and GitHub gives no signal that a review approval here is exactly what would unblock a merge the author says must not happen yet.

Suggested fix: convert back to Draft until rocksdb-js#799 ships and the pin here moves past 2.7.1, per the PR's own stated blocking condition.

[socket-security]

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff	Package	Supply Chain Security	Vulnerability	Quality	Maintenance	License
	msgpackr@​2.0.5 ⏵ 2.0.6			+1	+4
	@​harperfast/​rocksdb-js@​2.7.1 ⏵ 2.8.0	+19

View full report

[claude]

Re-raise resolved: the draft-status gate from #2338 (comment) was pinned on @harperfast/rocksdb-js staying at 2.7.1, which the PR description says lacks the retention repair. 5aeef6cde bumps the pin to 2.8.0 — a real npm-published release (registry integrity hash matches, published 2026-08-25, predating this thread) — and its commit message describes an ensureExtent() check added there that refuses to purge a boundary segment whose extent can't be resolved, which is the specific defect this gate was about. I can't execute rocksdb-js's native suite in this environment to independently verify that mechanism end-to-end, but the version bump itself is genuine, not a placeholder, so I'm not re-raising the draft-status finding. The remaining rocksdb-js#799 (contiguous-prefix restriction) is a disclosed, tracked follow-up (rocksdb-js#805, documented in DESIGN.md), not a blocker on this pin.

[claude]

Suggestion (non-blocking): No test exercises this legacy per-table drop branch (isLegacy config, LMDB-only) to confirm stopAuditCleanup()/removeStorageReclamation() actually fire before close()/unlinkSync(). The sibling teardown paths this mirrors (closeDatabase/dropDatabase in databases.ts) already got direct coverage in this PR (unitTests/resources/databases.test.js:412 and :442); consider adding an equivalent case here so a future regression on this rarer path isn't silent.

[claude]

Suggestion (non-blocking): This check runs after the for...of protocol has already called iterator.next() for the current iteration, not before it. So when a pass resumes from await removeAuditEntry(...) or await new Promise(setImmediate) following a concurrent stopAuditCleanup(), one more cursor read still reaches the store before this break takes effect — the comment above ("checked... underneath the resumed cursor") reads as a stronger guarantee than the code delivers, since the cursor is touched once more before the flag is checked. Driving the iterator manually would close that gap, e.g.:

const iterator = auditStore.getRange({ ... })[Symbol.iterator]();
while (!cleanupStopped) {
	const { value: auditRecord, done } = iterator.next();
	if (done) break;
	// ... existing body
}

This guards the .next() call itself, not just the processing of whatever it returns.

[claude]

Suggestion (non-blocking): This fixes the exact race the prior thread flagged (.next() guarded by the while condition instead of a for-of body check), but no test exercises that race directly — a stopAuditCleanup() landing between two entries.next() calls. Consider a test that mocks auditStore.getRange to return a controllable iterator and calls stopAuditCleanup() from inside the first removeAuditEntry, then asserts .next() is never called a second time and updateLastRemoved is skipped. That would lock in this fix against a future regression back to the for-of shape.