Add atomic_write utility and apply to all config/state file saves#92
Add atomic_write utility and apply to all config/state file saves#92
Conversation
Test defaults, serde round-trips, TOML deserialization with complete, partial, and empty inputs, and optional field handling across all three config modules. Adds 18 new tests.
Replace direct fs::write() with a tmp-file + rename pattern in save_user_config, save_persistent_state, remote save_config, and plugin set_config to prevent corruption from crashes or power loss. Add atomic_write utility to conch_core::config and a local copy in conch_remote. Include unit tests for the new utility.
![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: 35a4d58477
ℹ️ 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".
| let tmp = path.with_extension("tmp"); | ||
| fs::write(&tmp, data)?; | ||
| fs::rename(&tmp, path)?; |
There was a problem hiding this comment.
This helper now replaces config/state files via write(tmp) + rename, which creates a new inode with default umask-derived permissions instead of preserving the existing file mode. In paths that previously used fs::write (e.g., save_user_config, save_persistent_state, and plugin config saves), a file that was intentionally restricted (such as 0600 or read-only) can be silently widened or overwritten on the next save, which is a security/behavior regression on multi-user systems. Consider copying permissions from the existing target to the temp file (or setting them explicitly) before the rename.
Useful? React with 👍 / 👎.
|
Fixed in 6b929ee — both |
1 participant
What does this PR do?
Adds an
atomic_write(path, data)utility that writes to a.tmpfile then renames to the target — preventing corruption from crashes or power loss during writes. Applied to all config persistence:save_user_config()andsave_persistent_state()now useatomic_write()save_config()forservers.jsonnow uses a localatomic_writehelperset_config()in pluginTauriHostApinow usesconch_core::config::atomic_write()Previously only
vault.encused atomic writes. Now all persisted files are protected.Type of change
Testing
7 new tests verifying: correct content, no leftover .tmp, overwrite behavior, empty data.
Notes for reviewer
conch_remotehas its own localatomic_writesince it doesn't depend onconch_coretempfile = "3"added as dev-dependency to conch_core for test isolation