Make backup_previous_artifact link-based (#87)

[sehkone]

apply::backup_previous_artifact was a cp -fp. An interrupted copy leaves a truncated .previous that a later revert succeeds onto — a broken artifact installed by a rollback that reported success. It is now link-based, so an interrupted backup leaves no .previous at all and the revert fails where anyone looking can see it.

The Executor gains hard_link_over, the primitive that expresses the sequence: refuse a non-regular source, link(2) to a temporary sibling of the destination, rename(2) that over it, then flush the containing directory. Not ln -f, because link(2) fails with EEXIST and ln -f unlinks first — a window with no backup at all; not a bare link, because a directory entry is not durable until its directory is flushed. InDaemonExecutor runs it as direct syscalls, being root already; the shell transports run it as a single elevated sh -c script, so nothing can interleave between the link and the rename by a second elevation. Where the destination already names the artifact's inode — a second backup of an unchanged artifact — rename(2) is defined to succeed and do nothing, and the native path lets that stand; mv does not pass the case through, GNU's refusing it with are the same file, so the script compares the inode it already has against the destination and skips the rename rather than running it. Both transports report every on-host failure — of the link, the rename, the flush and the confirmation — as ExecutorError::Transfer, which backup_previous_artifact folds into the same subject-labelled CoreError::Command the cp raised — so neither consumer's call site nor its rendering changes.

The flush is the step whose mechanism differs most between the transports, and both are held to the same contract. Natively it is fsync on an open descriptor. On the shell transports it is the sync utility, which some hosts refuse an operand for; the script selects between the two forms exactly as PUT_FILE_SCRIPT does, falling back to the host-wide sync, because which form a target carries cannot be told from the name. Where the two scripts end differs, and deliberately: a host on which both forms fail carries no working sync at all, and this one fails the backup there rather than warning and letting it stand. PUT_FILE_SCRIPT can let that case stand because the caller still holds the bytes and can write them again; nothing holds the artifact's pre-apply inode once the backup is the only other name for it, and the caller writes its backup-taken record from this call's success — a record standing over a flush that never ran would suppress the retake the un-flushed entry is precisely what needs. What the issue declines to claim portably is a fault between the rename and a flush that ran, and that is untouched: after a power loss there, the caller's journal is what decides.

A symlink at the artifact is refused rather than followed, whether or not it resolves, and so is a directory or any other non-regular file. The presence probe that guards the backup looks for the directory entry rather than for what it resolves to, since a plain test -e reads a dangling symlink exactly as it reads an absent path and would skip it as a newly-added artifact. Past that probe, the refusal is made twice on the native path and twice in the script: the second check is against the inode the link actually captured, so a path swapped under the guard is caught rather than published. The backup's own name is the crate's rather than the caller's, so it is not guarded the same way — the publish is a rename, which displaces a symlink planted at .previous instead of writing through it, while a directory there is a failure both natively and in the script, which refuses it before the link because mv would otherwise take the temporary inside it and exit 0.

Closes #87

Test plan

• The backup shares the artifact's inode rather than copying its bytes, and carries its mode and mtime by that shared inode
• place_file's rename-over-the-destination swap does not write through the link and destroy the backup
• A second backup replaces the earlier one, and re-backing up an unchanged artifact leaves two names for one inode and no stray temporary
• An absent artifact is not backed up and no .previous is invented
• A symlink at the artifact is refused rather than followed, on both the native and the shell transports
• A symlink at the artifact that resolves to nothing is refused too, rather than passing the presence probe off as an absent path
• A directory and a fifo at the artifact are refused as the non-regular files they are
• A symlink at .previous is replaced rather than written through, and whatever it pointed at keeps its inode and bytes
• A directory at .previous is a failure, refused before the link on the shell transports
• Process interruption between the link and the rename, and between the rename and the flush, leaves the old .previous or the new one and never a partial or absent file — native and shell
• Interruption anywhere before a first .previous lands leaves none, with the artifact still live for the resumed apply to re-take — native and shell
• A host whose sync fails both with the operand and without it fails the backup rather than reporting one it could not make durable, asserted through the executor-facing path as ExecutorError::Transfer
• Every on-host failure arrives as ExecutorError::Transfer naming the destination, folded into the subject- and host-labelled CoreError::Command
• cargo fmt -- --check --config group_imports=StdExternalCrate
• cargo clippy --all-targets -- -D warnings and the same with --features test-support
• cargo test and cargo test --features test-support

[sehkone]

[Reviewer Round 1]

Request changes.

• [P1] Dangling artifact symlinks are silently skipped, not refused. backup_previous_artifact first uses test -e; that test is false for a dangling symlink, so the function returns Ok(()) before hard_link_over can enforce its symlink guard. This conflicts with the issue’s unconditional requirement to reject symlink artifacts and lets the later replacement proceed without a backup. Detect a link separately (for example, -e || -L) and add a dangling-symlink regression test. The existing test only covers a symlink whose target exists (apply.rs).

• [P1] Shell transports report a successful backup when directory synchronization fails. LINK_ASIDE_SCRIPT turns failure of both the targeted and fallback sync calls into a successful echo warning. Thus hard_link_over_through_shell returns Ok(()), allowing the caller to record a backup as taken even though its directory entry was not flushed. The native implementation returns ExecutorError::Transfer on that same failure (executor.rs). This violates both the required directory-fsync step and the documented uniform error contract. Let the flush failure terminate the script and test a nonzero-returning sync stub (the current interruption test only kills the process).

The PR body has the correct Closes #87 linkage and a test-plan checklist; I found no review-thread or PR-hygiene issue.

[sehkone]

[Review Verdict Round 1: NOT_APPROVED]

[sehkone]

[Author Round 1]

Pushed as 92873ad. Dispositions:

[P1] Dangling artifact symlinks are silently skipped — Fixed

The reviewer is right: path_present's test -e resolves the path, so a dangling symlink at dest read as absent and backup_previous_artifact returned Ok(()) before hard_link_over's symlink guard could run — the swap then replaced the operator's link with no backup taken and nothing said.

• src/apply.rs: added a private entry_present that asks test -h only where test -e said no, and the backup now gates on it. The refusal itself stays in hard_link_over (both transports already refuse a symlink for what it is, not what it resolves to), so the error text and the CoreError::Command folding are unchanged. The common cases — a regular file, or nothing at all — still cost the one probe they always did.
• Regression tests: a_dangling_symlink_at_the_artifact_is_refused_rather_than_skipped (native, apply.rs), and a dangling case added to the_shell_sequence_refuses_every_source_that_is_not_a_regular_file (shell, executor.rs).
• Doc comment, # Errors section, and the existing ### Added CHANGELOG entry amended (no new entry — hard_link_over has never shipped).

[P1] Shell flush failure reported as success — Partially addressed

I added the test you asked for and documented the divergence, but did not make the flush failure fatal.

Kept the behaviour. The only way to reach that echo is for sync "$dir" and bare sync to both fail — i.e. the host carries no working sync at all. flush() is not this script's invention: it is verbatim the helper PUT_FILE_SCRIPT uses, and the trade-off is already a recorded, tested decision in the_flush_survives_every_sync_a_target_can_carry ("an install that cannot land an artifact because the host's sync refuses an operand is worse than one whose flush was broader than it needed to be"). Making it fatal here would mean every apply fails outright on such a host — worse than the cp -fp it replaces, which synced nothing — and it would buy a power-loss guarantee the issue explicitly declines to make: "No portable durability claim is available between the rename and the directory fsync, so none is made" … "the journal decides." Changing the semantics for both scripts would also alter put_file behaviour, outside this issue.

The two paths are also not failing at the same thing: natively it is fsync on an open descriptor, where a failure is a real I/O error worth reporting; on the shell it is a missing utility, which is a capability gap. The uniform-error contract covers the link, the rename and the confirmation — all of which do report identically.

What I changed: a_host_with_no_working_sync_publishes_the_backup_and_says_so pins it with a sync stub that exits 127 both ways (backup published, warning on stderr, no strays) — distinct from the existing kill-at-sync test, which covers the flush never running. The LINK_ASIDE_SCRIPT and Executor::hard_link_over docs now state the asymmetry and why, and the PR body's "Both report every on-host failure the same way" was overclaiming and has been narrowed.

CI-equivalent checks all pass locally: fmt, clippy --all-targets and with --features test-support, cargo test and cargo test --features test-support (495 + 4 tests, 0 failures).

[sehkone]

[Reviewer Round 2]

Request changes.

• [P1, unresolved] Shell backups still succeed without the required directory flush. LINK_ASIDE_SCRIPT turns both failing sync calls into a successful warning, and hard_link_over_through_shell then returns Ok(()). The warning is discarded with the successful command output, so the caller neither sees it nor can avoid recording the backup as taken. After a power loss, that durable journal record can therefore suppress a retry while .previous was never durable—the condition the issue’s required directory fsync is meant to prevent. The new test explicitly locks in this noncompliant success path (executor.rs). Make failure of both flush attempts fail the script and surface as ExecutorError::Transfer/CoreError::Command; test it through the executor-facing path.

The prior dangling-symlink finding is resolved: entry_present now checks -h after a failed -e, and native plus shell tests cover the dangling case. PR linkage and test-plan hygiene are sound.

[sehkone]

[Review Verdict Round 2: NOT_APPROVED]

[sehkone]

[Author Round 2]

Pushed as 936eb7b. Round 2 had one item.

[P1, unresolved] Shell backups succeed without the required directory flush — Fixed

I accepted this and reversed my Round 1 position. The reviewer's chain is right and I under-weighted it: hard_link_over returning Ok is what the caller writes its durable backup-taken record from, and a record standing over a flush that never ran suppresses the retake that the un-flushed entry is precisely what needs. The issue names the directory fsync as step 3 of the sequence, and what it declines to claim portably is a fault between the rename and a flush that ran — not a flush that never ran at all. That distinction is what my earlier reasoning collapsed.

• src/executor.rs: LINK_ASIDE_SCRIPT's flush helper now return 1s when both sync "$dir" and bare sync fail, so set -e ends the script and hard_link_over_through_shell returns ExecutorError::Transfer, which backup_previous_artifact folds into the subject-labelled CoreError::Command — identical to the native fsync failure.
• Test: a_host_with_no_working_sync_publishes_the_backup_and_says_so is replaced by a_host_with_no_working_sync_fails_the_backup_rather_than_claiming_one, driven through the executor-facing path as asked. It needed a small test-only StubbedPath executor (run spawns with the stub directory prepended to the child's PATH, no sudo), so the assertion is on the ExecutorError a caller acts on rather than on the script's exit status. It also pins that the rename-then-failed-flush case leaves a link and never truncated bytes, and no strays. the_link_script_never_leaves_the_destination_absent gained a shape assertion so the warn-and-stand form cannot creep back.
• Docs: the LINK_ASIDE_SCRIPT flush bullet, the Executor::hard_link_over "flush is the exception" paragraph (now: both transports report it), and the PR body's flush paragraph and test-plan line.

Partial, with reasoning: I changed only LINK_ASIDE_SCRIPT, not PUT_FILE_SCRIPT. The two now diverge deliberately, and both the script doc and the commit body state why: put_file's caller still holds the bytes and can write them again, so warning and standing costs a retry; nothing holds the artifact's pre-apply inode once the backup is the only other name for it. Changing put_file's floor is a separate decision with its own recorded trade-off and its own test, and is outside this issue.

.previous docs in src/apply.rs needed no change — "nothing between the rename and the directory flush is claimed portably" is still exactly true. No CHANGELOG change: hard_link_over has never shipped, and the existing ### Added entry's description ("flushing the directory the new entry appeared in") is unaffected.

CI-equivalent checks pass locally: fmt --check, clippy --all-targets and with --features test-support, cargo test (495 + 3) and cargo test --features test-support (495 + 4), 0 failures.

[sehkone]

[Reviewer Round 3]

Approved.

The remaining Round 2 finding is resolved: the shell flush now fails the script when both sync forms fail, and that failure surfaces as ExecutorError::Transfer (src/executor.rs, src/executor.rs). The new executor-facing regression test covers that outcome (src/executor.rs).

The prior dangling-symlink finding remains correctly fixed via the entry-presence probe. The link → rename → directory-flush sequence now meets the issue across native and shell transports, with meaningful interruption and failure-path coverage. PR linkage and test-plan hygiene are also sound.

[sehkone]

[Review Verdict Round 3: APPROVED]

[sehkone]

Suggested squash commit

Title

Back up the prior artifact by link, not copy

Body

`apply::backup_previous_artifact` was a `cp -fp`, and an interrupted
copy leaves a truncated `.previous` that a later revert succeeds onto —
a broken artifact installed by a rollback that reported success. An
interrupted link leaves no `.previous` at all, so the revert fails where
anyone looking can see it, and sharing an inode preserves the mode and
timestamps by construction rather than by copying them.

The sequence is `link` to a temporary sibling, `rename` over the
destination, then flush the directory the entry appeared in. Not `ln
-f`: `link(2)` fails with `EEXIST`, so `ln -f` unlinks first and exposes
a window with no backup at all. Not a bare `link`: a directory entry is
not durable until its directory is flushed, and the backup is precisely
what must survive a power loss.

Expressing that needs a primitive the `Executor` lacked, so it gains
`hard_link_over`. `InDaemonExecutor` runs it as direct syscalls, being
root already; the shell transports run it as a single elevated `sh -c`
script, so nothing can interleave between the link and the rename by a
second elevation. Both report every on-host failure as
`ExecutorError::Transfer`, which the caller folds into the same
subject-labelled `CoreError::Command` the `cp` raised, leaving neither
consumer's call site nor its rendering changed.

A symlink at the artifact is refused rather than followed, whether or
not it resolves, and so is a directory or any other non-regular file:
following one would back up wherever the operator pointed it, and
linking one without following would capture the symlink itself. The
backup's own name is the crate's rather than the caller's, so a symlink
planted at `.previous` is displaced by the rename instead of written
through, while a directory there fails.

The flush is required rather than best effort. A host whose `sync`
works in neither form fails the backup, because the caller writes its
backup-taken record from this call's success, and a record standing over
a flush that never ran would suppress the retake the un-flushed entry
needs.

This is still not idempotent: a resumed apply that runs it twice backs
up the half-applied bytes. That is a property of when the backup runs,
and the caller's journal remains what guards it.

Closes #87