feat: add transaction log store validation#700
Conversation
Adds a native validateTransactionLog function that validates a
transaction log store directory offline (no open database): header
token/version, v1 entry framing (classified with the same scan as
open-time crash recovery, so validation and recovery can never
disagree), per-entry timestamp/flag anomalies, sequence continuity,
and txn.state shape/position sanity. A strict mode reports recoverable
torn tails as errors for contexts that must be framing-clean.
Exposed as validateTransactionLogStore(path, { strict? }) in the JS
API, wired into backups.verify() (validates the per-backup snapshot
under transaction_logs/<backupId>/ in strict mode; opt out with
verifyTransactionLogs: false), and into the CLI as verify-logs [name].
The validation core is pure C++ (no N-API) and covered by GoogleTest;
the JS API and backup verify integration are covered by Vitest.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Treat a sub-header (<13 byte) all-zero tail as clean padding rather
than a torn tail: Windows pre-extends and zero-pads log files, and
rotation can leave fewer padding bytes than the recovery scan needs
to see the zero-timestamp end marker, which made strict verification
reject healthy Windows backups.
- Escalate sequence gaps and txn.state-beyond-newest to errors in
strict mode: an intact backup snapshot can have neither, so both
indicate missing files that verification exists to catch.
- Reject leading-zero file names ("01.txnlog") which would alias
another file's sequence number.
- Read log files into an uninitialized buffer (skip the vector's
pre-zeroing of multi-megabyte files).
- Reject (best-effort) instead of leaving the promise pending if the
N-API result object cannot be built.
- Clarify the missing-directory error message and docs.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📊 Benchmark Resultsget-sync.bench.tsgetSync() > random keys - small key size (100 records)
getSync() > sequential keys - small key size (100 records)
ranges.bench.tsgetRange() > small range (100 records, 50 range)
realistic-load.bench.tsRealistic write load with workers > write variable records with transaction log
transaction-log.bench.tsTransaction log > read 100 iterators while write log with 100 byte records
Transaction log > read one entry from random position from log with 1000 100 byte records
worker-put-sync.bench.tsputSync() > random keys - small key size (100 records, 10 workers)
worker-transaction-log.bench.tsTransaction log with workers > write log with 100 byte records
Results from commit a39c7e5 |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
kriszyp
left a comment
There was a problem hiding this comment.
Nice work here — this is careful, durability-critical code and it shows.
Scoping: confirmed cold-path/opt-in. Traced every call site: the N-API entry point builds AsyncValidateLogState with no DBHandle (same shape as the existing backup ops), and validateTransactionLogStore() isn't wired into RocksDatabase, Store, DBIterator, or the log write/append path anywhere. It's only reachable from the standalone JS API, the CLI verify-logs command, and backups.verify() — all inherently offline/administrative. Zero cost on the write path or the replication read path, which is exactly the invariant worth protecting here. Reusing scanTransactionLogForRecovery for the torn-tail vs. mid-file-corruption classification is also the right call — validation structurally can't disagree with recovery about what's a normal crash artifact.
One real gap (strict mode severity taxonomy): transaction_log_validation.cpp:337 — a txn.state flushed offset beyond the file's actual size is always a warning, never escalated in strict mode, unlike its sibling checks (torn tail, sequence gap, flushed sequence beyond newest file). The comment a few lines above explains why the sequence-beyond-newest case escalates: txn.state is captured before the log files are enumerated during a backup, so files in the snapshot can only have grown since, never shrunk. By that same logic, a file smaller than its recorded flushed offset isn't a normal race — it's a definite-incompleteness signal, which is exactly what strict mode exists to escalate. Doesn't look intentional given the rest of the taxonomy is consistent elsewhere. Not a blocker (it's still visible in result.warnings), but worth escalating to match its siblings, or a one-line comment if it's deliberately excluded.
On the Gemini review: two of its "blocker" claims don't hold up, so no need to chase them —
- The
napi_throw_error/"uncaught C++ exception aborts the process" claim is incorrect — that macro schedules a pending JS exception via the normal N-API mechanism and returns; there's nothrow, nothing crosses a C boundary. Same pattern is already used elsewhere in this codebase (backup.cpp:376). - The
uint32_t posoverflow/infinite-loop claim isn't reachable — every frame the walk visits was already bounds-checked in wideneduint64_tbyscanTransactionLogForRecoverybefore being accepted intovalidEnd, sopos + 13 + lengthcan't wrap by construction.
(Gemini's offset-escalation finding, on the other hand, does hold up — that's the same gap flagged above.)
Two minor suggestions, non-blocking:
transaction_log_validation.cpp:144— the framing walk's safety depends on an invariant enforced in a different function (the recovery scan). Wideningpostouint64_there too (or a comment) would make that self-evident rather than implicit against a future refactor.transaction_log_validation.cpp:253— whole-file heap read viaifstream. Fine given the 16MB defaultmaxFileSize, the 4GB format cap, and the cold-path scope, but the codebase already has mmap plumbing for this exact file type (transaction_log_file_posix.cpp/_windows.cpp) that would avoid the double page-cache→heap copy if you want to reuse it opportunistically.
Solid addition to the backup verification story — thanks for closing this gap.
— Claude (Sonnet 5), reviewed via review-queue
A file shorter than its recorded flushed offset cannot occur in an intact backup snapshot (txn.state is captured before the files, which only grow after that), so strict mode now reports it as an error like its sibling checks (torn tail, sequence gap, sequence-beyond-newest). Also widens the framing walk's position to uint64_t so its bounds safety is locally evident instead of implicit in the recovery scan. Addresses kriszyp's review on #700. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Thanks for the thorough review — the call-site trace on the cold-path invariant and the fact-check of the Gemini claims both save real time. Offset-beyond-file-size gap: fixed in b095c6f. You're right that it was an unintentional inconsistency, not a deliberate exclusion — the same snapshot-ordering argument (txn.state captured first, files only grow afterward) applies, so strict mode now escalates it like its siblings. Added a GTest ( Suggestion 1 (widen Suggestion 2 (mmap reuse): deliberately passing for now. The existing mmap plumbing is entangled with the live-file lifecycle (fileMutex, frozen-map weak caching, the MAP_FIXED overlay), and validation runs on stores no — Chris (via Claude Fable 5) |
Summary
backups.verify()verified the RocksDB files but never checked the transaction log snapshot captured alongside them (transactionLogs: true). This adds a native transaction log store validator and exposes it at every level:src/binding/transaction_log/transaction_log_validation.cpp, no N-API, GoogleTest-covered): validates a store directory — header token/version, v1 entry framing (classified with the samescanTransactionLogForRecoveryused by open-time crash recovery, so validation and recovery can never disagree), per-entry timestamp/flag anomalies, file-name and sequence continuity, andtxn.stateshape/position sanity. Astrictmode escalates snapshot-incompleteness signals (torn tail, sequence gap,txn.statebeyond newest file) from warnings to errors.validateTransactionLogStore(path, { strict? })— module-level, async (worker thread), no open database required; resolves a per-file report rather than throwing on invalid.backups.verify(): now also validates the backup's log snapshot in strict mode; opt out withverifyTransactionLogs: false.verify-logs [name]command;backups <dir> verify <id>picks up the snapshot check automatically.Where to look
backups.verify()previously resolved for a backup whose log snapshot was corrupt; it now rejects. Existing signature/return type are unchanged and backups without a snapshot are unaffected, but a caller relying on the old (blind) behavior needsverifyTransactionLogs: false.transaction_log_validation.cpp,TruncateTailcase): Windows pre-extends/zero-pads log files and rotation can leave fewer than 13 padding bytes — without this, strict verification rejects healthy Windows backups. The premise is that a real torn frame always begins with a nonzero timestamp; worth a second opinion.strictescalates) is the main design decision — see the doc comment onvalidateTransactionLogStoreintransaction_log_validation.h.Generated by Claude (Fable 5); an independent model review was run pre-PR and its findings (Windows zero-padding false positive, strict-mode gap escalation, leading-zero file names, N-API failure paths) are addressed in the second commit.
🤖 Generated with Claude Code