fix(prune-cache): stop the aside sweeper depending on timing it cannot see
Review findings on #2. The first is the one that mattered: the sweeper this PR added had the shape the PR exists to remove. Pass A renames a candidate aside; pass B's sweep sees an aside with no readers and reclaims it; A then finds a reader and restores. `rm -rf` traverses fd-relative, so the rename does not stop it and A can republish a half-emptied tree under a live cache name. `capacity: 1` bounds it today, which is exactly the kind of reason this PR was written to stop relying on. The unlink itself was never the problem — the ordering proof covers it under any interleaving, since the aside name only exists after the evicting pass's rename. What was missing is that an aside with no readers is indistinguishable from one a pass has just created and not yet decided about. The sweeper now leaves an aside alone until it has settled (EVICTION_ASIDE_SETTLE_SECONDS, default 60), which separates the two without having to identify the pass that created it — a PID is meaningless across the job containers these passes run in, and recycles. Read from ctime, not mtime: rename(2) updates the first and leaves the second at whenever the cache was last written, which is the signal list_by_lru wants and the wrong one here. That is a bound, not a construction, and both the code comment and the README now say which of the two properties is which instead of asserting the broader one. Also from the review: a pass that declined every dead cache it found no longer signs off with "no dead-branch caches found", and evict_dir no longer promises a later reclamation of an aside that is already gone. Scenario 14 covers the settle window against the script's own default, with nothing faked — the directory really was set aside a moment ago. Scenario 4 gains the summary assertion. 31 -> 37 assertions; each new gate verified red by defeating it alone in a scratch copy.
This commit is contained in:
+57
-8
@@ -9,6 +9,10 @@
|
||||
# abandoned (default 7200)
|
||||
# CACHE_LIVENESS "false"/"0" to skip the liveness pass entirely
|
||||
# CACHE_DF_OVERRIDE "<total_kb> <free_kb>", for the selftest
|
||||
# EVICTION_ASIDE_SETTLE_SECONDS
|
||||
# how long a directory renamed aside for eviction is
|
||||
# left alone before another pass may reclaim it
|
||||
# (default 60)
|
||||
#
|
||||
# Three passes, in order:
|
||||
#
|
||||
@@ -82,6 +86,15 @@ OWN_DIR="${2:?}"
|
||||
PROTECTED_REFS="${3:-}"
|
||||
MIN_FREE_PCT="${4:-10}"
|
||||
STALE_LOCK_SECONDS="${STALE_LOCK_SECONDS:-7200}"
|
||||
# An aside directory is in flight for one rename plus one marker glob —
|
||||
# milliseconds. Anything older belongs to a pass that died between the two, so
|
||||
# an age is what separates "another pass is mid-eviction" from "a leftover",
|
||||
# and it separates them without having to identify the pass that created it: a
|
||||
# PID is meaningless across the job containers these passes run in, and
|
||||
# recycles. Three orders of magnitude of headroom over the operation it covers,
|
||||
# and short enough that a genuine leftover is reclaimed by the next run rather
|
||||
# than lingering while the volume is under pressure.
|
||||
EVICTION_ASIDE_SETTLE_SECONDS="${EVICTION_ASIDE_SETTLE_SECONDS:-60}"
|
||||
|
||||
declare -A protected_ns=()
|
||||
for ref in $PROTECTED_REFS; do
|
||||
@@ -172,8 +185,10 @@ evict_dir() {
|
||||
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"
|
||||
elif [ -d "$aside" ]; then
|
||||
echo "::warning::prune: ${name} was claimed mid-eviction and its own name is taken again — leaving $(basename "$aside") for a later pass to reclaim once its readers drain"
|
||||
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"
|
||||
echo "::warning::prune: ${name} was claimed mid-eviction and is already gone — another pass reclaimed it after its readers drained"
|
||||
fi
|
||||
return 1
|
||||
fi
|
||||
@@ -204,17 +219,41 @@ list_by_lru() {
|
||||
|
||||
# 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
|
||||
# window and its own name was taken 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.
|
||||
# TWO DISTINCT PROPERTIES HOLD HERE, and neither implies the other.
|
||||
#
|
||||
# No clone can be truncated by the unlink below, by construction: the aside
|
||||
# name only comes into existence after the evicting pass's rename, so a
|
||||
# consumer that resolved the directory published its marker strictly before
|
||||
# that rename and therefore before the count below — it cannot be missed. A
|
||||
# consumer that arrives later cannot resolve the path at all. This holds under
|
||||
# any interleaving and needs no settle window.
|
||||
#
|
||||
# No pass mid-eviction is mistaken for a leftover, by bound rather than by
|
||||
# construction, and this is what the settle window is for. Without it a
|
||||
# concurrent pass could unlink an aside its owner is still deciding about, and
|
||||
# `rm -rf` traverses fd-relative: the owner's restore can then republish a
|
||||
# half-emptied tree under a live cache name. What the window guarantees is that
|
||||
# the two cannot be confused within it. What it does not guarantee is the
|
||||
# pathological case beyond it — an evicting pass suspended past the window and
|
||||
# then resumed finds its aside reclaimed and fails its restore, saying so.
|
||||
now=$(date +%s)
|
||||
for aside in "$ROOT"/.evicting-*; do
|
||||
[ -d "$aside" ] || continue
|
||||
# Change time, not modification time. `mv` leaves a directory's mtime alone
|
||||
# (a cache last written days ago keeps a days-old mtime, which is what
|
||||
# list_by_lru wants and exactly the wrong signal here) but rename(2) does
|
||||
# update ctime, so %Z is when this directory was set aside and %Y is not.
|
||||
aside_age=$(( now - $(stat -c '%Z' "$aside" 2>/dev/null || echo "$now") ))
|
||||
if [ "$aside_age" -lt "$EVICTION_ASIDE_SETTLE_SECONDS" ]; then
|
||||
echo "prune: $(basename "$aside") was set aside ${aside_age}s ago — another pass may still be evicting it, leaving it alone"
|
||||
continue
|
||||
fi
|
||||
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"
|
||||
@@ -250,19 +289,29 @@ fi
|
||||
|
||||
if [ "$LIVENESS_AVAILABLE" = "1" ]; then
|
||||
pruned_any=0
|
||||
# Tracked separately so the line below cannot contradict the decline lines
|
||||
# above it: "none pruned" and "none found" are different outcomes, and a
|
||||
# pass that declined every dead cache it found has not found none.
|
||||
spared_any=0
|
||||
for dir in "$ROOT"/target-* "$ROOT"/snapshot-*; do
|
||||
[ -d "$dir" ] || continue
|
||||
name=$(basename "$dir")
|
||||
is_protected "$dir" && continue
|
||||
[ -n "${live_ns[$name]:-}" ] && continue
|
||||
is_locked "$dir" && continue
|
||||
is_locked "$dir" && { spared_any=1; continue; }
|
||||
dir_gb=$(usage_gb "$dir")
|
||||
evict_dir "$dir" || continue
|
||||
evict_dir "$dir" || { spared_any=1; 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"
|
||||
pruned_any=1
|
||||
done
|
||||
[ "$pruned_any" = "1" ] || echo "no dead-branch caches found"
|
||||
if [ "$pruned_any" = "0" ]; then
|
||||
if [ "$spared_any" = "1" ]; then
|
||||
echo "every dead-branch cache found is still in use — none pruned this pass"
|
||||
else
|
||||
echo "no dead-branch caches found"
|
||||
fi
|
||||
fi
|
||||
else
|
||||
echo "liveness: ${LIVENESS_REASON} — treating as UNAVAILABLE (not as \"no branches\"); pass 1 skipped"
|
||||
fi
|
||||
|
||||
Reference in New Issue
Block a user