Record the collection horizon in the commit slots - #1376
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## claude/multiprocess-1-directory #1376 +/- ##
===================================================================
+ Coverage 91.52% 91.55% +0.03%
===================================================================
Files 39 39
Lines 20166 20231 +65
===================================================================
+ Hits 18457 18523 +66
+ Misses 1709 1708 -1
🚀 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: 37eb21f23a
ℹ️ 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 MultiProcessDatabase::create() is called on an existing directory that has a valid data.redb but no metadata marker, this branch treats the missing marker as an in-progress create; open_inner() then opens the existing database and write_metadata_if_missing() writes the marker, silently converting a plain redb directory into a multi-process database. That undermines the new marker as the authority for recognizing directories that belong to this layout, so create() should distinguish a fresh/interrupted create from an unmarked existing directory before accepting the missing marker.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Taken, in 2ce8a94. Good catch, and it survives into 3/3 as well: a directory holding only data.redb passes that PR's foreign-directory check too, since data.redb is a name redb writes. So the rule 3/3 states -- "an unmarked directory is accepted only if it looks like an interrupted create" -- was weaker than it read.
Reproduced before fixing. create_does_not_adopt_a_plain_database builds a directory with Database::create(dir/data.redb) in it and asserts MultiProcessDatabase::create() fails leaving no marker; it fails against the previous commit.
What distinguishes the two states. A create() that got as far as data.redb made write.lock first -- the lock is taken before anything in the directory is read or written, and the database file is only renamed into place much later -- and nothing here ever removes a lock file. So an unmarked directory holding data.redb without write.lock cannot be an interrupted create of ours, and is refused. An interrupted one always has both, and is still finished: an_interrupted_create_is_still_finished removes the marker from a working database and checks the next create() recovers it and reads back the data.
This is not the "did I create it?" inference that produced several bugs in the earlier stack. Those guessed at another process's actions from disk state. This is a necessary precondition -- data.redb implies write.lock was made first -- used only in the direction that holds.
One consequence worth noting: 3/3's create_does_not_mark_a_directory_it_rejects got stronger. It used to assert that a rejected create() left write.lock behind, because the check needed the lock to run at all. The new check needs no lock, so a directory holding a foreign data.redb is now refused before the directory is touched, and the test asserts nothing is left at all.
The limit, stated in docs/design.md rather than implied: this reads the directory's contents rather than proving anything, and assumes nothing else is writing into it concurrently -- which is the contract the directory carries anyway.
3/3 is rebased on this: #1377 is now 56ebf32.
Generated by Claude Code
d0e0757 to
2ff91ad
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ff91adbb8
ℹ️ 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_unmarked_database()?; |
There was a problem hiding this comment.
When create() is pointed at an existing directory that contains an unrelated metadata file but no write.lock, this exists() check skips the preflight rejection and acquire_write_lock(true) then creates write.lock before read_metadata() rejects the invalid marker. That leaves a redb lock file behind in a directory the call already determined was not a multi-process database, so failed creates can still modify foreign directories instead of refusing them cleanly.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and taken in e04903d. The preflight was gated on the marker being absent, so a directory with a foreign metadata skipped it entirely and got a write.lock before the marker was ever looked at.
a_directory_belonging_to_something_else_is_left_alone already asserted the foreign file survived; it now also asserts no write.lock appears, and fails against the previous commit.
The fix is a preflight read_metadata when the name is occupied, rather than moving the lock or unlinking after the fact. That is sound for the same reason the marker can be trusted at all: it arrives by rename, so it is either absent or complete. There is no interleaving of concurrent creates that produces a metadata file which is not a marker -- that state can only come from outside redb -- so rejecting on it before the lock cannot turn away a legitimate call. The check runs again under the lock, which stays authoritative; the early pass only ever refuses, never accepts.
Two details worth noting:
- The branch tests
occupied()rather thanexists().exists()follows the link, so a danglingmetadatasymlink read as absent and fell through to the unmarked path; now it routes intoread_metadata, whoserequire_regular_filerefuses it -- also before the lock file exists.a_marker_that_is_a_symlink_is_not_a_markercovers that and still passes. - This does not make "a failed
create()leaves nothing behind" true in general, anddocs/design.mdstill says so. Acreate()that gets past the marker and then hasDatabasereject the data file has necessarily made a lock file already, and removing it on the way out would be worse than leaving it -- another process may be waiting on the lock on that same file. What changed is that the foreign directory case no longer needs the lock to reach its verdict.
#1377 is rebased on this: eb904c7.
Generated by Claude Code
e04903d to
5348c24
Compare
019d736 to
84f5064
Compare
5348c24 to
29e34ce
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: 29e34ce8d4
ℹ️ 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 occupied(&self.write_lock_file()) { | ||
| return Ok(()); |
There was a problem hiding this comment.
When create() targets an unmarked foreign directory containing a file named write.lock, this check treats any occupied entry as proof of an interrupted redb creation. In particular, a nonempty regular file or a symlink to an ordinary file passes here; acquire_write_lock() can then open it (following the symlink), after which the call creates data.redb and metadata and silently converts the foreign directory. The new preflight should only accept the empty, regular lock file that redb itself can create as evidence of an interrupted creation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Taken, in fe3c567 (with b2823c8 in between): the preflight now accepts a lock file as evidence only while it could be redb's own -- a regular file with no contents. A nonempty file, or anything under that name that is not a regular file, is refused before the directory is touched, and create_does_not_trust_a_nonempty_lock_file covers it with a plain database left intact beside the junk lock file. (acquire_write_lock itself additionally refuses non-regular files from #1377 on, where require_regular_file is introduced.)
Generated by Claude Code
84f5064 to
8727f0d
Compare
29e34ce to
ec39d2e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ec39d2e896
ℹ️ 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".
| std::fs::rename(&tmp, self.metadata_file()).map_err(StorageError::Io)?; | ||
| sync_dir(&self.root)?; |
There was a problem hiding this comment.
When the rename succeeds but the following directory fsync fails (for example with an I/O error or on a filesystem that cannot sync directories), write_metadata() returns an error after the authoritative metadata marker is already visible. Consequently MultiProcessDatabase::create() reports failure while leaving a directory that subsequent open() calls recognize as a completed database, contradicting the failed-call invariant in docs/design.md; either roll back the marker before returning the sync error or treat the successful rename as the create commit point.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Acknowledged, but this state heals itself rather than contradicting the invariant, so I am leaving the ordering as it is.
The marker goes in only after the database file is complete -- it is the last thing create() writes -- so the directory a failed sync leaves behind is a valid database whose marker entry merely is not durable yet. An open() that sees it works correctly; one after a crash that lost the entry sees an interrupted create, which the next create() finishes. And rerunning create() after the reported failure goes through write_metadata_if_missing, which finds the marker and repeats exactly the directory sync that failed, so the durability the error reported missing is what the retry supplies.
Rolling the marker back on a sync failure would need an unlink and another sync through the same directory that just failed to sync -- a rollback less reliable than what it rolls back -- and "treat the rename as the commit point" would report success for a create whose durability was never established.
Generated by Claude Code
8727f0d to
07de664
Compare
ec39d2e to
6cf4f9f
Compare
07de664 to
fe784c7
Compare
6cf4f9f to
5525382
Compare
fe784c7 to
878a87a
Compare
5525382 to
2f74809
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f74809ad6
ℹ️ 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".
| // lot. On every create(), not only the one that made the directory: finding it already | ||
| // there says nothing about whether anyone flushed it, and the lock cannot cover the | ||
| // directory's own creation because it lives inside it | ||
| sync_parent(&self.root)?; |
There was a problem hiding this comment.
When path contains two or more nonexistent parent components, create_dir_all() creates the whole chain, but sync_parent() fsyncs only the database directory's immediate parent. For example, creating /existing/a/b/db syncs /existing/a/b, while the new entries for a and b remain non-durable; a power loss after a successful create and durable commit can therefore remove the entire database. Create and sync each missing ancestor, or require the direct parent to exist before creating the database.
AGENTS.md reference: AGENTS.md:L65-L71
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Taken, in #1375 as 915add3, where the directory creation lives. create() records the deepest ancestor that already existed before create_dir_all, and afterwards flushes every entry from the database directory's parent up to and including that ancestor, bottom-up -- so /existing/a/b/db gets b, a's entry in /existing... each made durable, not just the immediate parent.
Generated by Claude Code
5badb1a to
f2b0504
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: f2b050417a
ℹ️ 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".
| // The lock file's entry must be durable before anything else is written under these | ||
| // names: its *absence* beside them is what says a directory is not this database's, | ||
| // and a crash could otherwise keep `data.redb` while losing `write.lock` | ||
| sync_dir(&self.root)?; |
There was a problem hiding this comment.
On Windows, this durability barrier is ineffective because the cfg(not(unix)) implementation of sync_dir() is a no-op. After a power loss, the already-synced data.redb can therefore survive while the new write.lock directory entry is lost; the next create() then interprets that missing lock as proof that the data file is foreign and refuses the interrupted database permanently. Implement a platform-specific write-through/flush for the sentinel, or avoid using its durable absence as ownership evidence on platforms where the entry cannot be synced.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Acknowledged, and leaving it, with the cost stated rather than hidden. Off Unix, std exposes no way to flush a directory entry, so the ordering the rule leans on cannot be established there -- the same gap docs/design.md already records for the marker's own rename. The failure it can produce is a loud refusal of the interrupted directory, never an adoption or a deletion: since a17bc8e a finished database restored under the temporary name is protected by the magic gate on every path, so what a person recovers from that state is intact. Weakening the rule instead -- not treating the missing lock as evidence on non-Unix -- would reopen silent adoption of plain databases on exactly the platform that cannot order writes, which is the worse trade.
Generated by Claude Code
| /// Whether a name is taken, by anything at all -- including a symlink that resolves to nothing, | ||
| /// which `Path::exists` reports as absent because it follows the link. | ||
| fn occupied(path: &Path) -> bool { | ||
| std::fs::symlink_metadata(path).is_ok() |
There was a problem hiding this comment.
When symlink_metadata() fails for anything other than NotFound, such as a transient network-filesystem I/O error, occupied() reports that the path is absent. If this happens while probing an existing data.redb or metadata.tmp, the preflight proceeds to create write.lock; once the transient error clears, the later open can adopt the data file or File::create() can truncate the existing temporary file. Return a Result<bool> and treat only an actual NotFound error as unoccupied.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Taken, in fe3c567. occupied() returns Result<bool> now: only a clean NotFound answers "absent", and every other error is reported rather than read as absence for the accepting paths to trust. All of its callers -- the preflight, the plausibility checks, the fresh-lock pass, and write_metadata_if_missing -- propagate it.
Generated by Claude Code
f2b0504 to
fb192ac
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fb192ac312
ℹ️ 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 occupied(&self.metadata_file()) { | ||
| self.read_metadata(create)?; |
There was a problem hiding this comment.
When metadata is a dangling symlink, occupied() selects this branch, but read_metadata(true) treats the resulting NotFound from File::open as a missing marker and returns success. The create then initializes data.redb and renames metadata.tmp over the symlink, silently taking over a directory whose metadata name was already occupied, contrary to the new refusal rule; inspect the entry without following it and reject non-regular markers.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Taken, in #1375 as 915add3, where read_metadata lives. Its missing-marker arm no longer trusts File::open's NotFound on its own: the name being occupied at all -- asked without following the link -- means the directory is not redb's to mark, so a dangling metadata symlink is refused before this PR's rules even run, and the rename can never land on it.
Generated by Claude Code
| } else { | ||
| self.reject_unmarked_database()?; |
There was a problem hiding this comment.
When another process creates and closes a plain data.redb in an initially empty directory after this preflight but before acquire_write_lock(true), this check sees no occupied name, the current call creates write.lock, and the under-lock pass only rereads metadata. It can then open the newly appeared plain database and install the marker, so the refusal is bypassed under concurrent creation; the code must atomically distinguish creating the sentinel/data entries from resuming entries that the sentinel already vouched for.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This window was closed in b2823c8 (now fe3c567), one push before this review's commit: the call records whether write.lock existed before it was opened, and a fresh lock file vouches for nothing -- once the lock is held, create() requires redb's other names (data.redb included) to be absent, so a plain database that landed between the preflight and the lock is refused rather than adopted. And since fe3c567 that late refusal also marks the fresh lock file it leaves behind, so the next create() refuses the directory too instead of trusting the sentinel.
Generated by Claude Code
| let tmp = std::fs::symlink_metadata(self.metadata_tmp_file()); | ||
| if tmp.is_ok_and(|metadata| metadata.is_file() && metadata.len() > METADATA_LEN as u64) | ||
| { |
There was a problem hiding this comment.
When an unmarked directory contains an empty regular write.lock and metadata.tmp is a symlink to an external file, symlink_metadata() reports the entry as a symlink, so metadata.is_file() is false and this condition accepts it regardless of the target's size. write_metadata() subsequently follows that link with File::create, truncating and overwriting the external target before renaming the symlink to metadata; require the temporary entry to be a regular file before treating it as interrupted redb state.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Taken, in fe3c567. The plausibility check on metadata.tmp now refuses anything that is not a regular file, alongside the size bound: redb's own temporary is always a regular file of at most the marker's length, so a symlink under that name cannot be an interrupted create's, and accepting it would write the marker through whatever the link points at. Refused before the directory is touched, like the other pre-lock rules.
Generated by Claude Code
fb192ac to
b2823c8
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b2823c8566
ℹ️ 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 !lock_preexisted { | ||
| self.reject_files_beside_a_fresh_lock()?; |
There was a problem hiding this comment.
When an ordinary Database creates data.redb after the preflight but before this post-lock check, the check returns AlreadyExists, but ? only drops the lock handle and leaves the newly created and synced write.lock entry behind. A later create() treats that empty preexisting lock as proof of an interrupted multi-process create, skips this fresh-lock check, and silently adopts the plain database. The new post-lock rejection path is fresh evidence that the earlier atomicity concern remains: a late rejection must not leave a sentinel that future calls trust.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Taken, in fe3c567. The lock file still cannot be unlinked -- nothing ever unlinks one -- but a fresh lock the call cannot stand behind is no longer left empty: on the late rejection it is written to and flushed before the error returns, so the next create() refuses it under the nonempty-lock-file rule instead of reading it as an interrupted create's. The refusal therefore stays a refusal across calls, at the cost of the directory needing a person to clear it -- which the concurrent direct-opener interference already implied.
Generated by Claude Code
b2823c8 to
fe3c567
Compare
16d83d4 to
915add3
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe3c567090
ℹ️ 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()); | ||
| } | ||
|
|
||
| let lock_preexisted = occupied(&self.write_lock_file())?; |
There was a problem hiding this comment.
When two MultiProcessDatabase::create() calls race, the second can observe no lock here, pause while the first creates the database and closes it, and then acquire that now-existing lock while still treating it as freshly created. The post-lock check consequently sees the first call's data.redb/metadata, marks the valid lock as abandoned, and returns AlreadyExists instead of opening the completed database. Fresh evidence beyond the earlier plain-data race is that the added lock_preexisted snapshot misclassifies a lock created by another legitimate create(); use atomic create-new/open-existing semantics to determine who created the lock.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Taken, in 6d97268 (and 824deda, where the protocol PR dissolves this sequence into WriteLock). Creation is now decided by the open itself -- create_new, falling back to opening the existing file on AlreadyExists -- rather than by a snapshot beforehand, so a lock file another create() made and completed under is never read as this call's own: the loser of the race opens the finished database instead of tombstoning its lock. The fresh-lock refusal is also gated on the marker still being absent under the lock, which keeps the other direction straight -- a call that really did create the lock file but finds a marker (a crash lost the lock's directory entry, and this call recreated it) opens the database rather than refusing it.
Generated by Claude Code
915add3 to
b0af83b
Compare
fe3c567 to
6d97268
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d97268cac
ℹ️ 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 lock_created | ||
| && !marked | ||
| && let Err(err) = self.reject_files_beside_a_fresh_lock() |
There was a problem hiding this comment.
When an ordinary Database::create() creates and closes data.redb after this fresh-lock check but before the later OpenOptions::open, that open accepts the existing file, Database::new recognizes it, and this call installs the multiprocess marker, silently adopting the plain database. Fresh evidence beyond the earlier atomicity discussion is that the authoritative check and the create-or-open of data.redb remain separate operations; the fresh-lock path needs to claim the data name atomically rather than checking and then opening with create(true).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Taken, in 68c9dcf, and carried through the stack. On a fresh claim -- this call created the lock file and found no marker -- the database file is now created exclusively rather than checked-then-opened: one that appears in the window fails create_new, the call refuses, and the lock file is marked so the next create() refuses the directory too instead of trusting the sentinel. From 3/6 on, where initialization happens under data.redb.tmp, the same rule refuses any data.redb that appears on a freshly claimed directory (the temporary was already create_new there), and 4/6 keeps both across the dissolved open sequence.
Generated by Claude Code
| let _ = (&write_lock).write_all(b"abandoned by a rejected create\n"); | ||
| let _ = write_lock.sync_all(); |
There was a problem hiding this comment.
When the late ownership check rejects a concurrently created data.redb, an ENOSPC, quota, or I/O failure can make write_all fail while leaving the newly created lock empty, but both invalidation errors are discarded and the call returns the original rejection. A subsequent create() then treats that empty lock as evidence of an interrupted multiprocess create and can adopt the foreign data file; the rejection path must not release the lock unless its sentinel invalidation is guaranteed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Acknowledged, and leaving it best-effort, because the guarantee asked for does not exist to give: a crash in the same window -- after the lock file is created, before or during the invalidation -- releases the lock with the file left empty no matter what the code does, since the operating system drops advisory locks with the process and the file's contents are whatever landed. An ENOSPC on the sentinel write leaves exactly the state that crash leaves, so refusing to release the lock (impossible past process death) or unlinking it (nothing ever unlinks one -- a waiter may hold the old inode) would narrow nothing. The residue needs a direct opener's interference and a simultaneous write failure and a later create() on the same directory; what that create() then adopts is bounded by the fresh-lock rule's other refusals, and since 68c9dcf the adoption itself requires the claim to hold exclusively at the data file too.
Generated by Claude Code
| let lock = std::fs::symlink_metadata(self.write_lock_file()); | ||
| let tmp = std::fs::symlink_metadata(self.metadata_tmp_file()); | ||
| if lock.is_ok_and(|metadata| !metadata.is_file() || metadata.len() > 0) |
There was a problem hiding this comment.
On a transient filesystem error during either of these direct symlink_metadata calls, Result::is_ok_and evaluates to false, so a preexisting lock is trusted without having proved that it is an empty regular file (and the temporary marker is likewise accepted without validation). Fresh evidence beyond the earlier error-propagation comment is that these new direct probes bypass occupied() and still discard every error; if the later opens succeed after the transient error clears, create() may adopt foreign files or overwrite metadata.tmp, so only NotFound should be treated as absence.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Taken, in 68c9dcf. The plausibility probes now go through the same rule occupied() follows -- only a clean NotFound reads as absence, and every other error is reported rather than treated as a name that passed the checks. Both questions sit behind one helper now, so they cannot drift apart again.
Generated by Claude Code
6d97268 to
68c9dcf
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 68c9dcf318
ℹ️ 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".
| // A marker that appeared since the pre-lock pass means another create() finished the | ||
| // database while this one was claiming the directory; this call now simply opens it, | ||
| // and the fresh-lock rule below has nothing to refuse | ||
| fresh_claim = lock_created && !occupied(&self.metadata_file())?; |
There was a problem hiding this comment.
When the preflight observes no marker and this call then successfully creates write.lock, any metadata that appears before this check cannot have come from another legitimate create(), because that process could not acquire the newly held lock. However, an external process can place a valid marker during this window; the condition then sets fresh_claim to false, skips the rejecting pass (including its metadata check), and proceeds to create or adopt data.redb. Preserve whether the marker passed the preflight, and reject a marker that first appears after this call creates the lock rather than treating it as a completed concurrent create.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Taken, in 810a3f0, and the reasoning is exactly as you state it: with the lock file freshly created and held, no other create() can have finished, so a marker that was not there at the preflight cannot be legitimate. The call now remembers whether the marker passed the preflight; the fresh-claim rule keys on that instead of re-reading the name, so the rejecting pass -- whose name list already includes metadata -- refuses a marker that first appears during the claim, tombstoning the lock like every other interference refusal. The one marker a fresh lock does not have to vouch for is the one seen before the lock file was made, which is what a crash that lost the lock's entry while the marker survived looks like -- that create() is still finished rather than refused. Carried through the stack: the protocol PR's prepare now reports the preflight state, and a marker first appearing under the held lock is refused there the same way.
Generated by Claude Code
68c9dcf to
810a3f0
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 810a3f0f56
ℹ️ 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".
| .open(&path) | ||
| { | ||
| Ok(file) => (file, true), | ||
| Err(err) if err.kind() == ErrorKind::AlreadyExists => (open_existing()?, false), |
There was a problem hiding this comment.
When an empty regular write.lock passes the preflight but is replaced or made nonempty before this fallback open, the opened file is never validated again and created is false, so the fresh-claim rejection is skipped. The call can then convert the directory despite its lock no longer satisfying the ownership rule, or even lock a substituted symlink target; validate the opened-and-locked handle and confirm it still represents the expected directory entry before trusting it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Taken, in fe5bffb, for the half of it that a handle can answer: once the lock is held, the file is re-read through that handle, and one that gained contents -- an abandonment mark included -- or stopped being a regular file is refused, so the preflight's snapshot no longer stands in for the file that was actually locked. The identity half -- proving the locked inode is still the one the directory entry names -- has no portable answer in std (no O_NOFOLLOW, no stable inode comparison off Unix), which is the boundary require_regular_file's comment already draws; what remains inside it is a substituted link to an empty regular file, planted mid-claim by something already reaching past the directory API.
Generated by Claude Code
| if fresh_claim && let Err(err) = self.reject_files_beside_a_fresh_lock() { | ||
| mark_lock_abandoned(&write_lock); | ||
| return Err(err); | ||
| } |
There was a problem hiding this comment.
When another process places a valid marker after this check returns but before read_metadata() or write_metadata_if_missing(), those later calls accept it, and this fresh claimant initializes data.redb and silently adopts the directory. Fresh evidence beyond the earlier marker-race comment is that the new rejecting pass now includes metadata, but the subsequent metadata reads still treat a marker appearing after that pass as preexisting; the fresh claim must retain an atomic refusal of the marker name through marker installation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The adoption here changes nothing this PR can observe: the marker is a constant thirteen bytes in this PR, so a marker that appears and validates is byte-for-byte the file this call was about to rename into place -- accepting it and having written it are the same directory. The one divergence a valid marker can carry arrives with the writer mode in 4/6, and there (f1b1aa2) write_metadata_if_missing now requires an existing marker to carry exactly the mode being created; one that validates but differs can only have arrived from outside during the claim, and is refused. Past that, a fully atomic hold on the name through installation would need a no-replace rename, which std does not expose portably -- the same boundary as the data file's promotion, recorded in the known-gap thread there.
Generated by Claude Code
| let claimed = Self::WRITTEN_UNDER_THE_LOCK.iter().copied(); | ||
| for name in claimed.chain([METADATA_FILE_NAME]) { | ||
| if occupied(&self.root.join(name))? { | ||
| return Err(StorageError::Io(io::Error::new( |
There was a problem hiding this comment.
When a process places a metadata.tmp symlink after this accepting pass but before write_metadata() calls File::create, the check has already succeeded and File::create follows the new link, truncating and overwriting its external target. Fresh evidence beyond the earlier temporary-symlink comment is that non-regular entries are now rejected only by snapshots taken before the destructive open, leaving the later replacement window; the temporary file must be opened or replaced atomically without following a name that appeared after validation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is #1377's subject, where the write is already shaped for it: write_metadata there unlinks the temporary and recreates it with create_new, and O_CREAT|O_EXCL refuses to open through a symbolic link no matter when it appeared -- the open fails with AlreadyExists rather than following a link planted after any check. Within this PR the temporary is still created in place with File::create; the unlink-and-create-exclusively treatment lands one PR up with the rest of the non-regular-file rules.
Generated by Claude Code
810a3f0 to
fe5bffb
Compare
There was a problem hiding this comment.
💡 Codex Review
redb/src/multiprocess/locks.rs
Line 314 in fe5bffb
Invalidate the sentinel when data locking loses the race
When an ordinary Database::create() opens and locks the newly created data.redb after options.open() succeeds but before this FileBackend::new() takes its lock, this call returns DatabaseAlreadyOpen while leaving the fresh write.lock empty. Once the ordinary database closes, a later multiprocess create() trusts that sentinel and adopts the plain database. The fresh evidence beyond the earlier create-new race is this unlocked interval after exclusive creation; on a FileBackend::new failure, a fresh claim should be marked abandoned before returning.
ℹ️ 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".
fe5bffb to
535ab8d
Compare
|
On the latest Codex finding ("Invalidate the sentinel when data locking loses the race", which has no thread to reply in): taken, in 535ab8d. On a fresh claim, failing to take the data file's lock now marks the lock file abandoned before the error returns -- the file was created exclusively a moment earlier, so losing its lock means something took it in between, and the lock file left empty would vouch, on the next Generated by Claude Code |
3d58e38 to
8041632
Compare
The multi-process protocol (docs/design.md) invalidates a process's page cache by watermark: a page can only change after it is freed and reused, reuse begins when a commit processes the freed-page records, and the newest transaction so processed is the collection horizon. A process whose cache was built against an older horizon than the file's has to drop it; one whose horizon is current can keep it across other processes' commits. Store that watermark in each commit slot, in the last eight bytes of what was padding, directly ahead of the transaction id. Living inside the checksummed region gives it the crash story of the roots it travels with for free: it becomes visible exactly at the flip that makes the reuse it describes visible, a torn write of it fails the slot checksum, and repair falling back to the other slot gets the horizon that matches that slot's roots. Every durable commit records the boundary its freed- page processing reached; commits that process none, the repair and shutdown commits among them, carry the stored value forward. The horizon only ever advances, since reuse an earlier commit made visible cannot be taken back. Files from before the field existed read as zero, the safe floor, and the file format version is unchanged: the bytes were padding, covered by the same checksum, and mean nothing to a version of redb that does not read them. The version advances when a database is opened for multi-process access, which is the point where an older redb -- unaware of range locks -- must be refused, and where a horizon this file never maintained is initialized rather than trusted. Nothing in a single process consults the stored horizon: the in-memory tracker knows strictly more. It is written now so that the protocol built on it reads a file that has been keeping it current. Assisted-by: Claude Code
535ab8d to
0f9cae4
Compare
cberner
changed the title
|
Closing for a restructure into a two-PR stack: the range-lock addition to the existing open modes alone, and the entire multi-process implementation stacked on top of it, to be split up incrementally as the first merges. Generated by Claude Code |
1 participant
Second PR of the multi-process implementation on the revised design. Based on #1375 -- review that one first; this diff is only the horizon field. No new public API, no file format version change.
The multi-process protocol invalidates a process's page cache by watermark: a page can only change after it is freed and reused, reuse begins when a commit processes the freed-page records, and the newest transaction so processed is the
collection horizon transaction idofdocs/design.md. This PR makes every commit keep that watermark current in the file, so the protocol built on it in later PRs reads a value that has always been maintained.Tests: a slot-level roundtrip that also proves a torn horizon write fails the slot checksum and that pre-field files parse as zero, and a database-level test that the horizon advances with commits, survives reopen, and never regresses.
🤖 Generated with Claude Code
https://claude.ai/code/session_01SJFSfcturbVcnPqY5CNzQv