Repository navigation
Add verifyTables to backups.verify() so a backup this build can't read fails before restore - #925
Merged
Merged
Conversation
…re restore A checksum-valid table in a format_version this build doesn't support, or compressed with an algorithm it doesn't link, passes backups.verify today and only fails when something reads it. A purgeAllFiles restore has already deleted the destination by then (harper#2965). verifyTables opens every SST in the backup with SstFileReader, one at a time, then checks every compression type the table records is supported. The first failure rejects and names the file. Blob files are not checked. Closes #920 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…erifyTables An EMFILE or EACCES while opening a healthy table was reported as Corruption, which reads as "this backup is bad". Only the file name is prefixed now. verifyTables: null was coerced to false before the type check. ZSTDNotFinal (type 0x40) no longer maps to ZSTD, since this RocksDB cannot decode it. The metadata path-confinement check is exposed so it can be tested. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… open Build the prefixed status with the public (code, subcode, severity, msg) constructor instead of mapping a handful of codes, and pin it with a native test that injects an IOError/NoSpace open failure. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
📊 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 0bff6f8 |
…ry platform std::filesystem::path only splits on '\' on Windows and converts through the active code page there. RocksDB always writes '/' in backup metadata, so this only hardens the untrusted-name check: '..' behind either separator, a leading separator, and any drive or stream colon are rejected on every platform. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
cb1kenobi
marked this pull request as ready for review
October 12, 2026 22:35
verifyTables to backups.verify() so a backup this build can't read fails before restore
kriszyp
approved these changes
Oct 13, 2026
kriszyp
left a comment
Member
There was a problem hiding this comment.
Excellent, great feature!
🤖 Reviewed with Codex
cb1kenobi
added a commit
to HarperFast/harper
that referenced
this pull request
Oct 14, 2026
Staging needed disk for a third copy (database, repository, staging), so a restore on a tight disk was always refused. Both restore paths now run backups.verify with checksums and verifyTables against the repository before the purge, which catches the same unreadable backups (corrupt file, unsupported log header, unsupported table format or compression) with no extra disk. The staging, .replaced recovery, space check and symlink/mount-point refusals go away with it. verify_backup checks tables too, so it agrees with what restore will accept. Requires the @harperfast/rocksdb-js release that adds verifyTables (HarperFast/rocksdb-js#925). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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.
⊙ Problem
backups.verify()checks file sizes, optionally checksums, and the transaction-log snapshot, but none of those catch a table this build cannot read. A checksum-valid SST written with a newerformat_version, or compressed with an algorithm this build doesn't link, passesverifyand only fails when something reads it. For Harper'srestore_backupthat is too late: the defaultpurgeAllFilesrestore has already deleted the database it was meant to replace (harper#2965: restore_backup purges the destination before anything verifies the backup can be read). harper#3121: Verify a backup can be read before restore_backup replaces the database needs a way to check this in place, before the purge.Closes #920.
💡 Solution
backups.verify(dir, id, { verifyTables: true })opens every SST in the backup, one at a time, and rejects with the file's name when this build cannot decode its footer (for exampleCorrupt or unsupported format_version 99) or properties, or when the table records a compression algorithm this build does not support. It is off by default, as decided on the issue, so existingverifycalls do the same work as before.SstFileReader::Openalone is not enough: it never reads a data block, so an LZ4 table opened by a build without LZ4 passesOpenand fails on the first cold read. The check therefore also decodes the table'scompression_nameproperty (both theformat_version >= 7encoding and the older single-name form) and fails closed on anything it cannot interpret.No companion HarperFast/documentation PR: rocksdb-js documents its API in this repo (README and
docs/backups.md, both updated here). Any user-facing change to Harper'srestore_backupbelongs with harper#3121.⚖️ Alternatives
compression_name, which would avoid our own parser. Rejected: a block that does not compress well enough is stored uncompressed, so the first block proves nothing about later ones.max_open_files = -1opens every table at once, so descriptors grow with table count, and a full scan reads every data block.BlockBasedTableOptions::format_versionso a newer build never writes a format an older one can't read. It doesn't help backups that already exist or the compression gap, so it would be a follow-up at most.🔧 Changes
src/backup.tsrejects a non-booleanverifyTables,nullincluded, with aTypeError, so a value from untyped config can't silently skip the check, then passes it to native. The JSDoc describes the option.src/load-binding.tsadds the sixth native argument.src/binding/database/backup.cppreads the new argument (a five-argument call leaves it false) and runs the table walk only afterVerifyBackupsucceeds and only when the option is set.backup_verify_tables.h):src/binding/database/backup_verify_tables.cpp..;backupDir + "/" + relative_filename, one reader at a time (BackupFileInfo::directoryis a virtualprivate/<id>path, so it can't be used);EMFILEorEACCESdoesn't read as a corrupt backup.tableCompressionTypes(declared incompression.h) decodes both encodings ofcompression_name.binding.gypadds the source to the addon and to the native test target, along with the new test file.README.mddocuments the option.docs/backups.mdexplains it and shows holding the shared lock across verify and restore.src/binding/database/DESIGN.mdrecords the three facts the check depends on, and the rootDESIGN.mdindex points to it.✅ Verification
End-to-end route: the Vitest cases exercise the shipped N-API path. Harper's own preflight lands in harper#3121.
test/native/backup_verify_tables_test.cc:format_version99 fails with its name, while RocksDB's own size-onlyVerifyBackupstill passes;NotSupportedwhen the build is made to lack its compressor;IOError/NoSpaceopen failure keeps its code and subcode;/or\, are rejected.test/native/compression_test.cccovers legacy names, v7 encodings, and fail-closed cases (empty, unknown manager, odd-length or non-hex codes,ZSTDNotFinal).test/backup.test.ts:verifyTablesand checksums;verifybut rejects withverifyTables, naming the file andformat_version;'true'andnullreject with aTypeError.pnpm test:native: all pass; the only skips are the 3 existing madvise tests.registry cleanup failedteardown line at worker exit for an unrelated test database. It did not reproduce in 4 repeats oftest/backup.test.ts, did not appear in the 2 other full runs, and the run still exited 0.test/backup.test.ts: 33/33 on Node, Bun and Deno.pnpm checkis clean.🤖 Generated by Anthropic Claude (Opus); posted via @cb1kenobi.
Related PRs: none found
Complexity: medium
Review-Coverage: authored=claude; ran=cursor-composer,gemini,codex; adjudicated=domain; declined=cursor-grok,cursor-kimi,cursor-muse; rounds=2; full=1 @ 140dbd3
Review-Attention: read ~5m (decisions: restore-preflight-vs-verify-option, default-off, blob-files-out-of-scope, fail-closed-compression-name, do-less-alternative) @ 140dbd3