From a8223cab65fc8d87b0c2eea826dd7c76a250b71f Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Mon, 24 Aug 2026 01:05:48 +0200 Subject: [PATCH] test(storage-check): drain the GC cascade instead of one pass MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit One dedup_gc pass cannot fully drain now that thumbnails are derived blobs. Reaping a source releases the references its derived artifacts hold (each content_derived_blobs row pins a manifest), and those releases happen mid-sweep — the derived chunks are stamped orphaned as the pass is already walking past them, because remove_manifest_reference deliberately does not unlink, to avoid racing a concurrent upload re-referencing the same chunk. They are collectible only on the NEXT sweep, which is why the check saw 15 leftover blobs. Loops until a pass reclaims nothing rather than hardcoding two. Two is correct only while the derivation graph is one level deep — a thumbnail is derived from a file, nothing is derived from a thumbnail. That is a property of the data, not an invariant the code enforces, so a fixed count would silently under-drain the day transcodes-of-thumbnails or E2E-wrapped derivatives exist, and the failure would surface as a confusing leftover-file assertion rather than the design change it is. Bounded at 3 with a warning if it does not settle. Sleeps between passes. The JobRegistry serialises runs of the same job, so a back-to-back trigger risks rejection as already-running — which returns 0 reaped and would exit the loop early, declaring success with blobs still on disk. A false pass is worse than a slow one. It also gives the previous pass's detached unlink tasks (spawned by on_blob_deleted, awaited by nothing) time to land. Deliberately NOT fixed in production code: derived chunks land inside the 1-hour orphan grace, so a second immediate sweep would collect nothing there and the next scheduled run picks them up. A fixpoint loop in garbage_collect would be dead code outside force=true, which is only this test. Co-Authored-By: Claude Opus 5 (1M context) --- tests/api/storage_cleanup_check.sh | 58 +++++++++++++++++++++++++++--- 1 file changed, 53 insertions(+), 5 deletions(-) diff --git a/tests/api/storage_cleanup_check.sh b/tests/api/storage_cleanup_check.sh index ec416797..a8c6745e 100755 --- a/tests/api/storage_cleanup_check.sh +++ b/tests/api/storage_cleanup_check.sh @@ -287,11 +287,59 @@ curl -sf -X POST -H "$AUTH" "$base_url/api/admin/jobs/usage_reconcile/trigger" > || fail "usage_reconcile trigger failed" log "Reconciliation sweep triggered." -GC_RESULT=$(curl -sf -X POST -H "$AUTH" "$base_url/api/admin/jobs/dedup_gc/trigger?force=true") -[[ -z "$GC_RESULT" ]] && fail "trigger-gc returned an empty body" -GC_BLOBS=$(echo "$GC_RESULT" | jq -r '.outcome.count') -GC_BYTES=$(echo "$GC_RESULT" | jq -r '.outcome.extra.bytes_reclaimed') -log "GC reaped $GC_BLOBS blob(s), $GC_BYTES byte(s) freed." +# One GC pass is NOT enough, and this is by design rather than a bug. +# Reaping a source blob releases the references its DERIVED artifacts hold +# (thumbnails live in storage.content_derived_blobs and each row pins a +# manifest). Those releases happen mid-sweep, so the derived chunks are +# only stamped orphaned as the pass is already walking past them — +# `remove_manifest_reference` deliberately does not unlink, to avoid racing +# a concurrent upload re-referencing the same chunk. They become +# collectible on the NEXT sweep. +# +# Loop until a pass reclaims nothing rather than hardcoding two passes. +# Two is correct only while the derivation graph is one level deep — a +# thumbnail is derived from a file and nothing is derived from a thumbnail. +# That is a property of the data, not an invariant the code enforces, so a +# fixed count would silently under-drain the day transcodes-of-thumbnails +# or E2E-wrapped derivatives appear, and the failure would surface as a +# confusing leftover-file assertion rather than as the design change it is. +# +# Production does NOT need this loop: derived chunks land inside the 1-hour +# orphan grace, so a second immediate pass would collect nothing and the +# next scheduled sweep picks them up. It is only `force=true` (grace 0) +# that can drain a cascade in one go, which is exactly this test. +GC_TOTAL_BLOBS=0 +GC_TOTAL_BYTES=0 +GC_DRAINED=0 +for gc_pass in 1 2 3; do + GC_RESULT=$(curl -sf -X POST -H "$AUTH" "$base_url/api/admin/jobs/dedup_gc/trigger?force=true") + [[ -z "$GC_RESULT" ]] && fail "trigger-gc returned an empty body (pass $gc_pass)" + GC_BLOBS=$(echo "$GC_RESULT" | jq -r '.outcome.count // 0') + GC_BYTES=$(echo "$GC_RESULT" | jq -r '.outcome.extra.bytes_reclaimed // 0') + GC_TOTAL_BLOBS=$((GC_TOTAL_BLOBS + GC_BLOBS)) + GC_TOTAL_BYTES=$((GC_TOTAL_BYTES + GC_BYTES)) + log "GC pass $gc_pass reaped $GC_BLOBS blob(s), $GC_BYTES byte(s) freed." + if [[ "$GC_BLOBS" -eq 0 ]]; then + GC_DRAINED=1 + break + fi + # Breathe before the next trigger, for two reasons: + # + # * The JobRegistry serialises runs of the same job. Firing the next + # trigger before the previous run has fully unwound risks it being + # rejected as already-running — which would come back as 0 reaped + # and exit this loop early, declaring success with blobs still on + # disk. A false pass is worse than a slow one. + # * `on_blob_deleted` spawns detached unlink tasks that nothing + # awaits, so some of the previous pass's disk work may still be in + # flight. + sleep 1 +done +if [[ "$GC_DRAINED" -ne 1 ]]; then + log "WARNING: GC still reaping after 3 passes — the derivation graph may" + log " be deeper than one level; raise the bound and check why." +fi +log "GC total: $GC_TOTAL_BLOBS blob(s), $GC_TOTAL_BYTES byte(s) freed." # ── 4. Disk verification ──────────────────────────────────────────────────────