From 719475831b62be84dd5cbebcae241b896c7e42fc Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 23 Aug 2026 16:49:44 -0500 Subject: [PATCH] fix(cargo-cache): scope the safety claim to what the code actually prevents MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to f57e2a6, addressing the reviewer's sharpest question: is the race "closed by construction", or merely detected and retried? The honest answer is "both, on different paths", and the README said only the first half. * README now states three claims separately instead of collapsing them: a publisher rotating a snapshot cannot tear a clone of it (by construction — the marker ordering prevents the unlink, and the consumer's verification is a redundant second check on that path); every OTHER way the source can change mid-clone is detected, not prevented (the eviction pass's reader check is check-then-delete, and a seed-fallback-dir has no interlock at all — there, verification plus a bounded retry and a loud failure is the whole guard); and disk reclamation is bounded rather than immediate. Overclaiming this property once was the finding; overclaiming it twice would be worse. * publish-snapshot-selftest.sh now covers the PUBLISHER's half of the race, where a reader of the swap belongs. Scenario 3 only ever covered a consumer that had already FINISHED cloning — safe for free, since its own hardlinks keep the inodes alive. New scenario 6 covers a reader still in flight past the grace period (the generation is left on disk, the deferral is warned about, and an earlier consumer is still unaffected); scenario 7 covers the sweep, so "we defer instead of forcing" cannot quietly become a disk leak. Red-proven against 248af306's scripts: ASSERTION FAILED: the previous generation was unlinked while a reader still held it The consumer's half stays in seed-target-dir-selftest.sh scenario 8, which still red-proves at 16693 of 48805 entries against the same scripts. * seed-target-dir-selftest.sh now asserts what happens when the retries are EXHAUSTED, not just what hardlink_clone_into returns: an unreadable source makes the seed script exit non-zero, name the reason, leave no target dir, and — the one that matters — not fall through to its cold-start branch. A corrupt-cache bug degrading into an invisible 4x-slower CI job is the failure mode worth pinning down. Skipped when running as root, where mode bits deny nothing. * usage_kb: a directory we cannot read measured as the empty string, which was then spliced into usage_gb's awk program and made it a syntax error at the exact moment something was already going wrong. Now measures 0. Verification: `bash scripts/selftest.sh` — 5 suites, exit 0, 82 assertions (was 75 after f57e2a6, 63 before). shellcheck over scripts/: no new findings. Measured the cost the reviewer asked about, on ext4, warm cache, 78,554 entries: `cp -al` 3126 ms against 44 ms for one `find | wc -l`. Two counts per attempt is ~2.8% on top of the clone. Not measured on the CI runner's volume. Refs: daniel/gitdan#11, zemyna#911 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Sqh2vscfzisk83VuPVQX9L --- README.md | 38 ++++++++++----- scripts/cache-lib.sh | 9 +++- scripts/publish-snapshot-selftest.sh | 51 ++++++++++++++++++++ scripts/seed-target-dir-selftest.sh | 71 +++++++++++++++------------- 4 files changed, 123 insertions(+), 46 deletions(-) diff --git a/README.md b/README.md index 58e1bc4..e8a98f4 100644 --- a/README.md +++ b/README.md @@ -163,16 +163,32 @@ mechanisms, both required: and the clone retried; one that fails the last attempt fails the job. A partial tree never reaches the final name. -**What this does and does not guarantee.** *Correctness* is closed by -construction: no combination of publish and seed timing produces a target -directory holding part of one generation, and a source that cannot be read -consistently fails the job loudly instead of seeding a truncated cache. -*Disk reclamation* is bounded, not immediate: a consumer slower than the grace -period leaves one extra snapshot generation of directory entries on the volume -until the next publish of that snapshot sweeps it. That residual is capped at -one deferred generation per publisher ref, and its real cost is close to the -inode count rather than the byte count, since the artifacts are hardlinked to -whatever cloned them. +**What this does and does not guarantee.** Three separate claims, deliberately +not collapsed into one: + +- **A publisher rotating a snapshot cannot tear a clone of it — by + construction.** This is the case zemyna #911 is about, and the marker + ordering above is what closes it: the publisher's scan cannot miss a + consumer that resolved the old generation, and on timeout it defers the + unlink rather than forcing it. On this path the consumer's own verification + is a redundant second check, not the thing holding the guarantee up. +- **Every other way the source can change mid-clone is detected, not + prevented.** The eviction pass's reader check is check-then-delete, so a + consumer publishing its marker inside that gap is narrowed but not excluded + — unreachable today only because snapshots belong to protected refs and + protected refs are never eviction candidates, which is policy rather than + structure. A `seed-fallback-dir` pointing at a directory something else + writes has no interlock at all. There, the per-attempt verification is what + stands between a torn read and a corrupt cache: the clone is retried + (`CACHE_CLONE_ATTEMPTS`, default 4) and then **fails the job loudly** — + never seeded partially, and never degraded to a silent cold build. +- **Disk reclamation is bounded, not immediate.** A consumer slower than the + grace period leaves one extra snapshot generation of directory entries on + the volume until a later publish sweeps it; a consumer whose job was killed + outright holds it until its marker passes `reader-stale-seconds`. The + residual is capped at one deferred generation per publisher ref, and its + real cost is close to inode count rather than byte count, since the + artifacts are hardlinked to whatever cloned them. **Eviction** runs three passes: caches for branches that no longer exist on origin are removed unconditionally; then, only if free space is under the @@ -289,7 +305,7 @@ bash scripts/selftest.sh --fast # fixture-only suites, no compiler |---|---| | `hardlink-clone-selftest.sh` | that a build in a clone cannot mutate its source — with a control proving a raw `cp -al` does. Needs a real compiler. | | `seed-target-dir-selftest.sh` | seed-source preference, lock-file stripping, two jobs racing on one cache key, **and a seed racing a publisher's rotation of the source it is reading** — the race that actually truncates a tree | -| `publish-snapshot-selftest.sh` | the atomic swap, and that a live consumer survives a republish | +| `publish-snapshot-selftest.sh` | the atomic swap, that a live consumer survives a republish, and the publisher's side of the rotation race: deferred reclamation under a live reader, and its sweep once the reader is gone | | `prune-cache-selftest.sh` | liveness, protection, locking, eviction order, self-clear — against a real scratch `origin` | | `restore-mtimes-selftest.sh` | the merge hazard and the watermark that closes it, including the two-jobs-one-namespace case. Needs a real compiler. | diff --git a/scripts/cache-lib.sh b/scripts/cache-lib.sh index 33a9288..55ceb9d 100755 --- a/scripts/cache-lib.sh +++ b/scripts/cache-lib.sh @@ -51,8 +51,15 @@ snapshot_dir_for() { printf '%s/snapshot-%s' "$1" "$2"; } # Disk accounting # --------------------------------------------------------------------------- +# Always prints a number. A directory we cannot read measures as 0 rather than +# as the empty string, which would otherwise be spliced into usage_gb's awk +# program and make it a syntax error at the exact moment something is already +# going wrong. usage_kb() { - if [ -d "$1" ]; then du -sk "$1" 2>/dev/null | awk '{print $1}'; else echo 0; fi + local kb="" + [ -d "$1" ] && kb=$(du -sk "$1" 2>/dev/null | awk '{print $1}') + printf '%s' "${kb:-0}" + return 0 } usage_gb() { diff --git a/scripts/publish-snapshot-selftest.sh b/scripts/publish-snapshot-selftest.sh index 4b70fc2..0bbe56d 100755 --- a/scripts/publish-snapshot-selftest.sh +++ b/scripts/publish-snapshot-selftest.sh @@ -19,6 +19,21 @@ # lock is still held while this runs, and must not be baked into the # snapshot: a lock timestamped at this run's start would look fresh to # the prune pass on every branch later seeded from it. +# 6. DEFERRED RECLAMATION — the publisher's half of the seed-vs-rotation +# race. Scenario 3 above covers a consumer that has ALREADY FINISHED +# cloning; that one is safe for free, because its own hardlinks keep the +# inodes alive. A consumer still WALKING the old generation is the case +# that actually tears, and unlinking underneath it is what produced a +# silently truncated clone. So when a reader is still in flight past the +# grace period, the swap leaves the rotated-away generation on disk +# instead of unlinking it. +# 7. AND THE SWEEP — a deferred generation is not leaked: the next publish +# of that snapshot reclaims it once no reader holds it. Without this the +# "we defer instead of forcing" answer would just be a disk leak with +# better manners. +# +# The consumer's half of the same race — a seed catching a rotation mid-clone +# — is in seed-target-dir-selftest.sh scenario 8. set -euo pipefail script_dir=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) . "$script_dir/cache-lib.sh" @@ -109,5 +124,41 @@ ok "no scratch directories left behind" || fail "the consumer's clone changed under it when the snapshot was replaced" ok "the live consumer still reads its own consistent generation-1 copy" +echo +echo "=== 6: a reader still in flight defers reclamation ===" +# A synthetic reader marker stands in for a consumer whose clone outlasts the +# grace period. Racing a real slow consumer would make the suite's runtime the +# thing under test; the marker IS the entire contract between the two sides, +# so holding one is being a reader. +SNAP_NAME=$(basename "$SNAP") +date +%s > "$root/.reading-${SNAP_NAME}-slowpoke" +replace_file "$TGT/debug/deps/libx.rlib" gen3 +CACHE_READ_GRACE_SECONDS=1 bash "$script_dir/publish-snapshot.sh" "$KEY" "$root" jobDefer \ + > "$scratch/log" 2>&1 || { cat "$scratch/log"; fail "publish-snapshot.sh exited non-zero"; } +[ "$(cat "$SNAP/debug/deps/libx.rlib")" = "gen3" ] || fail "the new generation was not published" +ok "the new generation is published even while a reader holds the old one" +deferred=$(find "$root" -maxdepth 1 -name ".publish-old-${KEY}-*" -print -quit) +[ -n "$deferred" ] || fail "the previous generation was unlinked while a reader still held it" +ok "the rotated-away generation is left on disk rather than unlinked under a reader" +grep -q 'deferring reclamation' "$scratch/log" || fail "the deferral was not reported" +ok "the deferral is surfaced as a warning, not silent" +[ "$(cat "$CONSUMER/debug/deps/libx.rlib")" = "gen1" ] \ + || fail "the earlier consumer's clone changed under it" +ok "the generation-1 consumer is still unaffected" + +echo +echo "=== 7: a later publish sweeps the deferred generation ===" +rm -f "$root/.reading-${SNAP_NAME}-slowpoke" +replace_file "$TGT/debug/deps/libx.rlib" gen4 +publish jobSweep +[ "$(cat "$SNAP/debug/deps/libx.rlib")" = "gen4" ] || fail "the fourth generation was not published" +ok "publishing continues normally after a deferral" +[ -z "$(find "$root" -maxdepth 1 -name '.publish-old-*' -print -quit)" ] \ + || fail "the deferred generation was never reclaimed — this is a disk leak" +ok "the deferred generation is reclaimed once no reader holds it" +[ -z "$(find "$root" -maxdepth 1 \( -name '.stage-*' -o -name '.reading-*' \) -print -quit)" ] \ + || fail "scratch left behind: $(find "$root" -maxdepth 1 \( -name '.stage-*' -o -name '.reading-*' \) -print)" +ok "no staging or reader-marker scratch left behind" + echo echo "publish-snapshot-selftest: ${pass_count} assertions passed" diff --git a/scripts/seed-target-dir-selftest.sh b/scripts/seed-target-dir-selftest.sh index ab3ec4c..452f55e 100755 --- a/scripts/seed-target-dir-selftest.sh +++ b/scripts/seed-target-dir-selftest.sh @@ -37,13 +37,15 @@ # one can truncate a tree. Against the unguarded version this scenario # reproduces a silent partial clone reported as success — 20,328 of # 48,805 entries, `seed: cloned in 1s`, exit 0, seeded-from=base-snapshot. -# 9. DEFERRED RECLAMATION — when a consumer is STILL reading after the grace -# period, the publisher leaves the rotated-away generation on disk rather -# than unlinking a tree under an in-flight walk, and a later publish -# sweeps it once the reader is gone. The residual is disk, not a torn -# clone. -# 10. AN UNREADABLE SOURCE FAILS LOUDLY — the clone reports a distinct -# status instead of renaming whatever it managed to produce into place. +# 9. AN UNREADABLE SOURCE FAILS LOUDLY — the clone reports a distinct status +# instead of renaming whatever it managed to produce into place, and the +# seed SCRIPT turns that status into a failed job rather than a silent +# cold build. Retries are what make a torn read survivable; exhausting +# them must not degrade into "start cold and rebuild everything", which +# would turn a corrupt-cache bug into an invisible 4x-slower CI job. +# (The publisher's half of the rotation race — deferring reclamation +# while a reader is still in flight — lives in +# publish-snapshot-selftest.sh, next to the swap it modifies.) set -euo pipefail script_dir=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) . "$script_dir/cache-lib.sh" @@ -225,38 +227,39 @@ ok "the torn read was detected and reported, not swallowed" ok "no staging, reader-marker or deferred-generation scratch left behind" echo -echo "=== 9: a reader that outlasts the grace period defers reclamation ===" -# A synthetic reader marker stands in for a consumer whose clone is slower -# than the grace period. Driving that with a real slow consumer would make the -# test's runtime the thing under test; the marker is the whole contract -# between the two sides, so holding one IS being a reader. -SNAP_NAME="snapshot-$BASE_KEY" -date +%s > "$root/.reading-${SNAP_NAME}-slowpoke" -make_wide_tree "$root/target-$BASE_KEY" gen3 4 -CACHE_READ_GRACE_SECONDS=1 bash "$script_dir/publish-snapshot.sh" "$BASE_KEY" "$root" pubDefer > "$scratch/logDefer" 2>&1 \ - || { tail -40 "$scratch/logDefer"; fail "publish-snapshot.sh exited non-zero"; } -assert_content "$root/$SNAP_NAME/debug/.fingerprint/x/dep-lib-x" gen3 "the new generation was published regardless" -deferred=$(find "$root" -maxdepth 1 -name ".publish-old-${BASE_KEY}-*" -print -quit) -[ -n "$deferred" ] || fail "the previous generation was unlinked while a reader still held it" -ok "the rotated-away generation was left on disk rather than unlinked under a reader" -grep -q 'deferring reclamation' "$scratch/logDefer" || fail "the deferral was not reported" -ok "the deferral is reported as a warning, not silent" - -rm -f "$root/.reading-${SNAP_NAME}-slowpoke" -make_wide_tree "$root/target-$BASE_KEY" gen4 4 -bash "$script_dir/publish-snapshot.sh" "$BASE_KEY" "$root" pubSweep > "$scratch/logSweep" 2>&1 \ - || { tail -40 "$scratch/logSweep"; fail "publish-snapshot.sh exited non-zero"; } -[ -z "$(find "$root" -maxdepth 1 -name '.publish-old-*' -print -quit)" ] \ - || fail "the deferred generation was never reclaimed" -ok "a later publish reclaims the deferred generation once no reader holds it" - -echo -echo "=== 10: a source that cannot be read fails loudly ===" +echo "=== 9: a source that cannot be read fails loudly ===" rc=0 hardlink_clone_into "$root/nosuch-source" "$root/target-nosuch" nosuch-tag > "$scratch/logMissing" 2>&1 || rc=$? [ "$rc" -eq 2 ] || fail "expected status 2 for an unreadable source, got ${rc}" ok "an unreadable source returns the distinct hard-failure status" assert_absent "$root/target-nosuch" "nothing was renamed into place" +# The status only matters if the script acts on it. A source that exists but +# cannot be read exercises the whole path: retries exhaust, the function +# returns 2, and seed-target-dir.sh must exit non-zero rather than falling +# through to its cold-start branch. +if [ "$(id -u)" = "0" ]; then + echo "SKIP: running as root — mode bits do not deny access" +else + UNREADABLE=$(cache_key feat/unreadable) + VICTIM=$(cache_key feat/victim) + make_tree "$root/snapshot-$UNREADABLE" locked-away + chmod 000 "$root/snapshot-$UNREADABLE" + rc=0 + CACHE_CLONE_ATTEMPTS=2 bash "$script_dir/seed-target-dir.sh" \ + "$VICTIM" "$UNREADABLE" "$root" jobUnread > "$scratch/logUnread" 2>&1 || rc=$? + chmod 755 "$root/snapshot-$UNREADABLE" + [ "$rc" -ne 0 ] || { tail -20 "$scratch/logUnread"; fail "the seed reported success against a source it could not read"; } + ok "the seed exits non-zero when its source cannot be cloned" + grep -q 'refusing to build against a partial cache' "$scratch/logUnread" \ + || { tail -20 "$scratch/logUnread"; fail "the failure was not reported as such"; } + ok "the failure names the reason rather than exiting silently" + if grep -q 'starts cold' "$scratch/logUnread"; then + tail -20 "$scratch/logUnread"; fail "an unreadable source degraded into a silent cold build" + fi + ok "it does not degrade into a silent cold build" + assert_absent "$root/target-$VICTIM" "no target dir was left behind by the failed seed" +fi + echo echo "seed-target-dir-selftest: ${pass_count} assertions passed"