Apply audit retention continuously to RocksDB transaction logs (v5.2) - #2508
Draft
kriszyp wants to merge 1 commit into
Draft
Apply audit retention continuously to RocksDB transaction logs (v5.2)#2508kriszyp wants to merge 1 commit into
kriszyp wants to merge 1 commit into
Conversation
Cherry-pick of #2338 onto v5.2. The Rocks audit cleanup loop re-arms on a retention-derived cadence instead of running once per boot, and every teardown path retires the loop and deregisters storage reclamation before releasing the stores it holds. Adapted for v5.2, which does not have main's branch-database feature or its storageReclamation deregistration helpers: - `server/storageReclamation.ts` gains `removeStorageReclamation()`, copied verbatim from main (added there by #2285/#2381, not by #2338). Nothing on v5.2 called it, so this is a pure addition. - The `closeBranchHandles()` retirement hunk and the `openBranchDatabase` test additions are dropped: neither the function nor the suite exists on v5.2. The two reclamation-deregistration tests #2338 placed inside that suite test `closeDatabase`/`dropDatabase`, not branches, so they move to their own describe. - `Table.ts`'s import keeps only what v5.2 uses: `removeStorageReclamationHandler` is main-only and unreferenced here. - DESIGN.md gains only #2338's new retention section; the removal-loop section it sat below is main-only. Its one sentence about branch `close()` is trimmed. - The `query-array-scoping.test.js` formatting fix is dropped — it repaired a main-only Format Check break and that file does not exist on v5.2. rocksdb-js stays at 2.8.0, which v5.2 already pins (6ca418d); #2338's own bump was to the same version. (cherry picked from commit d1eae3d) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Code Review
This pull request establishes an engine-independent, self-rearming audit retention cleanup lifecycle for both LMDB and RocksDB. It refactors scheduleAuditCleanup to support RocksDB log purging on a continuous cadence, introduces a stopAuditCleanup method to safely retire the cleanup loop and return a drain barrier, and ensures proper cleanup of storage reclamation handlers via removeStorageReclamation during database/table teardown (close or drop). Extensive integration and unit tests have been added to verify these behaviors, including cadence holding, retirement, disk pressure adjustments, and resource cleanup. There are no review comments, so I have no feedback to provide.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Backports Apply audit retention continuously to RocksDB transaction logs to the
v5.2release line. On RocksDB, audit retention ran exactly onepurgeLogs()per boot: the pass returned before scheduling its successor, so a long-lived node's transaction logs grew without bound until a restart or a disk-pressure signal. The Rocks pass now re-arms on a cadence derived from the pressure-adjusted retention window, and every teardown path —closeDatabase,dropDatabase, and the legacy per-table drop — retires the loop and deregisters its storage-reclamation handler before releasing the stores it holds.v5.2already pins@harperfast/rocksdb-js2.8.0 (6ca418d), which is the version #2338 needed for the purge extent-resolution fix, andmsgpackr2.0.6. No dependency change is required here, and the unreleased rocksdb-js Keep transaction-log retention on a durable sequence floor is not a prerequisite — it is a further safety improvement on top (contiguous-prefix deletion), not something continuous retention depends on.For the human reviewer
The cherry-pick did not apply cleanly; five files conflicted, all because
maincarries workv5.2does not.Whether the behavior change belongs on
v5.2at all. This carries one cadence change alongside the crash-safety and handler-leak fixes:v5.2nodes gain a permanent background purge loop they did not have before, where Rocks previously purged only on a pressure event. I kept Apply audit retention continuously to RocksDB transaction logs #2338 whole rather than splitting the loop from the fixes, because the fixes exist because of the loop — the teardown races and the reclamation-handler leak only become reachable once a pass can be in flight at close time — and a split would ship half of a reviewed change. Reversible only by a further cherry-pick, not a local revert, so it is worth ruling on before merge.removeStorageReclamation()is new tov5.2. Apply audit retention continuously to RocksDB transaction logs #2338 calls it from three teardown sites, but the helper was added tomainby Let a database be opened into a caller-owned table graph instead of the global map #2285/Keep a table being created invisible to catalog scans on other threads, so replication can never announce a partial attribute list #2381, not by Apply audit retention continuously to RocksDB transaction logs #2338. I copied the function verbatim frommainintoserver/storageReclamation.tsrather than open-codingreclamationHandlers.delete(path)at each call site. It is a pure addition — nothing onv5.2called it before — so inlining would only duplicate the same one-liner three times and diverge the two branches' modules for no gain.The branch-database hunks are dropped. Apply audit retention continuously to RocksDB transaction logs #2338 added a
stopAuditCleanup()call insidecloseBranchHandles()plus itsopenBranchDatabasetest additions. Neither the function nor the suite exists onv5.2(harper#643 ismain-only), so there is nothing on this line to retire; the alternative would be backporting the whole branch feature.Two tests moved out of the
openBranchDatabasesuite. Apply audit retention continuously to RocksDB transaction logs #2338 putderegisters the storage-reclamation handler when an ordinary database is closedand… when a database is droppedinside that describe, but they exercisecloseDatabase/dropDatabase, not branches — they were there for the fixture. Onv5.2they get their ownstorage-reclamation deregistration on teardowndescribe with the samesetupTestDBPath()/setMainIsWorker(true)before. The behavior under test is unchanged; only the enclosing block is.Table.ts's import keeps only whatv5.2uses. Apply audit retention continuously to RocksDB transaction logs #2338's version of that import listsremoveStorageReclamationHandlerbecausemain'sTable.tsalready used it;v5.2's does not, and importing it fails lint as unused. Same reasonremoveAuditEntryis absent fromauditLog.test.js's import here — themain-only harper#F-264 removal-loop tests that used it are not on this line.Two smaller adaptations, for completeness rather than decision: DESIGN.md gains only #2338's new retention section (the removal-loop section it sat below is
main-only), with its one sentence about branchclose()trimmed; and #2338'sunitTests/resources/query-array-scoping.test.jsformatting commit is dropped, since it repaired amain-only Format Check break against a file that does not exist onv5.2.resources/auditStore.tsapplied with no conflict, and diffing this branch's copy againstorigin/main's leaves only pre-existingmain-only differences (the tombstone-removal tracking and theput: 4audit type, both from other PRs) — #2338's content is carried in full.The pre-push review's two surviving findings — that the Rocks integration test asserts purge cadence rather than actual segment deletion, and that #2338's added comments are denser than Harper's zero-new-comment default — both describe code that is already merged on
main. I left them alone deliberately: fixing them here would makev5.2diverge frommainfor the same feature. They belong onmainfirst, if at all.Verification
End-to-end route: the existing integration suite, extended by #2338's own new RocksDB retention test.
npx harper-integration-test-run "integrationTests/database/audit-retention-*.test.ts" "integrationTests/database/blob-delete-reclaim-audit-expiry.test.ts"— 8 tests, 8 pass, 0 fail. That includes Apply audit retention continuously to RocksDB transaction logs #2338's newRocksDB audit retention self-rearms without storage pressure (#2140)suite, which drives a live instance and asserts more than one passive purge pass.npx mocha unitTests/resources/auditLog.test.js unitTests/resources/databases.test.js— 43 passing / 1 pending on RocksDB (the pending one is the LMDB-only drain-barrier test), and 35 passing / 9 pending underHARPER_STORAGE_ENGINE=lmdb. All five new Rocks cadence/retirement tests ran and passed.npm run test:unit:resources— 1718 passing / 17 pending / 0 failing. UnderHARPER_STORAGE_ENGINE=lmdb(thetest:unit:lmdbgate's first leg) — 1464 passing / 130 pending / 0 failing.npm run test:unit:main— 4525 passing / 195 pending / 2 failing. Both failures are known local-environment artifacts unrelated to this diff:nonInteractiveSpawn git credential scoping(a tmpdir leak between runs) andconfigValidator getDomainSocketPathLengthWarning(sensitive to the checkout path's length). Neither touches audit retention.npm run test:unit:server— not run to completion:unitTests/server/threads/fixtures/processGroupOwnerWorker.jsis picked up by that suite's glob and throws at load onv5.2before any test runs. Pre-existing, not in CI'stest:unit:all, and untouched by this diff;npx mocha unitTests/server/storageReclamation.test.js— the file that actually covers the changed module — passes 44/44.test:unit:apitestsdid not run: these runs used a privateHOMEso the shared system database on this box could not skew them, and that root has noharper install, so the API tests' server bootstrap has nohdb_roleto read.npm run build,npm run typecheck,npm run lint:required, andnpm run format:checkall clean.Complexity: complicated
Review-Coverage: authored=claude; ran=gemini,codex; adjudicated=domain; blocked=cursor-grok(failed); declined=cursor-composer; rounds=1 @ 221ebf1
Human-Review-Need: 3 (decisions: v5-2-cherry-pick-scope, closedatabase-engine-invariant, rocks-cadence-pressure-floor, rocks-only-rearm-guards) @ 221ebf1