Keep an ordinary Database out of a multi-process data file (6/6) - #1389
Keep an ordinary Database out of a multi-process data file (6/6)#1389cberner wants to merge 1 commit into
Conversation
![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: c9652e31dd
ℹ️ 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 path.file_name() != Some(OsStr::new(DATA_FILE_NAME)) || !marker_present(&parent_of(path)) { | ||
| return Ok(()); |
There was a problem hiding this comment.
Because this comparison only checks the spelling supplied by the caller, it is bypassed whenever another path resolves to the same file. For example, on a case-insensitive Windows or macOS volume, Database::open("db/DATA.REDB") opens db/data.redb while this function returns Ok(()); on Unix, a symlink with a different basename has the same effect. The ordinary database then acquires its otherwise-unused file lock and may write underneath active multi-process readers, recreating the corruption this change is intended to prevent. The check needs to identify the resolved/opened target (with appropriate handling for case-insensitive paths and symlinks), rather than relying on an exact lexical filename.
Useful? React with 👍 / 👎.
| if path.file_name() != Some(OsStr::new(DATA_FILE_NAME)) || !marker_present(&parent_of(path)) { | ||
| return Ok(()); |
There was a problem hiding this comment.
When an ordinary open races with MultiProcessBuilder::create, this check can run after promote_data() has renamed the initialized file to data.redb but before write_metadata_if_missing() installs the marker. In that interval marker_present() returns false, Database::open() successfully opens the now-valid file, and the multi-process creator subsequently installs the marker and returns too, leaving both uncoordinated handles able to access the same file. Since this can cause the same silent corruption the patch is intended to prevent, the refusal must also recognize the in-progress multi-process directory state or otherwise synchronize the marker check with creation.
Useful? React with 👍 / 👎.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## claude/multiprocess-5-savepoints #1389 +/- ##
====================================================================
+ Coverage 91.07% 91.14% +0.07%
====================================================================
Files 42 42
Lines 22119 22140 +21
====================================================================
+ Hits 20145 20180 +35
+ Misses 1974 1960 -14
🚀 New features to boost your workflow:
|
c04861a to
6caf721
Compare
c9652e3 to
4ecdae6
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ecdae69f8
ℹ️ 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".
| /// * otherwise this function will return an error | ||
| #[cfg(not(redb_no_std))] | ||
| pub fn create(&self, path: impl AsRef<Path>) -> Result<Database, DatabaseError> { | ||
| reject_multi_process_data_file(path.as_ref())?; |
There was a problem hiding this comment.
When a caller opens <multiprocess-dir>/data.redb with std::fs::OpenOptions and passes that handle to Builder::create_file(), none of these path-based checks run; create_file() proceeds through FileBackend::new, whose exclusive lock does not conflict with the lock-free multi-process handles, and returns an ordinary writable Database. Writes through it can therefore corrupt a concurrently used multi-process database, so the file-handle API must also be prevented from bypassing this protection (or be explicitly made incapable of opening such files).
Useful? React with 👍 / 👎.
6caf721 to
93e62d9
Compare
4ecdae6 to
f737b3d
Compare
93e62d9 to
e08c23c
Compare
f737b3d to
52f0d11
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 52f0d11759
ℹ️ 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".
| Ok(metadata) if metadata.is_file() && metadata.len() == METADATA_LEN as u64 => {} | ||
| _ => return false, | ||
| } | ||
| std::fs::read(&path).is_ok_and(|contents| contents.starts_with(&MAGIC)) |
There was a problem hiding this comment.
When data.redb is writable but metadata is unreadable (for example, because their ACLs or modes differ), std::fs::read returns an error and this converts it to false, allowing an ordinary writable Database to open the unlocked data file while existing multi-process handles continue using it. That recreates the silent-corruption path this guard is meant to prevent; errors encountered after finding a marker-shaped regular file should be propagated rather than treated as proof that no marker exists.
Useful? React with 👍 / 👎.
e08c23c to
d6f1cbc
Compare
52f0d11 to
2ac0c8c
Compare
d6f1cbc to
7db2b6b
Compare
2ac0c8c to
bdbcb11
Compare
7db2b6b to
7ac93db
Compare
bdbcb11 to
213eb43
Compare
7ac93db to
2256c24
Compare
213eb43 to
67c8e66
Compare
2256c24 to
c7061a4
Compare
67c8e66 to
0cd37a1
Compare
c7061a4 to
fc98478
Compare
0cd37a1 to
86c340a
Compare
fc98478 to
a280729
Compare
86c340a to
dab1f67
Compare
a280729 to
e01f184
Compare
598cdf1 to
d5692f8
Compare
b7a0669 to
4039c61
Compare
e4f4f9f to
ab42f01
Compare
6dc26da to
96c3e3d
Compare
ab42f01 to
3f7ed41
Compare
96c3e3d to
b953004
Compare
3f7ed41 to
240cb5e
Compare
b953004 to
4e4ae43
Compare
240cb5e to
337bc83
Compare
4e4ae43 to
b72097d
Compare
d5afb8c to
8dcdb0b
Compare
b72097d to
1f6a787
Compare
8dcdb0b to
9d5a5e1
Compare
22ea1a4 to
6846d48
Compare
06695bd to
41d293f
Compare
39afd44 to
d4710ff
Compare
41d293f to
9eab122
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9eab122f62
ℹ️ 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 processes using it properly; the marker beside it is what says to keep away. Costs one | ||
| /// `stat` of a name that is almost never there, and only for a file called `data.redb`. |
There was a problem hiding this comment.
The Costs one stat... sentence documents a minor performance detail of the current implementation rather than an invariant, and it will become stale if marker detection changes. Remove it and retain the preceding explanation of why the marker check is necessary.
AGENTS.md reference: AGENTS.md:L32-L35
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Taken, in 9f2c768 -- the sentence is removed, and the comment keeps only why the marker check exists.
Generated by Claude Code
9eab122 to
eacb386
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 845beab207
ℹ️ 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".
| @@ -1,5 +1,8 @@ | |||
| use crate::io; | |||
| use crate::multiprocess::ProcessCoordinator; | |||
| // Only the paths that open a file can be pointed at one, and those need std | |||
There was a problem hiding this comment.
This comment only explains why the adjacent import is conditionally compiled, which is a minor implementation detail rather than a lasting invariant or architectural constraint. Remove it so it cannot become stale if the import or feature gating moves.
AGENTS.md reference: AGENTS.md:L32-L35
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
| @@ -1,5 +1,8 @@ | |||
| use crate::io; | |||
| use crate::multiprocess::ProcessCoordinator; | |||
| // Only the paths that open a file can be pointed at one, and those need std | |||
There was a problem hiding this comment.
This commit records both its author and committer as Codex <codex@openai.com>, while the repository requires the human's name and email and permits the agent only in an Assisted-by: trailer. The inspected commit-authors CI job explicitly rejects author or committer names matching codex, so a PR containing this commit cannot pass CI until its authorship is corrected.
AGENTS.md reference: AGENTS.md:L37-L41
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The commit's recorded author and committer are both Christopher Berner <me@cberner.com> — git log --format='%an <%ae> / %cn <%ce>' on this branch shows exactly that for every commit in the stack, and the commit-authors CI job (which enforces the allowed-author list) has passed on every push. No commit in this PR carries a Codex identity; the only attribution present is the Assisted-by: Claude Code trailer, which AGENTS.md requires. Leaving as is.
Generated by Claude Code
| that belongs to a `MultiProcessDatabase` directory. That file carries no lock of its own -- every | ||
| process using the database has it open at once -- so nothing else stopped an ordinary `Database` | ||
| from writing to it underneath processes using it properly; the `metadata` marker beside it is | ||
| what turns them away. |
There was a problem hiding this comment.
The first sentence already communicates the user-facing change, while the remaining discussion of file-lock ownership and the metadata marker documents implementation mechanics. Shorten the entry to the refusal and corruption-prevention impact so the release note remains brief and does not become stale when marker detection changes.
AGENTS.md reference: AGENTS.md:L45-L48
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Taken, in 2ab215b — the entry is trimmed to the user-facing impact: the three open paths now refuse a data.redb that belongs to a MultiProcessDatabase directory, which an ordinary handle could otherwise corrupt by writing underneath the processes using it.
Generated by Claude Code
A multi-process database keeps its data in an ordinary redb file with no lock on it. It cannot have one: every process using the database has the file open at once, so an exclusive lock is impossible, and a shared lock would stop the writer writing on the platforms where locks are mandatory. The lock files beside it are what keep those processes out of each other's way -- and a Database opened directly on data.redb is not looking at any of them. It would take the file's own exclusive lock, find nobody holding it, and start writing pages that other processes are reading and reallocating. Refuse it. Database::open(), Database::create() and ReadOnlyDatabase::open() now check for the multi-process marker beside a file named data.redb, and fail with a message pointing at the directory. Nothing else changes: the check costs one stat of a name that is almost never there, and only for a file with that name, so a path that is not inside one of these directories cannot match. The refusal stands whether or not any process currently has the directory open, since a Database that writes to the file while nothing else holds it leaves a database the next multi-process writer never made. Assisted-by: Claude Code
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 74003293d0
ℹ️ 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".
| /// * otherwise this function will return an error | ||
| #[cfg(not(redb_no_std))] | ||
| pub fn create(&self, path: impl AsRef<Path>) -> Result<Database, DatabaseError> { | ||
| reject_multiprocess_data_file(path.as_ref())?; |
There was a problem hiding this comment.
Fresh evidence contradicts the earlier thread reply: inspecting the exact object with git cat-file -p ecbed483761bc64ad82137fdfd82e2c627d8d04c shows both author Codex <codex@openai.com> and committer Codex <codex@openai.com>. The commit-authors job in .github/workflows/ci.yml checks every PR commit and fails when either identity contains codex, so this commit cannot pass CI until it is recreated with the human's identity and the agent is recorded only in an Assisted-by: trailer.
AGENTS.md reference: AGENTS.md:L37-L41
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The cited object does not exist: git cat-file -t ecbed483761bc64ad82137fdfd82e2c627d8d04c fails with "could not get object info" -- that SHA is in no branch of this repository, so it cannot be evidence about this PR. The commits that are in the PR (git rev-list origin/master..HEAD lists exactly the six in this stack, 7400329 included) each record Christopher Berner <me@cberner.com> as both author and committer. And the commit-authors job the finding says cannot pass completed with success on this very head at 06:05 UTC, minutes before the finding was posted -- verifiable at https://github.com/cberner/redb/actions/runs/32936620437/job/98078928255. Leaving as is.
Generated by Claude Code
|
Closing for a restructure into a two-PR stack on the revised single-file range-lock design: the range-lock addition to the existing open modes alone, and the entire multi-process implementation stacked on top of it. The directory-based approach this PR implemented is superseded by the revised docs/design.md. Generated by Claude Code |
1 participant
Last of six. Based on #1388, which is based on #1387 → #1377 → #1376 → #1375. 73 lines, and the only one in the stack that changes behaviour outside the feature flag — which is why it is separate and last. Drop it if you disagree; nothing above depends on it.
metadatamarker, shared lockDatabaseoutThe regression this closes
#1375 put the ordinary exclusive lock on
data.redb, on top ofwrite.lock, precisely so that a process reaching past the directory could not open the database file directly. #1387 had to remove it: every process using a multi-process database has that file open at once, so an exclusive lock is impossible, and a shared lock would stop the writer writing on the platforms where locks are mandatory.So after #1387 nothing stops this:
while other processes are reading and reallocating pages in that file. It takes the file's own exclusive lock, finds nobody holding it, and starts writing. Silent corruption, no error anywhere.
The check
Database::open(),Database::create()andReadOnlyDatabase::open()now refuse a path whose file name isdata.redbwhen a valid multi-processmetadatamarker sits beside it, with a message pointing at the directory:The cost is one
statof a name that is almost never there, and only for a file calleddata.redb— a path that is not inside one of these directories cannot match. Anything that is not a complete marker (absent, wrong length, a symlink, someone else's file under that name) answers "not one of ours" and the open proceeds as before.The refusal stands whether or not any process currently has the directory open. A
Databasethat writes to the file while nothing else holds it leaves a database the next multi-process opener never made — commits nothing announced through the directory's protocol, an extended header binding no longer matching the commit slots — so "nobody is holding it right now" is not a safe moment to allow it.What I am unsure about
This is a behaviour change to three stable, non-feature-gated methods, so it is the one to push back on. The alternatives I considered:
experimental-multiprocess. Backwards, I think: the process at risk is the one that doesn't have the feature on and doesn't know these directories exist.data.redbkeeps the check from touching anything else, at the cost of not catching a directory a user renamed the file within — which is not a state redb can produce.Testing
One integration test (48 in the suite): all three entry points refused on a live directory, and
Database::openstill refused once the handle is dropped.cargo fmt --check,cargo clippy --all --all-targetsand the full test suite pass with--all-featuresand with default features.🤖 Generated with Claude Code
https://claude.ai/code/session_01SJFSfcturbVcnPqY5CNzQv