delete_transaction_logs_before with a table on RocksDB now returns an error instead of deleting the entire database's transaction log - #2064
Merged
Conversation
…stead of purging the whole database's log On RocksDB all tables in a database share one transaction log with no per-table purge granularity, so a table-scoped request silently purged every sibling table's history; a nonexistent (e.g. typo'd) table name fell through to the same whole-database purge. Table-scoped requests on RocksDB now fail with a 400 naming the database-wide alternative, and a nonexistent table 404s before any purge. The table/no-table checks use presence (!= null) rather than truthiness so a table named "0" addressed numerically stays table-scoped. Consequence: the deprecated delete_audit_logs_before op requires table, so on RocksDB it now always errors with the migration message. Fixes #2049 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Code Review
This pull request prevents table-scoped transaction log purges on RocksDB, which previously resulted in silent database-wide purges because RocksDB tables share a single transaction log. It introduces a 400 Bad Request error for table-scoped purge requests on RocksDB, returns a 404 Not Found error for nonexistent tables, and ensures that falsy table names (like '0') are correctly treated as table-scoped. Additionally, comprehensive integration and unit tests have been added to verify these behaviors. There are no review comments, so I have no further feedback to provide.
Contributor
|
Reviewed; no blockers found. |
cb1kenobi
marked this pull request as ready for review
August 3, 2026 20:09
delete_transaction_logs_before with a table on RocksDB now returns an error instead of deleting the entire database's transaction log
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.
Fixes #2049 (RocksDB: table-scoped delete_transaction_logs_before destroys the entire database's transaction log).
What changed
On RocksDB, all tables in a database share one transaction log, and rocksdb-js's
purgeLogs()has no table dimension (whole-file purges, logs partitioned per origin node). The bridge previously ignored atablescope and purged the whole database's log — reporting success. Now, inResourceBridge.deleteTransactionLogsBefore:tablenaming a RocksDB table → 400, with a message pointing at the supported database-wide form (delete_transaction_logs_beforewith onlydatabaseandtimestamp).tablenaming a nonexistent table → 404 on either engine. The guard sits before the engine branch, so this is a behavior change on LMDB too: previously a typo'd name fell into the no-table branch — the whole-database purge on RocksDB, a silententries_deleted: 0no-op on LMDB.!= null), not truthiness, in both the guard andgetTable(), so a table named"0"addressed numerically stays table-scoped instead of widening to a database purge (found by cross-model review).table→ database-wide purge, unchanged. LMDB table-scopeddeleteHistory→ unchanged.Deliberate design consequence — reviewer attention here
The deprecated
delete_audit_logs_beforeop requirestableand funnels into the same bridge method, so on RocksDB it now always errors with the migration message. There is no way to honor its per-table contract on RocksDB without the silent widening this PR removes. Three integration tests that previously asserted job success for table-scoped calls were updated to assert the rejection (transaction-logs.test.mjs,transactions.test.mjs,terminology.test.mjs— the last now verifies database-param resolution via the database name echoed in the error).Not addressed (same silent-ignore shape, but inert rather than destructive):
cleanup_deleted_recordsis still silently ignored on the RocksDB no-table path — flagged in #2049 as a decide-alongside; can be a follow-up.Testing
unitTests/resources/deleteTransactionLogsBeforeRocks.test.js, 5 passing): 400 on table-scoped, 404 on missing table/database, falsy-table-name regression, database-wide purge still succeeds. Skipped underHARPER_STORAGE_ENGINE=lmdb.test:unit:resourcesfull run green (1365 passing); the three touched integration files green (75 passing).test:integration:allfull run: 1624/1645 passing; the 3 failures (shutdown-drain-e2e.test.ts,rolling-restart.test.ts) reproduce identically onorigin/main— pre-existing local-environment failures, unrelated to this change.test:unit:mainhangs inunitTests/components/applicationSpawn.test.json this machine with and without this change (verified A/B against main'sResourceBridge.ts) — known local-environment issue; relying on CI for that suite.Docs
Companion docs PR: HarperFast/documentation#618 — documents database-wide-only deletion on RocksDB and the deprecated op's behavior.
Review
Cross-model review (thorough): author Claude (Fable 5); coverage: Codex ✓ (two findings — the falsy-table-name blocker and the dead-end error remediation — both fixed, then a final-artifact re-check ✓); Gemini ✗ (agy failing locally with a model/location config error; no Gemini coverage).
🤖 Generated with Claude Code