Make the in-process staged writes durable across a crash (#58)

[AcoPiper]

Atomic replacement is not durability. The three staged writes this crate performs in process each rename their temporary into place with nothing flushed, so a crash or a power loss can undo a write that already reported success — and every one of them publishes state the program reads back to resume from: a trust generation at every start, an artifact at every boot, whatever the root daemon lands. All of them run during an install or an upgrade, which is when a machine is most likely to be interrupted.

Each of the four renames gets the flush its own situation calls for, rather than four copies of one edit:

• put_file_natively syncs the temporary after fchown and set_permissions, since sync_all covers that metadata and a flush placed where the old no-op sat would leave it unflushed, then syncs dest.parent() after the rename — bound once immediately after staging_dir succeeds and expected there, because the staging directory is deliberately elsewhere and a leftover temporary is inert. The pre-existing file.flush() is deleted rather than kept beside it: std::fs::File has no userspace buffer, so it read exactly like the flush that was missing while moving no data.
• activate_generation syncs each material file in write_file_0600, then gen-<n>.tmp as a directory before the rename that finalises it, then the tree root twice — once after the generation rename and again after the active swap. One flush at the end is not equivalent: it would let the filesystem commit active ahead of the generation it names. The symlink swap adds no file flush, since a symlink cannot be fsynced at all.
• Payload::extract_to syncs each artifact after its hash check passes, while the descriptor is still open, then every directory between an artifact and dest — dest included, nothing above it, each exactly once.

Every directory flush goes through one pub(crate) helper, durability::sync_dir, returning std::io::Result<()> and attaching no context: the executor and generation call sites name the flushed path through their path-carrying Io variants, and the payload converts bare through its existing From, exactly as the rename above it does. No new dependency, no public signature change, no new or changed error variant, and no platform cfg. CHANGELOG.md and PUT_FILE_SCRIPT are untouched.

Two things the flushes drag along with them. activate_generation's doc contract names the staging flush in its step list and places a flush failure in the fail-closed case it belongs to, so a caller reading that contract has somewhere to put the one failure the sequence gained. And the test behind the "a failing flush names the path" criterion injects its fault through the validator — the one hook the sequence hands the staging directory to, running after the material is written and before the flush that finalises it — rather than re-spelling sync_dir plus GenerationError::io in the test itself, which would assert a copy of the production line instead of the line and stay green if the flush were deleted.

Closes #58

Test plan

• extract_to's directory set is computed by a named function (publish_dirs) over paths rather than inline in the publish loop
• Unit tests cover an artifact directly in dest, an artifact nested several levels deep, two artifacts sharing a parent yielding that parent once, and the walk stopping at dest
• sync_dir returns an error rather than succeeding silently when pointed at a path that does not exist
• The flushed path reaches the caller's error, asserted through the real activate_generation call site — deleting the production flush turns the test red
• Every test that existed before still passes with its assertions and expected values unchanged; the new tests join them rather than replacing any
• cargo test and cargo test --features test-support pass (429 tests each, run on macOS so the directory flush is exercised on the developer platform)
• cargo fmt -- --check --config group_imports=StdExternalCrate passes
• cargo clippy --all-targets -- -D warnings and cargo clippy --all-targets --features test-support -- -D warnings pass
• Flush placement read by a reviewer: durability itself is not assertable from a test, so the ordering in the acceptance criteria — flush after the metadata calls, directory flush after the rename, root flushed twice in order — is checked by reading

Follow-up

The shell transport's staged write is now tracked by #61. It stages and renames the same way with nothing flushed (cat > "$tmp" then mv -f "$tmp" "$dest") and is the path secrets.json is written by, so the gap is real — but it is deliberately not changed here. A POSIX shell has no fsync, and no shell construct flushes the directory mv publishes the entry into at all, so it is a portability decision to settle over its own argument rather than in passing. PUT_FILE_SCRIPT is untouched by this PR.

[AcoPiper]

[Reviewer Round 1]

The in-process implementation looks correct to me. put_file_natively syncs after its ownership/mode changes and then flushes dest.parent() after the rename (src/executor.rs:1119-1148). The generation path orders the staging-directory flush, generation rename/root flush, symlink swap/root flush correctly (src/generation.rs:290-301), and Payload::extract_to syncs verified files before publication then deduplicates every required directory through dest (src/payload.rs:1648-1653, src/payload.rs:1704-1713). The new path-set and failure-path tests are meaningful rather than merely duplicating the helper.

One required follow-up is missing. The issue deliberately excludes PUT_FILE_SCRIPT only with the instruction to “open the follow-up instead.” This PR's ## Follow-up not opened here section says that has not happened, so the known durability gap in the Local/Ssh transport has no tracked work item. Please create and link that follow-up issue before this is considered complete; it should cover the shell path's portability decision without expanding this PR’s code scope.

[AcoPiper]

[Review Verdict Round 1: NOT_APPROVED]

[AcoPiper]

[Author Round 1]

One item, accepted in full.

1. The follow-up issue for PUT_FILE_SCRIPT was never opened — Fixed

You are right that this was the one thing outstanding. #58 excludes the shell transport with an explicit instruction — "Do not modify PUT_FILE_SCRIPT in this work; open the follow-up instead" — and only the first half of that had been carried out, which left the sharper of the two durability gaps untracked while the PR that names it moved toward merge.

Opened as #61, Decide how the shell transport's staged write becomes durable.

It is scoped as a decision rather than as a fifth call site, which is the reason #58 held it out:

• It records why a shell cannot simply do this — sync "$tmp" is a GNU coreutils extension absent on busybox and behaved differently on the BSDs, bare sync flushes every filesystem on the host, dd conv=fsync is closer but not universally spelled the same way, and sh here is whatever the target has, over LocalExecutor and over an ssh channel alike.
• It names the part that is worse than the file flush: no POSIX shell construct flushes the directory mv publishes the entry into, so even a working file flush leaves uncovered the half this PR was most careful about.
• It lays out the four approaches worth arguing (probe for a working construct; take LocalExecutor off the script and onto put_file_natively, noting that the script exists because the write runs as one elevated sh -c, so that moves the privilege story rather than only adding a flush; invoke a helper that has an fsync; or accept the gap and say so) without pre-picking one.
• It carries the secrets.json stake from Make the in-process staged writes durable across a crash #58 verbatim — that path is what bootler writes over, and the runtime_service_add AppRole "lives only in this 0600 file".
• Its acceptance criteria require the outcome to land in PUT_FILE_SCRIPT's doc comment, not only in a pull request, and require portability to be stated rather than assumed: which shells and utilities the chosen construct works under, and what the script does where it is absent. Accepting the gap is a legitimate outcome there; leaving it undocumented is not.
• It is bounded to exactly what you asked for — "without expanding this PR's code scope". MAKE_DIR_SCRIPT, the in-process sites, and lint machinery are all listed out of scope, and it declares itself as following Make the in-process staged writes durable across a crash #58/Make the in-process staged writes durable across a crash (#58) #60.

Changes in this PR

No code changed, so the tree is byte-identical to what you reviewed and the green CI run above still stands for it. The PR description's ## Follow-up not opened here section — which was the accurate record of the omission you caught — is now ## Follow-up, linking #61 and stating why PUT_FILE_SCRIPT is untouched here. CHANGELOG.md remains untouched, per #58's Out of scope.

[AcoPiper]

[Reviewer Round 2]

Round 1 is resolved. The required follow-up is now open as #61 and directly covers the intentionally excluded PUT_FILE_SCRIPT durability decision without expanding this PR’s code scope.

I rechecked the updated diff against #58. The implementation still has the required ordering in all three in-process paths: the native executor syncs the file after ownership and mode changes, then the destination directory after its rename; generation activation syncs the staged files and staging directory before finalisation, then the root after each of the generation and active renames; and payload extraction syncs verified files before publication, then each required publish directory through dest exactly once. Flush errors continue to fail closed with the requested path handling, and the tests exercise the directory-set calculation plus a real generation call-site failure.

No findings remain. Approved.

[AcoPiper]

[Review Verdict Round 2: APPROVED]

[AcoPiper]

Suggested squash commit

Title

Flush the in-process staged writes before they are read back

Body

Atomic replacement is not durability. The three staged writes this crate
performs in process each renamed their temporary into place with nothing
flushed, so a crash or a power loss could undo a write that had already
reported success — and every one of them publishes state the program
reads back to resume from: a trust generation at every start, an artifact
at every boot, whatever the root daemon lands. All of them run during an
install or an upgrade, which is when a machine is most likely to be
interrupted.

Each of the four renames gets the flush its own situation calls for.

`put_file_natively` syncs the temporary after `fchown` and
`set_permissions`, since `sync_all` covers that metadata and a flush
placed where the old no-op sat would leave it unflushed, then syncs
`dest.parent()` after the rename — bound once immediately after
`staging_dir` succeeds and `expect`ed there, because the staging
directory is deliberately elsewhere and a leftover temporary is inert.
The pre-existing `file.flush()` is deleted rather than kept beside it:
`std::fs::File` has no userspace buffer, so it read exactly like the
flush that was missing while moving no data.

`activate_generation` syncs each material file in `write_file_0600`,
then `gen-<n>.tmp` as a directory before the rename that finalises it,
then the tree root twice — once after the generation rename and again
after the `active` swap. One flush at the end is not equivalent: it
would let the filesystem commit `active` ahead of the generation it
names. The symlink swap adds no file flush, since a symlink cannot be
`fsync`ed at all. Its doc contract names the staging flush in the step
list and places a flush failure in the fail-closed case it belongs to,
so the one failure the sequence gained has somewhere to be read.

`Payload::extract_to` syncs each artifact after its hash check passes,
while the descriptor is still open, then every directory between an
artifact and `dest` — `dest` included, nothing above it, each exactly
once. That set is computed by `publish_dirs` over paths rather than
inline in the publish loop, because the arithmetic is the part worth
testing and none of it is observable from the flushes themselves.

Every directory flush goes through one `pub(crate)` helper,
`durability::sync_dir`, returning `std::io::Result<()>` and attaching no
context: the executor and generation call sites name the flushed path
through their path-carrying `Io` variants, and the payload converts bare
through its existing `From`, exactly as the `rename` above it does. No
new dependency, no public signature change, no new or changed error
variant, and no platform `cfg`.

Durability itself is not assertable — nothing observable distinguishes a
flushed write from an unflushed one short of cutting power — so the
flush placement is left to a reviewer and the tests cover what is
testable: the directory set, and that a failing flush is an error naming
the path it flushed. That second test injects its fault through the
validator, the one hook the sequence hands the staging directory to,
rather than re-spelling `sync_dir` plus `GenerationError::io` itself,
which would assert a copy of the production line instead of the line.

The shell transport stages and renames the same way with nothing flushed
and is left alone: a POSIX shell has no `fsync`, and no shell construct
flushes the directory `mv` publishes the entry into, so that is a
portability decision to settle over its own argument. `PUT_FILE_SCRIPT`
is untouched here and tracked by #61.

Closes #58