From 60b94e1183eca985117cc9209363ed4cd0347d11 Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Sun, 23 Aug 2026 23:10:00 +0200 Subject: [PATCH] feat(thumbnails): serve derived blobs when the sidecar cannot MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Step 5, read path — Option 2 of the two shapes discussed: the derived blob is consulted LAST, after the sidecar, not first. Read order is now moka -> ext-{file_id}.jpg -> {blob_hash}.webp on disk -> derived blob For every thumbnail already on disk the new branch is never reached, so the database stays off the hot path and a fault in it cannot break a working gallery. It answers only what disk cannot: a thumbnail rendered by another instance, or a box whose sidecar was never populated. Legacy content keeps serving from disk until `derived_import` migrates it. That inverts the plan's stated order deliberately. Derived-blob-first is right for the END state, because it is what lets the sidecar be deleted; sidecar-first is right transitionally, because the risky reordering should happen after the table has been seen serving real reads. The flip belongs in the release that removes the sidecar, and the comment at the branch says so. The existing precedence is preserved and now documented: the file-keyed client upload (ext-) is checked BEFORE the content-keyed server render. That ordering is a security property, not a preference — content-keyed artifacts are shared across every file with that content, so checking the file-keyed one first is what keeps one user's uploaded preview from ever being served for another user's identical file. Shape notes: * `find_derived_blob` lands on DedupPort/DedupService as the read counterpart of `store_derived_blob`, so ThumbnailService needs no pool field — and therefore ThumbnailService::new, DI and three tests are untouched. * It carries `content_type`, which is what will retire the byte-sniffing in the handlers once reads are table-primary. * The parameter is `Option<&DedupService>`, concrete rather than `&dyn DedupPort`: DedupPort uses native `async fn` and so is not dyn-compatible, and ThumbnailPort is never used as a trait object (checked) — both handlers hold the concrete Arc. `None` means sidecar-only, which is exactly today's behaviour and what the abstract port impl passes. fmt, clippy --all-features --all-targets, 35 unit tests clean. Co-Authored-By: Claude Opus 5 (1M context) --- src/application/ports/dedup_ports.rs | 21 ++++++++ src/common/stubs.rs | 9 ++++ src/infrastructure/services/dedup_service.rs | 36 ++++++++++++++ .../services/thumbnail_service.rs | 48 +++++++++++++++++-- src/interfaces/api/handlers/file_handler.rs | 17 ++++++- src/interfaces/nextcloud/preview_handler.rs | 1 + 6 files changed, 126 insertions(+), 6 deletions(-) diff --git a/src/application/ports/dedup_ports.rs b/src/application/ports/dedup_ports.rs index 36deed77..046ca45c 100644 --- a/src/application/ports/dedup_ports.rs +++ b/src/application/ports/dedup_ports.rs @@ -24,6 +24,16 @@ pub struct BlobMetadataDto { pub content_type: Option, } +/// A stored server-derived artifact: which blob holds it, and what it is. +/// +/// `content_type` is carried so the read path can set the response header +/// without byte-sniffing the payload, which is what it does today. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct DerivedBlobRef { + pub blob_hash: String, + pub content_type: String, +} + /// Result of a deduplication store operation. #[derive(Debug, Clone)] pub enum DedupResultDto { @@ -83,6 +93,17 @@ pub trait DedupPort: Send + Sync + 'static { /// Check if a blob with the given hash exists. async fn blob_exists(&self, hash: &str) -> bool; + /// Look up a server-derived artifact by the content it was derived from. + /// + /// The read counterpart of `store_derived_blob`. Returns `None` when no + /// such variant has been derived yet — the caller then renders it. + async fn find_derived_blob( + &self, + source_hash: &str, + kind: &str, + variant: &str, + ) -> Option; + /// Get metadata for a blob. async fn get_blob_metadata(&self, hash: &str) -> Option; diff --git a/src/common/stubs.rs b/src/common/stubs.rs index 952874f7..63a0ac32 100644 --- a/src/common/stubs.rs +++ b/src/common/stubs.rs @@ -756,6 +756,15 @@ impl DedupPort for StubDedupPort { false } + async fn find_derived_blob( + &self, + _source_hash: &str, + _kind: &str, + _variant: &str, + ) -> Option { + None + } + async fn get_blob_metadata(&self, _hash: &str) -> Option { None } diff --git a/src/infrastructure/services/dedup_service.rs b/src/infrastructure/services/dedup_service.rs index ae013741..ff9e1191 100644 --- a/src/infrastructure/services/dedup_service.rs +++ b/src/infrastructure/services/dedup_service.rs @@ -625,6 +625,33 @@ impl DedupService { Ok(derived_hash) } + /// Look up a derived artifact by its source content. Read counterpart of + /// [`Self::store_derived_blob`]. + pub async fn find_derived_blob( + &self, + source_hash: &str, + kind: &str, + variant: &str, + ) -> Option { + sqlx::query_as::<_, (String, String)>( + "SELECT blob_hash, content_type FROM storage.content_derived_blobs + WHERE source_hash = $1 AND kind = $2 AND variant = $3", + ) + .bind(source_hash) + .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, + } + }) + } + /// The registry backing the reap predicate. /// /// Exposed so `blobs_consistency` recomputes refcounts from the *same* @@ -3295,6 +3322,15 @@ impl DedupPort for DedupService { self.blob_exists(hash).await } + async fn find_derived_blob( + &self, + source_hash: &str, + kind: &str, + variant: &str, + ) -> Option { + self.find_derived_blob(source_hash, kind, variant).await + } + async fn get_blob_metadata(&self, hash: &str) -> Option { self.get_blob_metadata(hash).await } diff --git a/src/infrastructure/services/thumbnail_service.rs b/src/infrastructure/services/thumbnail_service.rs index 1a6c0ced..52fee4af 100644 --- a/src/infrastructure/services/thumbnail_service.rs +++ b/src/infrastructure/services/thumbnail_service.rs @@ -478,6 +478,11 @@ impl ThumbnailService { blob_hash: Option<&str>, size: ThumbnailSize, format: ThumbnailFormat, + // Concrete, and optional: `ThumbnailPort` is never used as a trait + // object (checked), and `DedupPort` uses native `async fn` so it is + // not dyn-compatible anyway. `None` means sidecar-only — exactly + // today's behaviour, which is what the port impl wants. + dedup: Option<&DedupService>, ) -> Option { // 1. Check in-memory cache let cache_key = ThumbnailCacheKey { @@ -520,10 +525,42 @@ impl ThumbnailService { let bytes = Bytes::from(data); // Populate in-memory cache for next hit self.cache.insert(cache_key, bytes.clone()).await; - Some(bytes) - } else { - None + return Some(bytes); } + + // 4. Tier-3 derived blob. Deliberately LAST while the sidecar still + // exists: for every thumbnail already on disk this branch is never + // reached, so the DB stays off the hot path and a fault here cannot + // break a working gallery. It answers only what disk cannot — another + // instance's render, or a box whose sidecar was never populated. + // + // The order flips (derived blob first, sidecar as fallback) in the + // release that removes the sidecar; see docs/plan/derived-blobs.md. + let dedup = dedup?; + 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); + self.cache.insert(cache_key, bytes.clone()).await; + Some(bytes) } /// Store an externally-generated thumbnail (e.g. client-side video frame). @@ -1605,7 +1642,10 @@ impl ThumbnailPort for ThumbnailService { blob_hash: Option<&str>, size: PortThumbnailSize, ) -> Option { - self.get_cached_thumbnail(file_id, blob_hash, size.into(), ThumbnailFormat::Webp) + // `None` — the abstract port has no DedupService handle, so it stays + // sidecar-only. Callers wanting the tier-3 fallback use the concrete + // method, which both handlers already do. + self.get_cached_thumbnail(file_id, blob_hash, size.into(), ThumbnailFormat::Webp, None) .await } diff --git a/src/interfaces/api/handlers/file_handler.rs b/src/interfaces/api/handlers/file_handler.rs index f3cc381e..58b576f0 100644 --- a/src/interfaces/api/handlers/file_handler.rs +++ b/src/interfaces/api/handlers/file_handler.rs @@ -484,7 +484,13 @@ impl FileHandler { // Try moka (RAM) → disk before touching the database. // If the thumbnail exists it was authorized at creation time. if let Some(data) = thumbnail_service - .get_cached_thumbnail(&id, None, thumb_size.into(), format) + .get_cached_thumbnail( + &id, + None, + thumb_size.into(), + format, + Some(&state.core.dedup_service), + ) .await { return Response::builder() @@ -541,7 +547,13 @@ impl FileHandler { } }; if let Some(data) = thumbnail_service - .get_cached_thumbnail(&id, Some(&blob_hash), thumb_size.into(), format) + .get_cached_thumbnail( + &id, + Some(&blob_hash), + thumb_size.into(), + format, + Some(&state.core.dedup_service), + ) .await { return Response::builder() @@ -572,6 +584,7 @@ impl FileHandler { Some(&blob_hash), thumb_size.into(), ThumbnailFormat::Webp, + Some(&state.core.dedup_service), ) .await { diff --git a/src/interfaces/nextcloud/preview_handler.rs b/src/interfaces/nextcloud/preview_handler.rs index c137112b..8a32a0d6 100644 --- a/src/interfaces/nextcloud/preview_handler.rs +++ b/src/interfaces/nextcloud/preview_handler.rs @@ -204,6 +204,7 @@ pub async fn handle_preview( Some(&blob_hash), thumb_size.into(), ThumbnailFormat::Jpeg, + Some(&state.core.dedup_service), ) .await {