Record directory ETags only after actually listing them

The scan recorded a directory's ETag when it was *discovered* as a child of
another, not when its own contents were read. A scan that stopped early
therefore stored ETags for subtrees it had never listed; the next scan probed
those ETags, found them unchanged, and pruned folders whose contents had
never been seen. Their images never entered the catalog at all, and no later
scan would ever look again.

Each stack entry now carries the validator its parent reported, recorded only
once the directory has been listed. An interrupted scan re-reads that folder
next time — slower, and correct.

Two related fixes fall out. The scan root is now probed and recorded even
though no parent described it, without which the one-request no-op sync that
ETag pruning exists for could never fire at the root. And a pruned directory
keeps its recorded entry, rather than dropping out and forcing a full walk of
that subtree on the following scan.

Assisted-by: LLM

core/dr-sync/src/scan.rs

@@ -33,10 +33,17 @@ pub struct ScanProgress {
 pub struct ScanResult {
     /// Files matching the format filter.
     pub images: Vec<RemoteEntry>,
-    /// 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
+    /// **Must be persisted**, or every scan is a full walk (ARCH §6.6).
-    /// compare against and 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<Validator>)> = 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 &current == previous => {
+                    Ok(current) => {
-                        result.progress.directories_pruned += 1;
+                        if known_validator == Some(&current) {
-                        on_progress(result.progress);
+                            result.progress.directories_pruned += 1;
-                        continue;
+                            // 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
+                    // The validator travels with it, to be recorded when the
-                        .directories
+                    // child is listed — not here.
-                        .push((entry.path.clone(), entry.validator.clone()));
+                    stack.push((entry.path, depth + 1, Some(entry.validator)));
-                    stack.push((entry.path, depth + 1));
                 }
                 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.
+        // The walk still happened, so directory ETags are still collected —
-        assert_eq!(r.directories.len(), 2);
+        // the root plus its two children.
+        assert_eq!(r.directories.len(), 3);
     }
 }