From 90474aa8852dd9b8ac2e4c09b73f624f6cf9bd6a Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Wed, 24 Jun 2026 23:10:26 +0200 Subject: [PATCH] chore(test): blob lifecyclc with thumbnail cleanup renable thumbnail test, ensure that blob lifecycle correctly trigger thumbnail cleanup on blob deletion need to call `/api/admin/internal/trigger-gc?force=true` --- src/infrastructure/services/dedup_service.rs | 23 +++- src/interfaces/api/handlers/admin_handler.rs | 29 ++++- tests/api/storage_cleanup_check.sh | 106 ++++++++++++++----- 3 files changed, 130 insertions(+), 28 deletions(-) diff --git a/src/infrastructure/services/dedup_service.rs b/src/infrastructure/services/dedup_service.rs index eefa1045..cc9aa24e 100644 --- a/src/infrastructure/services/dedup_service.rs +++ b/src/infrastructure/services/dedup_service.rs @@ -1895,6 +1895,27 @@ impl DedupService { /// The grace window and reference cross-checks together make the sweep safe /// against a concurrent uploader re-referencing a just-orphaned chunk. pub async fn garbage_collect(&self) -> Result<(u64, u64), DomainError> { + self.garbage_collect_with_grace(Self::GC_ORPHAN_GRACE_SECS) + .await + } + + /// Test-only variant that bypasses the orphan grace window — used by + /// `POST /api/admin/internal/trigger-gc?force=true` so the + /// integration suite can reap just-orphaned blobs synchronously + /// (waiting out the production 1 h grace inside a test run is a + /// non-starter). Drops the same rows the regular sweep would, just + /// without the time floor. Unsafe under concurrent uploads because + /// it reopens the TOCTOU window the grace closes — only the + /// admin-internal route, itself gated by + /// `OXICLOUD_ENABLE_ADMIN_INTERNAL_ENDPOINTS`, may reach here. + pub async fn garbage_collect_force(&self) -> Result<(u64, u64), DomainError> { + self.garbage_collect_with_grace(0).await + } + + async fn garbage_collect_with_grace( + &self, + grace_secs: i64, + ) -> Result<(u64, u64), DomainError> { const BATCH_SIZE: i64 = 500; let mut total_deleted = 0u64; @@ -2001,7 +2022,7 @@ impl DedupService { RETURNING hash, size", ) .bind(BATCH_SIZE) - .bind(Self::GC_ORPHAN_GRACE_SECS as i32) + .bind(grace_secs as i32) .fetch_all(self.maintenance_pool.as_ref()) .await .map_err(|e| DomainError::internal_error("Dedup", format!("GC blobs: {e}")))?; diff --git a/src/interfaces/api/handlers/admin_handler.rs b/src/interfaces/api/handlers/admin_handler.rs index 5735959d..44ad42e3 100644 --- a/src/interfaces/api/handlers/admin_handler.rs +++ b/src/interfaces/api/handlers/admin_handler.rs @@ -2085,6 +2085,22 @@ pub async fn internal_trigger_sweep( .into_response() } +/// Query parameters for `POST /api/admin/internal/trigger-gc`. +/// +/// `force=true` bypasses the orphan-grace window so the sweep reaps +/// just-orphaned blobs in the same call. Without this, a blob orphaned +/// less than `GC_ORPHAN_GRACE_SECS` (1 h) ago survives the sweep — the +/// grace exists so a concurrent uploader pinning a just-orphaned chunk +/// can't race the row-delete → file-unlink gap. Integration tests +/// don't have concurrent uploaders, so the test runner sets +/// `force=true` to make the sweep deterministic within a test's +/// runtime. +#[derive(Debug, serde::Deserialize, Default)] +pub struct InternalTriggerGcQuery { + #[serde(default)] + pub force: bool, +} + /// `POST /api/admin/internal/trigger-gc` — run the blob garbage /// collector synchronously. /// @@ -2092,10 +2108,12 @@ pub async fn internal_trigger_sweep( /// to the orphan-grace window) and their on-disk content. Same call /// as the inline post-purge GC and the periodic blob-GC sweep — just /// exposed under an admin route so Hurl can wait for it -/// deterministically. +/// deterministically. Add `?force=true` to bypass the grace window — +/// see [`InternalTriggerGcQuery`]. #[utoipa::path( post, path = "/api/admin/internal/trigger-gc", + params(("force" = Option, Query, description = "Bypass the orphan-grace window (test-only)")), responses( (status = 200, description = "GC ran"), (status = 401, description = "Unauthorized"), @@ -2108,6 +2126,7 @@ pub async fn internal_trigger_sweep( pub async fn internal_trigger_gc( State(state): State>, headers: HeaderMap, + Query(query): Query, ) -> axum::response::Response { use axum::response::IntoResponse; if !state.core.config.features.enable_admin_internal_endpoints { @@ -2116,13 +2135,19 @@ pub async fn internal_trigger_gc( if let Err(e) = admin_guard(&state, &headers).await { return e.into_response(); } - match state.core.dedup_service.garbage_collect().await { + let result = if query.force { + state.core.dedup_service.garbage_collect_force().await + } else { + state.core.dedup_service.garbage_collect().await + }; + match result { Ok((blobs_deleted, bytes_freed)) => ( StatusCode::OK, Json(serde_json::json!({ "ok": true, "blobs_deleted": blobs_deleted, "bytes_freed": bytes_freed, + "forced": query.force, })), ) .into_response(), diff --git a/tests/api/storage_cleanup_check.sh b/tests/api/storage_cleanup_check.sh index 542855ed..1165be31 100755 --- a/tests/api/storage_cleanup_check.sh +++ b/tests/api/storage_cleanup_check.sh @@ -13,19 +13,6 @@ # bash tests/api/storage_cleanup_check.sh # ============================================================= -cat <", … } } ], +# "next_cursor": "…" } +# We trash one level deep — the server cascades into children. +# +# `GET /api/folders` (root listing) still uses the legacy +# `user_id`-keyed query, so it can surface folders the admin +# *created* but doesn't have a role on (e.g. shared drives spawned by +# `drive_quota.hurl` for other users). Those return 404 on +# `/resources` (no Read in the role bundle). Skip them — they aren't +# admin's content to drain. for folder_id in $ROOT_FOLDERS; do - CONTENTS=$(curl -sf -H "$AUTH" "$base_url/api/folders/$folder_id/listing") + RES_HTTP=$(curl -s -H "$AUTH" -o /tmp/storage_cleanup_resources.json \ + -w "%{http_code}" \ + "$base_url/api/folders/$folder_id/resources?limit=500") + if [[ "$RES_HTTP" == "404" ]]; then + log "Skipping folder $folder_id (404 on /resources — not readable by admin)" + continue + fi + if [[ "$RES_HTTP" != "200" ]]; then + fail "/api/folders/$folder_id/resources returned HTTP $RES_HTTP" + fi + CONTENTS=$(cat /tmp/storage_cleanup_resources.json) while IFS= read -r sub_id; do [[ -z "$sub_id" ]] && continue curl -sf -X DELETE -H "$AUTH" "$base_url/api/folders/$sub_id" >/dev/null - done < <(echo "$CONTENTS" | jq -r '.folders[].id') + done < <(echo "$CONTENTS" | jq -r '.items[] | select(.resource_type == "folder") | .resource.id') while IFS= read -r file_id; do [[ -z "$file_id" ]] && continue curl -sf -X DELETE -H "$AUTH" "$base_url/api/files/$file_id" >/dev/null - done < <(echo "$CONTENTS" | jq -r '.files[].id') + done < <(echo "$CONTENTS" | jq -r '.items[] | select(.resource_type == "file") | .resource.id') done log "All live objects moved to trash." @@ -135,9 +145,20 @@ log "All live objects moved to trash." # ── 2b. Verify all root folders are empty according to the API ──────────────── for folder_id in $ROOT_FOLDERS; do - CONTENTS=$(curl -sf -H "$AUTH" "$base_url/api/folders/$folder_id/listing") - SUB_COUNT=$(echo "$CONTENTS" | jq '.folders | length') - FILE_COUNT=$(echo "$CONTENTS" | jq '.files | length') + RES_HTTP=$(curl -s -H "$AUTH" -o /tmp/storage_cleanup_resources.json \ + -w "%{http_code}" \ + "$base_url/api/folders/$folder_id/resources?limit=500") + # Same skip-on-404 as the trash loop above — admin owns the row but + # has no role-grant Read on it (shared drive created for someone else). + if [[ "$RES_HTTP" == "404" ]]; then + continue + fi + if [[ "$RES_HTTP" != "200" ]]; then + fail "/api/folders/$folder_id/resources returned HTTP $RES_HTTP" + fi + CONTENTS=$(cat /tmp/storage_cleanup_resources.json) + SUB_COUNT=$(echo "$CONTENTS" | jq '[.items[] | select(.resource_type == "folder")] | length') + FILE_COUNT=$(echo "$CONTENTS" | jq '[.items[] | select(.resource_type == "file")] | length') if [[ "$SUB_COUNT" -ne 0 || "$FILE_COUNT" -ne 0 ]]; then fail "folder $folder_id still has $SUB_COUNT subfolder(s) and $FILE_COUNT file(s)" fi @@ -159,18 +180,53 @@ fi log "API confirms trash is empty." +# ── 3c. Force the maintenance sweeps synchronously ──────────────────────────── +# +# `trash/empty` already triggers an inline `garbage_collect()` at the end of +# its `clear_trash_in` path, but that GC honours the 1-hour orphan-grace +# window — a blob orphaned seconds ago survives the inline sweep. The +# regular periodic sweep would catch it eventually, but tests need the +# disk state to be quiescent NOW. The two admin-internal triggers below +# (gated by `OXICLOUD_ENABLE_ADMIN_INTERNAL_ENDPOINTS=true`, set in +# tests/common/server.env) make this deterministic: +# +# 1. trigger-sweep — reconciles users.storage_used_bytes and +# drives.used_bytes from SUM(size) — keeps the +# cached counters honest for any quota +# assertions that follow. +# 2. trigger-gc?force=true — same `garbage_collect()` as the inline +# call, but `force=true` bypasses the orphan +# grace so freshly-orphaned blobs ARE reaped. +# Safe here because the test has no concurrent +# uploaders to race the row-delete → unlink +# window the grace normally protects. +# +# Without `force=true`, the test would have to wait an hour for the +# probe blob's `orphaned_at` timestamp to age past the grace window — +# why this script was disabled until the admin-internal triggers +# landed (commit `74b33744`). + +curl -sf -X POST -H "$AUTH" "$base_url/api/admin/internal/trigger-sweep" >/dev/null \ + || fail "trigger-sweep failed (is OXICLOUD_ENABLE_ADMIN_INTERNAL_ENDPOINTS=true?)" +log "Reconciliation sweep triggered." + +GC_RESULT=$(curl -sf -X POST -H "$AUTH" "$base_url/api/admin/internal/trigger-gc?force=true") +[[ -z "$GC_RESULT" ]] && fail "trigger-gc returned an empty body" +GC_BLOBS=$(echo "$GC_RESULT" | jq -r '.blobs_deleted') +GC_BYTES=$(echo "$GC_RESULT" | jq -r '.bytes_freed') +log "GC reaped $GC_BLOBS blob(s), $GC_BYTES byte(s) freed." + # ── 4. Disk verification ────────────────────────────────────────────────────── THUMB_FILES=$(find "$STORAGE_PATH/.thumbnails" -type f 2>/dev/null || true) BLOB_FILES=$(find "$STORAGE_PATH/.blobs" -type f 2>/dev/null || true) if [[ -n "$THUMB_FILES" || -n "$BLOB_FILES" ]]; then - # Async thumbnail/blob workers may still be flushing writes from the - # last test's uploads when the cleanup phase reaches this point — - # particularly on fast CI runners where the test loop outpaces the - # worker. Poll for up to 5 s and exit the loop the moment storage - # drains. TODO: replace with a deterministic worker-drain signal - # (e.g. queue depth on /ready) when one exists. + # Even with the synchronous sweep + force-GC above, the on-disk + # unlink for thumbnails/blobs is handled by async workers that may + # still be draining when this `find` runs. Keep the short + # retry loop as a race guard. TODO: replace with a deterministic + # worker-drain signal (e.g. queue depth on /ready) when one exists. log "Thumb/blob leftovers detected — polling for async worker drain (race guard)" for attempt in 1 2 3 4 5; do sleep 1