diff --git a/src/infrastructure/services/thumb_attached_import_service.rs b/src/infrastructure/services/thumb_attached_import_service.rs index cb7235f7..2733a2db 100644 --- a/src/infrastructure/services/thumb_attached_import_service.rs +++ b/src/infrastructure/services/thumb_attached_import_service.rs @@ -106,8 +106,12 @@ impl ThumbAttachedImport { /// /// Sorted because the cursor resumes by skipping everything at or before /// it, which only works over a stable order. - async fn sidecar_names(&self, size: ThumbnailSize) -> Vec { - let dir = self.thumbnails_root.join(size.dir_name()); + /// + /// Takes the root rather than reading `self`, so the walk — the half that + /// decides which files this job claims, and therefore which keying they + /// get — is testable against a temp directory with no database in sight. + async fn sidecar_names(root: &std::path::Path, size: ThumbnailSize) -> Vec { + let dir = root.join(size.dir_name()); let Ok(mut entries) = fs::read_dir(&dir).await else { return Vec::new(); }; @@ -146,7 +150,9 @@ impl RecoverableJobHandler for ThumbAttachedImport { async fn count_total(&self) -> Option { let mut total = 0u64; for size in ThumbnailSize::all() { - total += self.sidecar_names(*size).await.len() as u64; + total += Self::sidecar_names(&self.thumbnails_root, *size) + .await + .len() as u64; } Some(total) } @@ -181,7 +187,7 @@ impl RecoverableJobHandler for ThumbAttachedImport { for size in ThumbnailSize::all() { let dir_name = size.dir_name().to_string(); - for name in self.sidecar_names(*size).await { + for name in Self::sidecar_names(&self.thumbnails_root, *size).await { let position = format!("{dir_name}/{name}"); if let Some(c) = &cursor @@ -341,6 +347,43 @@ mod tests { ); } + /// The other half of the partition. Reuses the same legacy tree as + /// `thumb_derived_import`'s test on purpose: the two jobs run over one + /// directory, so the property that matters is that together they claim + /// every real sidecar exactly once, and neither takes the other's. + #[tokio::test] + async fn walk_claims_only_uploaded_previews() { + use crate::infrastructure::services::thumb_derived_import_service::ThumbDerivedImport; + + let tmp = + crate::infrastructure::services::thumb_derived_import_service::tests::legacy_tree() + .await; + + let attached = ThumbAttachedImport::sidecar_names(tmp.path(), ThumbnailSize::Preview).await; + let derived = ThumbDerivedImport::sidecar_names(tmp.path(), ThumbnailSize::Preview).await; + + assert_eq!( + attached, + vec!["ext-3f2b1c00-1111-2222-3333-444455556666.jpg".to_string()], + "must claim the uploaded preview and nothing else" + ); + + // Disjoint: no file is imported under both keyings, which would take + // two references and — worse — content-key user-supplied bytes. + for a in &attached { + assert!( + !derived.contains(a), + "both jobs claimed {a}; keying would be ambiguous" + ); + } + // And nothing real is dropped: README.txt is the only unclaimed file. + assert_eq!( + attached.len() + derived.len(), + 3, + "the three real sidecars must be claimed exactly once between them" + ); + } + /// The content-keyed sidecars belong to `thumb_derived_import`. Importing /// one here would file-key bytes that are shared across every file with /// the same content, so each such file would take its own reference to diff --git a/src/infrastructure/services/thumb_derived_import_service.rs b/src/infrastructure/services/thumb_derived_import_service.rs index c01f7e31..6393b5a4 100644 --- a/src/infrastructure/services/thumb_derived_import_service.rs +++ b/src/infrastructure/services/thumb_derived_import_service.rs @@ -93,8 +93,12 @@ impl ThumbDerivedImport { /// /// Sorted so the cursor is meaningful: resume skips everything at or /// before it, which only works over a stable order. - async fn sidecar_names(&self, size: ThumbnailSize) -> Vec { - let dir = self.thumbnails_root.join(size.dir_name()); + /// + /// Takes the root rather than reading `self`, so the walk — the half that + /// decides which files this job claims, and therefore which keying they + /// get — is testable against a temp directory with no database in sight. + pub(crate) async fn sidecar_names(root: &std::path::Path, size: ThumbnailSize) -> Vec { + let dir = root.join(size.dir_name()); let Ok(mut entries) = fs::read_dir(&dir).await else { return Vec::new(); }; @@ -120,7 +124,9 @@ impl RecoverableJobHandler for ThumbDerivedImport { async fn count_total(&self) -> Option { let mut total = 0u64; for size in ThumbnailSize::all() { - total += self.sidecar_names(*size).await.len() as u64; + total += Self::sidecar_names(&self.thumbnails_root, *size) + .await + .len() as u64; } Some(total) } @@ -155,7 +161,7 @@ impl RecoverableJobHandler for ThumbDerivedImport { for size in ThumbnailSize::all() { let dir_name = variant_of(*size); - for name in self.sidecar_names(*size).await { + for name in Self::sidecar_names(&self.thumbnails_root, *size).await { let position = format!("{dir_name}/{name}"); // Resume: everything at or before the cursor is done. @@ -277,7 +283,11 @@ impl RecoverableJobHandler for ThumbDerivedImport { } #[cfg(test)] -mod tests { +// `pub(crate)` so the attached import's test can reuse `legacy_tree`. Both +// jobs walk ONE directory, so the property worth asserting spans them — that +// together they claim every sidecar exactly once — and that needs a shared +// fixture rather than two that can drift apart. +pub(crate) mod tests { use super::*; const H: &str = "0a1b2c3d4e5f60718293a4b5c6d7e8f90a1b2c3d4e5f60718293a4b5c6d7e8f9"; @@ -290,6 +300,75 @@ mod tests { ); } + /// A legacy `.thumbnails` tree as it exists before the migration: both + /// sidecar shapes side by side in the same size directory, which is + /// exactly how they are written today. + /// + /// Returns the temp dir — the caller must hold it, or the directory is + /// removed while the test is still reading it. + pub(crate) async fn legacy_tree() -> tempfile::TempDir { + let tmp = tempfile::tempdir().expect("create temp dir"); + for size in ThumbnailSize::all() { + let dir = tmp.path().join(size.dir_name()); + tokio::fs::create_dir_all(&dir).await.unwrap(); + // Server-rendered, content-keyed. `b` sorts after `0a…`, so the + // pair also proves the listing is ordered rather than incidental. + tokio::fs::write(dir.join(format!("{H}.webp")), b"webp") + .await + .unwrap(); + tokio::fs::write( + dir.join("b111111111111111111111111111111111111111111111111111111111111111.webp"), + b"webp2", + ) + .await + .unwrap(); + // User-uploaded, file-keyed. + tokio::fs::write( + dir.join("ext-3f2b1c00-1111-2222-3333-444455556666.jpg"), + b"jpeg", + ) + .await + .unwrap(); + // Neither: a stray file that must be claimed by no one. + tokio::fs::write(dir.join("README.txt"), b"nope") + .await + .unwrap(); + } + tmp + } + + /// The migration's core invariant: this job claims the content-keyed + /// sidecars and *only* those, leaving the uploaded previews for + /// `thumb_attached_import`. Getting this wrong content-keys user-supplied + /// bytes, which shares one user's preview onto every file with identical + /// content. + #[tokio::test] + async fn walk_claims_only_content_keyed_sidecars_in_sorted_order() { + let tmp = legacy_tree().await; + let names = ThumbDerivedImport::sidecar_names(tmp.path(), ThumbnailSize::Preview).await; + + assert_eq!( + names, + vec![ + format!("{H}.webp"), + "b111111111111111111111111111111111111111111111111111111111111111.webp".to_string(), + ], + "must claim both content-keyed sidecars, sorted, and nothing else" + ); + } + + /// A missing size directory is normal on a fresh install and must not + /// abort the walk — the job simply has nothing to import. + #[tokio::test] + async fn missing_size_directory_yields_no_work() { + let tmp = tempfile::tempdir().expect("create temp dir"); + assert!( + ThumbDerivedImport::sidecar_names(tmp.path(), ThumbnailSize::Icon) + .await + .is_empty() + ); + } + /// `ext-` files are user-supplied and file-keyed. Importing one here /// would content-key it and share it across every file with identical /// content — the exact poisoning the table split prevents.