From 88ab063b64770121c6491ae792b84990a18d200e Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 23 Aug 2026 23:09:35 -0500 Subject: [PATCH] fix(prune-cache): close the reader-marker check-then-delete window MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both eviction sites checked for a consumer's `.reading-` marker and then, seconds later — `usage_gb` runs `du -sk` over a multi-GB tree between the two — unlinked the directory. A consumer that started a clone inside that gap had its source removed mid-walk, which `cp -al` does not report: a subtree unlinked before its parent is listed is silently omitted. Unreachable today, and only by policy: snapshots belong to protected refs and protected refs never reach the marker check. `cargo-cache` and `cargo-cache-publish` take that ref list as two independent inputs, so a workflow listing a publisher in one and not the other arms this with no code change at all. Closed structurally, with publish-snapshot.sh's rotation rather than a new mechanism: the candidate is renamed aside and only then re-examined, so the scan the unlink rests on happens strictly after the rename. A consumer that resolved the directory published its marker before that scan and cannot be missed; one arriving after cannot resolve the path and starts cold, the same degrade the publisher's swap window already produces. Renaming disturbs no clone in flight — no entry is unlinked and the inode is unchanged — so a declined eviction costs a deferred eviction and nothing else. A reprieved cache is put back under its own name; one whose name a concurrent seed has retaken is left aside and swept by a later pass once its readers drain, since nothing else globs a dotted name. prune-cache-selftest gains three scenarios (21 -> 31 assertions). Scenario 12 is the one that bites: the `du` the pass runs on its candidate publishes the marker, placing it strictly after the check and strictly before the unlink. Against check-then-delete, 1-11 pass and 12 fails; breaking only the second look and leaving the rename fails it too. --- README.md | 27 +++++--- scripts/prune-cache-selftest.sh | 65 +++++++++++++++++++- scripts/prune-cache.sh | 105 ++++++++++++++++++++++++++++---- 3 files changed, 177 insertions(+), 20 deletions(-) diff --git a/README.md b/README.md index 63bd0a7..2daa71c 100644 --- a/README.md +++ b/README.md @@ -165,7 +165,7 @@ mechanisms, both required: walk each: measured on ext4 with a warm cache over a 78,554-entry tree, 44 ms per walk against 3,126 ms for the `cp -al` they guard — about 2.8%. -**What this does and does not guarantee.** Three separate claims, deliberately +**What this does and does not guarantee.** Four separate claims, deliberately not collapsed into one: - **A publisher rotating a snapshot cannot tear a clone of it — by @@ -174,12 +174,20 @@ not collapsed into one: 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. +- **Nor can the eviction pass — by the same construction.** A cache chosen for + eviction is renamed aside and only then re-examined for readers, so the scan + the unlink rests on happens strictly after that rename, exactly as the + publisher's does. A consumer that resolved the directory published its + marker before the scan and so cannot be missed; one arriving after the + rename cannot resolve the path at all and starts cold instead. A cache + claimed inside that window is put back under its own name, and one whose + name a concurrent seed has taken in the meantime is left aside and reclaimed + by a later pass once its readers drain. Until this was structural it was + merely policy — snapshots belong to protected refs, protected refs are never + eviction candidates — which is a property held by vigilance rather than by + construction. - **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 + prevented.** 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** — @@ -190,7 +198,10 @@ not collapsed into one: 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. + artifacts are hardlinked to whatever cloned them. A declined eviction is + bounded the same way: the cache stays, either under its own name or aside + awaiting the next pass, and is never unlinked under the job that claimed + it. **Eviction** runs three passes: caches for branches that no longer exist on origin are removed unconditionally; then, only if free space is under the @@ -308,7 +319,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, 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` | +| `prune-cache-selftest.sh` | liveness, protection, locking, eviction order, self-clear, **and that a cache a job claims *inside* the check-to-unlink window survives it** — 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. | Every suite runs the actual script, not a reimplementation of its logic, and diff --git a/scripts/prune-cache-selftest.sh b/scripts/prune-cache-selftest.sh index d95fd10..8c66e31 100755 --- a/scripts/prune-cache-selftest.sh +++ b/scripts/prune-cache-selftest.sh @@ -29,6 +29,15 @@ # 9. OWN CACHE NEVER EVICTED by a sibling pass. # 10. SCOPED TO THE CACHE ROOT — a decoy outside it (standing in for another # project's volume) is never touched. +# 11. A LIVE READER MARKER PROTECTS A CACHE the same way a lock file does — a +# directory somebody is hardlink-cloning this instant is not a candidate, +# however dead and however tight the disk. +# 12. AND SO DOES ONE PUBLISHED INSIDE THE CHECK-TO-UNLINK WINDOW, which is +# the property a check-then-delete eviction does NOT have. This is the +# one that fails against the pre-fix script. +# 13. A DEFERRED EVICTION IS RECLAIMED, but not while its reader is live. +# Nothing else globs a dotted name, so an unswept one is disk lost for +# good on the volume whose whole problem is disk. set -euo pipefail script_dir=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) . "$script_dir/cache-lib.sh" @@ -112,7 +121,7 @@ mk "snapshot-$LIVE" '2020-01-02' # evictable — which is what makes the ORDER the observable property here, not # what survives. Assert the eviction order directly from the log. run_prune "1000000 1000" -order=$(grep -o "evicting \(target\|snapshot\)-$LIVE" "$scratch/log" | sed "s/evicting //") +order=$(grep -o "evicted \(target\|snapshot\)-$LIVE" "$scratch/log" | sed "s/evicted //") [ "$(printf '%s\n' "$order" | head -1)" = "target-$LIVE" ] \ || fail "expected target-$LIVE to be evicted before snapshot-$LIVE, got: $order" ok "target dirs are evicted before snapshots" @@ -170,5 +179,59 @@ decoy="$scratch/other-project"; mkdir -p "$decoy/target-$DEAD"; touch "$decoy/ta run_prune "1000000 1000" assert_kept "$decoy/target-$DEAD" "a cache outside the cache root is never touched" +echo +echo "=== 11: a live reader marker protects a cache, like a lock file does ===" +reset_cache +date +%s > "$root/.reading-target-$DEAD-job1" +run_prune "1000000 1000" +assert_kept "$root/target-$DEAD" "a cache being hardlink-cloned right now survives both passes" +assert_log "currently cloning it" "the reader is named in the log, not silently honoured" +rm -f "$root/.reading-target-$DEAD-job1" + +echo +echo "=== 12: a reader marker published INSIDE the check-to-unlink window ===" +reset_cache +# A consumer publishes its marker whenever it starts a clone, which can be at +# any instant — including after the pass has checked for markers and before it +# unlinks. That window is real time, not a theoretical interleaving: `du -sk` +# on a multi-GB cache runs for seconds, and the pass runs one on every +# candidate. It is reproduced deterministically here by making that very `du` +# publish the marker, which places it strictly after the check and strictly +# before the unlink — exactly where a check-then-delete eviction cannot see +# it. The candidate must still be standing afterwards, with its contents +# intact and nothing left renamed aside. +mkdir -p "$scratch/bin" +real_du=$(command -v du) +cat > "$scratch/bin/du" < "$root/.reading-target-$DEAD-racer" ;; esac +done +exec "$real_du" "\$@" +EOF +chmod +x "$scratch/bin/du" +( PATH="$scratch/bin:$PATH"; run_prune "1000000 900000" ) +[ -e "$root/.reading-target-$DEAD-racer" ] || fail "the racing marker was never published — scenario 12 proves nothing" +assert_kept "$root/target-$DEAD" "a cache claimed inside the eviction window is not unlinked" +assert_kept "$root/target-$DEAD/blob" "the reprieved cache still has its contents" +assert_log "restored, not evicted" "the reprieve is reported, not silent" +[ -z "$(ls -d "$root"/.evicting-* 2>/dev/null)" ] || fail "an aside directory was left behind after the reprieve" +ok "nothing left renamed aside once the eviction is declined" +rm -f "$root/.reading-target-$DEAD-racer" + +echo +echo "=== 13: a deferred eviction is reclaimed, but not under a live reader ===" +reset_cache +aside="$root/.evicting-target-$DEAD-9999" +mkdir -p "$aside"; head -c 4096 /dev/zero > "$aside/blob" +date +%s > "$root/.reading-target-$DEAD-job1" +run_prune "1000000 900000" +assert_kept "$aside" "a deferred eviction is not reclaimed while a job is still reading it" +assert_log "deferring its reclamation again" "the continued deferral is reported" +rm -f "$root/.reading-target-$DEAD-job1" +run_prune "1000000 900000" +assert_gone "$aside" "a deferred eviction is reclaimed once its reader is gone" +assert_log "reclaiming deferred eviction" "the reclamation is reported" + echo echo "prune-cache-selftest: ${pass_count} assertions passed" diff --git a/scripts/prune-cache.sh b/scripts/prune-cache.sh index 7e6efa6..08e0ebd 100755 --- a/scripts/prune-cache.sh +++ b/scripts/prune-cache.sh @@ -57,6 +57,11 @@ # actually-still-running job that somehow exceeds the threshold is visible # in the log rather than silently losing its cache mid-build. # +# That exclusion is decided TWICE per eviction — once as the cheap filter +# that keeps a held directory out of the pass at all, and once after the +# directory has been renamed aside, which is the decision the unlink +# actually rests on. See evict_dir. +# # Liveness is resolved by `git ls-remote --heads origin`, wrapped in a # timeout. A directory name cannot be inverted back to a branch name (the # sanitiser is lossy and the disambiguating suffix is a one-way hash), so this @@ -93,34 +98,90 @@ is_protected() { return 1 } +# is_locked [name] +# +# `name` is the directory's own name for reporting and for the reader-marker +# lookup, which matters when has been renamed aside for eviction: the +# markers a consumer publishes are keyed on the name it resolved, not on +# whatever the eviction pass has since called the directory. is_locked() { - local dir="$1" now lock_file lock_age locked=1 readers + local dir="$1" name="${2:-$(basename "$1")}" now lock_file lock_age locked=1 readers now=$(date +%s) # A directory being hardlink-cloned right now carries no .ci-lock-* of its # own — a snapshot has its locks stripped by construction — so the reader # markers are the only signal that unlinking it would truncate somebody's # in-flight clone. Today no reachable configuration prunes a snapshot (only # protected refs publish them, and protected refs are excluded from every - # pass), which makes this guard redundant *by policy*. It is here so that - # stops being the reason it is safe. - readers=$(live_reader_count "$ROOT" "$(basename "$dir")") + # pass), which makes this guard redundant *by policy* — evict_dir's second + # look is what stops policy being the reason it is safe. + readers=$(live_reader_count "$ROOT" "$name") if [ "$readers" -gt 0 ]; then - echo " $(basename "$dir"): ${readers} job(s) currently cloning it — not a candidate" + echo " ${name}: ${readers} job(s) currently cloning it — not a candidate" locked=0 fi for lock_file in "$dir"/.ci-lock-*; do [ -e "$lock_file" ] || continue lock_age=$(( now - $(stat -c '%Y' "$lock_file") )) if [ "$lock_age" -lt "$STALE_LOCK_SECONDS" ]; then - echo " $(basename "$dir"): held open by $(basename "$lock_file") (${lock_age}s old)" + echo " ${name}: held open by $(basename "$lock_file") (${lock_age}s old)" locked=0 else - echo " $(basename "$dir"): ignoring stale lock $(basename "$lock_file") (${lock_age}s old > ${STALE_LOCK_SECONDS}s) — treating as abandoned" + echo " ${name}: ignoring stale lock $(basename "$lock_file") (${lock_age}s old > ${STALE_LOCK_SECONDS}s) — treating as abandoned" fi done return "$locked" } +# evict_dir +# +# Unlinks , or declines to and says why. Returns 0 only if the directory +# is actually gone. +# +# The caller has already established that is a candidate, which is not +# the same as establishing that it is still one at the instant of the unlink: +# a consumer publishes its reader marker whenever it starts a clone, and the +# `du` that measures the directory between those two points runs for seconds +# on a multi-GB tree. Unlinking under a live clone truncates it silently — +# `cp -al` never reports a subtree that was removed before it read the +# parent's listing (see cache-lib.sh's reader-marker section). +# +# So the directory is renamed aside first and only then re-examined, which is +# what makes the second look conclusive rather than merely closer to the +# unlink. It is publish-snapshot.sh's rotation, and it rests on the same +# ordering proof: a consumer publishes its marker BEFORE it resolves the +# source path, so one that resolved this directory did so before the rename +# and therefore published its marker before the scan below, which happens +# strictly after that rename. A consumer arriving after the rename cannot +# resolve the path at all and falls through to its own cold-start path — the +# same safe degrade the publisher's swap window already produces. +# +# The rename disturbs nothing already in flight: it unlinks no entry and +# leaves the source inode unchanged, which is precisely why the publish side +# can rotate a snapshot out from under a live reader. A declined eviction +# therefore costs a deferred eviction and nothing else. +evict_dir() { + local dir="$1" name aside + name=$(basename "$dir") + aside="${ROOT}/.evicting-${name}-$$" + + mv -T "$dir" "$aside" 2>/dev/null || { + echo " ${name}: could not be set aside for eviction — skipped this pass" + return 1 + } + + if is_locked "$aside" "$name"; then + if mv -T "$aside" "$dir" 2>/dev/null; then + echo " ${name}: claimed by a job while its eviction was in flight — restored, not evicted" + else + echo "::warning::prune: ${name} was claimed mid-eviction and its own name is occupied again — leaving $(basename "$aside") for a later pass to reclaim once its readers drain" + fi + return 1 + fi + + rm -rf "$aside" + return 0 +} + # Directories oldest-first, target-* before snapshot-* (see the header). # The sort key is a rank digit followed by a zero-padded mtime, so the two # groups sort as blocks rather than interleaving by age. `.cache-last-used` @@ -141,6 +202,28 @@ list_by_lru() { done | sort | cut -f2- } +# Deferred reclamations from an earlier pass: a directory renamed aside for +# eviction that could not be unlinked, because a job claimed it inside the +# window and its own name was occupied again before it could be restored — or +# whose run was killed between the rename and the unlink. Nothing below would +# ever see one: every pass globs target-*/snapshot-*, which a dotted name does +# not match. Left unswept it is permanently unreclaimable disk on the one +# volume whose entire problem is disk. +# +# Safe under a concurrent pass mid-eviction for the reason evict_dir is: the +# aside name only exists after that pass's rename, so a consumer that resolved +# the directory published its marker before the count below. +for aside in "$ROOT"/.evicting-*; do + [ -d "$aside" ] || continue + aside_name=$(basename "$aside"); aside_name="${aside_name#.evicting-}"; aside_name="${aside_name%-*}" + if [ "$(live_reader_count "$ROOT" "$aside_name")" -gt 0 ]; then + echo "prune: $(basename "$aside") is still being read — deferring its reclamation again" + continue + fi + echo "prune: reclaiming deferred eviction $(basename "$aside")" + rm -rf "$aside" +done + echo "=== pass 1: liveness (unconditional, not gated on free space) ===" declare -A live_ns=() LIVENESS_AVAILABLE=0 @@ -174,9 +257,9 @@ if [ "$LIVENESS_AVAILABLE" = "1" ]; then [ -n "${live_ns[$name]:-}" ] && continue is_locked "$dir" && continue dir_gb=$(usage_gb "$dir") - echo "::warning::pruning dead-branch cache ${name} (${dir_gb} GB) — no matching branch on origin" + evict_dir "$dir" || continue + echo "::warning::pruned dead-branch cache ${name} (${dir_gb} GB) — no matching branch on origin" summary_line "- pruned dead-branch cache \`${name}\` (${dir_gb} GB) — branch no longer exists on origin" - rm -rf "$dir" pruned_any=1 done [ "$pruned_any" = "1" ] || echo "no dead-branch caches found" @@ -207,9 +290,9 @@ for dir in "${LRU[@]}"; do [ "$FREE_KB" -ge "$THRESHOLD_KB" ] && break is_locked "$dir" && continue dir_gb=$(usage_gb "$dir") - echo "::warning::evicting $(basename "$dir") (${dir_gb} GB, LRU under disk pressure)" + evict_dir "$dir" || continue + echo "::warning::evicted $(basename "$dir") (${dir_gb} GB, LRU under disk pressure)" summary_line "- evicted \`$(basename "$dir")\` (${dir_gb} GB, LRU under disk pressure)" - rm -rf "$dir" done read -r TOTAL_KB FREE_KB <<< "$(read_df "$ROOT")"