Flush the shell transport's staged write before it is read back (#61)

[AcoPiper]

PUT_FILE_SCRIPT — the sh -c script both LocalExecutor::put_file and SshExecutor::put_file run — staged and renamed with nothing flushed anywhere, so a crash shortly after put_file reported success could leave dest absent, empty, or holding the previous contents. That is the path bootler writes secrets.json over, the file read_secrets reads back on a re-run. This closes the gap #58 deliberately left, making every transport's landing durable as well as atomic.

Changes

A flush() shell function, sync "$1" 2>/dev/null || sync || echo "warning: $1 was not flushed: no working sync" >&2, called twice: once after chown/chmod and before the mv, so what it protects is the temporary's bytes together with the owner and mode just applied to them; once after the mv, for the directory entry the rename created, which does not exist at the first point.

Selection is by what the construct does at run time, never by a name probe — sync is present in all three implementations a target can carry, so command -v cannot tell the one that honours the operand from the one that ignores it. Coreutils flushes the named object (a directory operand included); macOS and a busybox built without FEATURE_SYNC_FANCY accept the operand, ignore it and flush the host, having already done what the fallback would; an implementation that refuses the operand exits non-zero and the bare sync runs. A host with no working sync at all still lands the file and says on stderr that it was not flushed. dd appears nowhere, in either spelling.

PUT_FILE_SCRIPT's doc states what is flushed, which half each construct covers, which shells and utilities each was chosen against, and the cost the floor carries — a bare sync flushes every filesystem on the host, which is what a target without coreutils gets, and Linux's sync waits for the writeback where POSIX would let it return early. Executor::put_file's doc now states the durability promise for every transport, names the one thing it rests on that this crate does not supply, and says plainly that the skipped-flush warning reaches nobody: this script's stderr surfaces only through ExecutorError::Transfer, so a put_file returning Ok on such a host reports nothing.

set -e behaviour, the EXIT trap, and the post-mv inode check are unchanged in effect; the || list's left operands are exempt from set -e in dash, bash and busybox ash alike. No new dependency, no public signature change, no new error variant, and CHANGELOG.md is untouched — nothing here is observable to a user of a release this crate has not cut.

Closes #61

Test plan

• The script's shape is asserted: both flushes exist, the temporary's sits between chmod and mv, the directory's after mv
• The fallback is asserted to be the || on the targeted form, with no command -v probe and no dd anywhere in the script
• All three run-time behaviours of sync are exercised with a stub earlier on the child's PATH — honours the operand, refuses it, accepts and ignores it — and the write succeeds in every case, with the refusing stub asserted to reach the bare sync for both halves rather than to fail
• A fourth stub failing with or without an operand covers the host with no working sync: the write still succeeds and the skipped flush is asserted on stderr
• Each stub is asserted on what it was actually called with — the first flush names the temporary, a later one names the destination's directory
• The existing LocalExecutor landing tests pass unchanged, ownership, mode and inode-check assertions included
• LocalExecutor and SshExecutor still emit an identical script, through the single landing_argv construction site
• cargo fmt -- --check --config group_imports=StdExternalCrate
• cargo clippy --all-targets -- -D warnings and cargo clippy --all-targets --features test-support -- -D warnings
• cargo test and cargo test --features test-support, on macOS as well as Linux, where the host sync is itself the operand-ignoring implementation

[AcoPiper]

[Reviewer Round 1]

Approved — I found no blocking issues.

The script now flushes the temporary only after its ownership and mode have been applied, and flushes the destination directory after the rename (src/executor.rs:233, src/executor.rs:236). Its sync "$1" || sync || warning behavior matches the issue’s required runtime selection: a targeted coreutils flush stays targeted, operand rejection reaches the portable host-wide floor, and a target with no usable sync still lands the file with the prescribed stderr warning.

The new tests exercise—not merely inspect—all four relevant target behaviors, including the rejecting implementation’s bare fallback and the no-working-sync warning (src/executor.rs:4056). They also assert that the first flush names the staged temporary and that the directory call follows it (src/executor.rs:4137). The trait documentation correctly makes the durability contract transport-uniform and accurately preserves the caveat that a successful shell write cannot surface the skipped-flush warning to its caller (src/executor.rs:794).

The PR links Closes #61 and includes a complete test-plan checklist.

[AcoPiper]

[Review Verdict Round 1: APPROVED]

[AcoPiper]

Suggested squash commit

Title

Flush the shell transport's staged write

Body

`PUT_FILE_SCRIPT`, the `sh -c` script both shell transports run, staged
and renamed with nothing flushed anywhere, so a crash shortly after
`put_file` reported success could leave the destination absent, empty,
or holding the previous contents. That is the path `secrets.json` is
written over, and the file a re-run reads its AppRole back from. The
in-process paths were made durable earlier and this one was left out;
a caller holding an `Executor` could not tell which guarantee it had.

A `flush()` helper runs `sync "$1" 2>/dev/null || sync`, falling back to
a stderr warning when neither works. It is called twice, because the two
halves protect different things: after `chown` and `chmod` and before
the `mv`, for the temporary's bytes together with the owner and mode
just applied to them; and after the `mv`, for the directory entry the
rename created, which does not exist at the first point.

Which `sync` runs is settled by what it does rather than by probing for
the name, since the name is present in all three implementations a
target can carry. Coreutils flushes the named object, a directory
operand included; macOS and a busybox built without `FEATURE_SYNC_FANCY`
accept the operand, ignore it and flush the host, having already done
what the fallback would; one that refuses the operand exits non-zero and
the bare `sync` runs. A host with no working `sync` lands the file
anyway and says so, because an artifact the caller asked for is worth
more than a guarantee the host never had.

The flush is unconditional. Narrowing it to the writes that are read
back would take a parameter through `Executor::put_file` and every
caller; what bounds the cost instead is the ordering, so the host-wide
flush is what a target lacking coreutils falls to rather than what every
install does by default.

`Executor::put_file`'s doc now states the promise for every transport,
names the target's `sync` as the one thing it rests on that this crate
does not supply, and says that the skipped-flush warning reaches nobody:
the script's stderr surfaces only through `ExecutorError::Transfer`.

Closes #61