From 64ff98257127fb9596a42a98bcdc41680b7109f7 Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Tue, 25 Aug 2026 07:39:57 +0200 Subject: [PATCH] feat(thumbnails): uploaded previews survive a copy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Completes step 9. The PUT wrote `ext-{file_id}.jpg` and nothing else — keyed by file id, on local disk. No copy path duplicates it and no other instance can see it, so a copied file lost the preview its owner uploaded. Silently: the server falls back to rendering one from the source, or to 204 for a PDF, which has no render path at all. A user-supplied preview is not derivable from the content, so once lost it is gone. The PUT now also records a storage.file_attached_blobs row, which copy_file_satellites already duplicates, so both copy paths carry it. Best-effort: the sidecar has already succeeded by then and the user can see their thumbnail, so failing the request would report an error for an operation that visibly worked. Read path consults attachments ahead of every content-derived tier: an uploaded preview is an explicit choice about THIS file and must beat anything rendered from its content. Cached under the per-file key — a content key would leak those bytes to every other file sharing the content, which is the poisoning the file-keyed table exists to prevent. store_attached_blob is ON CONFLICT DO UPDATE, unlike its derived twin: re-uploading a preview is a deliberate replacement, where a re-derived thumbnail is the same bytes again. The superseded blob's reference is released, or it would be pinned forever with nothing pointing at it. Deletion goes through a trigger, not a hook. file_id is ON DELETE CASCADE, and on_file_deleted fires AFTER delete_file — by then the cascade has run and there is nothing left to enumerate. This matters most for folder deletion, where PG cascades folders to files to attachments and Rust never sees the rows at all. storage.decrement_blob_ref keys off OLD.blob_hash and is otherwise table-agnostic, so it is reused verbatim rather than transcribed into a second trigger that can drift. DELETE only: a replacement updates in place and is handled in Rust, so adding UPDATE would double-decrement. Extracted read_blob_to_bytes, shared by the attached and derived tiers — the only difference between them is which table produced the hash. tests/api/attached_thumbnail_copy.hurl guards it. The file is red and the uploaded thumbnail is green, so a render could never produce the uploaded bytes; the pre-upload render is captured first and required to change, which stops three identical renders from satisfying the byte-equality. Then both copy paths must serve the upload, and after the original is purged and GC runs, both copies must still serve it — each holds its own reference, because the rows are duplicated rather than shared. --- ..._file_attached_blobs_decrement_trigger.sql | 28 ++ src/infrastructure/services/dedup_service.rs | 100 +++++++ .../services/thumbnail_service.rs | 79 +++-- src/interfaces/api/handlers/file_handler.rs | 52 +++- tests/api/attached_thumbnail_copy.hurl | 275 ++++++++++++++++++ tests/api/run.sh | 1 + 6 files changed, 511 insertions(+), 24 deletions(-) create mode 100644 migrations/20261021000000_file_attached_blobs_decrement_trigger.sql create mode 100644 tests/api/attached_thumbnail_copy.hurl diff --git a/migrations/20261021000000_file_attached_blobs_decrement_trigger.sql b/migrations/20261021000000_file_attached_blobs_decrement_trigger.sql new file mode 100644 index 00000000..a1d8c368 --- /dev/null +++ b/migrations/20261021000000_file_attached_blobs_decrement_trigger.sql @@ -0,0 +1,28 @@ +-- Release the blob reference when an attachment row goes away. +-- +-- `storage.file_attached_blobs.file_id` is `ON DELETE CASCADE`, so deleting a +-- file removes its attachment rows inside the database — invisible to Rust. +-- The lifecycle hook cannot cover this: `on_file_deleted` fires AFTER +-- `delete_file`, by which point the cascade has already run and there is +-- nothing left to read. The references would survive with no row behind them, +-- and `dedup_gc` would see a positive count forever — bytes pinned for good. +-- +-- `storage.decrement_blob_ref()` already exists for exactly this, on +-- `storage.files`. It keys off `OLD.blob_hash` and is otherwise +-- table-agnostic, so it applies verbatim — and reusing it keeps the +-- manifest-first decrement contract defined in one place rather than +-- transcribed into a second trigger that can drift. +-- +-- Only DELETE. Replacing a preview updates `blob_hash` in place +-- (`store_attached_blob` is ON CONFLICT DO UPDATE), and the reference to the +-- superseded blob is released there, in Rust. Adding UPDATE here would +-- double-decrement it. + +CREATE OR REPLACE TRIGGER trg_file_attached_blobs_decrement_blob_ref + AFTER DELETE ON storage.file_attached_blobs + FOR EACH ROW + EXECUTE FUNCTION storage.decrement_blob_ref(); + +COMMENT ON TRIGGER trg_file_attached_blobs_decrement_blob_ref + ON storage.file_attached_blobs IS + 'Releases the blob reference held by an attachment row. Needed because file_id is ON DELETE CASCADE, so rows vanish inside the DB where the Rust lifecycle hooks cannot see them.'; diff --git a/src/infrastructure/services/dedup_service.rs b/src/infrastructure/services/dedup_service.rs index c4826648..c0d1d15f 100644 --- a/src/infrastructure/services/dedup_service.rs +++ b/src/infrastructure/services/dedup_service.rs @@ -576,6 +576,106 @@ impl DedupService { /// `docs/plan/derived-blobs.md`. /// /// Returns the derived blob hash. + /// Attach user-supplied bytes to a FILE — the file-keyed twin of + /// [`Self::store_derived_blob`]. + /// + /// Same storage path (the bytes are still content-addressed and still + /// deduplicated), different mapping: the row is keyed by `file_id`, so + /// two files holding identical attached bytes get two rows and two + /// references. Sharing the mapping is what must not happen — a + /// content-keyed client preview would let one user's upload be served + /// for another user's file. + /// + /// `ON CONFLICT … DO UPDATE`, unlike the derived twin: re-uploading a + /// preview for the same `(file_id, kind, variant)` is a deliberate + /// replacement, whereas a re-derived thumbnail is the same bytes again. + /// The reference held by the row being replaced is released. + pub async fn store_attached_blob( + &self, + file_id: &str, + kind: &str, + variant: &str, + content_type: &str, + bytes: Bytes, + uploaded_by: uuid::Uuid, + ) -> Result { + let stored = self + .store_from_stream( + stream::once(async move { Ok::(bytes) }), + Some(content_type.to_string()), + ) + .await?; + let attached_hash = stored.hash().to_string(); + + // `previous` is the hash this row pointed at before, when it existed + // and differed — the reference to release once the row no longer + // holds it. + let previous: Option<(Option,)> = sqlx::query_as( + "INSERT INTO storage.file_attached_blobs + (file_id, kind, variant, blob_hash, content_type, uploaded_by) + VALUES ($1::uuid, $2, $3, $4, $5, $6) + ON CONFLICT (file_id, kind, variant) DO UPDATE + SET blob_hash = EXCLUDED.blob_hash, + content_type = EXCLUDED.content_type, + uploaded_by = EXCLUDED.uploaded_by, + created_at = now() + RETURNING NULLIF(storage.file_attached_blobs.blob_hash, EXCLUDED.blob_hash)", + ) + .bind(file_id) + .bind(kind) + .bind(variant) + .bind(&attached_hash) + .bind(content_type) + .bind(uploaded_by) + .fetch_optional(self.pool.as_ref()) + .await + .map_err(|e| DomainError::internal_error("Dedup", format!("record attached blob: {e}")))?; + + // A replaced row's old blob loses its only reference from here. Not + // releasing it would pin those bytes forever — nothing else points at + // a superseded preview. + if let Some((Some(old_hash),)) = previous + && old_hash != attached_hash + && let Err(e) = self.remove_reference(&old_hash).await + { + tracing::warn!( + target: "oxicloud::dedup", + error = %e, + "failed to release replaced attached-blob reference for {}", + &old_hash[..old_hash.len().min(12)], + ); + } + + Ok(attached_hash) + } + + /// Look up bytes attached to a file. File-keyed counterpart of + /// [`Self::find_derived_blob`]. + pub async fn find_attached_blob( + &self, + file_id: &str, + kind: &str, + variant: &str, + ) -> Option { + sqlx::query_as::<_, (String, String)>( + "SELECT blob_hash, content_type FROM storage.file_attached_blobs + WHERE file_id = $1::uuid AND kind = $2 AND variant = $3", + ) + .bind(file_id) + .bind(kind) + .bind(variant) + .fetch_optional(self.pool.as_ref()) + .await + .ok() + .flatten() + .map(|(blob_hash, content_type)| { + crate::application::ports::dedup_ports::DerivedBlobRef { + blob_hash, + content_type, + } + }) + } + pub async fn store_derived_blob( &self, source_hash: &str, diff --git a/src/infrastructure/services/thumbnail_service.rs b/src/infrastructure/services/thumbnail_service.rs index 3da76cbe..92f91c07 100644 --- a/src/infrastructure/services/thumbnail_service.rs +++ b/src/infrastructure/services/thumbnail_service.rs @@ -505,6 +505,39 @@ impl ThumbnailService { /// `blob_hash` is used to locate the file on disk (dedup-aware). /// If `None`, only the in-memory cache is checked (used for video /// thumbnails where blob_hash is not yet resolved). + /// Drain a blob through the dedup stack into memory. + /// + /// Shared by the attached and derived tiers — the only difference between + /// them is which table produced the hash, so the read itself belongs in + /// one place. Returns `None` on a read fault rather than propagating: a + /// missing satellite must degrade to the next tier, never break a gallery. + async fn read_blob_to_bytes( + dedup: &DedupService, + blob_hash: &str, + file_id: &str, + size: ThumbnailSize, + ) -> Option { + use futures::StreamExt; + let mut stream = dedup.read_blob_stream(blob_hash).await.ok()?; + let mut buf = Vec::new(); + while let Some(chunk) = stream.next().await { + match chunk { + Ok(part) => buf.extend_from_slice(&part), + Err(e) => { + tracing::warn!( + target: "oxicloud::dedup", + error = %e, + "thumbnail blob read failed for {} {:?}", + file_id, + size, + ); + return None; + } + } + } + Some(Bytes::from(buf)) + } + pub async fn get_cached_thumbnail( &self, file_id: &str, @@ -553,6 +586,32 @@ impl ThumbnailService { return Some(bytes); } + // 2b. Bytes the USER attached to this file, if any. + // + // Ahead of every content-derived tier below on purpose: an uploaded + // preview is an explicit choice about THIS file and must beat + // anything the server would render from its content. It is also the + // only tier a copy can inherit — the `ext-` sidecar above is keyed by + // file_id and is not copied, so without this branch a copied file + // silently falls back to a rendered thumbnail, or to none at all for + // a PDF that has no server-side render path. + if let Some(dedup) = dedup + && let Some(attached) = dedup + .find_attached_blob(file_id, "preview", size.dir_name()) + .await + && let Some(bytes) = + Self::read_blob_to_bytes(dedup, &attached.blob_hash, file_id, size).await + { + // Cached under the per-file key: these bytes belong to this file, + // not to its content, so a content key would leak them to every + // other file sharing that content — the poisoning the file-keyed + // table exists to prevent. + self.cache + .insert(ThumbnailCacheKey::external(file_id, size), bytes.clone()) + .await; + return Some(bytes); + } + // 3. Check disk for blob-hash thumbnails (needs blob_hash to locate) let hash = blob_hash?; let thumb_path = self.get_thumbnail_path(hash, size, format); @@ -580,25 +639,7 @@ impl ThumbnailService { let derived = dedup .find_derived_blob(hash, "thumbnail", size.dir_name()) .await?; - use futures::StreamExt; - let mut stream = dedup.read_blob_stream(&derived.blob_hash).await.ok()?; - let mut buf = Vec::new(); - while let Some(chunk) = stream.next().await { - match chunk { - Ok(part) => buf.extend_from_slice(&part), - Err(e) => { - tracing::warn!( - target: "oxicloud::dedup", - error = %e, - "derived thumbnail read failed for {} {:?}", - file_id, - size, - ); - return None; - } - } - } - let bytes = Bytes::from(buf); + let bytes = Self::read_blob_to_bytes(dedup, &derived.blob_hash, file_id, size).await?; self.cache .insert( ThumbnailCacheKey::content(hash, size, format), diff --git a/src/interfaces/api/handlers/file_handler.rs b/src/interfaces/api/handlers/file_handler.rs index 7d2e7f71..7fdf95be 100644 --- a/src/interfaces/api/handlers/file_handler.rs +++ b/src/interfaces/api/handlers/file_handler.rs @@ -726,15 +726,57 @@ impl FileHandler { return AppError::from(err).into_response(); } - // Validate, re-encode to WebP, and store - match thumbnail_service + // Validate, re-encode, and store the per-file sidecar. + let stored = match thumbnail_service .store_external_thumbnail(&id, thumb_size.into(), body) .await { - Ok(_) => StatusCode::CREATED.into_response(), - Err(err) => AppError::internal_error(format!("Failed to store thumbnail: {}", err)) - .into_response(), + Ok(bytes) => bytes, + Err(err) => { + return AppError::internal_error(format!("Failed to store thumbnail: {}", err)) + .into_response(); + } + }; + + // Also record it as a file-keyed attachment. + // + // The sidecar above is `ext-{file_id}.jpg` on local disk, which no + // copy path duplicates and no other instance can see. Without this + // row a copied file loses the preview its owner uploaded — falling + // back to a rendered thumbnail, or to nothing at all for a PDF, which + // has no server-side render path. `copy_file_satellites` duplicates + // the row, so the copy inherits the bytes. + // + // File-keyed, never content-keyed: these bytes are the uploader's + // claim about THIS file, and sharing them across files with identical + // content is the poisoning vector `storage.file_attached_blobs` + // exists to prevent. + // + // Best-effort: the sidecar already succeeded, so the user has their + // thumbnail. Failing the request here would report an error for an + // operation that visibly worked. + if let Err(e) = state + .core + .dedup_service + .store_attached_blob( + &id, + "preview", + thumb_size.dir_name(), + "image/jpeg", + stored, + auth_user.id, + ) + .await + { + tracing::warn!( + target: "oxicloud::dedup", + error = %e, + file_id = %id, + "failed to record attached thumbnail; sidecar written, copies will not inherit it" + ); } + + StatusCode::CREATED.into_response() } // ═══════════════════════════════════════════════════════════════════════ diff --git a/tests/api/attached_thumbnail_copy.hurl b/tests/api/attached_thumbnail_copy.hurl new file mode 100644 index 00000000..bf4831b1 --- /dev/null +++ b/tests/api/attached_thumbnail_copy.hurl @@ -0,0 +1,275 @@ +# ============================================================= +# OxiCloud – An UPLOADED thumbnail survives both copy paths +# ============================================================= +# A user-supplied preview is not derivable from the file's content, so +# nothing can regenerate it. If a copy loses it, it is gone — and the loss +# is silent, because the server quietly falls back to rendering one from +# the source (or to 204 for a PDF, which has no render path at all). +# +# That was the behaviour before `storage.file_attached_blobs`: the PUT +# wrote `ext-{file_id}.jpg`, keyed by file id, which no copy path +# duplicates and no other instance can see. +# +# The test distinguishes "preserved" from "re-rendered" by making the two +# visibly different: the FILE is red-image.png, the uploaded thumbnail is +# derived from green-image.png. A server-side render of the file could +# only ever produce the red one. So byte-equality with the post-upload +# bytes proves the copy served the ATTACHMENT, not a fresh render. +# +# Step 4 is what makes that airtight — it captures the rendered thumbnail +# BEFORE the upload and requires the upload to change it. Without that, +# byte-equality across copies could be satisfied by three identical +# renders. +# +# Prerequisites: setup.hurl must have run (admin user exists). +# ============================================================= + + +# ───────────────────────────────────────────────────────────── +# Step 1 – Login +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/api/auth/login +Content-Type: application/json +{ + "username": "{{username}}", + "password": "{{password}}" +} + +HTTP 200 +[Captures] +token: jsonpath "$.access_token" + + +# ───────────────────────────────────────────────────────────── +# Step 2 – Source and destination folders +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/api/folders +Authorization: Bearer {{token}} +Content-Type: application/json +{ + "name": "hurl-attach-src" +} + +HTTP 201 +[Captures] +src_folder_id: jsonpath "$.id" + + +POST {{base_url}}/api/folders +Authorization: Bearer {{token}} +Content-Type: application/json +{ + "name": "hurl-attach-dst" +} + +HTTP 201 +[Captures] +dst_folder_id: jsonpath "$.id" + + +# ───────────────────────────────────────────────────────────── +# Step 3 – Upload the file (RED) +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/api/files/upload +Authorization: Bearer {{token}} +[MultipartFormData] +folder_id: {{src_folder_id}} +file: file,fixtures/red-image.png; image/png + +HTTP 201 +[Captures] +orig_file_id: jsonpath "$.id" +orig_file_name: jsonpath "$.name" + + +# ───────────────────────────────────────────────────────────── +# Step 4 – The server-rendered thumbnail, before any upload. +# Captured so the upload can be shown to have replaced it. +# ───────────────────────────────────────────────────────────── +GET {{base_url}}/api/files/{{orig_file_id}}/thumbnail/preview +Authorization: Bearer {{token}} + +HTTP 200 +[Captures] +rendered_thumb: bytes + + +# ───────────────────────────────────────────────────────────── +# Step 5 – Upload a custom thumbnail (GREEN) for that file +# ───────────────────────────────────────────────────────────── +PUT {{base_url}}/api/files/{{orig_file_id}}/thumbnail/preview +Authorization: Bearer {{token}} +Content-Type: image/png +file,fixtures/green-image.png; + +HTTP 201 + + +# It must now serve the upload, not the render. +GET {{base_url}}/api/files/{{orig_file_id}}/thumbnail/preview +Authorization: Bearer {{token}} + +HTTP 200 +[Captures] +uploaded_thumb: bytes +[Asserts] +bytes != {{rendered_thumb}} + + +# ───────────────────────────────────────────────────────────── +# Step 6 – Single-file copy → the attachment comes with it. +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/api/batch/files/copy +Authorization: Bearer {{token}} +Content-Type: application/json +{ + "file_ids": ["{{orig_file_id}}"], + "target_folder_id": "{{dst_folder_id}}" +} + +HTTP 200 +[Captures] +file_copy_id: jsonpath "$.successful[0].id" +[Asserts] +jsonpath "$.successful[0].id" != "{{orig_file_id}}" + + +GET {{base_url}}/api/files/{{file_copy_id}}/thumbnail/preview +Authorization: Bearer {{token}} + +HTTP 200 +[Asserts] +bytes == {{uploaded_thumb}} +bytes != {{rendered_thumb}} + + +# ───────────────────────────────────────────────────────────── +# Step 7 – Folder copy → same, through storage.copy_folder_tree. +# +# The other copy path. It reaches the attachment through the same +# `copy_file_satellites` call, and this is the leg that would break if +# the tree path ever grew its own fan-out again. +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/api/batch/folders/copy +Authorization: Bearer {{token}} +Content-Type: application/json +{ + "folder_ids": ["{{src_folder_id}}"], + "target_folder_id": "{{dst_folder_id}}" +} + +HTTP 200 +[Captures] +tree_root_id: jsonpath "$.successful[0].new_root_folder_id" +[Asserts] +jsonpath "$.stats.failed" == 0 + + +GET {{base_url}}/api/files?folder_id={{tree_root_id}} +Authorization: Bearer {{token}} + +HTTP 200 +[Captures] +tree_copy_id: jsonpath "$[0].id" +[Asserts] +jsonpath "$" count == 1 +jsonpath "$[0].name" == "{{orig_file_name}}" +jsonpath "$[0].id" != "{{orig_file_id}}" + + +GET {{base_url}}/api/files/{{tree_copy_id}}/thumbnail/preview +Authorization: Bearer {{token}} + +HTTP 200 +[Asserts] +bytes == {{uploaded_thumb}} +bytes != {{rendered_thumb}} + + +# ───────────────────────────────────────────────────────────── +# Step 8 – Delete the ORIGINAL, run GC, and require both copies to keep +# serving the upload. +# +# Each copy holds its own reference on the attached blob — the rows are +# duplicated, not shared, because the table is file-keyed. If the copy +# had failed to take one, deleting the original would walk the count to +# zero and GC would reap bytes that cannot be regenerated. +# ───────────────────────────────────────────────────────────── +DELETE {{base_url}}/api/files/{{orig_file_id}} +Authorization: Bearer {{token}} + +HTTP 204 + + +GET {{base_url}}/api/trash/resources +Authorization: Bearer {{token}} + +HTTP 200 +[Captures] +trash_orig_id: jsonpath "$.items[?(@.resource.id == '{{orig_file_id}}')].resource.id" + + +DELETE {{base_url}}/api/trash/{{trash_orig_id}} +Authorization: Bearer {{token}} + +HTTP 200 + + +POST {{base_url}}/api/admin/jobs/dedup_gc/trigger +Authorization: Bearer {{token}} +[Options] +delay: 500ms + +HTTP 200 + + +GET {{base_url}}/api/files/{{file_copy_id}}/thumbnail/preview +Authorization: Bearer {{token}} + +HTTP 200 +[Asserts] +bytes == {{uploaded_thumb}} + + +GET {{base_url}}/api/files/{{tree_copy_id}}/thumbnail/preview +Authorization: Bearer {{token}} + +HTTP 200 +[Asserts] +bytes == {{uploaded_thumb}} + + +# ───────────────────────────────────────────────────────────── +# Step 9 – Teardown. Hurl files share one database within run.sh. +# ───────────────────────────────────────────────────────────── +DELETE {{base_url}}/api/folders/{{src_folder_id}} +Authorization: Bearer {{token}} + +HTTP 204 + + +DELETE {{base_url}}/api/folders/{{dst_folder_id}} +Authorization: Bearer {{token}} + +HTTP 204 + + +GET {{base_url}}/api/trash/resources +Authorization: Bearer {{token}} + +HTTP 200 +[Captures] +trash_src_id: jsonpath "$.items[?(@.resource.id == '{{src_folder_id}}')].resource.id" +trash_dst_id: jsonpath "$.items[?(@.resource.id == '{{dst_folder_id}}')].resource.id" + + +DELETE {{base_url}}/api/trash/{{trash_src_id}} +Authorization: Bearer {{token}} + +HTTP 200 + + +DELETE {{base_url}}/api/trash/{{trash_dst_id}} +Authorization: Bearer {{token}} + +HTTP 200 diff --git a/tests/api/run.sh b/tests/api/run.sh index d644513b..3d17c303 100755 --- a/tests/api/run.sh +++ b/tests/api/run.sh @@ -168,6 +168,7 @@ hurl --variables-file "$API_DIR/test.env" --file-root "$REPO_ROOT/tests" --test "$API_DIR/dedup_blob_cleanup.hurl" \ "$API_DIR/derived_blob_copy.hurl" \ "$API_DIR/thumbnail_etag_content_keyed.hurl" \ + "$API_DIR/attached_thumbnail_copy.hurl" \ "$API_DIR/dedup_admin_gate.hurl" \ "$API_DIR/admin_jobs.hurl" \ "$API_DIR/recoverable_jobs.hurl" \