From 38ac3656c6e949138a2dc2e0d30e5e8a6156c967 Mon Sep 17 00:00:00 2001 From: Cameron Cordes Date: Fri, 2 Oct 2026 08:53:39 -0400 Subject: [PATCH 1/2] fix(image): never stream originals into thumb slots; unique preview tempfiles A thumb miss (no thumb on disk, or an `.unsupported` sentinel) fell through to NamedFile on the original. For a 12432x3504 panorama that decodes to 174MB, past Android's 100MB draw limit, and crashed the FileViewer app natively on the Memories tab. Still-image thumb misses now serve the 2048px large preview, or 404 if even that can't be made. Large/xlarge generation wrote to a shared `.jpg.tmp`, so two concurrent requests for the same file (common: tile remounts) raced and the loser's rename failed with os error 2, falling back to the original. Generation now goes through a per-call tempfile, and a failed request still serves a preview a concurrent request produced. Large and xlarge branches are folded into one `serve_preview` helper. Co-Authored-By: Claude Opus 5.5 --- src/handlers/image.rs | 451 +++++++++++++++++++++++++----------------- 1 file changed, 272 insertions(+), 179 deletions(-) diff --git a/src/handlers/image.rs b/src/handlers/image.rs index a33f96d..31b5f91 100644 --- a/src/handlers/image.rs +++ b/src/handlers/image.rs @@ -94,100 +94,24 @@ pub async fn get_image( image_size }; - if image_size == PhotoSize::Large { - let relative_path = path - .strip_prefix(&library.root_path) - .expect("Error stripping library root prefix from large preview"); - let relative_path_str = relative_path.to_string_lossy().replace('\\', "/"); - let thumbs = Path::new(&app_state.thumbnail_path); - let large_dir = thumbs.join("_large"); - - // Lookup chain mirrors the Thumb branch — hash-keyed first so - // multi-library deployments share derivative bytes across - // libraries, then library-scoped legacy as the fallback for - // rows that aren't hashed yet. - let hash_large_path: Option = { - let mut dao = exif_dao.lock().expect("Unable to lock ExifDao"); - match dao.get_exif(&relative_path_str) { - Ok(Some(row)) => row - .content_hash - .as_deref() - .map(|h| content_hash::large_preview_path(thumbs, h)), - _ => None, - } + if image_size == PhotoSize::Large || image_size == PhotoSize::XLarge { + let tier = if image_size == PhotoSize::Large { + PreviewTier::Large + } else { + PreviewTier::XLarge }; - let scoped_legacy_large_path = - content_hash::library_scoped_legacy_path(&large_dir, library.id, relative_path); - - let existing = hash_large_path - .as_ref() - .filter(|p| p.exists()) - .cloned() - .or_else(|| { - if scoped_legacy_large_path.exists() { - Some(scoped_legacy_large_path.clone()) - } else { - None - } - }); - - if let Some(found) = existing - && let Ok(file) = NamedFile::open(&found) + if let Some(resp) = serve_preview( + tier, + library, + &path, + Path::new(&app_state.thumbnail_path), + &exif_dao, + &request, + ) + .await { span.set_status(Status::Ok); - return file - .use_etag(true) - .use_last_modified(true) - .prefer_utf8(true) - .into_response(&request); - } - - // Cache miss — generate. Resize + JPEG-encode can take 100–500ms - // for a 24MP source (longer for RAW), so run on the blocking pool - // to keep the actix worker free. Prefer the hash-keyed - // destination when a hash is known so the result is reusable - // across libraries that hold the same bytes. - let dest = hash_large_path - .clone() - .unwrap_or_else(|| scoped_legacy_large_path.clone()); - let src = path.clone(); - let dest_for_block = dest.clone(); - let generated = web::block(move || { - if let Some(parent) = dest_for_block.parent() { - std::fs::create_dir_all(parent)?; - } - // Write to a sibling tempfile then atomically rename so a - // concurrent reader never observes a half-written JPEG. - let tmp = dest_for_block.with_extension("jpg.tmp"); - crate::thumbnails::generate_large_preview(&src, &tmp)?; - std::fs::rename(&tmp, &dest_for_block)?; - Ok::<(), std::io::Error>(()) - }) - .await; - - match generated { - Ok(Ok(())) => { - if let Ok(file) = NamedFile::open(&dest) { - span.set_status(Status::Ok); - return file - .use_etag(true) - .use_last_modified(true) - .prefer_utf8(true) - .into_response(&request); - } - } - Ok(Err(e)) => { - warn!( - "Large preview generation failed for {:?}: {} — falling back to original", - path, e - ); - } - Err(e) => { - warn!( - "Large preview blocking-pool error for {:?}: {} — falling back to original", - path, e - ); - } + return resp; } // Fall through to the Full branch below so the caller gets // *something* useful (the original bytes — or the RAW @@ -195,94 +119,6 @@ pub async fn get_image( // unrenderable RAW containers) instead of a 404. image_size = PhotoSize::Full; } - - if image_size == PhotoSize::XLarge { - let relative_path = path - .strip_prefix(&library.root_path) - .expect("Error stripping library root prefix from xlarge preview"); - let relative_path_str = relative_path.to_string_lossy().replace('\\', "/"); - let thumbs = Path::new(&app_state.thumbnail_path); - let xlarge_dir = thumbs.join("_xlarge"); - - let hash_xlarge_path: Option = { - let mut dao = exif_dao.lock().expect("Unable to lock ExifDao"); - match dao.get_exif(&relative_path_str) { - Ok(Some(row)) => row - .content_hash - .as_deref() - .map(|h| content_hash::xlarge_preview_path(thumbs, h)), - _ => None, - } - }; - let scoped_legacy_xlarge_path = - content_hash::library_scoped_legacy_path(&xlarge_dir, library.id, relative_path); - - let existing = hash_xlarge_path - .as_ref() - .filter(|p| p.exists()) - .cloned() - .or_else(|| { - if scoped_legacy_xlarge_path.exists() { - Some(scoped_legacy_xlarge_path.clone()) - } else { - None - } - }); - - if let Some(found) = existing - && let Ok(file) = NamedFile::open(&found) - { - span.set_status(Status::Ok); - return file - .use_etag(true) - .use_last_modified(true) - .prefer_utf8(true) - .into_response(&request); - } - - let dest = hash_xlarge_path - .clone() - .unwrap_or_else(|| scoped_legacy_xlarge_path.clone()); - let src = path.clone(); - let dest_for_block = dest.clone(); - let generated = web::block(move || { - if let Some(parent) = dest_for_block.parent() { - std::fs::create_dir_all(parent)?; - } - let tmp = dest_for_block.with_extension("jpg.tmp"); - crate::thumbnails::generate_xlarge_preview(&src, &tmp)?; - std::fs::rename(&tmp, &dest_for_block)?; - Ok::<(), std::io::Error>(()) - }) - .await; - - match generated { - Ok(Ok(())) => { - if let Ok(file) = NamedFile::open(&dest) { - span.set_status(Status::Ok); - return file - .use_etag(true) - .use_last_modified(true) - .prefer_utf8(true) - .into_response(&request); - } - } - Ok(Err(e)) => { - warn!( - "XLarge preview generation failed for {:?}: {} — falling back to original", - path, e - ); - } - Err(e) => { - warn!( - "XLarge preview blocking-pool error for {:?}: {} — falling back to original", - path, e - ); - } - } - image_size = PhotoSize::Full; - } - if image_size == PhotoSize::Thumb { let relative_path = path .strip_prefix(&library.root_path) @@ -375,6 +211,30 @@ pub async fn get_image( .prefer_utf8(true) .into_response(&request); } + + // No thumb on disk (never generated, or an `.unsupported` + // sentinel from a failed scan). Never stream the original into + // a thumb slot: a 12432×3504 panorama decodes to 174MB, past + // Android's 100MB draw limit, and crashes the app natively. + // The large tier is capped at 2048px; if even that can't be + // made, the client gets a 404 and shows a blank tile. + if !file_types::is_video_file(&path) { + if let Some(resp) = serve_preview( + PreviewTier::Large, + library, + &path, + Path::new(&app_state.thumbnail_path), + &exif_dao, + &request, + ) + .await + { + span.set_status(Status::Ok); + return resp; + } + span.set_status(Status::error("Thumbnail unavailable")); + return HttpResponse::NotFound().finish(); + } } // Full-size requests for RAW formats (NEF/CR2/ARW/etc.) can't just @@ -414,6 +274,143 @@ pub async fn get_image( } } +/// On-demand resized tiers served by [`serve_preview`]. +#[derive(Clone, Copy)] +enum PreviewTier { + Large, + XLarge, +} + +impl PreviewTier { + fn name(self) -> &'static str { + match self { + PreviewTier::Large => "Large", + PreviewTier::XLarge => "XLarge", + } + } + + fn legacy_dir(self) -> &'static str { + match self { + PreviewTier::Large => "_large", + PreviewTier::XLarge => "_xlarge", + } + } + + fn hash_path(self, thumbs: &Path, hash: &str) -> PathBuf { + match self { + PreviewTier::Large => content_hash::large_preview_path(thumbs, hash), + PreviewTier::XLarge => content_hash::xlarge_preview_path(thumbs, hash), + } + } + + fn generate(self, src: &Path, dest: &Path) -> std::io::Result<()> { + match self { + PreviewTier::Large => crate::thumbnails::generate_large_preview(src, dest), + PreviewTier::XLarge => crate::thumbnails::generate_xlarge_preview(src, dest), + } + } +} + +/// Serve the cached `tier` preview of `path`, generating it on a miss. +/// `None` when it can't be produced; the caller picks the fallback. +async fn serve_preview( + tier: PreviewTier, + library: &libraries::Library, + path: &Path, + thumbs: &Path, + exif_dao: &Mutex>, + request: &HttpRequest, +) -> Option { + let relative_path = path + .strip_prefix(&library.root_path) + .expect("Error stripping library root prefix from preview"); + let relative_path_str = relative_path.to_string_lossy().replace('\\', "/"); + + // Lookup chain mirrors the Thumb branch — hash-keyed first so + // multi-library deployments share derivative bytes across + // libraries, then library-scoped legacy as the fallback for + // rows that aren't hashed yet. + let hash_path: Option = { + let mut dao = exif_dao.lock().expect("Unable to lock ExifDao"); + match dao.get_exif(&relative_path_str) { + Ok(Some(row)) => row + .content_hash + .as_deref() + .map(|h| tier.hash_path(thumbs, h)), + _ => None, + } + }; + let scoped_legacy_path = content_hash::library_scoped_legacy_path( + &thumbs.join(tier.legacy_dir()), + library.id, + relative_path, + ); + + let serve = |p: &Path| { + NamedFile::open(p).ok().map(|file| { + file.use_etag(true) + .use_last_modified(true) + .prefer_utf8(true) + .into_response(request) + }) + }; + + let existing = hash_path + .as_ref() + .filter(|p| p.exists()) + .or_else(|| Some(&scoped_legacy_path).filter(|p| p.exists())); + if let Some(resp) = existing.and_then(|p| serve(p)) { + return Some(resp); + } + + // Cache miss — generate. Resize + JPEG-encode can take 100–500ms + // for a 24MP source (longer for RAW), so run on the blocking pool + // to keep the actix worker free. Prefer the hash-keyed + // destination when a hash is known so the result is reusable + // across libraries that hold the same bytes. + let dest = hash_path.unwrap_or(scoped_legacy_path); + let src = path.to_path_buf(); + let dest_for_block = dest.clone(); + let generated = web::block(move || generate_preview(tier, &src, &dest_for_block)).await; + + match generated { + Ok(Ok(())) => {} + Ok(Err(e)) => warn!( + "{} preview generation failed for {:?}: {}", + tier.name(), + path, + e + ), + Err(e) => warn!( + "{} preview blocking-pool error for {:?}: {}", + tier.name(), + path, + e + ), + } + serve(&dest) +} + +/// Generate `tier` for `src` into a per-call tempfile, then atomically +/// rename it to `dest`, so a concurrent reader never sees a half-written +/// JPEG. The tempfile name must be unique: clients fire the same request +/// twice at once (tile remounts), and a shared `.tmp` let the first +/// rename steal the second's file — "cannot find the file specified +/// (os error 2)". +fn generate_preview(tier: PreviewTier, src: &Path, dest: &Path) -> std::io::Result<()> { + let parent = dest + .parent() + .ok_or_else(|| std::io::Error::other("preview path has no parent"))?; + std::fs::create_dir_all(parent)?; + let tmp = tempfile::Builder::new() + .suffix(".jpg.tmp") + .tempfile_in(parent)? + .into_temp_path(); + tier.generate(src, &tmp)?; + tmp.persist(dest)?; + Ok(()) +} + async fn create_circular_thumbnail( thumb_path: &Path, thumbs_dir: &str, @@ -1253,3 +1250,99 @@ pub async fn upload_image( HttpResponse::Ok().finish() } + +#[cfg(test)] +mod tests { + use super::*; + use crate::database::SqliteExifDao; + use crate::database::test::in_memory_db_connection; + use actix_web::test::TestRequest; + use image::GenericImageView; + + fn library(root: &Path) -> libraries::Library { + libraries::Library { + id: libraries::PRIMARY_LIBRARY_ID, + name: "main".to_string(), + root_path: root.to_string_lossy().to_string(), + enabled: true, + excluded_dirs: Vec::new(), + } + } + + fn exif_dao() -> Mutex> { + Mutex::new(Box::new(SqliteExifDao::from_connection( + in_memory_db_connection(), + ))) + } + + /// Write a solid-color JPEG of the given size under `root`. + fn write_jpeg(root: &Path, name: &str, w: u32, h: u32) -> PathBuf { + let path = root.join(name); + image::RgbImage::from_pixel(w, h, image::Rgb([90, 140, 200])) + .save(&path) + .unwrap(); + path + } + + #[test] + fn concurrent_generation_of_one_preview_all_succeed() { + let root = tempfile::tempdir().unwrap(); + let thumbs = tempfile::tempdir().unwrap(); + let src = write_jpeg(root.path(), "pano.jpg", 3000, 800); + let dest = thumbs.path().join("_large").join("ab").join("abcd.jpg"); + + // Clients fire the same preview twice at once; with a shared + // tempfile name the loser's rename failed with os error 2. + let barrier = std::sync::Barrier::new(8); + std::thread::scope(|s| { + let handles: Vec<_> = (0..8) + .map(|_| { + s.spawn(|| { + barrier.wait(); + generate_preview(PreviewTier::Large, &src, &dest) + }) + }) + .collect(); + for h in handles { + h.join().unwrap().expect("concurrent generation failed"); + } + }); + + assert!(dest.exists()); + let leftovers: Vec<_> = std::fs::read_dir(dest.parent().unwrap()) + .unwrap() + .filter_map(Result::ok) + .filter(|e| e.file_name() != "abcd.jpg") + .collect(); + assert!(leftovers.is_empty(), "stray tempfiles: {:?}", leftovers); + } + + #[actix_rt::test] + async fn large_preview_caps_a_panorama_at_2048() { + let root = tempfile::tempdir().unwrap(); + let thumbs = tempfile::tempdir().unwrap(); + let lib = library(root.path()); + let src = write_jpeg(root.path(), "pano.jpg", 6000, 1600); + let req = TestRequest::default().to_http_request(); + + serve_preview( + PreviewTier::Large, + &lib, + &src, + thumbs.path(), + &exif_dao(), + &req, + ) + .await + .expect("preview served"); + + // Unhashed row → library-scoped legacy path under `_large/`. + let dest = content_hash::library_scoped_legacy_path( + &thumbs.path().join("_large"), + lib.id, + Path::new("pano.jpg"), + ); + let (w, h) = image::open(dest).unwrap().dimensions(); + assert_eq!((w, h), (2048, 546)); + } +} -- 2.54.0 From 4f839954b02be2df365378726ef58187dab79d0b Mon Sep 17 00:00:00 2001 From: Cameron Cordes Date: Fri, 2 Oct 2026 22:01:25 -0400 Subject: [PATCH 2/2] chore(idea): mark tests/ as a test source root Co-Authored-By: Claude Opus 5.5 --- .idea/image-api.iml | 1 + 1 file changed, 1 insertion(+) diff --git a/.idea/image-api.iml b/.idea/image-api.iml index 3c60e56..6a40898 100644 --- a/.idea/image-api.iml +++ b/.idea/image-api.iml @@ -3,6 +3,7 @@ + -- 2.54.0