From 7d9418f63cb87371cfe2b9cf8cb6b6ec7a39a4cf Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Tue, 25 Aug 2026 21:28:34 +0200 Subject: [PATCH] fix(thumbnails): ETag names the blob actually served MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit fe9c4f49 keyed the ETag on the SOURCE file's content hash. That is wrong whenever the response comes from a satellite table, and for attachments it is wrong in two ways. Uploading a preview does not change the file's content, so a source-keyed ETag does not change either — and with `immutable` set, clients never revalidate and keep the previous render for up to a year. The exact staleness fe9c4f49 set out to fix, re-entering through the attachment path. Worse: a copy inherits the source hash, so an original and a copy have identical ETags. Give either one a different uploaded preview and they serve different bytes under one validator, which a shared cache may hand to either request. That is a collision, not just staleness. thumbnail_content_id resolves the identity through the same tier precedence the read path uses: an attached blob's own hash, else a derived blob's own hash, else the source-keyed form. An ETag naming a different tier than the one answering is worse than a coarse one, so the two orders must not drift. Derived-hash keying is strictly better than source-keying and never worse. The sidecar and the derived row are written from the same bytes; where they can diverge — a sidecar re-rendered while the derived row stays pinned by ON CONFLICT DO NOTHING — source-keying is wrong too, because the renderer is not part of that key. This is the step 10 change arriving early, forced by the attachment case; the plan note stands for the read-order flip itself. Known gap: a legacy ext-{file_id}.jpg with no file_attached_blobs row yet falls through to the source-keyed form. No worse than today, and it resolves when the import backfills. attached_thumbnail_copy.hurl now asserts ETags, which is why this went unnoticed: it compared bytes only, and thumbnail_etag_content_keyed covers content replacement rather than preview upload. A fresh GET returned the right bytes throughout — the same "healthy locally, broken for anyone caching" shape as the two bugs before it. --- .../services/thumbnail_service.rs | 56 +++++++++++++++++++ src/interfaces/api/handlers/file_handler.rs | 28 ++++++---- src/interfaces/nextcloud/preview_handler.rs | 27 +++++---- tests/api/attached_thumbnail_copy.hurl | 33 +++++++++++ 4 files changed, 122 insertions(+), 22 deletions(-) diff --git a/src/infrastructure/services/thumbnail_service.rs b/src/infrastructure/services/thumbnail_service.rs index 4e76938a..dee210e0 100644 --- a/src/infrastructure/services/thumbnail_service.rs +++ b/src/infrastructure/services/thumbnail_service.rs @@ -505,6 +505,62 @@ 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). + /// Identity of the bytes a thumbnail request will serve — the body of its + /// HTTP ETag. + /// + /// Mirrors the tier precedence in [`Self::get_cached_thumbnail`], because + /// an ETag that names a different tier than the one answering is worse + /// than a coarse one: it lets two resources serving different bytes share + /// a validator, and a shared cache may then hand either to either. + /// + /// * An **attached** blob wins, and its own hash is the identity. Nothing + /// else works: uploading a preview does not change the file's content, + /// so a source-keyed ETag would not change either — and with + /// `immutable` set, clients would never revalidate. Worse, a copy + /// inherits the source hash, so an original and a copy carrying + /// *different* uploaded previews would collide on one ETag. + /// * Otherwise a **derived** blob's own hash. Strictly better than + /// source-keying, never worse: the pair is written from the same bytes, + /// and where they can diverge — a sidecar re-rendered while the derived + /// row stays pinned by `ON CONFLICT DO NOTHING` — a source-keyed ETag + /// is wrong too, because the renderer is not part of the key. + /// * Otherwise the source-keyed form, which still identifies a render of + /// known content at a known size and format. + /// + /// Known gap: a legacy `ext-{file_id}.jpg` with no `file_attached_blobs` + /// row yet falls through to the source-keyed form, so those bytes keep + /// today's coarse validator until the import backfills the row. No worse + /// than current behaviour, and it disappears with the migration. + pub async fn thumbnail_content_id( + &self, + file_id: &str, + blob_hash: &str, + size: ThumbnailSize, + format: ThumbnailFormat, + dedup: Option<&DedupService>, + ) -> String { + if let Some(dedup) = dedup { + if let Some(attached) = dedup + .find_attached_blob(file_id, "preview", size.dir_name()) + .await + { + return attached.blob_hash; + } + if let Some(derived) = dedup + .find_derived_blob(blob_hash, "thumbnail", size.dir_name()) + .await + { + return derived.blob_hash; + } + } + format!( + "thumb-{}-{}-{}", + blob_hash, + size.dir_name(), + format.as_str() + ) + } + /// Drain a blob through the dedup stack into memory. /// /// Shared by the attached and derived tiers — the only difference between diff --git a/src/interfaces/api/handlers/file_handler.rs b/src/interfaces/api/handlers/file_handler.rs index 6fbc0845..9413fe19 100644 --- a/src/interfaces/api/handlers/file_handler.rs +++ b/src/interfaces/api/handlers/file_handler.rs @@ -487,18 +487,22 @@ impl FileHandler { Ok(h) => h, Err(err) => return AppError::from(err).into_response(), }; - let etag = { - let (s, f) = (thumb_size.as_str(), format.as_str()); - let mut e = String::with_capacity(9 + blob_hash.len() + s.len() + f.len()); - e.push_str("\"thumb-"); - e.push_str(&blob_hash); - e.push('-'); - e.push_str(s); - e.push('-'); - e.push_str(f); - e.push('"'); - e - }; + // The identity of the bytes about to be served, resolved through the + // same tier precedence the read path uses — an uploaded preview's own + // hash, else a derived thumbnail's own hash, else the source-keyed + // form. See `ThumbnailService::thumbnail_content_id`. + let etag = format!( + "\"{}\"", + thumbnail_service + .thumbnail_content_id( + &id, + &blob_hash, + thumb_size.into(), + format, + Some(&state.core.dedup_service), + ) + .await + ); if let Some(if_none_match) = headers.get(header::IF_NONE_MATCH) && let Ok(val) = if_none_match.to_str() && (val == etag || val == "*") diff --git a/src/interfaces/nextcloud/preview_handler.rs b/src/interfaces/nextcloud/preview_handler.rs index 385501b8..6c169fe7 100644 --- a/src/interfaces/nextcloud/preview_handler.rs +++ b/src/interfaces/nextcloud/preview_handler.rs @@ -166,16 +166,23 @@ pub async fn handle_preview( .unwrap(); } }; - let etag = { - let s = thumb_size.as_str(); - let mut e = String::with_capacity(9 + blob_hash.len() + s.len()); - e.push_str("\"thumb-"); - e.push_str(&blob_hash); - e.push('-'); - e.push_str(s); - e.push('"'); - e - }; + // Same tier-precedence resolution as the REST endpoint: an uploaded + // preview's own hash, else a derived thumbnail's own hash, else the + // source-keyed form. NC pins JPEG, so that is the format asked for. + let etag = format!( + "\"{}\"", + state + .core + .thumbnail_service + .thumbnail_content_id( + &object_id, + &blob_hash, + thumb_size.into(), + ThumbnailFormat::Jpeg, + Some(&state.core.dedup_service), + ) + .await + ); if let Some(inm) = req.headers().get(header::IF_NONE_MATCH) && let Ok(client_etag) = inm.to_str() && (client_etag == etag || client_etag == "*") diff --git a/tests/api/attached_thumbnail_copy.hurl b/tests/api/attached_thumbnail_copy.hurl index bf4831b1..a14683e5 100644 --- a/tests/api/attached_thumbnail_copy.hurl +++ b/tests/api/attached_thumbnail_copy.hurl @@ -92,6 +92,7 @@ Authorization: Bearer {{token}} HTTP 200 [Captures] rendered_thumb: bytes +rendered_etag: header "ETag" # ───────────────────────────────────────────────────────────── @@ -112,8 +113,33 @@ Authorization: Bearer {{token}} HTTP 200 [Captures] uploaded_thumb: bytes +uploaded_etag: header "ETag" [Asserts] bytes != {{rendered_thumb}} +# The ETag must move with the bytes. It is keyed on the ATTACHED blob's own +# hash, because uploading a preview leaves the file's content — and so a +# source-keyed ETag — unchanged. With `immutable` set, an unchanged +# validator means clients never revalidate and keep the old render for a +# year. +header "ETag" != "{{rendered_etag}}" + + +# A client holding the pre-upload validator must be told to refetch. +GET {{base_url}}/api/files/{{orig_file_id}}/thumbnail/preview +Authorization: Bearer {{token}} +If-None-Match: {{rendered_etag}} + +HTTP 200 +[Asserts] +header "ETag" == "{{uploaded_etag}}" + + +# ...and the new one revalidates. +GET {{base_url}}/api/files/{{orig_file_id}}/thumbnail/preview +Authorization: Bearer {{token}} +If-None-Match: {{uploaded_etag}} + +HTTP 304 # ───────────────────────────────────────────────────────────── @@ -141,6 +167,12 @@ HTTP 200 [Asserts] bytes == {{uploaded_thumb}} bytes != {{rendered_thumb}} +# Same bytes, so the same validator — the copy's attachment row points at +# the same blob. This is also what stops the collision a source-keyed ETag +# would allow: the copy inherits the source hash, so if either side later +# gets a DIFFERENT preview the two would serve different bytes under one +# ETag, and a shared cache could hand either to either. +header "ETag" == "{{uploaded_etag}}" # ───────────────────────────────────────────────────────────── @@ -184,6 +216,7 @@ HTTP 200 [Asserts] bytes == {{uploaded_thumb}} bytes != {{rendered_thumb}} +header "ETag" == "{{uploaded_etag}}" # ─────────────────────────────────────────────────────────────