feat(backup): hold shared backup directory lock during restore#699
feat(backup): hold shared backup directory lock during restore#699cb1kenobi wants to merge 2 commits into
Conversation
A backups.restore() previously took no lock on the backup directory, so a concurrent db.backup(), backups.delete(), or backups.purge() could delete the very backup a restore was copying from. The failure is asymmetric: the default purgeAllFiles restore mode wipes the destination before copying, so a restore failed mid-purge leaves no usable database, while a rejected writer just retries. tryFileLock/tryAcquireFileLock gain a shared (reader) mode — flock LOCK_SH on POSIX, LockFileEx without LOCKFILE_EXCLUSIVE_LOCK on Windows. Restore holds the .backup.lock in shared mode for its duration: concurrent restores coexist, but writers and restores mutually reject with the existing "locked" error. list/verify remain unlocked so cheap listings don't reject during a long backup. Also: - tryFileLock now creates missing parent directories (the point of the function is to create the lock), and coerces its shared argument by truthiness. withBackupDirLock rejects a missing backup directory explicitly so delete/purge/restore on a typo'd path surface a clear error instead of conjuring an empty directory. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request introduces support for shared (reader) file locks alongside exclusive (writer) locks in the native file locking mechanism. This is utilized to allow concurrent database restores while preventing concurrent writers (such as backups, deletes, or purges) from modifying files during a restore. Additionally, the lock acquisition now automatically creates missing parent directories, and explicit checks are added to prevent creating empty directories for nonexistent paths. The review feedback highlights a critical issue in the Node-API binding code (src/binding/binding.cpp) where accessing argv[1] without checking argc can lead to undefined behavior or crashes if the second argument is omitted by the caller.
📊 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 a0c1afc |
Problem
backups.restore()took no lock on the backup directory, so a concurrent writer —db.backup(),backups.delete(), orbackups.purge()— could delete the very backup a restore was copying from. RocksDB offers no protection here: it only serializes work within a singleBackupEngineinstance, and restore/purge each open their own.The failure is asymmetric, which is what makes it worth fixing: the default
purgeAllFilesrestore mode wipes the destination directory before copying, so a restore that fails mid-copy leaves no usable database and no fallback. A rejected writer, by contrast, just retries later.Change
tryFileLock/tryAcquireFileLockgain a shared (reader) mode —flock(LOCK_SH)on POSIX,LockFileExwithoutLOCKFILE_EXCLUSIVE_LOCKon Windows (same byte range, so shared and exclusive locks conflict correctly across processes, containers sharing a kernel, andworker_threads).backups.restore()now holds the directory's.backup.lockin shared mode for its duration (native restore + transaction-log snapshot copy):Backup directory is lockederror, and vice versa — contention still rejects, never queues, consistent with the existing writer-vs-writer contract.list/verifyremain unlocked, so cheap listings don't reject during a long backup.The no-op degradation on filesystems without
flocksupport (FUSE/9p mounts) applies to shared mode the same way it already did for exclusive.Also in this PR
tryFileLocknow creates missing parent directories — the point of the function is to create the lock — and coerces itssharedargument by truthiness, matching normal JS semantics for the public utility API.withBackupDirLocknow rejects a missing backup directory explicitly, sodelete/purge/restoreon a typo'd path surfaceBackup directory does not exist: …instead of silently conjuring an empty directory and failing deeper in the engine.Tests
test/file-lock.test.tsand the native GoogleTest suite (test/native/file_lock_test.cc).test/backup.test.ts: writers (purge/delete/backup) reject while a restore's shared lock is held and a restore succeeds alongside another shared holder; a restore rejects while a writer holds the lock; the missing-directory error now also covers restore and asserts no directory is created.LockFileExshared path follows documented semantics but is only exercised in CI.Docs updated: README (backup locking note,
backups.restore(), file-locking API), AGENTS.md backup-lock section, and the doc comments at each layer.🤖 Generated with Claude Code