cjw/Oxicloud
1
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases Wiki Activity

fix(ref_count): use SQL to correct ref_count on cascading deletion

Browse Source 
then dedup_gc will trigger blob life cycle and ensure chunk deletions
This commit is contained in:
Edouard Vanbelle
2026-08-23 22:44:39 +02:00
parent 2760fe9efc
commit 0f29614a3b
4 changed files with 198 additions and 62 deletions
Download Patch File Download Diff File Expand all files Collapse all files
migrations/20261017000000_file_delete_trigger_manifest_aware.sql
+108
View File
@@ -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.';
src/infrastructure/repositories/pg/file_blob_write_repository.rs
+13 -2
View File
@@ -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;
}
src/infrastructure/services/dedup_service.rs
+42 -60
View File
@@ -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<String>)>(
"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}");
}
}
tests/api/refcount_cascade.hurl
+35
View File
@@ -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"