Skip to content

Two atomic writes set no permissions and never flush #803

Description

@sehkone

AGENTS.md moved to 0.1.4, which extends the atomic-write rule in two directions: 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.

This crate already does both correctly in one place and neither in two others. The work is making the three agree, not adopting something new.

Already correct

stage_key_file in src/cert_group.rs:397 creates the temporary file with an explicit mode and flushes it before the rename:

opts.create_new(true).write(true).mode(KEY_FILE_MODE_DEFAULT);
…
f.sync_all()

src/fs_util.rs takes a mode parameter on its write helpers and calls sync_all at three sites.

Not correct

src/fast_poll.rs:212 and src/commands/trust.rs:84 both do:

fs::write(&tmp_path, body).await?;   // no mode — umask decides
fs::rename(&tmp_path, path).await?;  // no sync_all

Two problems each:

  • Permissions. fs::write creates with 0o666 & ~umask. rename puts that inode at the destination, so the file ends up with whatever the temporary happened to get — not the mode of the file it replaced, and not a mode anyone chose. Decide what each destination needs and create the temporary with it.
  • Durability. Both files are state the program reads back to resume. load_rotation_state sits directly below the trust.rs write; the fast-poll state is read on the next tick. Atomic replacement settles which of two versions a reader sees and says nothing about either surviving a power loss, which is exactly the case the rule now names. sync_all the temporary before the rename.

Not in this issue

Syncing the containing directory. The rule's second half — open the parent directory and sync_all it after the rename — is unimplemented everywhere in the crate, including the two sites that are otherwise correct. Fixing it here would mean deciding for cert_group.rs and fs_util.rs too, and the rule itself calls it a decision per file rather than a default, since it costs a disk round trip.

That decision is worth taking deliberately, on its own, with an answer for each file about what it is protecting. Filing it separately keeps this issue to the part with no judgement in it.

Done when

  • The temporary files in fast_poll.rs and trust.rs are created with the mode their destinations need, stated at the creation site
  • Both call sync_all on the temporary before the rename
  • The three atomic-write paths in this crate agree on both points
  • cargo clippy --lib --tests --all-features -- -D warnings clean, full CI green

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions