From 03246305f6c17bb839cc308d522f4dfbfd118a8e Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Sat, 29 Aug 2026 02:21:16 +0200 Subject: [PATCH] feat(thumbnails): the sidecar fallback disables itself MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Step 10e was written as a removal release: delete the fallback read path once the directories are empty. That has the same flaw as gating deletion on an empty tail, one level up — sidecars are local disk, so no release can know that every instance has drained. The only removal that can actually be written is "if the tier is gone, return". `initialize` now probes the size directories once at boot; when absent, every fallback read short-circuits on a relaxed atomic load and touches no filesystem. The code stays, costs nothing, and can be deleted whenever — or never. Two things had to change for absence to be reachable at all: * `initialize` no longer creates the directories. It create_dir_all-ed all three at every boot, so the import job removed them and the next restart put them back — the absence this gates on was unreachable by construction. Found on a sandbox where the job had drained the tier and a restart left three empty directories behind. Nothing has written a sidecar since step 10d2, so there was nothing to create them for. * The probe tests the size directories, not the root. On macOS Finder leaves a .DS_Store in the root, which blocks remove_dir there permanently; gating on the root would keep the fallback alive on every developer machine for a reason unrelated to thumbnails. No size directory means no sidecar. Every sidecar read and existence check now goes through `read_sidecar` / `sidecar_exists`, so the guard exists once rather than at each of the twelve sites that built a path and read it — the build-then-read pair was duplicated six times over. The import job's root removal reports its outcome instead of discarding it. It is the one result an operator is waiting for, and "directory not empty" with no sidecars left is a failure worth naming. Falls open: the flag starts true, so a service constructed without `initialize` behaves as before. A drain completing mid-process leaves it stale-true until restart, which costs the same failed opens as today; it never goes false while sidecars remain. Co-Authored-By: Claude Opus 5 (1M context) --- docs/plan/derived-blobs.md | 33 ++- .../services/thumb_derived_import_service.rs | 27 ++- .../services/thumbnail_service.rs | 197 +++++++++++++++--- 3 files changed, 226 insertions(+), 31 deletions(-) diff --git a/docs/plan/derived-blobs.md b/docs/plan/derived-blobs.md index d959d663..d3133f94 100644 --- a/docs/plan/derived-blobs.md +++ b/docs/plan/derived-blobs.md @@ -1299,8 +1299,37 @@ Register it as a **scheduled tick**, not a boot-time trigger: it is idempotent and resumable, so periodic is safe, whereas walking a large `.thumbnails/` during startup delays readiness for nothing. -The only remaining *release* is removing the fallback read path once the -directories are empty — by which point no data is at stake. +**Phase 4: the fallback disables itself.** *(revised 2026-08-29 — +supersedes "the only remaining release is removing the fallback read +path")* + +Removing the fallback in a release has the same flaw as gating deletion +on an empty tail, one level up: sidecars are local disk, so no release +can know that every instance has drained. Ed's framing is the answer — +the only removal you can actually write is `if the tier is gone, return`. + +So `ThumbnailService::initialize` probes the size directories once at +boot and stores the result. When absent, every fallback read +short-circuits on a relaxed atomic load, no syscall. The code stays, +costs nothing, and can be deleted whenever — or never. No coordination, +no named release. + +Two things had to change for absence to be reachable at all: + +- **`initialize` no longer creates the directories.** It + `create_dir_all`-ed all three at every boot, so the job removed them + and the next restart put them back; the absence this gates on was + unreachable by construction. Nothing has written a sidecar since step + 10d2, so there was nothing to create them for. +- **The probe tests the size directories, not the root.** On macOS + Finder leaves a `.DS_Store` in the root, which blocks `remove_dir` + there permanently. Gating on the root would keep the fallback alive on + every developer machine for a reason unrelated to thumbnails. No size + directory means no sidecar. + +The root removal now reports its outcome instead of discarding it — +it is the one result an operator is waiting for, and `.DS_Store` is a +failure worth naming rather than a silent no-op. ### Prerequisite: one persist function (found 2026-08-26) diff --git a/src/infrastructure/services/thumb_derived_import_service.rs b/src/infrastructure/services/thumb_derived_import_service.rs index f18fc7e2..596d7f01 100644 --- a/src/infrastructure/services/thumb_derived_import_service.rs +++ b/src/infrastructure/services/thumb_derived_import_service.rs @@ -577,7 +577,32 @@ impl RecoverableJobHandler for ThumbDerivedImport { let dir = self.thumbnails_root.join(size.dir_name()); let _ = fs::remove_dir(&dir).await; } - let _ = fs::remove_dir(&self.thumbnails_root).await; + // Report the root, rather than discarding the result as the size + // directories do. This is the one outcome an operator is waiting + // for — absence is what makes the fallback inert — and it fails + // for a reason worth naming: on macOS Finder leaves a `.DS_Store` + // in the root, so `remove_dir` refuses forever while every + // sidecar underneath is long gone. + match fs::remove_dir(&self.thumbnails_root).await { + Ok(()) => tracing::info!( + target: "oxicloud::dedup", + event = "thumb_derived_import.root_removed", + run_id = %store.run_id(), + path = %self.thumbnails_root.display(), + "🧹 legacy sidecar directory removed — the fallback read path \ + is inert from the next restart" + ), + Err(e) => tracing::info!( + target: "oxicloud::dedup", + event = "thumb_derived_import.root_kept", + run_id = %store.run_id(), + path = %self.thumbnails_root.display(), + reason = %e, + "legacy sidecar directory not removed; if this says \ + 'directory not empty' with no sidecars left, something \ + else put a file there (a .DS_Store, typically)" + ), + } } tracing::info!( diff --git a/src/infrastructure/services/thumbnail_service.rs b/src/infrastructure/services/thumbnail_service.rs index 72b45e97..018bfafd 100644 --- a/src/infrastructure/services/thumbnail_service.rs +++ b/src/infrastructure/services/thumbnail_service.rs @@ -17,6 +17,7 @@ use rayon::prelude::*; */ use std::path::{Path, PathBuf}; use std::sync::Arc; +use std::sync::atomic::{AtomicBool, Ordering}; use std::time::Duration; use tokio::fs; use tokio::sync::Semaphore; @@ -199,6 +200,15 @@ pub struct ThumbnailService { /// Timeout for thumbnail generation operations to prevent hanging on large images. /// Defaults to 30 seconds. generation_timeout: Duration, + /// Whether the legacy sidecar tier still exists on disk, probed once by + /// [`Self::initialize`]. `false` short-circuits every fallback read + /// without a syscall. + /// + /// Starts `true` so a service used without `initialize()` (tests, and any + /// future construction path) keeps the old behaviour: fall back and let + /// the open fail. Failing open is the safe direction — the wrong value + /// costs syscalls, the opposite would hide sidecars that are still there. + legacy_sidecars: AtomicBool, } impl ThumbnailService { @@ -239,22 +249,95 @@ impl ThumbnailService { max_cache_bytes: max_cache_bytes as u64, decode_semaphore: Arc::new(Semaphore::new(max_concurrent_decodes())), generation_timeout: generation_timeout.unwrap_or(Duration::from_secs(30)), + legacy_sidecars: AtomicBool::new(true), } } - /// Initialize the thumbnail directories + /// Probe the legacy sidecar tier and record whether it still holds + /// anything. + /// + /// **This no longer creates the directories.** It used to `create_dir_all` + /// every size directory at boot, which silently undid the migration: the + /// import job removes them once drained, the next restart put them back, + /// and the absence step 10e gates on could never be reached. Nothing has + /// written a sidecar since step 10d2, so there is nothing to create them + /// for. + /// + /// The probe tests the SIZE directories, not the root. On macOS Finder + /// drops a `.DS_Store` in the root, which blocks `remove_dir` there + /// forever — gating on the root would keep the fallback alive on every + /// developer machine for a reason that has nothing to do with thumbnails. + /// If no size directory exists, no sidecar can exist. + /// + /// Result is cached for the process lifetime. It can only be stale in the + /// harmless direction: a drain completing mid-life leaves the flag `true` + /// until restart, which costs the same failed opens as today. It never + /// goes `false` while sidecars remain. pub async fn initialize(&self) -> std::io::Result<()> { + let mut present = false; for size in ThumbnailSize::all() { - let dir = self.thumbnails_root.join(size.dir_name()); - fs::create_dir_all(&dir).await?; + if fs::metadata(self.thumbnails_root.join(size.dir_name())) + .await + .is_ok() + { + present = true; + break; + } + } + self.legacy_sidecars.store(present, Ordering::Relaxed); + + // Asymmetric on purpose. "Present" is actionable and temporary — it + // names the two jobs that clear it and stops appearing once they + // have. "Absent" is the steady state of every drained deployment + // forever, so at info it would be pure boot noise. + if present { + tracing::info!( + target: "oxicloud::thumbnails", + event = "thumbnail.legacy_tier_present", + root = ?self.thumbnails_root, + "🖼️ legacy sidecar tier present — reads fall back to it. Run \ + thumb_derived_import and thumb_attached_import with ?repair=true \ + to drain it." + ); + } else { + tracing::debug!( + target: "oxicloud::thumbnails", + event = "thumbnail.legacy_tier_absent", + root = ?self.thumbnails_root, + "🖼️ no legacy sidecar tier — fallback reads are skipped entirely" + ); } - tracing::info!( - "🖼️ Thumbnail service initialized at {:?}", - self.thumbnails_root - ); Ok(()) } + /// Whether the legacy sidecar tier is worth touching at all. + /// + /// This is the whole of step 10e. Removing the fallback in a release was + /// never workable: sidecars are local disk, so no release can know that + /// every instance has drained. Making the path self-disabling costs one + /// relaxed atomic load and needs no coordination — once a deployment has + /// drained, the code is inert and can be deleted whenever, or never. + fn legacy_tier_active(&self) -> bool { + self.legacy_sidecars.load(Ordering::Relaxed) + } + + /// Read a legacy sidecar, or `None` when the tier is inert. + /// + /// Every sidecar read goes through here so the guard exists once rather + /// than at each of the dozen sites that used to build a path and read it. + async fn read_sidecar(&self, path: &Path) -> Option { + if !self.legacy_tier_active() { + return None; + } + fs::read(path).await.ok().map(Bytes::from) + } + + /// Presence test with the same guard — for the paths that only need to + /// know whether a sidecar is there. + async fn sidecar_exists(&self, path: &Path) -> bool { + self.legacy_tier_active() && fs::metadata(path).await.is_ok() + } + /// Check if a file is an image that can have thumbnails pub fn is_supported_image(mime_type: &str) -> bool { matches!( @@ -375,13 +458,13 @@ impl ThumbnailService { .entry(cache_key) .or_insert_with(async { // 1. Try loading from disk - if let Ok(data) = fs::read(&thumb_path).await { + if let Some(bytes) = self.read_sidecar(&thumb_path).await { tracing::debug!( "💾 Thumbnail loaded from disk: {} {:?}", file_id_owned, size ); - return Bytes::from(data); + return bytes; } // 2. Generate thumbnail (CPU-bound, runs in spawn_blocking) @@ -454,13 +537,13 @@ impl ThumbnailService { .cache .entry(cache_key) .or_insert_with(async move { - if let Ok(data) = fs::read(&thumb_path).await { + if let Some(bytes) = self.read_sidecar(&thumb_path).await { tracing::debug!( "💾 Thumbnail loaded from disk: {} {:?}", file_id_owned, size ); - return Bytes::from(data); + return bytes; } let Ok(_permit) = self.decode_semaphore.acquire().await else { @@ -519,13 +602,13 @@ impl ThumbnailService { .cache .entry(cache_key) .or_insert_with(async move { - if let Ok(data) = fs::read(&thumb_path).await { + if let Some(bytes) = self.read_sidecar(&thumb_path).await { tracing::debug!( "💾 Thumbnail loaded from disk: {} {:?}", file_id_owned, size ); - return Bytes::from(data); + return bytes; } let Ok(_permit) = self.decode_semaphore.acquire().await else { @@ -752,8 +835,7 @@ impl ThumbnailService { .thumbnails_root .join(size.dir_name()) .join(format!("ext-{}.jpg", file_id)); - if let Ok(data) = fs::read(&ext_path).await { - let bytes = Bytes::from(data); + if let Some(bytes) = self.read_sidecar(&ext_path).await { // Cache under a Jpeg-pinned key: these bytes are always JPEG, so the // key's format must describe them. Inserting under `cache_key` (whose // format is the *requested* format, possibly Webp) would store JPEG @@ -847,8 +929,7 @@ impl ThumbnailService { // 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); + if let Some(bytes) = self.read_sidecar(&thumb_path).await { self.cache .insert( ThumbnailCacheKey::content(hash, size, format), @@ -1271,7 +1352,7 @@ impl ThumbnailService { for size in ThumbnailSize::all() { let thumb_path = self.get_thumbnail_path(&blob_hash, *size, ThumbnailFormat::Webp); - if fs::metadata(&thumb_path).await.is_err() { + if !self.sidecar_exists(&thumb_path).await { ok = false; break; } @@ -1282,10 +1363,10 @@ impl ThumbnailService { for size in ThumbnailSize::all() { let thumb_path = self.get_thumbnail_path(&blob_hash, *size, ThumbnailFormat::Webp); - if let Ok(data) = fs::read(&thumb_path).await { + if let Some(bytes) = self.read_sidecar(&thumb_path).await { let cache_key = ThumbnailCacheKey::content(&blob_hash, *size, ThumbnailFormat::Webp); - self.cache.insert(cache_key, Bytes::from(data)).await; + self.cache.insert(cache_key, bytes).await; } } tracing::info!( @@ -1389,7 +1470,7 @@ impl ThumbnailService { for size in ThumbnailSize::all() { let thumb_path = self.get_thumbnail_path(&blob_hash, *size, ThumbnailFormat::Webp); - if fs::metadata(&thumb_path).await.is_err() { + if !self.sidecar_exists(&thumb_path).await { ok = false; break; } @@ -1400,10 +1481,10 @@ impl ThumbnailService { for size in ThumbnailSize::all() { let thumb_path = self.get_thumbnail_path(&blob_hash, *size, ThumbnailFormat::Webp); - if let Ok(data) = fs::read(&thumb_path).await { + if let Some(bytes) = self.read_sidecar(&thumb_path).await { let cache_key = ThumbnailCacheKey::content(&blob_hash, *size, ThumbnailFormat::Webp); - self.cache.insert(cache_key, Bytes::from(data)).await; + self.cache.insert(cache_key, bytes).await; } } tracing::info!( @@ -1523,7 +1604,7 @@ impl ThumbnailService { let mut ok = true; for size in ThumbnailSize::all() { let p = self.get_thumbnail_path(&blob_hash, *size, ThumbnailFormat::Webp); - if fs::metadata(&p).await.is_err() { + if !self.sidecar_exists(&p).await { ok = false; break; } @@ -1533,10 +1614,10 @@ impl ThumbnailService { if all_exist { for size in ThumbnailSize::all() { let p = self.get_thumbnail_path(&blob_hash, *size, ThumbnailFormat::Webp); - if let Ok(data) = fs::read(&p).await { + if let Some(bytes) = self.read_sidecar(&p).await { let key = ThumbnailCacheKey::content(&blob_hash, *size, ThumbnailFormat::Webp); - self.cache.insert(key, Bytes::from(data)).await; + self.cache.insert(key, bytes).await; } } return; @@ -1664,7 +1745,7 @@ impl ThumbnailService { .thumbnails_root .join(size.dir_name()) .join(format!("ext-{}.jpg", file_id)); - if fs::metadata(&ext_path).await.is_ok() { + if self.sidecar_exists(&ext_path).await { let _ = fs::remove_file(&ext_path).await; } } @@ -1682,7 +1763,7 @@ impl ThumbnailService { // Delete both the primary WebP and any lazily-materialized JPEG. for format in [ThumbnailFormat::Webp, ThumbnailFormat::Jpeg] { let path = self.get_thumbnail_path(blob_hash, *size, format); - if fs::metadata(&path).await.is_ok() { + if self.sidecar_exists(&path).await { let _ = fs::remove_file(&path).await; } } @@ -2117,6 +2198,66 @@ mod tier_selection_tests { .unwrap(); } + /// `initialize()` must not recreate what the import job removed. + /// + /// It used to `create_dir_all` every size directory at boot, so a drained + /// deployment grew its `.thumbnails/` tree back on the next restart and + /// the absence the fallback gates on was unreachable. Found on a sandbox + /// where the job had removed the directories and a restart put three + /// empty ones back. + #[tokio::test] + async fn initialize_does_not_recreate_a_drained_tier() { + let tmp = tempfile::tempdir().unwrap(); + let svc = service(tmp.path()); + + svc.initialize().await.unwrap(); + + assert!( + tokio::fs::metadata(tmp.path().join(".thumbnails")) + .await + .is_err(), + "boot recreated the legacy sidecar tree" + ); + assert!( + !svc.legacy_tier_active(), + "no directories on disk, so the fallback must be inert" + ); + } + + /// The other direction: a tier that still holds sidecars stays live, or + /// the migration would strand every un-imported thumbnail. + #[tokio::test] + async fn initialize_keeps_the_fallback_when_sidecars_remain() { + let tmp = tempfile::tempdir().unwrap(); + let svc = service(tmp.path()); + write_blob_sidecar(tmp.path(), b"legacy").await; + + svc.initialize().await.unwrap(); + + assert!(svc.legacy_tier_active()); + let got = svc + .get_cached_thumbnail(FILE_ID, Some(HASH), SIZE, FMT, None) + .await; + assert_eq!(got.as_deref(), Some(&b"legacy"[..])); + } + + /// With the tier inert, a sidecar on disk is deliberately NOT served — + /// the guard short-circuits before the read. This is what makes the + /// fallback free rather than merely cheap, and it is only sound because + /// nothing has written a sidecar since step 10d2. + #[tokio::test] + async fn inert_tier_skips_the_read_entirely() { + let tmp = tempfile::tempdir().unwrap(); + let svc = service(tmp.path()); + write_blob_sidecar(tmp.path(), b"legacy").await; + svc.legacy_sidecars.store(false, Ordering::Relaxed); + + let got = svc + .get_cached_thumbnail(FILE_ID, Some(HASH), SIZE, FMT, None) + .await; + assert!(got.is_none(), "inert tier must not touch the filesystem"); + } + /// The bug from 2026-08-25: a render cached under the content key /// shadowed a preview the user uploaded afterwards, permanently, because /// the content tier was consulted first. The PUT looked like a no-op.