Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1359 +/- ##
==========================================
- Coverage 91.22% 91.14% -0.09%
==========================================
Files 37 39 +2
Lines 18753 18909 +156
==========================================
+ Hits 17108 17235 +127
- Misses 1645 1674 +29
🚀 New features to boost your workflow:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b86754a06a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4110cf97fe
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // is what it tries to take, so a shared one here turns it away. Shared locks do not | ||
| // exclude each other, so this leaves the file open to the read-only handles a later | ||
| // release adds | ||
| let data = FileBackend::new_internal(data, FileLockKind::Shared)?; |
There was a problem hiding this comment.
On Windows, std::fs::File::try_lock_shared is backed by LockFileEx, and a shared byte-range lock denies writes to the locked region even for the process that took the lock. This opens data.redb read/write and then all MultiProcessDatabase commits and shutdown writes go through the same FileBackend, so enabling experimental-multiprocess on Windows will make the database open successfully but fail as soon as it tries to initialize or write. Use a lock mode/mechanism for the writer that still permits its own writes, or gate this shared data-file lock off on Windows.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and Windows CI caught it on the same commit: Os { code: 33, ... "The process cannot access the file because another process has locked a portion of the file." }, 11 of the 13 multi-process tests failing. My mistake -- I took the shared lock without checking what LockFileEx does to the holder's own writes.
Fixed in 489fff3 by taking the exclusive lock rather than by gating on Windows, since the same lock also has to exclude read-only openers (see the sibling thread). That is FileBackend's default, so the FileLockKind refactor is gone and this branch no longer touches the file backends at all.
Worth recording for the next step, because it constrains the design: since a writer cannot hold a shared lock on Windows, making room for readers means removing the database file's lock, not weakening it. All reader/writer coordination has to live in the directory's separate lock files. docs/design.md now says so.
Generated by Claude Code
| // is what it tries to take, so a shared one here turns it away. Shared locks do not | ||
| // exclude each other, so this leaves the file open to the read-only handles a later | ||
| // release adds | ||
| let data = FileBackend::new_internal(data, FileLockKind::Shared)?; |
There was a problem hiding this comment.
When another process opens <dir>/data.redb directly with ReadOnlyDatabase::open while a MultiProcessDatabase writer is live, this shared lock lets that open succeed because both handles request shared file locks. That read-only process is not registered in the writer's TransactionTracker, so after later commits the writer can free and reuse pages that the external reader's snapshot still needs; until the multiprocess reader protocol exists, this needs to exclude direct read-only opens as well as direct writers.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, and this is the better argument of the two -- the Windows problem forced a change, but this is why the exclusive lock is the right change rather than a workaround. A reader that isn't in the writer's TransactionTracker can have its pages freed underneath it, so until readers can publish what they're pinning, they have to be turned away along with writers.
Fixed in 489fff3. an_ordinary_database_cannot_open_the_data_file now covers Database::open, Database::create and ReadOnlyDatabase::open, and checks the file is a plain redb database again once the directory is closed. I mutation-tested it: reinstating the shared lock fails the test on the ReadOnlyDatabase assertion, which is the one the previous version was missing.
Generated by Claude Code
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 489fff3b8b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| let mut bytes = vec![]; | ||
| file.read_to_end(&mut bytes).map_err(StorageError::Io)?; | ||
| if bytes.is_empty() && create { |
There was a problem hiding this comment.
When create() is interrupted or write_all() errors after only part of the 12-byte marker reaches disk, the retry sees a non-empty metadata file and this condition skips the initialization path, so it returns InvalidData before it ever recreates or initializes data.redb. The fresh remaining case is that the current retry path only rewrites when bytes.is_empty(); a short metadata file from a failed create still wedges the directory until manual cleanup.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed -- I reproduced it by truncating the marker to 4 bytes, and the retry returned InvalidData forever, the same wedge as the earlier one by a different route.
Fixed in 9492254, but by removing the state rather than tolerating it: the marker is now written under a temporary name and renamed into place, so it is either absent or complete. Recovering from a short marker would mean create() overwriting a metadata file it did not write, and a directory that already has one is far more likely to be a mistyped path than an interrupted create -- taking it over would be a destructive way to report that. So create() still refuses anything that is not a valid marker, which is only safe because a half-written one can no longer exist.
Two tests: a_marker_that_never_landed_is_written_again covers the interrupted create (partial copy left under the temporary name, no marker), and a_directory_belonging_to_something_else_is_left_alone pins the other half -- that a foreign metadata file is refused and left byte-for-byte intact.
Generated by Claude Code
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9492254adc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| )) | ||
| .into()); | ||
| } | ||
| return self.write_metadata(); |
There was a problem hiding this comment.
When create() is pointed at an existing directory with no metadata marker but an invalid or unrelated data.redb, this branch writes a valid multi-process marker before the data file is opened and validated. The later Database::new() then returns an error, but the failed call has already converted/modified the unrelated directory, so subsequent opens treat it as one of these databases instead of leaving it untouched. Please validate/initialize the data file before installing the marker, or reject non-empty unmarked directories.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 593ca15. Reproduced with a directory holding a junk data.redb: create() failed in Database::new as expected, but metadata was left behind, so the directory had been converted on the way out.
Took the "validate before installing the marker" option rather than rejecting non-empty unmarked directories, because the reject rule can't tell an interrupted create of ours from a foreign directory -- the recovery case from the previous commit is itself a non-empty unmarked directory. Ordering solves both: DatabaseDir::open no longer writes the marker, and MultiProcessBuilder::open_inner calls write_metadata() once Database::new has returned. Writing it again over an identical marker costs one rename and saves tracking whether this particular call was the one that created the database.
Test: create_does_not_mark_a_directory_it_rejects, which also checks the file it refused to read is still there byte for byte.
Together with your two earlier reports this gives the directory a single rule -- a call that fails leaves it exactly as it found it -- which is now stated as such in docs/design.md. That rule is what makes it safe to refuse a metadata file that is not a marker rather than overwrite it.
Generated by Claude Code
| /// Taken before anything in the directory is read or written, so that a process which gets it | ||
| /// has the directory to itself -- including while it is being created. | ||
| fn acquire_write_lock(&self) -> Result<File, DatabaseError> { | ||
| let file = open_or_create(&self.write_lock_file()).map_err(StorageError::Io)?; |
There was a problem hiding this comment.
MultiProcessDatabase::open() reaches this helper before reading metadata, and open_or_create() always uses .create(true). When path is an existing but non-database directory, open() still returns an error but leaves a new write.lock behind, so a read-only validation path mutates arbitrary directories despite not creating missing paths. Open the lock file without creating it until the marker has been validated, or remove the file on validation failure.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 593ca15 -- open() on a directory holding an unrelated file returned an error and left a write.lock next to it.
Took the first option: the lock file is only created when create is set, and on the open path a missing one now reads as "not a multi-process database directory", which is what its absence means. Removing on failure would have been the weaker fix, since it cannot cover a crash between the two.
Test: open_does_not_touch_a_directory_it_rejects, which counts the directory entries afterwards rather than naming write.lock, so it also catches anything else open() might start leaving behind.
Generated by Claude Code
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 593ca15e65
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let file = if create { | ||
| open_or_create(&path) |
There was a problem hiding this comment.
When create() is pointed at an existing directory that is later rejected, for example because metadata is not this marker or data.redb is an unrelated/invalid file, this branch creates write.lock before any validation runs. The call still returns an error, but the unrelated directory has been modified, which breaks the documented invariant that failed validation leaves the directory as it was found; either avoid creating the lock until the directory is accepted or remove it on these failure paths.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
You're right that the invariant as written was false. Fixed in a74fff3 — by correcting the invariant rather than the code, because neither option here is available.
The lock cannot wait until the directory is accepted: taking it is what serializes two processes creating the same directory, so validation has to happen underneath it. A create() that fails validation has necessarily already made the lock file.
And removing it on the failure paths is worse than leaving it. Another process can have opened that same write.lock and be blocked on the lock. Unlinking and releasing leaves that process holding a lock on an unlinked inode, while a third creates a fresh write.lock at the same path and locks it successfully — both then believe they have the directory. That trades a cosmetic problem for a real mutual-exclusion break.
So docs/design.md now claims the thing that is both true and the one that matters: a call that fails never leaves a marker behind, and a directory that is not one of these is never turned into one by a call that did not succeed. An empty write.lock confers nothing on its own — the marker is what makes a directory a database, which is precisely why the marker is the thing worth guarding. open() still creates nothing whatsoever, and the doc now says why create() is the weaker case rather than implying the two are the same.
create_does_not_mark_a_directory_it_rejects pins the boundary: after a rejected create() the directory holds data.redb and write.lock and nothing else, and a second create() and an open() both still refuse it — so the lock file left behind cannot be read by a later call as evidence that the directory is ours.
Generated by Claude Code
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a74fff3df0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| )) | ||
| .into()); | ||
| } | ||
| return Ok(()); |
There was a problem hiding this comment.
When create() is pointed at an existing directory that has no metadata marker but does contain unrelated entries, this branch accepts it solely because the marker is absent; open_inner() then creates data.redb and installs the marker, so a mistyped path like a directory containing notes.txt is silently converted even though the new MultiProcessDatabase docs say nothing else may be put there. The fresh case is a non-empty directory without an invalid data.redb, so the earlier validation fixes do not run; please reject unmarked directories that contain entries other than the known interrupted-create files before proceeding.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, and done in 3548080. I turned this down when you raised the general version of it earlier, on the grounds that "non-empty" can't distinguish an interrupted create from a foreign directory — but your refinement here, entries other than the known interrupted-create files, is exactly the distinction that was missing, and a directory containing notes.txt was a real gap: no invalid data.redb meant nothing else caught it.
create() now accepts an unmarked existing directory only if everything in it is a file create() itself writes — data.redb, write.lock, metadata.tmp.
One constraint worth recording: the check applies only when the marker is absent. Applying it to a directory that is already a database would mean a stray .DS_Store, or an editor scratch file, could lock its owner out of their own database — a much worse failure than the one being prevented. a_stray_file_does_not_stop_an_existing_database_opening pins that.
A side benefit: this check needs no lock, so create() runs it before taking one. The mistyped-path case now leaves nothing behind at all, which closes the residue you noted in your previous comment for every case except a directory holding a junk data.redb. It runs again under the lock, which stays the authoritative one.
Tests: create_refuses_a_directory_holding_other_files (and it asserts the directory still has exactly its one file afterwards), plus the .DS_Store test above.
Generated by Claude Code
| file.write_all(&contents).map_err(StorageError::Io)?; | ||
| file.sync_all().map_err(StorageError::Io)?; | ||
| drop(file); | ||
| std::fs::rename(&tmp, self.metadata_file()).map_err(StorageError::Io)?; |
There was a problem hiding this comment.
On Windows, std::fs::rename fails when the destination already exists, but MultiProcessBuilder::create() calls write_metadata() even for an existing multi-process database. In that scenario metadata is already present, so MultiProcessDatabase::create(path) returns an I/O error instead of opening the existing database as documented; please either skip rewriting an already-validated marker or replace it using a Windows-compatible atomic update.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This one is not a bug — the premise about Windows is wrong.
std::fs::rename replaces an existing destination on Windows. Its documentation says so ("replacing the original file if to already exists"), and the implementation calls MoveFileExW with MOVEFILE_REPLACE_EXISTING precisely so the behaviour matches Unix. The rename that fails when the destination exists is the raw Win32 MoveFile, not the one std uses.
There is also direct evidence in this PR. create_and_reopen ends by calling MultiProcessDatabase::create() on an already-created database, which is exactly the path you describe: metadata present, write_metadata() renaming over it. That has run on windows-latest on every head since the always-rewrite behaviour went in at 593ca15 — including a74fff3, where the full Windows job is green — and it asserts the database opens and returns the value written earlier. If the rename failed with an I/O error, that test would fail on Windows and nowhere else.
I have left the behaviour as it is. Skipping the rewrite when the marker is already valid would be a reasonable micro-optimization, but it would mean tracking whether this particular call created the database, and the rewrite costs one rename of a 12-byte file on a path that runs once per process.
Generated by Claude Code
| file.write_all(&contents).map_err(StorageError::Io)?; | ||
| file.sync_all().map_err(StorageError::Io)?; | ||
| drop(file); | ||
| std::fs::rename(&tmp, self.metadata_file()).map_err(StorageError::Io)?; |
There was a problem hiding this comment.
After this rename, the containing directory is never synced, so on Unix filesystems a crash or power loss immediately after a successful MultiProcessDatabase::create() can lose the metadata directory entry even though the call returned Ok. A later open() then rejects the directory as not being a multi-process database until callers know to retry create(), so the parent directory should be fsynced after the rename before reporting success.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right, and fixed in 3548080. The rename made the marker atomic but not durable, so create() could return Ok and a crash could still lose the directory entry — leaving a database file with no marker, which open() refuses until someone calls create() again. The failure is recoverable but it is an error the caller has no way to anticipate, which is the worst kind.
write_metadata() now fsyncs the directory after the rename, via a sync_dir() helper that is a no-op off Unix — Windows has no directory handle to sync, and MoveFileEx orders the metadata write itself.
Worth noting this is the one durability gap of its kind here: data.redb needs no equivalent, since redb's own commit path already syncs it and a lost data.redb entry in a directory that still has a marker is just an interrupted create, which the next create() finishes.
Generated by Claude Code
| /// The caller must hold the write lock, which is what makes this safe against another process | ||
| /// creating the same directory. | ||
| fn read_metadata(&self, create: bool) -> Result<(), DatabaseError> { | ||
| let bytes = match std::fs::read(self.metadata_file()) { |
There was a problem hiding this comment.
Because this reads the entire metadata file before checking the 12-byte marker, pointing open() or create() at a non-database directory with a very large metadata file can allocate unbounded memory, and a special file or symlink can make validation block instead of quickly returning an error. Please read only the fixed-size marker, then verify EOF or reject extra bytes, so a mistyped or hostile directory cannot exhaust resources during validation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3548080 — the read is now bounded to METADATA_LEN + 1 bytes via take(), which is enough to tell a marker from a longer file without reading the rest of it.
This also tightened a real hole I had missed: the length check was bytes.len() < METADATA_LEN, so a metadata file that began with a valid magic and version and then kept going was accepted. It is now != METADATA_LEN, which is right — the marker is a fixed size, and a longer file is not one. an_oversized_marker_is_rejected covers it.
I did not chase the second half. Bounding the read does nothing about a FIFO or a device node named metadata, which would block on open rather than on read, and the fix for that (checking the file type first) doesn't generalize — the same argument would apply to data.redb and write.lock, and past that to every path redb is ever handed. A caller that can plant a FIFO in the target directory can do more direct damage than making open() hang.
Generated by Claude Code
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 354808030f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| DATA_FILE_NAME, | ||
| WRITE_LOCK_FILE_NAME, | ||
| METADATA_FILE_NAME, | ||
| METADATA_TMP_FILE_NAME, |
There was a problem hiding this comment.
When create() is pointed at a markerless directory, this name-only whitelist treats any metadata.tmp entry as an interrupted redb create. If that entry is a symlink, the later File::create(&tmp) follows it and truncates/writes the target outside the database directory, so a mistyped path can still clobber an unrelated file. Please verify reserved recovery entries are regular non-symlink files, or recreate the temp marker without following an existing path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right, and fixed in 44dfdc1 by the same change as the data.redb symlink report — reject_foreign_directory now requires each entry to be a regular file as well as to carry one of redb's names, so metadata.tmp is covered along with the rest. Answered in full on that thread; create_refuses_a_directory_of_symlinks plants a symlink under this name too and asserts the target is untouched.
Generated by Claude Code
| .write(true) | ||
| .create(create) | ||
| .truncate(false) | ||
| .open(self.data_file()) |
There was a problem hiding this comment.
When create() is retried in a markerless directory, the whitelist accepts an entry named data.redb as something a prior create may have written, but this open follows symlinks. If the directory was mistyped or tampered with and data.redb points at an external file, create(true) can initialize, repair, or write that target and then install a marker here, modifying data outside the database directory. Please require accepted recovery data files to be regular non-symlink files before opening them.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Both symlink reports are right and both are fixed in 44dfdc1 by the same change, so I've answered them together here.
The whitelist matched on name alone. reject_foreign_directory now also requires the entry to be a regular file, which covers data.redb and metadata.tmp and anything else added to the list later. DirEntry::file_type() reports the entry rather than its target, so a symlink is foreign whatever it is named — no extra symlink_metadata call needed.
create_refuses_a_directory_of_symlinks plants a symlink under each name pointing at a file outside the directory, and asserts both that create() fails and that the target is still byte-for-byte intact. It fails without the change.
Scope worth naming: this protects the recovery path, where redb decides an unmarked directory is its own unfinished work. It does not try to make redb symlink-proof in general — a directory that already holds a valid marker is redb's own, and if something can replace data.redb in it with a symlink it already has write access to the database. The same is true of any path handed to Database::create.
Generated by Claude Code
| let inner = Database::new( | ||
| backend, | ||
| create, |
There was a problem hiding this comment.
The fresh remaining interrupted-create case is a crash after TransactionalMemory::new has resized or written the new data.redb header but before the database magic and metadata marker are durable: the directory then contains only redb-created names, but Database::new(..., create, ...) still rejects the non-empty file because its magic does not match, so every later MultiProcessDatabase::create() returns InvalidData instead of finishing creation. Consider initializing through a temporary data file or otherwise recognizing/removing this markerless partial data file before retrying.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is the best finding of the series. Confirmed against the code rather than taken on faith — page_manager.rs writes the header with to_bytes(false), flushes, and only then writes it again with the magic, under a comment saying it does so deliberately for crash safety; and the check above it returns InvalidData whenever initial_storage_len > 0 and the magic does not match, allow_initialize notwithstanding. So the state you describe is real, reachable, and permanent, and it is the likeliest shape of an interrupted create — which is exactly what this PR claimed to have covered. My an_interrupted_create_can_be_redone only ever exercised a zero-length file, which is the easy half.
Fixed in 44dfdc1 by your first suggestion, initializing through a temporary data file. I did not take the second — recognizing and removing a markerless partial data.redb — because it cannot be told apart from a directory this call was pointed at by mistake, and adopting that reading would mean deleting a file redb did not write. That is the conflict create_does_not_mark_a_directory_it_rejects was pinning, and it has no resolution based on content.
The temporary name dissolves it. data.redb.tmp is initialized, and renamed to data.redb only once Database has accepted the file, so:
data.redbexists only when it is a database that was finished. Adata.redbthat is not one is definitively not redb's, and is still refused rather than overwritten.- An unfinished attempt is left under a name nothing else uses, so the next
create()can throw it away without a judgement call.
Both guarantees now hold at once, where before they were in tension. an_initialization_that_crashed_partway_can_be_redone builds the exact state — no marker, no data.redb, and a data.redb.tmp that is non-empty with its magic zeroed — and asserts create() finishes the job and leaves no temporary behind.
Generated by Claude Code
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 44dfdc163a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if !tmp.exists() { | ||
| return Ok(()); | ||
| } | ||
| std::fs::rename(&tmp, self.data_file()).map_err(StorageError::Io)?; |
There was a problem hiding this comment.
When create() is called on an already-created multi-process database and a stale data.redb.tmp is present, this rename still promotes the temp file over the valid data.redb even though the backend opened and locked the original data file. On Unix that leaves the returned handle writing to the old unlinked inode while the directory now contains the stale temp file, so subsequent opens see the wrong/corrupt database and the data written by the successful handle is lost. Only promote the temp file when this open actually initialized that temp path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
You're right, and this was the serious one — I introduced it in 44dfdc1, and it destroys databases. Fixed in bf3b1ec.
I reproduced it exactly as described before changing anything: create a database, write a value, drop it, leave a data.redb.tmp beside it, call create() again. The value is gone. promote_data() was written to be idempotent so that the caller would not have to track whether it had initialized anything — but "a temporary exists" is not the same claim as "I wrote that temporary", and conflating them is how a stale file gets renamed over a good database while the handle just returned goes on writing to the unlinked inode.
The fix keeps the flagless shape but earns it. Any data.redb.tmp is now discarded on the way in, on every create() — including one that is only reopening a finished database, where the temporary is not otherwise touched. A temporary present at promote time is therefore necessarily this call's own. And promote_data() now refuses to rename over a data.redb that has appeared since, rather than clobbering it.
a_stale_temporary_does_not_replace_the_database is the reproduction, kept: it asserts the old value survives, that a write made through the new handle is in the database the directory actually points at afterwards, and that no temporary is left.
Generated by Claude Code
| contents[8..12].copy_from_slice(&FORMAT_VERSION.to_le_bytes()); | ||
|
|
||
| let tmp = self.metadata_tmp_file(); | ||
| let mut file = File::create(&tmp).map_err(StorageError::Io)?; |
There was a problem hiding this comment.
Fresh evidence is that marker-present directories skip the new file_type() rejection path, but create() still reaches this write even for an already-valid database. If that directory contains a metadata.tmp symlink, File::create follows it and truncates/writes the target outside the database directory before the symlink is renamed over metadata; skip rewriting an existing marker or create the temp marker in a way that refuses pre-existing symlinks.
Useful? React with 👍 / 👎.
| // was pointed at by mistake, and refusing it forever is the only safe reading. Under the | ||
| // temporary name it is unambiguously an unfinished attempt of ours, so it can be thrown | ||
| // away and redone | ||
| let path = if create && !self.data_file().exists() { |
There was a problem hiding this comment.
The fresh case is the initial create window: while this branch initializes data.redb.tmp, <dir>/data.redb does not exist and therefore carries no file lock. A process that bypasses the directory API and calls Database::create(dir/data.redb) in that window can create and lock the final path; on Unix the later promote_data() rename replaces that locked inode, so the direct opener is not rejected and its successful handle writes an unlinked file while the directory points at a different database. Create and lock a placeholder at the final path, or otherwise block direct openers, before initializing the temp file.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Real, and I've narrowed it rather than closed it — saying so plainly rather than claiming otherwise.
The window is exactly as you describe: during the first create() of a directory, data.redb does not exist yet, so nothing holds the ordinary file lock on that path, and a process calling Database::create(dir/data.redb) directly in that window would get it. My temporary-file change introduced that window; before it, the file was created and locked up front.
What bf3b1ec does about it: promote_data() now fails rather than renaming when a data.redb has appeared since this call started. The race is still there, but its outcome is an error on this side instead of two processes silently believing they own the database, one of them writing to an unlinked inode. That is the part I was not willing to leave.
I did not take the placeholder. Creating and locking an empty data.redb before initializing the temporary means the promoting rename replaces the very inode this process holds its lock on, so the lock would have to be dropped and retaken on the new file — reopening a window of its own, in the same place, plus a state where data.redb exists while the database is not yet initialized. That is more moving parts than the problem justifies at this size.
The honest scope: write.lock is the real exclusion mechanism, and it is held throughout — every MultiProcessDatabase is correctly excluded for the whole of create(). The lock on data.redb is a courtesy for processes that reach past the directory API, which the PR describes as such, and this window is a gap in that courtesy during first creation only. Worth recording, and I'd rather it were recorded in an issue against the later releases in this series than papered over here — the reader/writer work has to revisit that lock anyway, since on Windows a shared range denies the holder's own writes.
Generated by Claude Code
| /// The caller must hold the write lock, which is what makes this safe against another process | ||
| /// creating the same directory. | ||
| fn read_metadata(&self, create: bool) -> Result<(), DatabaseError> { | ||
| let file = match File::open(self.metadata_file()) { |
There was a problem hiding this comment.
When create() is pointed at a directory whose metadata is a symlink to a valid marker, this open follows the symlink and accepts the directory without running the reserved-name file-type check used for markerless recovery. If data.redb is also a symlink to an external empty or valid redb file, the later data open follows that path and initializes or writes outside the database directory; reject non-regular metadata entries before reading the marker.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Both right, and fixed together in bf3b1ec — this and the metadata.tmp symlink report share a cause, so answering here for both.
My previous fix put the regular-file check only on the recovery path, and you've correctly spotted that a directory with a marker skips it entirely — which is exactly the case where create() still rewrites the marker and so still writes a temporary. So the rule is now applied wherever redb trusts one of its own names, not just during recovery:
metadatamust be an ordinary file before it is read, so a symlink pointing at a valid marker elsewhere cannot vouch for a directory that holds nothing of redb's.data.redb(or the temporary standing in for it) must be an ordinary file before it is opened.- Both temporary files are unlinked and then created with
create_newrather than truncated in place, so a planted symlink is removed instead of written through.
symlinks_are_refused_in_a_directory_that_has_a_marker covers both halves: rewriting the marker of a valid database with a metadata.tmp symlink planted in it leaves the target byte-for-byte intact, and a directory whose metadata is a symlink to a genuinely valid marker is refused by both open() and create().
One limit stated plainly in docs/design.md rather than papered over: this closes the door, it does not lock it. Between the check and the open the entry could be replaced. Doing better needs O_NOFOLLOW, which std does not expose portably, and redb has no libc dependency to reach past it.
Generated by Claude Code
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf3b1ec0f2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if !self.metadata_file().exists() { | ||
| self.reject_foreign_directory()?; | ||
| } | ||
| std::fs::create_dir_all(&self.root).map_err(StorageError::Io)?; |
There was a problem hiding this comment.
When create() is called with a path that does not already exist, this create_dir_all is the operation that links the new database directory into its parent, but the later sync_dir(&self.root) calls only flush entries inside the database directory. On Unix filesystems, a power loss after MultiProcessDatabase::create() returns Ok can therefore lose the parent directory entry for the whole database; fsync the parent directory (and any newly-created ancestors, if supporting missing parents) before reporting a successful create.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and fixed in b8c4ec0. I'd applied the durability argument one level too shallow: syncing entries inside a directory whose own entry in the parent was never flushed loses the entire database, not just the marker. create() now syncs the parent as well, when this call is the one that made the directory.
I did not chase the newly-created ancestors. create_dir_all can make several levels, and making all of them durable means walking back up and syncing each — for a path the caller passed in, whose ancestors are their business, not the database's. The directory holding the database is the boundary this type is responsible for; if ~/a/b/c had to be created on the way, ~/a/b's durability is the same concern any other file in it would have. Worth saying out loud rather than leaving implicit, so: the guarantee is that the database directory's own entry is durable, not that an arbitrary path prefix is.
Generated by Claude Code
| let file = if create { | ||
| open_or_create(&path) | ||
| } else { | ||
| OpenOptions::new().read(true).write(true).open(&path) | ||
| } |
There was a problem hiding this comment.
When an already-marked database has write.lock replaced with a symlink, these opens follow it before any regular-file check runs; on the create() path, .create(true) can even create the symlink target outside the database directory, and both paths then take redb's directory lock on a file whose identity was not validated. Please require write.lock itself to be a regular non-symlink file, or create/open it with a no-follow mechanism where available, before locking it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right — I applied the regular-file rule to metadata and data.redb and left out the one whose entire job is identity. Fixed in b8c4ec0: require_regular_file now runs before the lock file is opened, on both paths, so the rule covers every name this directory trusts.
symlinks_are_refused_in_a_directory_that_has_a_marker grew a third case for it: a valid database whose write.lock has been replaced with a symlink is refused by open() and create() alike, and the target is untouched.
Generated by Claude Code
| // points at, so a symlink wearing one of these names is caught here -- otherwise | ||
| // opening `data.redb` with `create` would follow it and initialize a database over | ||
| // whatever it pointed at, somewhere outside this directory entirely | ||
| let ours = named_like_ours && entry.file_type().map_err(StorageError::Io)?.is_file(); |
There was a problem hiding this comment.
When create() is retried in a markerless directory, this check accepts any regular file with a redb-reserved name, so a hard link named data.redb to an external empty or valid database file is adopted as recovery state. The later Database::new(..., create = true, ...) can initialize, repair, or write allocator-state data through that link and then install a marker here, modifying a file outside the database directory; require accepted recovery files not to have other links where the platform exposes that, or avoid opening an existing unmarked data.redb for writes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Accurate as a description, but I'm not taking this one, and I'd rather say why than quietly implement it.
nlink is Unix-only through MetadataExt, so the check would be a partial defence that Windows does not get, against an attacker who already has write access to the directory you pointed redb at. Someone in that position does not need a hard link — they can replace data.redb wholesale, or corrupt it in place, or take the write lock and hold it. The link only matters if the goal is specifically to have redb write through to a file elsewhere that the attacker cannot write to but redb's process can, which is a real class but not one a single nlink check closes: the same argument applies to Database::create on any path, and to every file redb is ever handed.
There is also a cost. nlink > 1 is not a reliable signal of hostility — backup and deduplication tools, and some copy-on-write filesystems' tooling, produce multiply-linked files as a matter of course — so this would reject legitimate directories in order to narrow an attack that has easier routes.
Where I think this genuinely lands: the last several reports (symlinked metadata, symlinked metadata.tmp, symlinked write.lock, hard-linked data.redb, and the check-then-open race under all of them) are one question — what does redb assume about a directory it has been pointed at, and against whom — and it deserves an answer at the redb level rather than a growing pile of special cases in this one module. This PR now refuses non-regular files under every name it trusts, and docs/design.md says plainly that this closes the door rather than locking it, since std offers no portable O_NOFOLLOW. That is a defensible place to stop for a feature that is still behind an experimental flag.
I'd suggest an issue for the general question, to be settled once for Database and MultiProcessDatabase together. Happy to be overruled by @cberner if the hard-link case is worth the Unix-only check on its own.
Generated by Claude Code
b8c4ec0 to
09d5f49
Compare
cberner
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 09d5f49dd7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| /// file that holds the write lock for as long as the database is open. | ||
| pub(super) fn open(&self, create: bool) -> Result<Box<dyn StorageBackend>, DatabaseError> { | ||
| if create { | ||
| std::fs::create_dir_all(&self.root).map_err(StorageError::Io)?; |
There was a problem hiding this comment.
Fresh evidence in the current revision is that there is no marker or entry validation before this succeeds: create_dir_all also succeeds for an already-existing directory, and the next steps create/follow write.lock and data.redb. When a caller mistypes a path to a non-empty or tampered directory, for example one with unrelated files or a data.redb symlink, MultiProcessDatabase::create() mutates that directory or the symlink target instead of rejecting it, despite the public contract that nothing else may be in the database directory. Please validate that an existing directory is empty or already a valid multi-process directory with regular files before opening with create.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct for this PR in isolation, and deliberately so — it is deferred to #1362, which is the third of three and does exactly what you describe.
This PR was split out of a larger one earlier today. 1/3 (here) is the directory and the lock, 2/3 (#1361) adds the metadata marker, and 3/3 (#1362) is entirely this concern: refusing an existing directory whose contents redb did not write, requiring every trusted name to be a regular file so a data.redb symlink is not followed, and initializing the database file under a temporary name so an interrupted create() cannot wedge the directory. All of it is already written and green — it is on the branch, not on a todo list.
The split was at the repository owner's request, on the grounds that 1.4k lines was too much to review at once. Pulling the validation back into this PR would undo it, so I would rather leave the three parts each arguing one thing and land them in order.
One thing your comment does改 correctly identify as wrong here: the doc comment on MultiProcessDatabase promises "nothing else may be put in it" while this step does not enforce it. That promise is only true once #1362 lands, and it should not read as a guarantee before then — I will fix the wording rather than let it overstate what this PR does.
Generated by Claude Code
| # Multi-process access (in progress) | ||
|
|
||
| `MultiProcessDatabase`, behind the `experimental-multiprocess` feature, stores a database in a |
There was a problem hiding this comment.
This new section replaces, rather than appends to, the prior Version changes and Assumptions about underlying media sections, so docs/design.md no longer records the v1-v3 file-format history or the fsync/PSOW assumptions that define redb's crash-safety model. Since this file is the repository's reference for non-trivial storage-format, transaction, and B-tree changes, losing those sections makes future format and durability work under-specified; please keep them and add the multi-process design separately.
AGENTS.md reference: AGENTS.md:L66-L67
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Caught a real regression — thank you. Fixed in 06261a7 (force-pushed).
You are exactly right about the mechanism. When I split the original PR into three earlier today, the script that rebuilt this file truncated it at the multi-process heading and appended the new section there. In the original PR that section had been inserted before Version changes, so truncating at it destroyed both that section and Assumptions about underlying media — 24 lines of pre-existing reference material, including the v1–v3 format history and the fsync/PSOW assumptions. Nothing about the multi-process work needed them moved; it was purely an artifact of how I re-cut the file.
The multi-process section now goes at the end, after both. git diff master -- docs/design.md on the full stack has zero deleted lines, which is what it should have been all along, and I checked all three branches rather than just this one — 2/3 and 3/3 inherited the same damage and are fixed by the rebase.
Worth noting the class of mistake, since it is the second time a scripted edit here has cost me: the earlier one lost a doc line out of the middle of a function. Rebuilding a file by index into its own text does not fail loudly when the index means something different than you assumed. Reconstructing by appending to a known-good base — which is what the fix does — cannot lose content, and that is the shape I should have used from the start.
Generated by Claude Code
09d5f49 to
06261a7
Compare
redb takes an exclusive advisory lock on the database file to stop a second process opening it. That also stops any other process reading it, which is what has to change before a database can be shared: the exclusion has to move somewhere the file itself is not. MultiProcessDatabase stores the database in a directory -- data.redb beside an empty write.lock -- and takes its exclusion from the lock file. Today that is the same restriction by a different mechanism, one process at a time, so this type has no advantage over Database yet. It is the first step, and sits behind a feature flag until the rest lands: recognizing a directory as one of these, readers attaching while a writer works, and handing the writer role between processes. The lock is held by the storage backend rather than beside the Database. A live write transaction keeps the database open past the point where the handle is dropped, so a lock held by the handle would be released while that transaction was still writing, and another process could start writing too. Tying it to the backend gives it exactly the lifetime of the open file. the_lock_outlives_a_handle_dropped_during_a_write covers this; moving the lock onto the handle makes that test fail and no other. The database file keeps its ordinary exclusive lock as well, since a process that reaches past the directory would not be looking at write.lock. It has to be the exclusive one rather than a shared one, and design.md records why: that constraint is what the later steps have to work around. Assisted-by: Claude Code
06261a7 to
635181c
Compare
|
Closing in favour of #1375, which is this work rebased on current master with the review outcomes folded in. Nine rounds of review left this PR and the two stacked on it hard to read: most threads are outdated, several describe code that later rounds replaced, and the discussion is longer than the diff. The replacements carry the conclusions rather than the argument.
What changed beyond the rebase:
Three things are still open questions for you rather than changes I made, and they are recorded on #1377: whether to add Generated by Claude Code |
1 participant
This PR was 1.4k lines and has been split into three. This one is now just the directory and the lock; the marker and the hardening follow as stacked PRs. Force-pushed, so the earlier review threads show as outdated — the code they point at now lives in #1361 and #1362, and each of those PRs describes the rules those threads produced.
metadatamarkerredb takes an exclusive advisory lock on the database file to stop a second process opening it. That also stops any other process reading it, which is what has to change before a database can be shared: the exclusion has to move somewhere the file itself is not.
This adds
MultiProcessDatabase, which stores the database in a directory:data.redbwrite.lockToday that is the same restriction by a different mechanism -- one process at a time,
DatabaseError::DatabaseAlreadyOpenfor the rest -- so this type has no advantage overDatabaseyet. It is the first step of an incomplete feature, and sits behind the newexperimental-multiprocessfeature flag until the rest of it lands: recognizing a directory as one of these (#1361), refusing directories that are not (#1362), and then the actual point of the exercise -- readers attaching to a database another process is writing, and handing the writer role between processes.Where the lock lives
The lock is held by the storage backend rather than alongside the
Database. A live write transaction keeps the database open past the point where theDatabaseis dropped, so a lock held by the handle would be released while that transaction was still writing, and another process could start writing too. Tying it to the backend gives it exactly the lifetime of the open file: it is released byclose(), which redb calls once, when it has really finished with the file.the_lock_outlives_a_handle_dropped_during_a_writecovers this. Moving the lock onto the handle -- the obvious placement -- makes that test fail and no other.The database file keeps its own lock, for now
write.lockis only visible to a process that goes throughMultiProcessDatabase. A process that reaches past the directory and opensdata.redbdirectly would not be looking at it, so the file still takes the ordinary exclusive lock as well.It has to be the exclusive one rather than a shared one, for two reasons. A shared lock would let a
ReadOnlyDatabasein, and nothing here yet stops this process from freeing pages that such a reader is still using -- readers are only safe once they can publish what they are reading, which is a later step. And on Windows a shared range denies writes to every process including the one holding the lock, so a writer cannot hold one on a file it writes to at all.That second point constrains what comes next, so
docs/design.mdrecords it: making room for readers means removing this lock, not weakening it, and all reader/writer coordination has to live in the directory's separate lock files.Other notes
write.lockin it. Mark a multi-process database directory (2/3) #1361 replaces that with an explicit marker, which is also what gives the layout a version to change later.Databasedoes.transaction_tracker.rs,transactions.rs,page_manager.rsandheader.rshave no changes, and neither do the file backends. The whole thing sits on top of the existingDatabase.Testing
12 integration tests plus 2 unit tests, including real child processes for the cases the lock exists for: a child refused while this process holds the database, a child opening it after this one closes, and a child that dies without closing -- where the operating system drops the lock and nothing has to clean up.
cargo build,cargo clippy --all-targetsand the full test suite pass both with--all-featuresand with default features, and clippy is checked againstx86_64-pc-windows-msvcas well, since this crate deniesclippy::pedanticand some of the later parts are platform-gated.🤖 Generated with Claude Code
https://claude.ai/code/session_01SJFSfcturbVcnPqY5CNzQv
Generated by Claude Code