From a51224e6d44639b7cf71d62b0f2f482942732bc3 Mon Sep 17 00:00:00 2001 From: Nathaniel Ledford Date: Fri, 9 Oct 2026 10:34:56 -0400 Subject: [PATCH] LRC-LIB-SMARTCOLL: smart albums testing smart albums stay linear From final review: the loop guard bounded the depth of smart albums testing smart albums, but not repeated work. Each album tested by the next one twice ("all of: album is X(k-1), album is X(k-1)") doubled the cost per level, so 25 levels meant 2^25 evaluations per photo, enough to freeze the sidebar's counts. Any agent or catalog file could build such a chain. rules::smart_album_holds remembers each smart album's answer for each photo during one outermost question, and forgets them when it ends (a drop guard, however it ends). A regression test counts a 26-deep doubled chain in well under its 2 s limit; before the fix it ran for minutes. Co-Authored-By: Claude Opus 5.5 --- crates/catalog/src/lib.rs | 2 +- crates/catalog/src/rules.rs | 31 +++++++++++++++++++++++++++++++ crates/catalog/src/tests.rs | 22 ++++++++++++++++++++++ 3 files changed, 54 insertions(+), 1 deletion(-) diff --git a/crates/catalog/src/lib.rs b/crates/catalog/src/lib.rs index 10ca3617d..15914442a 100644 --- a/crates/catalog/src/lib.rs +++ b/crates/catalog/src/lib.rs @@ -325,7 +325,7 @@ impl Catalog { pub fn album_contains(&self, id: AlbumId, p: &Photo) -> bool { match self.albums.get(&id) { // guarded: a smart album testing smart albums can't loop or recurse without end - Some(Album { smart: Some(rules), .. }) => !p.deleted && rules::evaluating(id, || rules.matches(p, self)).unwrap_or(false), + Some(Album { smart: Some(rules), .. }) => !p.deleted && rules::smart_album_holds(id, p.id, || rules.matches(p, self)), Some(a) => a.photos.contains(&p.id), None => false, } diff --git a/crates/catalog/src/rules.rs b/crates/catalog/src/rules.rs index 9cea37f0d..1fda4b0bb 100644 --- a/crates/catalog/src/rules.rs +++ b/crates/catalog/src/rules.rs @@ -413,6 +413,37 @@ thread_local! { static EVALUATING: std::cell::RefCell> = const { std::cell::RefCell::new(Vec::new()) }; } +thread_local! { + /// While one outermost question is answered (is this photo in album X?): what each smart album + /// it leads to answered for each photo, so an album tested many times along the way (X tests Y + /// twice, Y tests Z twice…) is worked out once, not 2^depth times. Cleared when it ends. + static ANSWERS: std::cell::RefCell> = + std::cell::RefCell::new(std::collections::HashMap::new()); +} + +/// Whether `photo` is in smart album `id`, by `matches` (its rules), remembered for the rest of the +/// outermost question and guarded by [`evaluating`] (a loop or a chain too deep holds nothing). +pub(crate) fn smart_album_holds(id: crate::AlbumId, photo: crate::PhotoId, matches: impl FnOnce() -> bool) -> bool { + if let Some(known) = ANSWERS.with_borrow(|a| a.get(&(id, photo)).copied()) { + return known; + } + // the outermost question forgets the answers when it ends, however it ends + struct Forget(bool); + impl Drop for Forget { + fn drop(&mut self) { + if self.0 { + ANSWERS.with_borrow_mut(std::collections::HashMap::clear); + } + } + } + let _forget = Forget(EVALUATING.with_borrow(Vec::is_empty)); + let answer = evaluating(id, matches); + if let Some(a) = answer { + ANSWERS.with_borrow_mut(|known| known.insert((id, photo), a)); + } + answer.unwrap_or(false) +} + /// How deep smart albums may test smart albums before the innermost counts as holding nothing. const MAX_ALBUM_DEPTH: usize = 32; diff --git a/crates/catalog/src/tests.rs b/crates/catalog/src/tests.rs index 354d046e9..571958049 100644 --- a/crates/catalog/src/tests.rs +++ b/crates/catalog/src/tests.rs @@ -565,3 +565,25 @@ fn summaries_name_albums() { let f = Filter { rule_set: Some(rules), ..Default::default() }; assert!(f.describe_with(&c).contains("album isn't “Excluded Photos”")); } + +/// A chain of smart albums each testing the one before twice ("all of: album is X(k-1), album is +/// X(k-1)") costs a pass per album, not 2^depth: each album's answer for a photo is worked out once +/// per question. 25 albums deep stays instant. +#[test] +fn chains_of_smart_albums_stay_linear() { + let mut c = Catalog::new(); + let smart = |id: u64, f: Filter| Album { smart: Some(Box::new(f)), ..Album::new(AlbumId(id), "X") }; + c.apply(Op::AddAlbum { album: smart(1, Filter::default()) }).unwrap(); + for k in 2..=26u64 { + let f: Filter = serde_json::from_value(serde_json::json!({"ruleSet": {"rules": [ + {"field": "album", "op": "is", "value": k - 1}, {"field": "album", "op": "is", "value": k - 1}]}})) + .unwrap(); + c.apply(Op::AddAlbum { album: smart(k, f) }).unwrap(); + } + for i in 0..20 { + photo(&mut c, &format!("p{i}.jpg"), "2026-09-01T10:00:00"); + } + let start = std::time::Instant::now(); + assert_eq!(c.album_count(AlbumId(26)), 20); + assert!(start.elapsed() < std::time::Duration::from_secs(2), "took {:?}", start.elapsed()); +}