fix: use tempfile for atomic writes - #423
paolodamico wants to merge 2 commits into
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ae26909. Configure here.
There was a problem hiding this comment.
Pull request overview
Reworks atomic wallet-data writes using NamedTempFile and directory synchronization.
Changes:
- Replaces manual staging files with
tempfile. - Syncs staged data and destination directories.
- Adds secure-permission and cleanup coverage.
Assessment: No P0 issues found. A P1 Apple-platform failure and P2 durability gap remain unresolved.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
cspell.json |
Allows “fsync.” |
Cargo.lock |
Locks tempfile and transitive updates. |
bedrock/Cargo.toml |
Adds tempfile. |
bedrock/src/primitives/filesystem.rs |
Implements tempfile-based atomic writes and tests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![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: 1eaf36c98a
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// Apple targets that issues `F_FULLFSYNC`, which a directory descriptor does not accept. | ||
| fn sync_directory(directory: &Path) { | ||
| let flushed = File::open(directory) | ||
| .and_then(|handle| rustix::fs::fsync(&handle).map_err(io::Error::from)); |
There was a problem hiding this comment.
rustix here is for the cheaper sync rather than compatibility right? F_FULLFSYNC on a directory fd works for me on darwin 24.6/apfs.
| fs::set_permissions(&staged, existing.permissions()) | ||
| .map_err(|error| io_failure("carry over file permissions", &error))?; | ||
| } | ||
| if let Ok(existing) = fs::metadata(destination) { |
| if result.is_err() { | ||
| drop(fs::remove_file(&staged)); // Best effort | ||
| } | ||
| sync_directory(parent); |
There was a problem hiding this comment.
the dir fsync repeats for every file restore writes into the same directory. worth doing it once per directory at the end instead?
Uses
tempfilefor the atomic file writes to disk, and adds one more directory sync to reduce surface area of stale content being served from diskNote
Medium Risk
Touches wallet data-directory persistence and durability semantics; behavior should be equivalent but directory fsync and tempfile naming differ from the previous implementation.
Overview
Atomic disk writes in
write_atomicallynow stage withtempfile::NamedTempFileinstead of hand-rolled{pid}-{counter}.tmpnames, so failed or panicking writes clean up via drop and commits usepersistinstead of a rawfs::rename.After a successful commit, the PR best-effort
fsyncs the destination parent directory throughrustix(warn-only on failure) so the rename is less likely to leave readers seeing stale directory metadata. Staged file data is still flushed withsync_allbefore rename; permission carry-over on overwrite is unchanged in behavior.Adds
tempfileandrustix(fs) tobedrock, bumps lockfile transitive deps, and adjusts tests (orphan staged filename shape, permissions assertion0o640).Reviewed by Cursor Bugbot for commit 1eaf36c. Bugbot is set up for automated code reviews on this repo. Configure here.