From 12158ccf592d327b89a432e65d590547ef666d50 Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Thu, 27 Aug 2026 19:06:09 +0200 Subject: [PATCH] fix(storage): thumb_derived_import claims JPEG sidecars too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The filter was strip_suffix(".webp"), but persist_rendered writes {hash}.{format.ext()} — so any client not advertising WebP leaves {hash}.jpg on disk. Correct only while the derived tier was WebP-only; once variant carried the format (20261022000000) a JPEG sidecar became ordinary content, and leaving it unclaimed would keep .thumbnails/ permanently non-empty — the very signal step 10e gates on. The migration could never finish. Both codecs are now claimed and the format comes from the file's own extension, so a .jpg imports AS JPEG. Deriving the variant and content_type from it rather than hardcoding WebP is the point: a mislabelled row would serve the wrong codec to whoever the read path then matched it for. ThumbnailFormat::ALL exists so the claim list and the write path cannot drift — adding a format without teaching the import about it would strand that codec silently. The `ext-` rejection now carries real weight. Previously .jpg was rejected wholesale, so the two jobs could not overlap by construction; now they share an extension and only the prefix separates them. Both directions stay under test. Caught by the cross-job assertion, which counts every real sidecar being claimed exactly once — the fixture gained a .jpg and the total moved 3 to 4, which is the test noticing rather than a test to update. --- src/application/ports/thumbnail_ports.rs | 6 ++ .../services/thumb_attached_import_service.rs | 7 +- .../services/thumb_derived_import_service.rs | 96 +++++++++++++------ 3 files changed, 78 insertions(+), 31 deletions(-) diff --git a/src/application/ports/thumbnail_ports.rs b/src/application/ports/thumbnail_ports.rs index 195f384b..1fa7cff7 100644 --- a/src/application/ports/thumbnail_ports.rs +++ b/src/application/ports/thumbnail_ports.rs @@ -77,6 +77,12 @@ pub enum ThumbnailFormat { } impl ThumbnailFormat { + /// Every format, for callers that must handle all of them — notably + /// `thumb_derived_import`, which claims one sidecar extension per format + /// and would silently strand a codec if this list and the write path + /// drifted apart. + pub const ALL: [ThumbnailFormat; 2] = [ThumbnailFormat::Webp, ThumbnailFormat::Jpeg]; + /// Stable name, byte-identical to the derived `Debug` output (see /// [`ThumbnailSize::as_str`] — same ETag-stability contract). pub fn as_str(self) -> &'static str { diff --git a/src/infrastructure/services/thumb_attached_import_service.rs b/src/infrastructure/services/thumb_attached_import_service.rs index 0486f660..63a19542 100644 --- a/src/infrastructure/services/thumb_attached_import_service.rs +++ b/src/infrastructure/services/thumb_attached_import_service.rs @@ -386,10 +386,13 @@ mod tests { ); } // And nothing real is dropped: README.txt is the only unclaimed file. + // Two content-keyed .webp, one content-keyed .jpg, one ext- upload. + // The .jpg pair is the interesting one: same extension, opposite + // keying, and only the `ext-` prefix separates them. assert_eq!( attached.len() + derived.len(), - 3, - "the three real sidecars must be claimed exactly once between them" + 4, + "every real sidecar must be claimed exactly once between the two jobs" ); } diff --git a/src/infrastructure/services/thumb_derived_import_service.rs b/src/infrastructure/services/thumb_derived_import_service.rs index 570305ed..c52da984 100644 --- a/src/infrastructure/services/thumb_derived_import_service.rs +++ b/src/infrastructure/services/thumb_derived_import_service.rs @@ -39,7 +39,7 @@ use async_trait::async_trait; use bytes::Bytes; use tokio::fs; -use crate::application::ports::thumbnail_ports::ThumbnailSize; +use crate::application::ports::thumbnail_ports::{ThumbnailFormat, ThumbnailSize}; use crate::infrastructure::scheduler::{ JobRegistry, JobRunArgs, JobStore, JobStoreProvider, RecoverableJobHandler, RunOutcome, RunStatus, record_or_log, @@ -76,19 +76,35 @@ impl ThumbDerivedImport { self } - /// The hash a sidecar filename names, or `None` when the file is not one. + /// The hash and format a sidecar filename names, or `None` when the file + /// is not one of ours. /// /// Strict, and deliberately rejects `ext-{file_id}.jpg`: those are /// user-supplied, file-keyed bytes. Importing them here would content-key /// them and share one user's uploaded preview onto every file with /// identical content — the poisoning `file_attached_blobs` exists to - /// prevent. They belong to `thumb_attached_import`. - fn hash_from_sidecar_name(name: &str) -> Option<&str> { - let stem = name.strip_suffix(".webp")?; + /// prevent. They belong to `thumb_attached_import`. That rejection + /// carries the weight now that `.jpg` is otherwise claimed, since the two + /// jobs would otherwise both want it. + /// + /// Returns the format too, because the row + /// key needs both since migration `20261022000000`. + /// + /// Both codecs are claimed. `persist_rendered` writes + /// `{hash}.{format.ext()}`, so any client that does not advertise WebP + /// leaves `{hash}.jpg` on disk. While the derived tier was WebP-only + /// those were unmigratable by design; now that `variant` carries the + /// format they are ordinary content, and skipping them would leave + /// `.thumbnails/` permanently non-empty — which is the signal step 10e + /// gates the fallback removal on. + fn hash_from_sidecar_name(name: &str) -> Option<(&str, ThumbnailFormat)> { + let (stem, format) = ThumbnailFormat::ALL + .iter() + .find_map(|f| name.strip_suffix(&format!(".{}", f.ext())).map(|s| (s, *f)))?; if stem.len() != 64 || !stem.chars().all(|c| c.is_ascii_hexdigit()) { return None; } - Some(stem) + Some((stem, format)) } /// Sorted sidecar filenames for one size directory. @@ -159,25 +175,14 @@ impl RecoverableJobHandler for ThumbDerivedImport { let mut already = 0u64; let mut failed = 0u64; let mut since_checkpoint = 0usize; - // Two different strings, and conflating them is a real trap: the - // DIRECTORY is `{size}` on disk, while the VARIANT is - // `{size}.{ext}` since migration `20261022000000`. Using the variant - // as a path yields `.thumbnails/preview.webp/…`, which does not - // exist, so every file reads as unreadable and nothing imports. - // - // Sidecars here are always `{hash}.webp` — the name filter requires - // that extension — so the variant is unconditionally the WebP one. - let variant_of = |s: ThumbnailSize| { - format!( - "{}.{}", - s.dir_name(), - crate::application::ports::thumbnail_ports::ThumbnailFormat::Webp.ext() - ) - }; - + // The DIRECTORY is `{size}` on disk; the VARIANT is `{size}.{ext}` + // since migration `20261022000000`. Conflating them is a real trap: + // using the variant as a path yields `.thumbnails/preview.webp/…`, + // which does not exist, so every file reads as unreadable and nothing + // imports. The variant is therefore built per FILE, from the format + // its extension names, not once per size. for size in ThumbnailSize::all() { let dir_name = size.dir_name(); // on-disk directory - let variant = variant_of(*size); // content_derived_blobs.variant for name in Self::sidecar_names(&self.thumbnails_root, *size).await { // Cursor position uses the DIRECTORY, so a run paused before // this change resumes at the same place. @@ -204,9 +209,15 @@ impl RecoverableJobHandler for ThumbDerivedImport { } } - let Some(hash) = Self::hash_from_sidecar_name(&name) else { + let Some((hash, format)) = Self::hash_from_sidecar_name(&name) else { continue; }; + // Both derived from the file's OWN extension, so a `.jpg` + // sidecar becomes a JPEG row rather than being mislabelled + // WebP — which would serve the wrong codec to anyone the read + // path then matched it for. + let variant = format!("{dir_name}.{}", format.ext()); + let content_type = format.mime(); // Already mapped — the common case on a re-run, and the // reason this job is safe to trigger repeatedly. @@ -227,7 +238,7 @@ impl RecoverableJobHandler for ThumbDerivedImport { hash, "thumbnail", &variant, - "image/webp", + content_type, Bytes::from(data), ) .await @@ -310,12 +321,27 @@ pub(crate) mod tests { use super::*; const H: &str = "0a1b2c3d4e5f60718293a4b5c6d7e8f90a1b2c3d4e5f60718293a4b5c6d7e8f9"; + /// A second hash, for the JPEG sidecar in `legacy_tree`. + const H2: &str = "c222222222222222222222222222222222222222222222222222222222222222"; + /// BOTH codecs are claimed, and the format comes from the extension. + /// + /// `.jpg` was previously rejected here, which was correct only while the + /// derived tier was WebP-only. Once `variant` carried the format + /// (migration `20261022000000`) a JPEG sidecar became ordinary content, + /// and leaving it unclaimed would keep `.thumbnails/` permanently + /// non-empty — the very signal step 10e gates on. #[test] - fn accepts_a_canonical_sidecar_name() { + fn accepts_both_codecs_and_reports_the_format() { assert_eq!( ThumbDerivedImport::hash_from_sidecar_name(&format!("{H}.webp")), - Some(H) + Some((H, ThumbnailFormat::Webp)) + ); + assert_eq!( + ThumbDerivedImport::hash_from_sidecar_name(&format!("{H}.jpg")), + Some((H, ThumbnailFormat::Jpeg)), + "a JPEG sidecar must import, and as JPEG — labelling it WebP \ + would serve the wrong codec" ); } @@ -349,6 +375,13 @@ pub(crate) mod tests { .await .unwrap(); // Neither: a stray file that must be claimed by no one. + // Server-rendered JPEG: what a client not advertising WebP + // leaves behind. Claimed by the derived import, and must not be + // confused with the `ext-` upload above despite sharing an + // extension. + tokio::fs::write(dir.join(format!("{H2}.jpg")), b"jpeg") + .await + .unwrap(); tokio::fs::write(dir.join("README.txt"), b"nope") .await .unwrap(); @@ -371,8 +404,10 @@ pub(crate) mod tests { vec![ format!("{H}.webp"), "b111111111111111111111111111111111111111111111111111111111111111.webp".to_string(), + format!("{H2}.jpg"), ], - "must claim both content-keyed sidecars, sorted, and nothing else" + "must claim every content-keyed sidecar of EITHER codec, sorted, \ + and nothing else" ); } @@ -394,12 +429,15 @@ pub(crate) mod tests { #[test] fn rejects_external_and_malformed_names() { for name in [ + // `ext-` prefixed: user-supplied and file-keyed, whatever the + // extension. Now that .jpg is otherwise claimed, this is the case + // that keeps the two jobs disjoint. format!("ext-{H}.jpg"), "ext-3f2b1c00-0000-0000-0000-000000000000.jpg".to_string(), - format!("{H}.jpg"), format!("{}.webp", &H[..63]), H.to_string(), "junk.webp".to_string(), + "junk.jpg".to_string(), ] { assert_eq!( ThumbDerivedImport::hash_from_sidecar_name(&name),