Skip to content

Route the last two atomic writes through fs_util - #804

Merged
sehkone merged 2 commits into
mainfrom
sehkone/issue-803
Aug 9, 2026
Merged

Route the last two atomic writes through fs_util#804
sehkone merged 2 commits into
mainfrom
sehkone/issue-803

Conversation

@sehkone

@sehkone sehkone commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Closes #803.

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 separated from 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 temporary at whatever the umask gave, rename carried 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 inside spawn_blocking, so it splits cleanly: the closure becomes atomic_write_blocking and the async form is a wrapper around it.

trust.rs is synchronous all the way out to its callers in rotate/ca.rs, so it takes the blocking half. fast_poll takes 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

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 through atomic_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_state gets the same mode and a sync_all as well. It was create_new with 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_group and fs_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 warnings clean
  • cargo test --lib — 398 passed, 0 failed
  • cargo fmt applied
  • The only remaining raw fs::rename outside fs_util is cert_group.rs:378, which stages with an explicit mode and sync_all already

sehkone added 2 commits August 9, 2026 20:15
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.
@sehkone

sehkone commented Aug 9, 2026

Copy link
Copy Markdown
Contributor Author

Fixed in the latest commit — the review is right, and it was a regression the split introduced.

atomic_write_blocking copied path and contents a second time. Those copies only ever existed to satisfy the move into spawn_blocking; nothing in the body needs ownership, since metadata, write_all, and persist all take borrows. It now takes the arguments as they come, and the vestigial scope the closure needed goes with them. The async wrapper keeps its single copy, which the move still requires — so both paths are back to one copy, as before the split.

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.

cargo clippy --lib --bins --tests --all-features -- -D warnings clean, cargo test --lib 398 passed.

@sehkone
sehkone merged commit 960dc73 into main Aug 9, 2026
18 checks passed
@sehkone
sehkone deleted the sehkone/issue-803 branch August 9, 2026 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Two atomic writes set no permissions and never flush

1 participant