fix(cargo-cache): close the seed-vs-republish race the design claimed to close
The shared action's justification over zemyna's and emowheel's schemes was that hardlink-cloning from a published snapshot closes gitdan #911 "by construction, not by the single job slot". Review disproved that. This makes the claim true, and corrects the README where it could only be bounded. Finding 1 (verdict-level) — silent partial clone ------------------------------------------------ `hardlink_clone_into` ran `cp -al` with no exit-status check, and both call sites invoked it as a condition, which suppresses `set -e` for the whole call. A publisher's `rm -rf` of the generation it rotated away therefore unlinked entries beneath an in-flight consumer walk, and the truncated tree was renamed into place and reported as success. Both layers are fixed: * The consumer verifies its own clone. Every attempt checks `cp -al`'s status explicitly, the source directory's inode before and after (a wholesale replacement mid-walk splices two generations), and the entry count — the only signal for a subtree unlinked before its parent was listed, since `cp -al` reports no error for one it never saw. Any failure discards the staging tree and retries; exhausting the attempts returns a distinct status 2 and fails the job rather than seeding a partial cache. `unshare_subtree` / `_unshare_files` now propagate failure too — a swallowed unshare leaves the clone aliasing its source, the exact corruption that step exists to prevent. * The publisher does not unlink under a reader. A consumer publishes a `.reading-<snapshot>-<tag>` marker before it resolves the snapshot path; the publisher scans for markers after its first rename. A consumer holding the old generation therefore published its marker before that scan and cannot be missed; one arriving after the scan necessarily resolves to the new generation. The publisher waits for readers to drain and, on timeout, DEFERS reclamation rather than forcing it — the old generation is left as `.publish-old-<key>-<tag>` and swept by a later publish. So correctness is closed by construction; disk reclamation is bounded, not immediate. The residual is capped at one deferred generation per publisher ref, and the README now says exactly that instead of the disproved claim. Finding 2 — restore-mtimes.sh ran with no errexit ------------------------------------------------- `set -euo pipefail` was glued to the end of a comment (`# soundness.set -euo pipefail`), so it was entirely commented out: a partial failure of the `git log | awk` pipeline would have produced wrong mtimes across the whole restore instead of failing loudly. Moved to its own line. Audited every other script for the same defect — this was the only instance. Independent confirmation: shellcheck's two SC2164 warnings on this file's `cd "$repo_root"` disappear now that errexit is actually in effect. Finding 3 — lock-acquire window ------------------------------- A just-seeded directory was unlocked until a later action step, so a concurrent job's prune pass could evict it. `seed-target-dir.sh` now takes an optional lock-id and writes the lock marker on every path out of the script, including into the staging tree before its rename, so the directory carries a lock the instant it appears under its final name. The action's acquire step stays (it is idempotent and stamps the LRU marker). Also hardened `prune-cache.sh` to treat a directory with live reader markers as locked. Today no reachable configuration prunes a snapshot — only protected refs publish them and protected refs are excluded from every pass — so this is redundant by policy; it is here so that stops being the reason it is safe. Verification ------------ New selftest scenario 8 races a real seed against a real publish rotation, gating the rotation on the seed's *observed* clone progress so the window is hit deterministically rather than on a fast machine's coin flip. Red-proven against the unguarded scripts, three consecutive runs: ASSERTION FAILED: the seeded tree is truncated: 15443 entries against the snapshot's 493 (was 48805 before the rotation) (15443 / 16986 / 16498) Green after the fix, six consecutive runs, catching the clone mid-walk at ~10.5k of 48805 entries each time. Scenario 9 covers deferred reclamation and its later sweep; scenario 10 covers an unreadable source failing loudly. `bash scripts/selftest.sh`: 5 suites, exit 0, 75 assertions (was 63). shellcheck over `scripts/`: no new findings, two SC2164 warnings resolved. Docs: README's republish-safety paragraph replaced with what the code now guarantees, including the bounded disk residual stated explicitly; new `read-grace-seconds` / `reader-stale-seconds` inputs documented in the `cargo-cache-publish` table; the selftest table names the new race. Refs: daniel/gitdan#11, zemyna#911 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Sqh2vscfzisk83VuPVQX9L
This commit is contained in:
@@ -23,11 +23,23 @@
|
||||
# own cold-start path — a safe degrade that self-heals on its next run, not
|
||||
# corruption.
|
||||
#
|
||||
# `rm -rf` on the old snapshot removes directory entries only. Any consumer
|
||||
# that already hardlink-cloned from it keeps every inode alive through its own
|
||||
# links, so a republish never pulls data out from under a running job — it
|
||||
# just stops new consumers from seeing the old generation. Disk is reclaimed
|
||||
# when the last clone referencing those inodes is itself evicted.
|
||||
# `rm -rf` on the old snapshot removes directory entries only, so a consumer
|
||||
# that has ALREADY FINISHED cloning from it keeps every inode alive through
|
||||
# its own links. That is the easy half, and on its own it is not enough: a
|
||||
# consumer still WALKING the old generation has its entries unlinked out from
|
||||
# under it, and `cp -al` does not report a subtree that was removed before it
|
||||
# read the parent's listing. That is a silently truncated clone — the failure
|
||||
# mode this script's own selftest (scenario 8) reproduces against the
|
||||
# unguarded version.
|
||||
#
|
||||
# So the unlink is interlocked with the consume side rather than
|
||||
# unconditional: after the swap, this script waits for every in-flight reader
|
||||
# of this snapshot to drain (see the reader-marker ordering proof in
|
||||
# cache-lib.sh) and only then reclaims the rotated-away generation. If the
|
||||
# grace period expires first, reclamation is DEFERRED — the directory is left
|
||||
# under `.publish-old-<key>-<tag>` and swept by a later publish once no reader
|
||||
# holds it. The residual of a very slow consumer is therefore one extra
|
||||
# generation of directory entries on disk, never a torn clone.
|
||||
set -euo pipefail
|
||||
. "$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/cache-lib.sh"
|
||||
|
||||
@@ -36,7 +48,11 @@ ROOT="${2:?}"; TAG="${3:?}"
|
||||
|
||||
SRC=$(target_dir_for "$ROOT" "$OWN_KEY")
|
||||
DST=$(snapshot_dir_for "$ROOT" "$OWN_KEY")
|
||||
OLD="${ROOT}/.publish-old-${TAG}"
|
||||
# Keyed by cache key as well as tag, so a deferred generation can be matched
|
||||
# back to the snapshot whose readers must drain before it is safe to reclaim.
|
||||
OLD="${ROOT}/.publish-old-${OWN_KEY}-${TAG}"
|
||||
SNAP_NAME=$(basename "$DST")
|
||||
GRACE="${CACHE_READ_GRACE_SECONDS}"
|
||||
|
||||
if [ ! -d "$SRC" ]; then
|
||||
echo "publish: no target dir at ${SRC} — nothing to snapshot"
|
||||
@@ -47,6 +63,21 @@ fi
|
||||
# concurrently running job's staging directory is never touched.
|
||||
rm -rf "${ROOT}/.stage-${TAG}" "$OLD"
|
||||
|
||||
# Deferred reclamation from an earlier publish of THIS snapshot whose readers
|
||||
# had not drained in time. Safe to sweep now only if nothing is reading the
|
||||
# snapshot at all: a reader holds the snapshot path, not the deferred name, so
|
||||
# "no readers of snapshot-<key>" is the condition that makes every deferred
|
||||
# generation of it unreachable. Over-conservative by design — a reader of the
|
||||
# CURRENT generation also defers the sweep to the next publish, which costs a
|
||||
# directory listing, not correctness.
|
||||
if [ "$(live_reader_count "$ROOT" "$SNAP_NAME")" -eq 0 ]; then
|
||||
for stale_old in "${ROOT}/.publish-old-${OWN_KEY}-"*; do
|
||||
[ -d "$stale_old" ] || continue
|
||||
echo "publish: reclaiming deferred snapshot generation $(basename "$stale_old")"
|
||||
rm -rf "$stale_old"
|
||||
done
|
||||
fi
|
||||
|
||||
start=$(date +%s)
|
||||
# The staged snapshot is hardlinked to SRC's artifacts and holds its OWN copy
|
||||
# of every file Cargo rewrites in place (unshare_mutable_paths, called inside
|
||||
@@ -63,7 +94,19 @@ hardlink_clone_into "$SRC" "$TMP_DST" "$TAG" || {
|
||||
|
||||
if [ -d "$DST" ]; then mv -T "$DST" "$OLD"; fi
|
||||
mv -T "$TMP_DST" "$DST"
|
||||
rm -rf "$OLD"
|
||||
|
||||
# The scan below happens strictly after the rename above, which is what makes
|
||||
# it impossible for a consumer holding the OLD generation to be missed: such a
|
||||
# consumer resolved the path before that rename, and published its marker
|
||||
# before that. See cache-lib.sh's reader-marker section.
|
||||
if [ -d "$OLD" ]; then
|
||||
if wait_for_readers "$ROOT" "$SNAP_NAME" "$GRACE"; then
|
||||
rm -rf "$OLD"
|
||||
else
|
||||
echo "::warning::publish: a consumer is still cloning the previous ${SNAP_NAME} after ${GRACE}s — deferring reclamation of $(basename "$OLD") rather than unlinking a tree being read"
|
||||
summary_line "- deferred reclaiming the previous \`${SNAP_NAME}\` generation (a consumer is still cloning it); it will be swept by a later publish"
|
||||
fi
|
||||
fi
|
||||
|
||||
echo "publish: ${DST} ($(usage_gb "$DST") GB) published in $(( $(date +%s) - start ))s"
|
||||
summary_line "- published cache snapshot \`$(basename "$DST")\` ($(usage_gb "$DST") GB)"
|
||||
|
||||
Reference in New Issue
Block a user