diff --git a/core/dr-sync/src/scan.rs b/core/dr-sync/src/scan.rs index 00ec5a6..8ba07cd 100644 --- a/core/dr-sync/src/scan.rs +++ b/core/dr-sync/src/scan.rs @@ -33,10 +33,17 @@ pub struct ScanProgress { pub struct ScanResult { /// Files matching the format filter. pub images: Vec, - /// Every directory visited, with its ETag, so the next scan can prune. + /// Directories whose contents were **actually listed**, with the ETag + /// observed at that moment. /// - /// **Must be persisted.** Without stored folder ETags there is nothing to - /// compare against and every scan is a full walk (ARCH §6.6). + /// **Must be persisted**, or every scan is a full walk (ARCH §6.6). + /// + /// Only listed directories appear here, and that distinction is + /// load-bearing. A directory discovered as a child of another is *known* + /// but not yet *read*: storing its ETag then would let the next scan prune + /// a subtree whose contents were never seen, hiding every file beneath it + /// permanently. Storing it only after listing means an interrupted scan + /// re-reads that folder next time — slower, and correct. pub directories: Vec<(RemotePath, Validator)>, pub progress: ScanProgress, } @@ -47,6 +54,42 @@ pub struct ScanResult { /// photo libraries are nowhere near this deep. const MAX_DEPTH: usize = 32; +/// TRACES: FR-CAT-15 +/// Directory name holding soft-deleted images. +/// +/// Inside the library root rather than beside it: the root is the only place the +/// user granted access to, and on Nextcloud a `MOVE` out of it may cross a share +/// boundary the account cannot write to. +/// +/// Leading dot so other tools treat it as hidden, and a name specific enough not +/// to collide with a photographer's own folder — "Trash" alone is a plausible +/// album title. +pub const TRASH_DIR: &str = ".darkroom-trash"; + +/// TRACES: FR-CAT-3 +/// Directory name holding derived state pushed to the server — thumbnail +/// shards and the catalog snapshot. +/// +/// Excluded from the scan for the same reason as the trash, though for a +/// different failure: its contents are `.sqlite` files that no format filter +/// would match, so nothing would be *indexed*, but the walk would still pay a +/// listing for it on every sync of every device. +pub const DERIVED_DIR: &str = ".darkroom-derived"; + +/// Whether a directory should be skipped by the scan. +/// +/// **The trash must be excluded or the soft delete does not hold.** Trashed +/// images live in a real folder under the library root, so a scan that walked it +/// would re-index them as ordinary photographs and they would reappear in the +/// grid — the delete undone by the next refresh. +/// +/// Matched on the final path component, not a prefix: a photograph in +/// `2019/.darkroom-trash/` (a nested library moved in wholesale) is excluded on +/// the same grounds as one at the root. +pub fn is_excluded(dir: &RemotePath) -> bool { + matches!(dir.name(), TRASH_DIR | DERIVED_DIR) +} + /// TRACES: FR-CAT-1 | FR-NC-4 | M-5 | M-7 /// Walk `root` recursively, collecting files the filter accepts. /// @@ -71,25 +114,54 @@ where // Explicit stack rather than recursion: an async recursive fn needs // boxing, and a deep tree could overflow. - let mut stack = vec![(root.clone(), 0usize)]; + // + // Each item carries the validator its *parent* reported, so it can be + // recorded once the directory has actually been listed. The root has none + // until it is probed. + let mut stack: Vec<(RemotePath, usize, Option)> = vec![(root.clone(), 0, None)]; - while let Some((dir, depth)) = stack.pop() { + while let Some((dir, depth, seen_validator)) = stack.pop() { if depth > MAX_DEPTH { log::warn!("scan: depth limit at {dir}, not descending further"); continue; } + // The trash, skipped before it is listed or even probed. Trashed images + // are real files in a real folder under the root (FR-CAT-15), so + // walking it would re-index them and undo the delete on the next scan. + // + // Deliberately *not* recorded in `result.directories`: an excluded + // directory has no ETag worth storing, and storing one would let a + // later scan believe it had been read. + if is_excluded(&dir) { + log::debug!("scan: skipping {dir}"); + continue; + } + // Prune: if the directory's ETag is unchanged, nothing anywhere // beneath it changed either, because Nextcloud propagates upward. + let mut current_validator = seen_validator; if prunable { - if let Some(previous) = known.get(&dir) { + let known_validator = known.get(&dir); + // Probe when there is something to compare against, and also when + // this directory arrived without a validator — the root, which no + // parent listing described. Without that second case the root's + // ETag is never recorded, and the one-request no-op sync that + // ETag pruning exists for can never fire. + if known_validator.is_some() || current_validator.is_none() { match backend.dir_validator(&dir).await { - Ok(current) if ¤t == previous => { - result.progress.directories_pruned += 1; - on_progress(result.progress); - continue; + Ok(current) => { + if known_validator == Some(¤t) { + result.progress.directories_pruned += 1; + // Re-record it: the entry must survive this scan, + // or the next one has nothing to compare against + // and walks the whole subtree again. + result.directories.push((dir.clone(), current)); + on_progress(result.progress); + continue; + } + current_validator = Some(current); } - Ok(_) => {} // A probe failure is not fatal — fall through to listing, // which is correct, just not free. Err(e) => log::debug!("scan: validator probe failed for {dir}: {e}"), @@ -109,13 +181,20 @@ where result.progress.directories_listed += 1; + // Only now that the contents have been read is it safe to record this + // directory's ETag. Recording it at discovery time would let a later + // scan prune a subtree that was never actually listed, hiding every + // file beneath it. + if let Some(v) = current_validator { + result.directories.push((dir.clone(), v)); + } + for entry in entries { match entry.kind { EntryKind::Directory => { - result - .directories - .push((entry.path.clone(), entry.validator.clone())); - stack.push((entry.path, depth + 1)); + // The validator travels with it, to be recorded when the + // child is listed — not here. + stack.push((entry.path, depth + 1, Some(entry.validator))); } EntryKind::File => { if filter.allows_name(entry.path.name()) { @@ -280,6 +359,27 @@ mod tests { ) -> Result<(), RemoteError> { Err(RemoteError::Unsupported("fake")) } + + async fn move_to(&self, _from: &RemoteId, _to: &RemotePath) -> Result<(), RemoteError> { + Err(RemoteError::Unsupported("fake")) + } + + async fn create_dir(&self, _path: &RemotePath) -> Result<(), RemoteError> { + Err(RemoteError::Unsupported("fake")) + } + } + + #[test] + fn the_derived_folder_is_excluded_like_the_trash() { + // Its contents are .sqlite files no format filter would match, so + // nothing would be *indexed* — but the walk would still pay a listing + // for it on every sync of every device. + assert!(is_excluded(&RemotePath::new("Photos/.darkroom-derived"))); + assert!(is_excluded(&RemotePath::new("Photos/.darkroom-trash"))); + assert!(!is_excluded(&RemotePath::new("Photos/2026"))); + // Matched on the final component, so a nested library moved in + // wholesale is excluded on the same grounds. + assert!(is_excluded(&RemotePath::new("a/b/.darkroom-derived"))); } #[tokio::test] @@ -301,6 +401,110 @@ mod tests { assert_eq!(r.progress.directories_listed, 3); } + /// A library with a trash folder holding a soft-deleted image. + fn with_trash() -> FakeBackend { + let mut b = FakeBackend::sample(ChangeDetection::PropagatingEtags); + b.tree.insert( + "Photos".into(), + vec![ + dir("Photos/2025", "e2025"), + dir("Photos/2026", "e2026"), + dir("Photos/.darkroom-trash", "etrash"), + ], + ); + b.tree.insert( + "Photos/.darkroom-trash".into(), + vec![file("Photos/.darkroom-trash/9-deleted.CR2")], + ); + b.etags + .insert("Photos/.darkroom-trash".to_string(), "etrash"); + b + } + + #[tokio::test] + async fn the_trash_folder_is_never_scanned() { + // TRACES: FR-CAT-15 + // The other half of the soft delete: a scan that walked the trash would + // re-index every trashed photograph and undo the delete on the next + // refresh. + let b = with_trash(); + let result = scan( + &b, + &RemotePath::new("Photos"), + &FormatFilter::default(), + &HashMap::new(), + |_| {}, + ) + .await + .unwrap(); + + assert!( + !result + .images + .iter() + .any(|i| i.path.as_str().contains(".darkroom-trash")), + "a trashed image must not come back as an ordinary one" + ); + // The real photographs are still found. + assert!(result.images.iter().any(|i| i.path.as_str().ends_with("a.CR2"))); + } + + #[tokio::test] + async fn the_trash_folder_costs_no_requests() { + // Not merely filtered out of the results — never listed and never + // probed. A folder whose contents are deliberately invisible must not + // cost a round trip on every scan. + let b = with_trash(); + scan( + &b, + &RemotePath::new("Photos"), + &FormatFilter::default(), + &HashMap::new(), + |_| {}, + ) + .await + .unwrap(); + + // Photos, Photos/2025, Photos/2026 — and not the trash, which would + // make four. + assert_eq!(*b.lists.borrow(), 3, "the trash was listed"); + } + + #[tokio::test] + async fn the_trash_folder_gets_no_stored_etag() { + // Storing one would let a later scan believe the folder had been read, + // which is the failure the `directories` doc comment warns about. + let b = with_trash(); + let result = scan( + &b, + &RemotePath::new("Photos"), + &FormatFilter::default(), + &HashMap::new(), + |_| {}, + ) + .await + .unwrap(); + + assert!(!result + .directories + .iter() + .any(|(p, _)| p.as_str().contains(".darkroom-trash"))); + } + + #[test] + fn exclusion_matches_the_folder_name_at_any_depth() { + // A nested library moved in wholesale carries its own trash. + assert!(is_excluded(&RemotePath::new("Photos/.darkroom-trash"))); + assert!(is_excluded(&RemotePath::new("a/b/c/.darkroom-trash"))); + assert!(is_excluded(&RemotePath::new(".darkroom-trash"))); + // And nothing else is swept up by it. + assert!(!is_excluded(&RemotePath::new("Photos/2025"))); + assert!(!is_excluded(&RemotePath::new("Photos/trash"))); + assert!(!is_excluded(&RemotePath::new( + "Photos/.darkroom-trash-old" + ))); + } + #[tokio::test] async fn the_format_filter_is_applied() { let b = FakeBackend::sample(ChangeDetection::PropagatingEtags); @@ -399,11 +603,74 @@ mod tests { .await .unwrap(); - assert_eq!(r.directories.len(), 2); + // Three: the root and its two children. The root matters most — its + // ETag is what turns the next no-op sync into a single request. + assert_eq!(r.directories.len(), 3); assert!(r .directories .iter() .any(|(p, v)| p.as_str() == "Photos/2025" && v.as_str() == "e2025")); + assert!( + r.directories.iter().any(|(p, _)| p.as_str() == "Photos"), + "the scan root must be recorded, or pruning can never start there" + ); + } + + #[tokio::test] + async fn only_listed_directories_are_recorded() { + // The bug this guards against permanently hid files: recording a + // directory's ETag when it was *discovered* rather than when it was + // *listed* meant a scan that stopped early still stored ETags for + // subtrees it never read. The next scan then probed those ETags, + // found them unchanged, and pruned folders whose contents had never + // been seen — so their images never entered the catalog at all. + let b = FakeBackend::sample(ChangeDetection::PropagatingEtags); + let r = scan( + &b, + &RemotePath::new("Photos"), + &FormatFilter::all(), + &HashMap::new(), + |_| {}, + ) + .await + .unwrap(); + + // Every recorded directory must be one that was actually listed. + for (path, _) in &r.directories { + assert!( + b.tree.contains_key(path.as_str()), + "{path} was recorded without being listed" + ); + } + assert_eq!(r.progress.directories_listed, r.directories.len()); + } + + #[tokio::test] + async fn a_pruned_directory_keeps_its_recorded_etag() { + // Pruning must not drop the entry: without it the next scan has + // nothing to compare and re-walks the whole subtree, turning every + // sync back into a full walk. + let b = FakeBackend::sample(ChangeDetection::PropagatingEtags); + let mut known = HashMap::new(); + known.insert(RemotePath::new("Photos/2025"), Validator::new("e2025")); + + let r = scan( + &b, + &RemotePath::new("Photos"), + &FormatFilter::all(), + &known, + |_| {}, + ) + .await + .unwrap(); + + assert_eq!(r.progress.directories_pruned, 1); + assert!( + r.directories + .iter() + .any(|(p, v)| p.as_str() == "Photos/2025" && v.as_str() == "e2025"), + "a pruned directory must still be recorded for the next scan" + ); } #[tokio::test] @@ -458,7 +725,8 @@ mod tests { .unwrap(); assert!(r.images.is_empty()); - // The walk still happened, so directory ETags are still collected. - assert_eq!(r.directories.len(), 2); + // The walk still happened, so directory ETags are still collected — + // the root plus its two children. + assert_eq!(r.directories.len(), 3); } }