From d71dd973e7b834ecf25b3139c47fb8deee879069 Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Tue, 25 Aug 2026 08:13:51 +0200 Subject: [PATCH] fix(thumbnails): two defects the copy test exposed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. Per-file overrides must beat the content tier in RAM as well as on disk. The content-keyed lookup ran first, so a thumbnail already rendered from the file's content sat in RAM under content(blob_hash) and shadowed a preview uploaded afterwards — permanently. Invisible before the moka rekey, because both lived under one file-id key and the upload simply overwrote the render. Order is now uniform: per-file RAM, per-file disk (ext-), per-file DB, then content RAM, blob-hash disk, derived blob. 2. store_attached_blob never wrote a row. Its RETURNING clause compared the stored hash against EXCLUDED, and PostgreSQL only permits EXCLUDED in the SET and WHERE of DO UPDATE — a runtime syntax error on every call. The superseded hash now comes from a SELECT taken before the upsert; losing that race leaves one stale reference, which the manifest recompute reports, rather than anything being lost. The second hid behind the first for a whole cycle, and behind `ext-{file_id}.jpg`: the ORIGINAL kept serving its uploaded preview from local disk, so the feature looked healthy. Only a copy, which has a different file_id and therefore no ext- file, depends on the row — and the row was never there. The handler's best-effort warn! completed the disguise, so it is now error!: a failure there means copies silently lose the preview, and nothing else signals it. --- src/infrastructure/services/dedup_service.rs | 34 ++++++++++---- .../services/thumbnail_service.rs | 47 ++++++++++++++----- src/interfaces/api/handlers/file_handler.rs | 8 +++- 3 files changed, 66 insertions(+), 23 deletions(-) diff --git a/src/infrastructure/services/dedup_service.rs b/src/infrastructure/services/dedup_service.rs index c0d1d15f..62684f30 100644 --- a/src/infrastructure/services/dedup_service.rs +++ b/src/infrastructure/services/dedup_service.rs @@ -607,10 +607,29 @@ impl DedupService { .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( + // Read the hash being superseded BEFORE upserting. + // + // It cannot come from `RETURNING`: PostgreSQL only permits `EXCLUDED` + // in the `SET` and `WHERE` of `DO UPDATE`, so a RETURNING clause + // comparing old against new is a syntax error — and one that surfaces + // only at runtime, where this method's best-effort caller swallows it + // into a warning while the sidecar keeps the feature looking healthy. + // + // The gap between this SELECT and the upsert is benign: losing the + // race leaves one stale reference, which the manifest recompute + // reports rather than anything being lost or served wrongly. + let previous: Option<(String,)> = sqlx::query_as( + "SELECT blob_hash 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 + .map_err(|e| DomainError::internal_error("Dedup", format!("read attached blob: {e}")))?; + + sqlx::query( "INSERT INTO storage.file_attached_blobs (file_id, kind, variant, blob_hash, content_type, uploaded_by) VALUES ($1::uuid, $2, $3, $4, $5, $6) @@ -618,8 +637,7 @@ impl DedupService { 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)", + created_at = now()", ) .bind(file_id) .bind(kind) @@ -627,14 +645,14 @@ impl DedupService { .bind(&attached_hash) .bind(content_type) .bind(uploaded_by) - .fetch_optional(self.pool.as_ref()) + .execute(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 + if let Some((old_hash,)) = previous && old_hash != attached_hash && let Err(e) = self.remove_reference(&old_hash).await { diff --git a/src/infrastructure/services/thumbnail_service.rs b/src/infrastructure/services/thumbnail_service.rs index 92f91c07..4e76938a 100644 --- a/src/infrastructure/services/thumbnail_service.rs +++ b/src/infrastructure/services/thumbnail_service.rs @@ -550,19 +550,22 @@ impl ThumbnailService { // today's behaviour, which is what the port impl wants. dedup: Option<&DedupService>, ) -> Option { - // 1. Check in-memory cache. + // A file-specific override beats anything derived from the content, + // and that has to hold at EVERY tier — including RAM. Checking the + // content-keyed entry first would let a previously-rendered + // thumbnail shadow a preview the user has since uploaded: the render + // is cached under `content(hash)`, the upload lands under + // `external(file_id)`, and the content key would win forever. // - // Keyed by content, so a caller that did not resolve the hash cannot - // consult this tier — it falls through to disk, which is correct - // rather than merely acceptable: a file-id key would be the stale - // entry the content key exists to avoid. Every HTTP path passes the - // hash (both handlers resolve it to build the ETag), so the fall - // through is confined to internal callers that never had one. - if let Some(hash) = blob_hash - && let Some(bytes) = self - .cache - .get(&ThumbnailCacheKey::content(hash, size, format)) - .await + // So the order is: per-file RAM, per-file disk, per-file DB, then the + // content-keyed tiers. Same precedence as the disk tiers below, just + // applied one level up. + + // 1. Per-file override in RAM (uploaded preview / video frame). + if let Some(bytes) = self + .cache + .get(&ThumbnailCacheKey::external(file_id, size)) + .await && !bytes.is_empty() { return Some(bytes); @@ -612,8 +615,26 @@ impl ThumbnailService { return Some(bytes); } - // 3. Check disk for blob-hash thumbnails (needs blob_hash to locate) + // 3. Content-keyed RAM tier. Below the per-file tiers by the rule + // above; still ahead of every disk read. + // + // A caller that did not resolve the hash cannot consult it and + // falls through to disk. That is correct rather than merely + // acceptable: a file-id key here would be the stale entry content + // keying exists to avoid. Both HTTP handlers resolve the hash to + // build the ETag, so the fall-through is confined to internal + // callers that never had one. let hash = blob_hash?; + if let Some(bytes) = self + .cache + .get(&ThumbnailCacheKey::content(hash, size, format)) + .await + && !bytes.is_empty() + { + return Some(bytes); + } + + // 4. Check disk for blob-hash thumbnails (needs blob_hash to locate) let thumb_path = self.get_thumbnail_path(hash, size, format); if let Ok(data) = fs::read(&thumb_path).await { let bytes = Bytes::from(data); diff --git a/src/interfaces/api/handlers/file_handler.rs b/src/interfaces/api/handlers/file_handler.rs index 7fdf95be..6fbc0845 100644 --- a/src/interfaces/api/handlers/file_handler.rs +++ b/src/interfaces/api/handlers/file_handler.rs @@ -768,11 +768,15 @@ impl FileHandler { ) .await { - tracing::warn!( + // ERROR, not WARN: the sidecar keeps the feature looking healthy + // on this box, so nothing else signals that copies are silently + // losing the preview. A syntax error in the upsert hid behind a + // warning for an entire test cycle exactly this way. + tracing::error!( target: "oxicloud::dedup", error = %e, file_id = %id, - "failed to record attached thumbnail; sidecar written, copies will not inherit it" + "failed to record attached thumbnail; sidecar written, copies will NOT inherit it" ); }