From 4d2fa1269b34024fbf34b665a7182a4e4b1085ab Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Tue, 3 Nov 2026 11:51:22 -0500 Subject: [PATCH] lightcraft-cli: calibrate walks folders with the engine's bounded walk (#3643) * lightcraft-cli: calibrate walks folders with the engine's bounded walk * lightcraft-cli: calibrate keeps only regular raw files --- apps/lightcraft-cli/src/main.rs | 61 +++++++++++++++++++++------ crates/engine/src/walk.rs | 73 +++++++++++++++++++++++++++++++-- 2 files changed, 118 insertions(+), 16 deletions(-) diff --git a/apps/lightcraft-cli/src/main.rs b/apps/lightcraft-cli/src/main.rs index 01d6f23b39..7ace1a8981 100644 --- a/apps/lightcraft-cli/src/main.rs +++ b/apps/lightcraft-cli/src/main.rs @@ -196,7 +196,12 @@ fn main() -> ExitCode { } /// Raw files below `path` (or `path` itself), skipping hidden and NAS metadata folders. -fn raw_files(path: &Path, out: &mut Vec, depth: usize) { +/// +/// Folders go through the engine's bounded walk ([`lightcraft_engine::walk::files_in`]): it enters +/// a folder reached through a link once at most and stops at a depth and an entry budget. A +/// folder of links pointing back at their parents made the old recursive walk re-read the same +/// folders exponentially often before its depth limit cut it off (CWE-400). +fn raw_files(path: &Path, out: &mut Vec) { let is_raw = |p: &Path| { p.extension().and_then(|e| e.to_str()).is_some_and(|e| ["arw", "nef", "nrw", "raf", "crw"].iter().any(|x| e.eq_ignore_ascii_case(x))) }; @@ -206,17 +211,20 @@ fn raw_files(path: &Path, out: &mut Vec, depth: usize) { } return; } - let Ok(entries) = std::fs::read_dir(path) else { return }; - if depth > 32 { - return; - } - for entry in entries.flatten() { - let name = entry.file_name(); - if name.to_string_lossy().starts_with(['.', '@']) { - continue; - } - raw_files(&entry.path(), out, depth + 1); + // (hidden entries are left out by the walk; `@eaDir` and other NAS metadata folders and files + // here, folders before they are listed so their entries do not use up the walk's budget) + let nas_metadata = |p: &Path| p.file_name().is_some_and(|n| n.to_string_lossy().starts_with('@')); + let walked = lightcraft_engine::walk::files_in_entering( + path, + None, + lightcraft_engine::walk::Limits::default(), + |p| is_raw(p) && !nas_metadata(p) && p.is_file(), + |p| !nas_metadata(p), + ); + if walked.truncated { + eprintln!("calibrate: {} has more folders or entries than a walk looks at; the rest is left out", path.display()); } + out.extend(walked.files); } fn calibrate(args: &[String]) -> Result<(), String> { @@ -238,7 +246,7 @@ fn calibrate(args: &[String]) -> Result<(), String> { } let mut files = Vec::new(); for input in &inputs { - raw_files(Path::new(input), &mut files, 0); + raw_files(Path::new(input), &mut files); } files.sort(); files.dedup(); @@ -1070,6 +1078,35 @@ fn denoise_serve(_: &[String]) -> Result<(), String> { Err("denoise-serve: this build has no denoise support (build with the `denoise` feature)".into()) } +#[cfg(all(test, unix))] +mod raw_files_tests { + use super::raw_files; + + /// Links back to their own folders are entered once, not once per path that reaches them: a + /// handful of such links made the old walk list the same folders exponentially often. + #[test] + fn folder_links_back_up_are_walked_once() { + let dir = std::env::temp_dir().join(format!("lightcraft-cli-raw-files-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + let a = dir.join("a"); + std::fs::create_dir_all(a.join("@eaDir")).unwrap(); + std::fs::write(a.join("one.NEF"), b"").unwrap(); + std::fs::write(a.join("@eaDir").join("thumb.nef"), b"").unwrap(); + std::fs::write(a.join("notes.txt"), b"").unwrap(); + for i in 0..8 { + std::os::unix::fs::symlink(&dir, a.join(format!("up{i}"))).unwrap(); + } + // a named pipe with a raw extension: not a file, so never opened (a read would block) + assert!(std::process::Command::new("mkfifo").arg(a.join("pipe.nef")).status().unwrap().success()); + let started = std::time::Instant::now(); + let mut files = Vec::new(); + raw_files(&dir, &mut files); + assert!(started.elapsed() < std::time::Duration::from_secs(10), "walk took {:?}", started.elapsed()); + assert_eq!(files, vec![a.join("one.NEF")]); + let _ = std::fs::remove_dir_all(&dir); + } +} + #[cfg(test)] mod import_target_tests { use super::{import_and_select, import_targets, json}; diff --git a/crates/engine/src/walk.rs b/crates/engine/src/walk.rs index 98290130ea..db024f7098 100644 --- a/crates/engine/src/walk.rs +++ b/crates/engine/src/walk.rs @@ -95,17 +95,37 @@ pub type NoteFolder<'a> = &'a mut dyn FnMut(&Path, Vec<(std::ffi::OsString, Entr /// are left out, the folder `skip` (e.g. the library's own) is never entered, and the walk stops /// at `limits` (see [`Walked::truncated`]). Unreadable folders are skipped. pub fn files_in(root: &Path, skip: Option<&Path>, limits: Limits, keep: impl FnMut(&Path) -> bool) -> Walked { - walk(root, skip, limits, keep, None) + walk(root, skip, limits, keep, |_| true, None) +} + +/// [`files_in`], entering a folder below `root` only when `enter` accepts it: a folder it turns +/// down is neither listed nor counted against `limits` (e.g. a NAS's `@eaDir` thumbnail folders, +/// which can hold more entries than the photos beside them). +pub fn files_in_entering( + root: &Path, + skip: Option<&Path>, + limits: Limits, + keep: impl FnMut(&Path) -> bool, + enter: impl FnMut(&Path) -> bool, +) -> Walked { + walk(root, skip, limits, keep, enter, None) } /// [`files_in`], handing the listing of every folder it reads to `note`: who walks a folder to /// find its files has, with that, the answer to "is this name there" for everything in it, and /// need not ask the disk again (a round trip for each name on a network share). pub fn files_in_noting(root: &Path, skip: Option<&Path>, limits: Limits, keep: impl FnMut(&Path) -> bool, note: NoteFolder) -> Walked { - walk(root, skip, limits, keep, Some(note)) + walk(root, skip, limits, keep, |_| true, Some(note)) } -fn walk(root: &Path, skip: Option<&Path>, limits: Limits, mut keep: impl FnMut(&Path) -> bool, mut note: Option) -> Walked { +fn walk( + root: &Path, + skip: Option<&Path>, + limits: Limits, + mut keep: impl FnMut(&Path) -> bool, + mut enter: impl FnMut(&Path) -> bool, + mut note: Option, +) -> Walked { let mut out = Walked::default(); // canonical folders reached through a link, so a link loop is walked at most once let mut linked: HashSet = HashSet::new(); @@ -171,6 +191,11 @@ fn walk(root: &Path, skip: Option<&Path>, limits: Limits, mut keep: impl FnMut(& // a link (or a Windows junction): a folder is entered once, a file is a file match std::fs::metadata(&p) { Ok(m) if m.is_dir() => { + // (asked before the folder is marked as entered, so a folder turned down + // here can still be entered through another link to it) + if !enter(&p) { + continue; + } let Ok(c) = std::fs::canonicalize(&p) else { continue }; if !linked.insert(c) { continue; @@ -179,8 +204,13 @@ fn walk(root: &Path, skip: Option<&Path>, limits: Limits, mut keep: impl FnMut(& } _ => false, } + } else if ft.is_dir() { + if !enter(&p) { + continue; + } + true } else { - ft.is_dir() + false }; children.push((p, dir)); } @@ -343,6 +373,41 @@ mod tests { let _ = std::fs::remove_dir_all(&d); } + /// A folder `enter` turns down is not listed: its entries neither come back nor use up the + /// entry budget, so a sibling folder after it is still walked. + #[test] + fn folders_turned_down_are_not_listed_or_counted() { + let d = tmp("enter"); + std::fs::create_dir_all(d.join("@eaDir")).unwrap(); + std::fs::create_dir_all(d.join("z")).unwrap(); + for i in 0..20 { + std::fs::write(d.join("@eaDir").join(format!("{i:02}.jpg")), b"x").unwrap(); + } + std::fs::write(d.join("z/keep.jpg"), b"x").unwrap(); + let not_at = |p: &Path| !p.file_name().is_some_and(|n| n.to_string_lossy().starts_with('@')); + let w = files_in_entering(&d, None, Limits { max_depth: 8, max_entries: 5 }, |_| true, not_at); + assert_eq!((w.files, w.entries, w.truncated), (vec![d.join("z/keep.jpg")], 3, false)); + let _ = std::fs::remove_dir_all(&d); + } + + /// A link `enter` turns down does not use up its folder's one visit through a link: another + /// link to the same folder is still entered. + #[cfg(unix)] + #[test] + fn folder_turned_down_through_one_link_is_entered_through_another() { + let d = tmp("enter-link"); + std::fs::create_dir_all(d.join("real")).unwrap(); + std::fs::write(d.join("real/p.jpg"), b"x").unwrap(); + let root = d.join("root"); + std::fs::create_dir_all(&root).unwrap(); + std::os::unix::fs::symlink(d.join("real"), root.join("@meta")).unwrap(); + std::os::unix::fs::symlink(d.join("real"), root.join("photos")).unwrap(); + let not_at = |p: &Path| !p.file_name().is_some_and(|n| n.to_string_lossy().starts_with('@')); + let w = files_in_entering(&root, None, Limits::default(), |_| true, not_at); + assert_eq!(w.files, vec![root.join("photos/p.jpg")]); + let _ = std::fs::remove_dir_all(&d); + } + /// A link back up the tree (a loop) is followed at most once: the walk ends. #[cfg(unix)] #[test]