Route the last two atomic writes through fs_util - #804
Conversation
0.1.4 of the shared blocks extends the atomic-write rule twice: the temporary file is created with the permissions the finished file needs, and atomic replacement is not durability -- a file the program reads back to resume needs sync_all before the rename. fs_util::atomic_write already did both, and cert_group's stage_key_file does them by hand. fast_poll's state and trust's rotation-state.json did neither: fs::write left the temp at whatever the umask gave, rename carried that inode to the destination, and nothing was ever flushed. Both files are exactly the case the rule names -- the next tick resumes from one, the rotation command resumes from the other. Rather than repeat the logic a third time, atomic_write splits: the body was already a closure inside spawn_blocking, so it becomes atomic_write_blocking and the async form is a wrapper. trust.rs is sync all the way to its callers in rotate/ca.rs, so it takes the blocking half; fast_poll takes the async one. All three paths are now the same code. 0o600 on both. Neither file is read by anything but the process that writes it, and the crate already writes the agent config that way. That is a decision rather than a discovery -- previously they were whatever the umask happened to be, which nobody chose. create_rotation_state gets the same mode and a sync_all too. It was create_new with no mode, so leaving it alone would have left the file's permissions depending on whether it was created or updated last. Not included: syncing the containing directory after the rename, which no path in this crate does. That is a decision per file about what must survive a power loss, and it belongs with cert_group and fs_util rather than bolted onto two callers. Closes #803
Splitting atomic_write left the copies where they were: the async wrapper owns path and contents so it can move them into spawn_blocking, and the extracted function then copied both a second time. Before the split there was one copy; after it there were two. The copies in the blocking half only ever existed to satisfy the move. Nothing in the body needs ownership -- metadata, write_all and persist all take borrows -- so it takes the arguments as they come. The wrapper keeps its single copy, which the move still requires. The scope the closure needed goes with them.
|
Fixed in the latest commit — the review is right, and it was a regression the split introduced.
Taken as a plain borrow rather than an internal owned helper: the helper would have to exist only to hold values the blocking path never needs owned, and would put a third function between the two.
|
Closes #803.
0.1.4of the shared blocks extends the atomic-write rule twice: the temporary file is created with the permissions the finished file needs, and atomic replacement is separated from durability — a file the program reads back to resume needssync_allbefore the rename.fs_util::atomic_writealready did both, andcert_group'sstage_key_filedoes them by hand.fast_poll's state andtrust'srotation-state.jsondid neither:fs::writeleft the temporary at whatever the umask gave,renamecarried that inode to the destination, and nothing was flushed. Both are exactly the case the rule names — the next tick resumes from one, the rotation command from the other.One implementation, not three
atomic_write's body was already a closure insidespawn_blocking, so it splits cleanly: the closure becomesatomic_write_blockingand the async form is a wrapper around it.trust.rsis synchronous all the way out to its callers inrotate/ca.rs, so it takes the blocking half.fast_polltakes the async one. All three atomic-write paths in the crate are now the same code rather than three versions of it.The mode is a decision
0o600on both. Neither file is read by anything but the process that writes it, and the crate already writes the agent config that way throughatomic_write. Previously they were whatever the umask gave, which nobody chose — the point of the rule is that this is now stated at the site.create_rotation_stategets the same mode and async_allas well. It wascreate_newwith no mode, so leaving it would have made the file's permissions depend on whether it was last created or updated — the inconsistency the rule exists to prevent, in one file.Not in this pull request
Syncing the containing directory after the rename. No path in this crate does it, including
cert_groupandfs_util, which are otherwise correct. The rule calls it a decision per file rather than a default, since it costs a disk round trip, so it needs an answer about what each file is protecting — worth taking deliberately rather than bolting onto two callers here. Worth its own issue.Verified locally
cargo clippy --lib --bins --tests --all-features -- -D warningscleancargo test --lib— 398 passed, 0 failedcargo fmtappliedfs::renameoutsidefs_utiliscert_group.rs:378, which stages with an explicit mode andsync_allalready