fix: atomic file writes in ghost-consensus and ghost-mpc - #863
Merged
Merged
Conversation
Five writers staged their atomic write through a fixed `.tmp` sibling of the target. That path is shared by every concurrent writer of that file: two racing writers truncate each other's staging file, and once the winner has renamed it away the loser's `rename` fails with ENOENT. The same defect in the wallet's stores took down request handlers under concurrency. Two of them were also missing durability rather than only racing. `mesh.rs::write_sequence_ceiling` staged with a bare `fs::write` and fsynced neither the file nor its directory, so a power loss could take back a ceiling already reported as persisted — and that ceiling exists precisely so a restart cannot reuse a sequence number. All three `ghost-mpc` writers fsynced the staging file and then renamed without ever fsyncing the directory. A rename is a metadata change: without that sync the file's own fsync survives a crash and the rename does not, losing a write that reported success. `ghost_common::atomic_file` now holds the one implementation. All three crates already depend on `ghost-common`, so nothing new enters the graph. It exposes `write_atomic` for callers with a finished body, and `staging_path` + `sync_parent_dir` for callers that stream into the staging file — the MPC ones write proving keys that must not be buffered in memory just to reuse a whole-body helper. `ghost-mpc`'s `realcrypto_lifecycle` drives the real save/load/copy paths and passes (it needs `--features mpc-test-cap`; without it the harness refuses to run, which is not a failure of this change). Follow-up: `feat/locks-wraith-wallet` carries the same helper as `ghost_lock::atomic_file` for the wallet crates, which cannot reach `ghost-common` without a new dependency edge. Whichever lands second should collapse the two into one. Claude-Session: https://claude.ai/code/session_01NM8n9eAt68PPGbEvyrD78z
defenwycke
added a commit
that referenced
this pull request
Sep 8, 2026
* chore: one atomic-file implementation, not two #853 and #863 each carried a copy of the same durable-write helper, because the wallet crates and the consensus/MPC crates could not reach each other's homes for it. Both have now landed, so the duplication is real rather than prospective, and it is exactly the shape of defect that produced them: the fixed `.tmp` staging path was duplicated across seven stores and every copy carried the same race. `ghost_common::atomic_file` is the implementation. `ghost_lock::atomic_file` becomes a re-export of it, which is the route the wallet crates take — they all depend on `ghost-lock` already and none depended on `ghost-common`, so this collapses the code without threading a new dependency through four crates. `ghost-lock` takes `ghost-common` with `default-features = false`, dropping its UNIX-only `zmq`/`tmq` feature, which the wallet cannot build for Windows. `grep -rl "pub fn write_atomic"` now returns exactly one file. Workspace builds, clippy clean under the CI lint set, machete clean, 1,242 tests pass under --no-fail-fast, regtest smoke 13/13. * chore: refresh the fuzz lockfile for the new dependency edge CI's Format job builds the fuzz crate with `--locked`, and adding `ghost-common` to `ghost-lock` changed the dependency graph, so `fuzz/Cargo.lock` no longer matched: "cannot update the lock file because --locked was passed". One line, `ghost-common` under ghost-lock's dependencies. Worth noting where the blind spot was: workspace-level fmt, clippy and machete all passed, because none of them look at the fuzz crate's lockfile. The seven shell checks in that job are the ones that would have caught it, and I had not run them. All seven pass now.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Five writers staged their atomic write through a fixed
.tmpsibling of the target. That path is shared by every concurrent writer of that file: two racing writers truncate each other's staging file, and once the winner has renamed it away the loser'srenamefails withENOENT. The same defect in the wallet's stores took down request handlers under concurrency, which is what prompted this sweep.Two of them were also missing durability rather than only racing.
mesh.rs::write_sequence_ceilingstaged with a barefs::writeand fsynced neither the file nor its directory, so a power loss could take back a ceiling already reported as persisted — and that ceiling exists precisely so a restart cannot reuse a sequence number.All three
ghost-mpcwriters fsynced the staging file and then renamed without ever fsyncing the directory. A rename is a metadata change: without that sync the file's own fsync survives a crash and the rename does not, losing a write that reported success.Shape of the fix
ghost_common::atomic_filenow holds the one implementation. All three crates already depend onghost-common, so nothing new enters the dependency graph. It exposes:write_atomic— for callers with a finished body.staging_path+sync_parent_dir— for callers that stream into the staging file. The MPC writers persist proving keys that must not be buffered into memory just to reuse a whole-body helper.Verification
ghost-commonunit tests cover the race directly: 16 concurrent writers to one target, asserting no writer fails, the result is a whole body rather than a mixture, and no staging files are left behind.ghost-mpc'srealcrypto_lifecycledrives the real save/load/copy paths and passes. It needs--features mpc-test-cap; without that flag the harness refuses to run, which is a guard rather than a failure of this change.ghost-consensus: 380 tests pass.Follow-up
feat/locks-wraith-wallet(#853) carries the same helper asghost_lock::atomic_file, because the wallet crates cannot reachghost-commonwithout a new dependency edge. Whichever of the two lands second should collapse them into one.https://claude.ai/code/session_01NM8n9eAt68PPGbEvyrD78z