Recover interrupted link backups (#89) - #90
Conversation
An attempt interrupted between the link and the rename leaves a completed temporary sibling behind, and the shell script's trap is registered on the line *after* the link, so an ordinary TERM strands one as surely as a SIGKILL does. Both transports then drew a name that a resumed apply could draw again: the script used $$ alone, and the native path a pid with a counter that restarts at 0 in every process. Pids are reused, quickly inside a container, so the first candidate a resumed apply tries is exactly the name the interrupted one left. link(2) refuses an occupied name on purpose, which turned that collision into a host stuck until somebody removed the file by hand. The refusal stays; the choice of name changes. Both transports now carry an attempt number and walk it upwards until a name is free, stepping over an occupied candidate rather than adopting, unlinking or resolving through it, and reporting a directory in which 64 consecutive candidates are taken instead of walking forever. The link, rename and directory-flush sequence is otherwise untouched. Closes #89
The walk advances only on an occupied candidate, and nothing held it to that: an `ln` failing for any other reason could have been stepped over 64 times and reported as a directory with no free name, naming the wrong problem. A stubbed `ln` now pins the diagnostic and the single call. The shell script carries its bound as a literal, so the native constant and the script could drift apart in the direction the staged-collision test cannot see — raising the constant leaves the script failing earlier and the test still passing. It now asserts the script's own number. Recovery is also asserted where the criterion states it, through `backup_previous_artifact` rather than only through the executor: a leftover under this process's pid is what a resumed apply meets first, and the backup lands out of a free sibling with the leftover untouched. Part of #89
Both stale-candidate tests spell the temporary name a second time — the native one through a helper, the shell one inside the wrapper that plants under the script's own pid. Neither was held to the name the production walk actually draws, and a drift there would not fail them: a leftover at a name nothing collides with leaves the backup published, the leftovers standing and the stray count unchanged, so every assertion passes while the recovery under test never happens. The native leftovers are now made by step one itself, twice, which is what two attempts interrupted between the link and the publish leave, and the names it returns are asserted. The shell wrapper is held to the script's own candidate line.
The recovery this sequence gained is built on a leftover being left standing, and the window that strands one is the gap between the `ln` and the trap on the line after it. That reads as an invitation to hoist the trap above the walk, which would arm `rm -f "$tmp"` while `$tmp` still names a candidate the walk is about to refuse — clearing away the very leftover the sequence is required to leave alone. Record the trade-off where the next reader will meet it. The same paragraph named the walk's bound as a bare 64 while `LINK_TEMP_ATTEMPTS` holds it, so point at the constant instead. Part of #89
|
[Reviewer Round 1] Request changes.
The PR body otherwise has the required |
|
[Review Verdict Round 1: NOT_APPROVED] |
`ln` is the one step of this sequence that does not fail on a name that is taken. Handed a directory, or a symlink to one, it links the source *inside* it and exits 0, so the walk broke out on a link it had not made at the name it thought it had taken. The type guard below caught that, but only after the trap was armed: `rm -f "$tmp"` then deleted a planted symlink on the way out and left the stray link standing in the directory it pointed at — deleting the operator's entry and writing through it in one step, which is exactly what the walk exists to avoid. No portable `ln` suppresses the dereference — `-T` is GNU's, `-h` and `-n` are the BSDs' and cover only the symlink — so the walk now asks whether the candidate exists before linking, `-e` for an entry and `-h` for a dangling symlink `-e` does not see. What is left is the window between that test and the `ln`, and the type guard closes it: the trap moves below the guard, and the guard removes the link `ln` was diverted into making by the name it was given rather than the candidate it was handed. Nothing on either path touches the planted entry. The native side needed no change — `link(2)` does not resolve the new name, so a directory there is `EEXIST` like anything else — but the criterion covers both transports, so its staged collision now includes both spellings too. Part of #89
|
[Author Round 1] Round 1 disposition[P1] Shell transport follows a planted directory candidate — Fixed. The finding is correct, and I confirmed the failure empirically before and after the change. What changed in
Coverage added, as requested — both spellings:
Docs: the CI commands all green locally (fmt check, both clippy configurations, |
|
[Reviewer Round 2] Request changes.
The PR body has the required |
|
[Review Verdict Round 2: NOT_APPROVED] |
The shell walk tested each candidate and then ran `ln` on it, which left a window: a directory or a symlink to one appearing between the test and the link is followed, and the source lands inside it. The type guard caught that and removed the diverted link, but only by resolving through the very entry the sequence must not touch. `link(1)` closes the window rather than narrowing it. It hands `link(2)` the two names it was given and resolves nothing, so an occupied candidate is EEXIST there whatever sits at it -- the same refusal `link_aside` already gets from the same syscall. It is not POSIX, so a host without it keeps the tested `ln`, whose remaining window needs write access to the root-owned directory holding the artifact this sequence protects. Part of #89
|
[Author Round 2] Round 2 disposition[P1] Shell candidate check remains TOCTOU-vulnerable — Fixed. The finding is correct. The test-then- The fix is not a tighter test but a different utility. if command -v link >/dev/null 2>&1; then
claim() { link "$1" "$2"; }
else
claim() { …existence test…; ln "$1" "$2"; }
fi
Partially addressed: Coverage for the interleaving branch, which the review correctly noted was missing:
Docs: the CI green locally: fmt check, both clippy configurations, |
|
[Reviewer Round 3] Request changes.
The PR body’s |
|
[Review Verdict Round 3: NOT_APPROVED] |
The shell walk kept `ln` behind a test of the candidate for hosts carrying no `link`, and that test is a window: a directory, or a symlink to one, created between it and the `ln` is followed just as one that was already there would be, so the source is linked inside somebody else's directory and the guard that cleans up afterwards resolves through the same planted entry to do it. There is no portable non-following `ln`, so the fallback cannot be repaired, only removed. A host with no `link` now fails the backup before the walk begins, reported as a transfer failure naming the destination like every other on-host failure here. That is a diagnosable stop instead of a link made through a planted entry, and the set of hosts it stops is close to empty: coreutils, busybox, toybox and the BSDs all ship `link`. With `ln` gone the temporary can only be the entry `link(2)` just made, so the trap moves above the type guard, which no longer needs cleanup of its own. Part of #89
|
[Author Round 3] Round 3 disposition[P1, unresolved] Shell fallback still follows a raced-in candidate — Fixed. The finding is correct, and it cannot be answered by tightening the test: there is no portable non-dereferencing
if ! command -v link >/dev/null 2>&1; then
echo "no link utility: a temporary beside $dest cannot be claimed without following it" >&2
exit 1
fiThat host loses the backup — surfaced as Two things simplified out with the fallback:
Coverage:
Docs: the CI green locally: fmt check, both clippy configurations, |
|
[Reviewer Round 4] [Reviewer Round 4] Approved — no unresolved findings. Round 3’s P1 is resolved. The shell script now requires The staged shell/native collision tests and caller-level recovery test meaningfully cover the required retry behavior. PR linkage and test-plan hygiene are correct. |
|
[Review Verdict Round 4: APPROVED] |
Suggested squash commitTitle Body |
1 participant
Summary
hard_link_overno longer assumes the temporary sibling it links to is free. Both transports now walk a bounded sequence of candidate names —.bootler.link.<pid>.<attempt>— and step over an occupied one instead of failing on it, so a completed link stranded by an attempt interrupted between thelnand the rename cannot stop a resumed apply from publishing.previous. That mattered because the old names could not distinguish this run from the one that left the leftover: the shell script used$$alone, and the native path'sNATIVE_TEMP_COUNTERrestarts at 0 in every process, so pid reuse alone was enough to make the first candidate a retry drew the exact name it had to avoid — and the resulting error left the host stuck until somebody removed the file by hand.LINK_ASIDE_SCRIPTwalks candidates in onesh -cinvocation, linking withlinkand with nothing else.lnis the one step here that does not fail on a taken name: given a directory, or a symlink to one, it links the source inside it and exits0. No option suppresses that portably —-Tis GNU's,-hand-nare the BSDs' and cover only the symlink — and no test run ahead of it closes it either, since an entry created between the test and thelnis followed exactly as one that was already there.linkis a different utility rather than another spelling of the same one: it passes the two names it was given tolink(2)and does nothing else, so an occupied candidate isEEXISTthere whatever kind of entry sits at it, with no name resolved and no window to race. Only a failure that left the name taken advances the walk; every other failure is reported with the linker's own diagnostic.linkis not POSIX, so a host could in principle carry none. That is refused before the walk begins rather than degraded ontoln: refusing a planted entry is what this recovery is for, and anlnpath would give that up on exactly the hosts nobody checked. Such a host loses the backup — reported asExecutorError::Transfernaming the destination, like every other on-host failure here — which is a diagnosable stop rather than a link made through somebody else's directory. The set is close to empty in practice: coreutils, busybox, toybox and the BSDs all shiplink, and the check is acommand -v, not a probe run against the candidate.$tmpnames whichever candidate is in hand, so a trap armed inside the loop wouldrm -fa refused leftover this sequence must leave standing. Below it,$tmpcan only be the entrylink(2)just created, so the trap is armed there — above the temporary's type guard, whose refusal that samerm -fthen cleans up.link_asidetakes the destination's directory rather than a caller-composed name, returns the name it linked under, and advances only onEEXIST. Nothing is unlinked, adopted, or resolved through — andlink(2)does not resolve the new name at all, so a directory sitting at a candidate isEEXISTthere like anything else. That is the same syscall, and the same refusal, the shell side reaches throughlink(1).LINK_TEMP_ATTEMPTS = 64, and exhausting it is reported asExecutorError::Transfernaming the destination rather than walked forever — sobackup_previous_artifactkeeps folding it into its subject-labelledCoreError::Commandunchanged.The link → rename → directory-flush sequence, the regular-file refusal, the sibling-on-the-same-filesystem constraint, and the refusal to clear a planted entry are all unchanged.
Closes #89
Part of #87
Test plan
a_stale_native_candidate_is_stepped_over_rather_than_reused— two leftovers made bylink_asideitself occupy the first candidates a retry draws under the same pid, then a symlink to an operator file, a directory, and a symlink to an operator directory; the retry still publishes.previousfrom a free sibling while leaving all five untouched and both directories empty.a_stale_shell_candidate_is_stepped_over_rather_than_reused— the two leftovers and the symlink to an operator file staged the same way for the shell path, planted from inside a wrapper thatexecs the script so the leftovers carry the pid the script actually runs under.a_shell_candidate_that_is_a_directory_is_neither_followed_nor_removed— a directory and a symlink to one occupy the first two candidates; the backup lands out of a free sibling, both entries are still what they were, and both directories are still empty. It fails on the revision beforelinkwas preferred, with the symlink deleted by the trap and the diverted link left in the operator's directory.a_shell_host_without_link_refuses_the_backup_rather_than_linking_with_ln— the same staging on a host that carries nolink, reached by handing the script aPATHwith neitherlinknorlnon it, which is what such a host is. No.previousis published, the failure names the missing utility, and both planted entries and both directories are untouched. Deleting the requirement from the script fails this test and the shape test below, and nothing else.a_temporary_stranded_by_an_interrupted_attempt_does_not_block_the_resumed_backup(apply.rs) — the caller-level view: a stranded link at attempt 0 does not stopbackup_previous_artifact, and the leftover is still there afterwards.a_directory_with_no_free_candidate_fails_rather_than_walking_foreverand its shell counterpart — the bound is reported asExecutorError::Transfernaming the destination, no.previousis published, and no occupied candidate is cleared.a_link_failure_that_is_not_occupancy_is_reported_as_itself— a stubbedlinkfailing for a non-occupancy reason is reported with its own diagnostic after exactly one call, not folded into the bound's message.the_link_script_never_leaves_the_destination_absentpins the script's shape: the link goes throughlink, the wordlnappears nowhere in it, a host withoutlinkis refused before the walk begins, and the trap sits below the walk and above the temporary's type guard.LINK_ASIDE_SCRIPT's own candidate line, and the shell bound test against the script's own-ge 64, so a rename of the production spelling cannot leave them silently colliding with nothing.cargo fmt -- --check --config group_imports=StdExternalCratecargo clippy --all-targets -- -D warningsandcargo clippy --all-targets --features test-support -- -D warningscargo testandcargo test --features test-support