Skip to content

Transaction log deletion is database-wide on RocksDB; table-scoped requests are rejected - #618

Open
cb1kenobi wants to merge 5 commits into
mainfrom
fix/2049-rocksdb-table-scoped-txn-log-purge
Open

Transaction log deletion is database-wide on RocksDB; table-scoped requests are rejected#618
cb1kenobi wants to merge 5 commits into
mainfrom
fix/2049-rocksdb-table-scoped-txn-log-purge

Conversation

@cb1kenobi

Copy link
Copy Markdown
Member

Companion to HarperFast/harper#2064 (fixes HarperFast/harper#2049 — RocksDB: table-scoped delete_transaction_logs_before destroys the entire database's transaction log).

Updates reference/database/transaction.md:

  • Adds a delete_transaction_logs_before section: on RocksDB (the default engine) deletion is database-wide because all tables in a database share one transaction log; as of v5.2.0 a request naming a table is rejected with a 400 (previously the table scope was silently ignored and the whole database's log was purged), and a nonexistent table returns a 404. On LMDB, table scopes deletion to that table, unchanged.
  • Notes on delete_audit_logs_before that it is deprecated in favor of delete_transaction_logs_before, and that on RocksDB it always errors since it requires table.

npm run build and npm run format:check pass locally.

Generated by Claude (Fable 5).

🤖 Generated with Claude Code

delete_transaction_logs_before now rejects table-scoped requests on
RocksDB (HarperFast/harper#2049), and the deprecated
delete_audit_logs_before always errors there since it requires table.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the database transaction documentation to deprecate the delete_audit_logs_before operation and introduces documentation for the delete_transaction_logs_before operation, detailing its behavior on RocksDB and LMDB. The feedback suggests adding a dedicated ### Transaction Log Operations section header to improve the document's structure and clarity.

Comment thread reference/database/transaction.md
@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown

🚀 Preview Deployment

Your preview deployment is ready!

🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-618

This preview will update automatically when you push new commits.

@github-actions
github-actions Bot temporarily deployed to pr-618 August 3, 2026 19:41 Inactive
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown

🚀 Preview Deployment

Your preview deployment is ready!

🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-618

This preview will update automatically when you push new commits.

@github-actions
github-actions Bot temporarily deployed to pr-618 August 3, 2026 20:38 Inactive
@cb1kenobi
cb1kenobi marked this pull request as ready for review August 3, 2026 20:39
@cb1kenobi
cb1kenobi requested a review from a team as a code owner August 3, 2026 20:39
Comment thread reference/database/transaction.md Outdated
Comment thread reference/database/transaction.md
…ecords

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@github-actions
github-actions Bot temporarily deployed to pr-618 August 3, 2026 21:03 Inactive
@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown

🚀 Preview Deployment

Your preview deployment is ready!

🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-618

This preview will update automatically when you push new commits.

@kriszyp kriszyp left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably should clarify the response, since it is returns a job-id.

🤖 Reviewed with Codex

Comment thread reference/database/transaction.md Outdated
Comment thread reference/database/transaction.md Outdated
Comment thread reference/database/transaction.md Outdated
Comment thread reference/database/transaction.md Outdated
Comment thread reference/database/transaction.md Outdated
…ment, type style

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@github-actions
github-actions Bot temporarily deployed to pr-618 August 4, 2026 05:27 Inactive
@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown

🚀 Preview Deployment

Your preview deployment is ready!

🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-618

This preview will update automatically when you push new commits.

@Ethan-Arrowood Ethan-Arrowood left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went through this against the merged implementation in harper#2064 rather than taking the prose on trust — the 400 guard for a RocksDB table scope, the engine-agnostic 404 that sits before the engine branch, cleanup_deleted_records against Table.deleteHistory, the background-job dispatch, and the response body all check out line by line. Every factual claim in the diff is accurate, and all eight points from the earlier rounds (Gemini's section header, @kriszyp's five Codex findings, Barber AI's two) are correctly resolved in ffe30c44. I'm not re-raising any of those.

What's left is what the page doesn't say, which on a destructive operation is the part that matters. Four gaps:

  1. On LMDB, omitting table is a silent no-op — and the section's only example omits it. That request resolves getTable() to undefined, falls into the if (!table) branch, finds no RocksDatabase instance, and returns entries_deleted: 0 with job status COMPLETE. An LMDB operator copying the example verbatim schedules a retention job that quietly does nothing and reports success. Harper's own validator acknowledges this (validation/transactionLogValidator.ts:20: // \table` will need to be required for lmdb, but not for rocksdb`). Inline suggestion below.

  2. Requires clustering: Yes in the comparison table is false on v5/RocksDB. The 5.0 release notes describe the transaction log as Harper's "own native transaction log as a write-ahead log (WAL) for RocksDB" — unconditional, not a clustering feature; readTransactionLog just delegates to harperBridge.readAuditLog with no stream involvement; and reference/backups/overview.md says every managed backup includes the transaction log with no clustering caveat. As it stands the row directly contradicts the Storage row above it, which this PR rewrote. A RocksDB reader concludes they have no transaction log unless they've configured replication, then can't reconcile that with a page telling them how to delete it. Inline suggestion below. (The Available since v4.1.0 row has the same v4-era problem, but I'd leave that one for a separate pass.)

  3. A database-wide destructive purge is documented in plain body prose. Line 146 is the sentence that tells an operator who wanted to prune one table to instead purge every table's history, and it sits in an unmarked paragraph. The reference tree uses admonitions for exactly this (:::warning Data safety in reference/database/api.md:54). Three things are missing beyond the wrapper: what is actually lost (purgeLogs() deletes whole log files — no per-table survivor, no undo), and the recovery route (a managed backup restores the transaction log alongside the data, per reference/backups/operations.md:113). Inline suggestion below covers both.

    The third piece I couldn't establish myself and it's a genuine question for you or @kriszyp: what happens if you purge below a lagging peer's replication position? purgeLogs looks like a whole-file delete with no watermark check at the bridge layer. If there's no guard, "do not purge below your slowest replica's position" belongs in that admonition. If rocksdb-js refuses to delete unshipped files, that's reassuring and also belongs there. Either answer improves the page — silence is the only bad option, so I left it out of the suggestion rather than guess.

  4. No entry in release-notes/v5-lincoln/5.2.md. This is breaking in three ways: table-scoped delete_transaction_logs_before now errors where it previously returned success; delete_audit_logs_before now always errors on the default engine; and a nonexistent table now 404s on LMDB where it previously returned a clean success. Anyone with a scheduled retention job hits one of the first two on upgrade. 5.2.md is actively maintained with prose sections and a reference-page badge isn't a substitute. A short ### Transaction Log Deletion section would do it. Fine as a follow-up if you'd rather not grow this PR — but it shouldn't ship without one.

Everything else below is a suggestion, take or leave. Items 1-3 are cheap enough that I'd land them here.

sent with Claude Opus 5

Comment thread reference/database/transaction.md Outdated
Comment thread reference/database/transaction.md Outdated
Comment thread reference/database/transaction.md Outdated
Comment thread reference/database/transaction.md Outdated
Comment thread reference/database/transaction.md Outdated
Comment thread reference/database/transaction.md
…fety admonition, params/results, get_job error example, 5.2 release note

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@cb1kenobi

Copy link
Copy Markdown
Member Author

@Ethan-Arrowood thanks for the verification-grade pass. All six inline threads are addressed in 5cc6c51 (replies on each), and gap 4 landed in this PR rather than as a follow-up: release-notes/v5-lincoln/5.2.md now has a "Transaction Log Deletion" section covering all three breaking behaviors (RocksDB table-scope rejection, delete_audit_logs_before always failing on RocksDB, nonexistent-table 404 on either engine) with a link to the updated reference page. Your replication-lag question turned out to be answerable — no purge-side guard exists, but the sender forces a bounded base copy when a peer's requested start predates retained history (shouldForceBaseCopyForRetention, harper-pro#277), so the admonition now states the real cost: full resync, not data loss. — Claude (Fable 5)

@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown

🚀 Preview Deployment

Your preview deployment is ready!

🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-618

This preview will update automatically when you push new commits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RocksDB: table-scoped delete_transaction_logs_before destroys the entire database's transaction log

3 participants