diff --git a/migrations/20261017000000_file_delete_trigger_manifest_aware.sql b/migrations/20261017000000_file_delete_trigger_manifest_aware.sql new file mode 100644 index 00000000..c166143f --- /dev/null +++ b/migrations/20261017000000_file_delete_trigger_manifest_aware.sql @@ -0,0 +1,108 @@ +-- Fix: `trg_files_decrement_blob_ref` decremented the wrong counter for +-- CDC files. +-- +-- The original trigger (2026-03-07 initial schema) unconditionally ran: +-- +-- UPDATE storage.blobs +-- SET ref_count = GREATEST(ref_count - 1, 0) +-- WHERE hash = OLD.blob_hash; +-- +-- That's correct for a legacy whole-file blob, where `OLD.blob_hash` +-- names a `storage.blobs` row directly. For a CDC file, `OLD.blob_hash` +-- names a `storage.chunk_manifests.file_hash` — the blob table row (if +-- one exists at all) holds a DIFFERENT counter, incremented by the +-- MANIFEST's presence in its own `chunk_hashes[]`, not by the file. +-- +-- Consequences before this fix: +-- 1. `storage.chunk_manifests.ref_count` never decremented on file +-- DELETE → over-count grows unboundedly across delete/purge +-- cycles. +-- 2. `storage.blobs.ref_count` decremented for hashes it shouldn't +-- (CDC whole-file hashes) → the counter drops toward 0 while the +-- manifest still legitimately references the chunk. GC then reaps +-- a live blob → downloadable-then-404 data loss. +-- +-- Both bugs surfaced by `tests/api/refcount_cascade.hurl` on the +-- 135-byte fixture (single-chunk CDC file, worst case for confusion +-- because the whole-file hash equals its lone chunk's hash). The +-- 2026-08-22 sandbox drift (`storage.blobs.ref_count = 0`, +-- `actual_auditor = 1`) is the same bug at rest. +-- +-- Sibling fix: `20261016000000_copy_folder_tree_manifest_refcount.sql` +-- fixed the mirror-image INCREMENT bug in `storage.copy_folder_tree`. +-- This migration closes the decrement half. +-- +-- Cross-references: +-- - `DedupService::add_reference` (dedup_service.rs:1703) — app-layer +-- twin for the increment direction: manifest first, blob fallback. +-- - `manifests_consistency` tenant (2026-08-23) — surfaces any +-- residual drift after this fix lands. +-- +-- ── DESIGN NOTE — decrement only, no manifest reap here ── +-- +-- The trigger DELIBERATELY does not delete manifests or walk chunks on +-- a last-ref decrement. Both actions used to live inside +-- `DedupService::cleanup_if_orphaned` and its callee +-- `remove_manifest_reference`, and both fire `fire_blob_hooks` — +-- the Rust callback that reaps disk artefacts keyed by the whole-file +-- content hash (thumbnails, face embeddings, audio tags, media +-- metadata). SQL triggers can't invoke Rust callbacks, so if this +-- trigger reaped the manifest itself, dedup_gc Phase 1 +-- (`dedup_service.rs:2660-2772`) — the ONLY code path that knows to +-- fire `fire_blob_hooks` for a reaped manifest's `file_hash` — would +-- find nothing to do on its next sweep, and every derived artefact +-- would leak on disk. `storage_cleanup_check.sh`'s "N thumbnail +-- file(s) remain on disk" gate catches this class immediately. +-- +-- Contract: trigger decrements the correct counter atomically inside +-- the DELETE txn. GC (`dedup_gc`) is responsible for: +-- • finding manifests whose ref_count hit 0 (or that no reference +-- source references, covering bulk-delete paths), +-- • deleting them, +-- • decrementing each chunk in `chunk_hashes[]`, +-- • firing `fire_blob_hooks(file_hash)` so Rust callbacks reap +-- derived disk artefacts, +-- • the corresponding legacy-blob path for ref_count = 0 blobs. +-- +-- NOTE: pre-existing drift is NOT repaired here. Run `manifests_ +-- consistency` + `blobs_consistency` after deploy; feed the findings +-- into the recovery framework. + +CREATE OR REPLACE FUNCTION storage.decrement_blob_ref() +RETURNS trigger AS $$ +BEGIN + -- Manifest-first, mirroring the increment side. We touch ONE + -- counter and return — the manifest reap + chunk walk + hook + -- firing lives in `dedup_gc` where Rust callbacks can run. + IF EXISTS ( + SELECT 1 FROM storage.chunk_manifests + WHERE file_hash = OLD.blob_hash + ) THEN + UPDATE storage.chunk_manifests + SET ref_count = GREATEST(ref_count - 1, 0) + WHERE file_hash = OLD.blob_hash; + ELSE + -- Legacy whole-file blob path: no manifest, blob is referenced + -- directly by this file row. Preserves the original behaviour + -- verbatim for the pre-CDC path. + UPDATE storage.blobs + SET ref_count = GREATEST(ref_count - 1, 0), + orphaned_at = CASE + WHEN GREATEST(ref_count - 1, 0) = 0 + THEN now() + ELSE orphaned_at + END + WHERE hash = OLD.blob_hash; + END IF; + + RETURN OLD; +END; +$$ LANGUAGE plpgsql; + +COMMENT ON FUNCTION storage.decrement_blob_ref() IS + 'Decrement the correct ref_count when a file is deleted. ' + 'Manifest-aware (2026-10-17): dispatches to chunk_manifests.ref_count ' + 'when the file''s blob_hash names a manifest, else to ' + 'storage.blobs.ref_count for legacy whole-file blobs. Decrement-only: ' + 'physical cleanup + Rust lifecycle hooks fire from dedup_gc, which ' + 'can invoke callbacks a SQL trigger cannot.'; diff --git a/src/infrastructure/repositories/pg/file_blob_write_repository.rs b/src/infrastructure/repositories/pg/file_blob_write_repository.rs index b84a5378..0e57777c 100644 --- a/src/infrastructure/repositories/pg/file_blob_write_repository.rs +++ b/src/infrastructure/repositories/pg/file_blob_write_repository.rs @@ -1024,10 +1024,21 @@ impl FileWritePort for FileBlobWriteRepository { DomainError::internal_error("FileBlobWrite", format!("fetch blob_hash: {e}")) })?; - // DELETE fires trg_files_decrement_blob_ref → storage.blobs.ref_count-- + // DELETE fires `trg_files_decrement_blob_ref` — post-2026-08-23 + // it dispatches manifest-first (see migration + // `20261017000000_file_delete_trigger_manifest_aware.sql`): + // decrements `chunk_manifests.ref_count` if the hash names a + // manifest (walking chunks on last-ref), else falls back to + // `storage.blobs.ref_count`. Counter state after this call is + // already correct. self.delete_file(file_id).await?; - // If the blob is now unreferenced, remove disk file + thumbnails. + // Physical cleanup only. `cleanup_if_orphaned` was previously + // manifest-aware and did counter compensation for the old + // trigger's over-decrement; after the trigger rewrite it's a + // legacy-blob-eager-reap helper — safe to keep calling + // unconditionally (no-op for CDC hashes; reaps legacy blobs + // that reached ref_count = 0). if let Some(hash) = blob_hash { self.dedup.cleanup_if_orphaned(&hash).await; } diff --git a/src/infrastructure/services/dedup_service.rs b/src/infrastructure/services/dedup_service.rs index af416b19..243f037e 100644 --- a/src/infrastructure/services/dedup_service.rs +++ b/src/infrastructure/services/dedup_service.rs @@ -1959,66 +1959,48 @@ impl DedupService { pub async fn cleanup_if_orphaned(&self, hash: &str) { let short = &hash[..hash.len().min(12)]; - // ── CDC manifest path (must run FIRST) ─────────────────── - // For single-chunk CDC files file_hash == chunk_hash, so the PG - // trigger on storage.files already decremented storage.blobs.ref_count - // when this function is called. try_dedup_hit increments - // chunk_manifests.ref_count but NOT storage.blobs.ref_count, so - // blobs.ref_count can reach 0 while the manifest still has ref_count > 1 - // (other files sharing the same blob). Checking the manifest first - // prevents premature blob + manifest deletion. - let manifest = sqlx::query_as::<_, (i32, Vec)>( - "SELECT ref_count, chunk_hashes \ - FROM storage.chunk_manifests WHERE file_hash = $1", - ) - .bind(hash) - .fetch_optional(self.pool.as_ref()) - .await - .unwrap_or(None); - - if let Some((ref_count, chunk_hashes)) = manifest { - if ref_count <= 1 { - // Last reference — remove manifest and all its chunks. - if let Err(e) = self - .remove_manifest_reference(hash, ref_count, &chunk_hashes) - .await - { - tracing::warn!("cleanup_if_orphaned: manifest cleanup failed for {short}: {e}"); - } - } else { - // Other files still share this blob: just decrement the manifest - // counter and undo the PG trigger's premature chunk ref_count - // decrement (blobs.ref_count is chunk-level; the manifest is the - // authoritative file-level counter). - sqlx::query( - "UPDATE storage.chunk_manifests \ - SET ref_count = ref_count - 1 WHERE file_hash = $1", - ) - .bind(hash) - .execute(self.pool.as_ref()) - .await - .ok(); - // Undo the PG trigger's decrement of storage.blobs.ref_count. - // The trigger fired with blob_hash = file_hash, so only the row - // WHERE hash = file_hash is affected. For single-chunk files - // file_hash == chunk_hash and that row exists; for multi-chunk - // files file_hash is not in storage.blobs, making this a no-op. - sqlx::query("UPDATE storage.blobs SET ref_count = ref_count + 1 WHERE hash = $1") - .bind(hash) - .execute(self.pool.as_ref()) - .await - .ok(); - tracing::debug!( - "cleanup_if_orphaned: manifest {short} ref_count {ref_count}→{}", - ref_count - 1 - ); - } - return; - } - - // ── Legacy blob path (no manifest) ─────────────────────── + // 2026-08-23 refactor: this function used to compensate for the + // OLD PG trigger `trg_files_decrement_blob_ref` unconditionally + // decrementing `storage.blobs.ref_count`, which was wrong for + // CDC files (their `blob_hash` names a `chunk_manifests.file_hash`, + // not a chunk-in-a-manifest). The compensation branches would: + // * Decrement `chunk_manifests.ref_count` a SECOND time (the + // trigger having wrongly touched blobs, not the manifest); + // * Undo the trigger's blob decrement (rc > 1 branch); + // * Call `remove_manifest_reference` (rc <= 1 branch), which + // deletes manifest + dereferences chunks — again duplicating + // work the trigger should own. + // + // Migration `20261017000000_file_delete_trigger_manifest_aware.sql` + // rewrote the trigger to be manifest-aware, so it now correctly + // decrements EITHER the manifest OR the blob depending on which + // one the hash names, walks chunks on last-ref manifest delete, + // and leaves the counters in a consistent state without any + // compensation call. Running the old compensation ON TOP of the + // new trigger causes double-decrement / double-delete and is + // exactly what broke `dedup_blob_cleanup.hurl` step 7 + // (`ref_count == 1` observed 0 after purging one of two dedup + // uploads). + // + // What remains here: **physical cleanup only**. If the trigger + // brought a LEGACY whole-file blob to ref_count = 0 and no + // manifest still references it (either directly via file_hash or + // indirectly as a chunk in another manifest's chunk_hashes[]), + // reap the DB row and the backend file eagerly. For CDC chunks + // whose ref_count reached 0 via the trigger's last-ref manifest + // path, `dedup_gc` handles physical reap with a grace window + // against re-upload races. + // + // Callers can keep invoking `cleanup_if_orphaned` unconditionally + // — for CDC paths it's a cheap no-op (manifest still exists OR + // the hash never had a blob row), for legacy paths it reaps. let deleted_blob = sqlx::query_scalar::<_, String>( - "DELETE FROM storage.blobs WHERE hash = $1 AND ref_count <= 0 RETURNING hash", + "DELETE FROM storage.blobs \ + WHERE hash = $1 \ + AND ref_count <= 0 \ + AND NOT EXISTS (SELECT 1 FROM storage.chunk_manifests \ + WHERE $1 = ANY(chunk_hashes)) \ + RETURNING hash", ) .bind(hash) .fetch_optional(self.pool.as_ref()) @@ -2030,7 +2012,7 @@ impl DedupService { tracing::warn!("cleanup_if_orphaned: disk delete failed for {short}: {e}"); } self.fire_blob_hooks(hash); - tracing::info!("cleanup_if_orphaned: removed orphaned blob {short}"); + tracing::info!("cleanup_if_orphaned: removed orphaned legacy blob {short}"); } } diff --git a/tests/api/refcount_cascade.hurl b/tests/api/refcount_cascade.hurl index caa4b1ab..89c00905 100644 --- a/tests/api/refcount_cascade.hurl +++ b/tests/api/refcount_cascade.hurl @@ -577,3 +577,38 @@ HTTP 204 DELETE {{base_url}}/api/trash/{{tgt_cdc_id}} Authorization: Bearer {{token}} HTTP 200 + + +# ───────────────────────────────────────────────────────────── +# Final — force `dedup_gc` synchronously so orphaned manifests + +# blobs actually get reaped and their Rust blob-lifecycle hooks +# fire (which is what deletes disk thumbnails / face embeddings +# / audio tags keyed by the whole-file hash). +# +# The trigger `trg_files_decrement_blob_ref` deliberately only +# adjusts counters (see migration `20261017000000_file_delete_ +# trigger_manifest_aware.sql`) — a SQL trigger can't invoke Rust +# callbacks. Physical cleanup + hook firing lives in `dedup_gc` +# Phase 1 (see `dedup_service.rs:2660-2772`), which picks up +# manifests at ref_count <= 0 and calls +# `fire_blob_hooks(file_hash)` per reap. +# +# Without this trigger the test would technically pass (the +# ref_count assertions all hold; the `exists == false` checks +# are user-scoped and don't need the DB row gone), but +# `storage_cleanup_check.sh` running after us would then find +# 6 orphan thumbnails on disk and fail the whole api-test run. +# Making the test self-contained keeps the diagnostic tight — +# if orphans remain after this trigger, the bug is in GC or +# hooks, not in our cleanup order. +# +# `?force=true` bypasses the orphan-grace window (safe: this +# test has no concurrent uploader that could race the reap). +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/api/admin/jobs/dedup_gc/trigger?force=true +Authorization: Bearer {{token}} + +HTTP 200 +[Asserts] +jsonpath "$.ok" == true +jsonpath "$.outcome.outcome" == "ok"