From 260e6bb74f5c67c11a46a8585aa9741e5d97b8a2 Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Wed, 26 Aug 2026 22:51:30 +0200 Subject: [PATCH] feat(thumbnails): read the derived tier ahead of the sidecar MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Step 10c. The sidecar is local disk — invisible to other instances, uncarried by a backend migration, uncovered by any consistency job. Reading the derived tier first is what makes that state deletable. Not the cost it appears to be: CachedBlobBackend gives the blob read a local disk cache and moka absorbs the repeats above it. Not a two-line swap, for two reasons. A derived MISS must fall through to the sidecar; the old code terminated the lookup with `?` because it was last. While the imports drain, most content has a sidecar and no row — terminating there would report "no thumbnail" for nearly all of it. And the derived tier is WebP-only. store_derived_blob writes image/webp and keys `variant` on the size alone, with no format term, so a JPEG request matches the WebP row and would be served the wrong codec. The old ordering hid this because the .jpg sidecar won first. So the lookup is gated to WebP, JPEG clients stay on the sidecar — and the sidecar cannot be deleted for them until `variant` encodes format. That is a new prerequisite for step 10e, recorded in the plan rather than discovered later. Also corrects the plan: I had written that this flip removes the derived-hash ETag hazard. It does not. A first render still creates the row as a side effect of producing the body, whatever the read order, so two consecutive reads still straddle its appearance. The real fix is resolving the ETag after generation on the 200 path — a 304 only fires when the client already holds a validator, which implies the row exists. That is a handler restructure, not an ordering change. --- docs/plan/derived-blobs.md | 37 ++++++++-- .../services/thumbnail_service.rs | 69 ++++++++++++------- 2 files changed, 74 insertions(+), 32 deletions(-) diff --git a/docs/plan/derived-blobs.md b/docs/plan/derived-blobs.md index 6e460d74..55c201fc 100644 --- a/docs/plan/derived-blobs.md +++ b/docs/plan/derived-blobs.md @@ -1003,9 +1003,19 @@ first render was immediately stale. Caught by `thumbnail_etag_content_keyed.hurl`, where two consecutive GETs of an unchanged file stopped revalidating to 304. -The flip is what removes the hazard: once the derived tier is -authoritative it is populated before it is consulted, so there is no -window in which the row appears between two reads. +**The flip alone does NOT remove the hazard** *(corrected 2026-08-26 — +an earlier revision of this paragraph claimed it did)*. A first render +still creates the row as a side effect of producing the body, whatever +the read order, so two consecutive reads would still straddle its +appearance. + +What actually removes it is resolving the ETag **after** generation on +the 200 path. A 304 can only fire when the client already holds a +validator, which means it has been served before, which means the row +exists — so the *conditional* path can safely consult the derived hash +up front, while the *generating* path computes it from bytes it now +holds. That is a handler restructure, not an ordering change, and it is +the actual prerequisite for the derived-hash ETag. **The disk cache is `CachedBlobBackend`, reused unchanged.** No thumbnail-specific cache, no second root path. Routing derived @@ -1310,10 +1320,23 @@ hardcoded SQL). New sources bolt on independently. **re-keyed** file→content on import (legitimate only because a transcode is derivable). Its `.skip` markers — a cached negative verdict with no bytes — remain an open question. - c. **Flip the read order**, derived first. The HTTP ETag's - source-keyed fallback becomes unreachable here; the attached and - derived halves already landed early, forced by the attachment - case (see *HTTP ETag*). + c. **Flip the read order**, derived first — **done 2026-08-26**. Two + things it is not: a two-line swap, and an ETag fix. + + A derived miss must **fall through** to the sidecar, where the old + code terminated the lookup — while the imports drain, most content + has a sidecar and no row, so terminating would report "no + thumbnail" for nearly everything. + + And it is **WebP-only**. `store_derived_blob` writes `image/webp` + with `variant` keyed on size alone, no format term, so a JPEG + request matches the WebP row and gets the wrong codec — a + regression the old ordering hid, because the `.jpg` sidecar won + first. JPEG clients therefore stay on the sidecar, **and the + sidecar cannot be deleted for them** until `variant` encodes + format. That is a new prerequisite for (e), not a detail: it means + a migration to `(kind, variant, format)` — or a format term inside + `variant` — has to land before the directories can go. d. **Enable deletion** in the import jobs (opt-in, readback-verified). e. **Remove the fallback read path** once the directory no longer *exists* — not merely once it is empty. Two reasons. Empty is a diff --git a/src/infrastructure/services/thumbnail_service.rs b/src/infrastructure/services/thumbnail_service.rs index 85a5a861..6794f794 100644 --- a/src/infrastructure/services/thumbnail_service.rs +++ b/src/infrastructure/services/thumbnail_service.rs @@ -792,11 +792,35 @@ impl ThumbnailService { 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); - // Populate in-memory cache for next hit + // 4. Derived blob — the authoritative content tier (step 10c). + // + // Ahead of the sidecar now, rather than last. The sidecar is local + // disk: invisible to other instances, uncarried by a backend + // migration, uncovered by any consistency job. Reading the derived + // tier first is what lets that disk state become deletable, and it is + // not the cost it looks like — `CachedBlobBackend` gives the blob read + // a local disk cache, and moka absorbs the repeats above it. + // + // A miss FALLS THROUGH rather than ending the lookup. That is the + // whole reason this is not a two-line swap: while the imports are + // draining, most content has a sidecar and no row, and terminating + // here would return "no thumbnail" for all of it. + // + // **WebP only.** `store_derived_blob` writes `image/webp` and keys + // `variant` on the size alone, with no format term, so a JPEG request + // would match the WebP row and be served the wrong codec — a + // regression the old ordering hid, because the `.jpg` sidecar won + // first. Until `variant` encodes format, JPEG clients stay on the + // sidecar, and the sidecar therefore cannot be deleted for them. See + // docs/plan/derived-blobs.md. + if format == ThumbnailFormat::Webp + && let Some(dedup) = dedup + && let Some(derived) = dedup + .find_derived_blob(hash, "thumbnail", size.dir_name()) + .await + && let Some(bytes) = + Self::read_blob_to_bytes(dedup, &derived.blob_hash, file_id, size).await + { self.cache .insert( ThumbnailCacheKey::content(hash, size, format), @@ -806,26 +830,21 @@ impl ThumbnailService { 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?; - let bytes = Self::read_blob_to_bytes(dedup, &derived.blob_hash, file_id, size).await?; - self.cache - .insert( - ThumbnailCacheKey::content(hash, size, format), - bytes.clone(), - ) - .await; - Some(bytes) + // 5. Blob-hash sidecar — fallback for content not yet imported, and + // the only content tier a non-WebP request can reach. + let thumb_path = self.get_thumbnail_path(hash, size, format); + if let Ok(data) = fs::read(&thumb_path).await { + let bytes = Bytes::from(data); + self.cache + .insert( + ThumbnailCacheKey::content(hash, size, format), + bytes.clone(), + ) + .await; + return Some(bytes); + } + + None } /// Store an externally-generated thumbnail (e.g. client-side video frame).