Merge remote-tracking branch 'origin/docs/bugs-issue-audit' into fix/sync-scheduling-watches

# Conflicts:
#	docs/BUGS.md

crates/pdfs-fuse/src/sync.rs

@@ -48,11 +48,11 @@ mod engine;
 mod local_scan;
 mod model;
 mod planner;
-#[cfg(test)]
-use engine::Running;
 pub(crate) use engine::SYNC_CONCURRENCY;
 pub(crate) use engine::{EngineHealth, SyncMsg, spawn};
 #[cfg(test)]
+use engine::{Running, burst_order, keep_wanted};
+#[cfg(test)]
 use engine::{classify, event_target, settle_with};
 #[cfg(test)]
 use local_scan::is_write_mode;
@@ -2478,6 +2478,28 @@ mod tests {
         assert_eq!(health.report().panics, 1);
     }
 
+    /// Folders named by id run before the rest of a `ReconcileAll` in the
+    /// same burst, so a folder just added does not wait behind a pass over
+    /// every other folder (`docs/BUGS.md` B260).
+    #[test]
+    fn named_folders_run_before_the_rest_of_a_burst() {
+        let ids: HashSet<i64> = [9, 4].into();
+        assert_eq!(burst_order(&ids, &[1, 4, 6, 9]), [4, 9, 1, 6]);
+        assert_eq!(burst_order(&HashSet::new(), &[1, 4]), [1, 4]);
+        // A folder the list no longer has still gets the pass it asked for.
+        assert_eq!(burst_order(&[12].into(), &[1]), [12, 1]);
+    }
+
+    /// A folder removed and added again keeps its watch under the new id;
+    /// a folder that is gone loses its watch (`docs/BUGS.md` B272).
+    #[test]
+    fn a_re_added_folder_keeps_its_watch_under_the_new_id() {
+        let mut have = vec![(PathBuf::from("/a"), 1), (PathBuf::from("/b"), 2)];
+        let dropped = keep_wanted(&mut have, &[(PathBuf::from("/a"), 7)]);
+        assert_eq!(have, [(PathBuf::from("/a"), 7)]);
+        assert_eq!(dropped, [PathBuf::from("/b")]);
+    }
+
     /// While a pass runs, the report names its folder, which is what tells a
     /// long pass apart from an idle engine.
     #[test]

crates/pdfs-fuse/src/sync/engine.rs

@@ -29,6 +29,9 @@ const POLL_INTERVAL: Duration = Duration::from_secs(120);
 /// unusable done one-at-a-time; this bounds concurrency like the bulk-upload
 /// engine does.
 pub(crate) const SYNC_CONCURRENCY: usize = 8;
+/// How long after a failed watch the engine tries the watches again. Each try
+/// walks the folder's whole tree, so not on every burst.
+const REWATCH_RETRY: Duration = Duration::from_secs(600);
 
 /// A message to the sync engine's control thread.
 pub(crate) enum SyncMsg {
@@ -183,24 +186,35 @@ fn engine_loop(core: Core, rx: Receiver<SyncMsg>) {
 
     let tx_events = core.sync_tx.clone();
     let watched_events = watched.clone();
-    let mut watcher =
-        match notify::recommended_watcher(move |res: notify::Result<notify::Event>| {
-            let Ok(event) = res else { return };
-            // Ignore pure metadata/access noise; content changes are what matter.
-            if matches!(event.kind, notify::EventKind::Access(_)) {
-                return;
-            }
-            let msg = event_target(&watched_events.lock(), &event.paths);
-            let _ = tx_events.send(msg);
-        }) {
-            Ok(w) => w,
+    let handler = move |res: notify::Result<notify::Event>| {
+        let event = match res {
+            Ok(event) => event,
+            // Running out of inotify watches for a new subfolder, say: that
+            // subfolder's changes are then found by the poll only (B262).
             Err(e) => {
-                warn!(error = %e, "filesystem watcher unavailable; sync is poll-only");
-                // Still run the loop so polling and explicit SyncNow work.
-                poll_only_loop(&core, rx);
+                warn!(error = %e, "sync: filesystem watcher error");
                 return;
             }
         };
+        // Ignore pure metadata/access noise; content changes are what matter.
+        if matches!(event.kind, notify::EventKind::Access(_)) {
+            return;
+        }
+        let msg = event_target(&watched_events.lock(), &event.paths);
+        let _ = tx_events.send(msg);
+    };
+    // The local walk skips symlinks, so following them here would only watch
+    // trees outside the folder, and run passes over it for changes made there.
+    let config = notify::Config::default().with_follow_symlinks(false);
+    let mut watcher = match notify::RecommendedWatcher::new(handler, config) {
+        Ok(w) => w,
+        Err(e) => {
+            warn!(error = %e, "filesystem watcher unavailable; sync is poll-only");
+            // Still run the loop so polling and explicit SyncNow work.
+            poll_only_loop(&core, rx);
+            return;
+        }
+    };
 
     // Periodic remote poll.
     {
@@ -218,8 +232,9 @@ fn engine_loop(core: Core, rx: Receiver<SyncMsg>) {
             .ok();
     }
 
-    rewatch(&core, &mut watcher, &watched);
-    reconcile_all(&core);
+    let mut unwatched = rewatch(&core, &mut watcher, &watched);
+    let mut last_rewatch = Instant::now();
+    run_burst(&core, &HashSet::new(), true);
 
     while let Ok(msg) = rx.recv() {
         let mut ids: HashSet<i64> = HashSet::new();
@@ -232,19 +247,16 @@ fn engine_loop(core: Core, rx: Receiver<SyncMsg>) {
             return;
         }
 
+        // A folder whose watch failed is tried again now and then, not only
+        // when the folder set changes.
+        if unwatched && last_rewatch.elapsed() >= REWATCH_RETRY {
+            do_rewatch = true;
+        }
         if do_rewatch {
-            rewatch(&core, &mut watcher, &watched);
-        }
-        if all {
-            reconcile_all(&core);
-        } else {
-            for id in ids {
-                if core.sync_paused() {
-                    break;
-                }
-                run_pass_by_id(&core, id);
-            }
+            unwatched = rewatch(&core, &mut watcher, &watched);
+            last_rewatch = Instant::now();
         }
+        run_burst(&core, &ids, all);
     }
 }
 
@@ -261,7 +273,7 @@ fn poll_only_loop(core: &Core, rx: Receiver<SyncMsg>) {
             }
         });
     }
-    reconcile_all(core);
+    run_burst(core, &HashSet::new(), true);
     while let Ok(msg) = rx.recv() {
         let mut ids: HashSet<i64> = HashSet::new();
         let mut all = false;
@@ -272,19 +284,59 @@ fn poll_only_loop(core: &Core, rx: Receiver<SyncMsg>) {
             info!("sync engine stopping");
             return;
         }
-        if all {
-            reconcile_all(core);
-        } else {
-            for id in ids {
-                if core.sync_paused() {
-                    break;
-                }
-                run_pass_by_id(core, id);
+        run_burst(core, &ids, all);
+    }
+}
+
+/// Run the passes one burst asked for: the folders named by id, then, when the
+/// burst also holds a `ReconcileAll`, every other folder.
+///
+/// The named folders go first. A poll's `ReconcileAll` in the same burst used
+/// to take their place, and a folder just added, which has the highest id,
+/// waited behind a full pass over every other folder (`docs/BUGS.md` B260).
+///
+/// Skipped while sync is paused, and cut short when a pause lands between
+/// passes: the resume sends a fresh [`SyncMsg::ReconcileAll`], which walks
+/// everything the watcher reported meanwhile.
+fn run_burst(core: &Core, ids: &HashSet<i64>, all: bool) {
+    if core.sync_paused() {
+        debug!("sync: paused, skipping reconcile");
+        return;
+    }
+    let folders = if all {
+        match core.db.sync_folder_list() {
+            Ok(folders) => folders,
+            Err(e) => {
+                warn!(error = ?e, "sync: cannot list folders");
+                Vec::new()
             }
         }
+    } else {
+        Vec::new()
+    };
+    let listed: Vec<i64> = folders.iter().map(|f| f.id).collect();
+    for id in burst_order(ids, &listed) {
+        if core.sync_paused() {
+            break;
+        }
+        // A folder named by id is read again: it may have been added or
+        // changed after the list was read.
+        match folders.iter().find(|f| f.id == id) {
+            Some(folder) if !ids.contains(&id) => run_pass(core, folder),
+            _ => run_pass_by_id(core, id),
+        }
     }
 }
 
+/// The order of a burst's passes: the folders named by id, lowest first, then
+/// the rest of `listed` in its own order.
+pub(super) fn burst_order(ids: &HashSet<i64>, listed: &[i64]) -> Vec<i64> {
+    let mut order: Vec<i64> = ids.iter().copied().collect();
+    order.sort_unstable();
+    order.extend(listed.iter().copied().filter(|id| !ids.contains(id)));
+    order
+}
+
 /// Absorb the rest of an event burst, returning once the watcher has been quiet
 /// for `quiet` or `cap` has elapsed since the first event — whichever comes
 /// first. The caller has already classified the event that opened the burst.
@@ -356,26 +408,6 @@ pub(super) fn classify(
     true
 }
 
-/// Reconcile every mirror folder in turn. Skipped while sync is paused: the
-/// resume sends a fresh [`SyncMsg::ReconcileAll`], which walks everything the
-/// watcher reported meanwhile.
-fn reconcile_all(core: &Core) {
-    if core.sync_paused() {
-        debug!("sync: paused, skipping reconcile");
-        return;
-    }
-    let folders = match core.db.sync_folder_list() {
-        Ok(f) => f,
-        Err(e) => {
-            warn!(error = ?e, "sync: cannot list folders");
-            return;
-        }
-    };
-    for folder in folders {
-        run_pass(core, &folder);
-    }
-}
-
 /// The reconcile a watcher event asks for: the one mirror folder all its paths
 /// lie in, so an edit in one folder does not walk every other one too. An event
 /// that spans folders, or that no watched folder claims (a watch list a moment
@@ -396,7 +428,8 @@ pub(super) fn event_target(watched: &[(PathBuf, i64)], paths: &[PathBuf]) -> Syn
 }
 
 /// Bring the filesystem watches in line with the current mirror-folder set:
-/// watch newly-added folders, drop removed ones.
+/// watch newly-added folders, drop removed ones. Returns whether a folder is
+/// left unwatched because its watch failed.
 ///
 /// The watch calls run on a copy, outside the lock: the watcher's event
 /// callback takes the same lock, and a backend that hands a watch request to
@@ -405,7 +438,7 @@ fn rewatch(
     core: &Core,
     watcher: &mut notify::RecommendedWatcher,
     watched: &Mutex<Vec<(PathBuf, i64)>>,
-) {
+) -> bool {
     let want: Vec<(PathBuf, i64)> = match core.db.sync_folder_list() {
         Ok(f) => f
             .into_iter()
@@ -414,30 +447,60 @@ fn rewatch(
             .collect(),
         Err(e) => {
             warn!(error = ?e, "sync: cannot list folders to watch");
-            return;
+            return false;
         }
     };
     let mut have = watched.lock().clone();
     // Drop watches no longer wanted.
-    have.retain(|(path, _)| {
-        if want.iter().any(|(p, _)| p == path) {
-            true
-        } else {
-            let _ = watcher.unwatch(path);
-            false
-        }
-    });
+    for path in keep_wanted(&mut have, &want) {
+        let _ = watcher.unwatch(&path);
+    }
     // Add newly wanted watches.
+    let mut failed = false;
     for (path, id) in &want {
         if have.iter().any(|(p, _)| p == path) {
             continue;
         }
         match watcher.watch(path, RecursiveMode::Recursive) {
             Ok(()) => have.push((path.clone(), *id)),
-            Err(e) => warn!(path = %path.display(), error = %e, "sync: watch failed"),
+            Err(e) => {
+                // A recursive watch that fails partway keeps the watches it had
+                // added. Their events would match no folder and run a pass over
+                // every one, and they hold watches the next folder needs (B262).
+                let _ = watcher.unwatch(path);
+                failed = true;
+                warn!(
+                    path = %path.display(),
+                    error = %e,
+                    "sync: watch failed; the poll finds this folder's changes until a retry \
+                     works. A low fs.inotify.max_user_watches is the usual cause"
+                );
+            }
         }
     }
     *watched.lock() = have;
+    failed
+}
+
+/// Keep the watches in `have` that `want` still asks for, and return the paths
+/// of the others, to unwatch.
+///
+/// A path is matched alone, but takes the id `want` gives it: a folder removed
+/// and added again in one burst keeps its watch, and its events go to the new
+/// row rather than to the removed one (`docs/BUGS.md` B272).
+pub(super) fn keep_wanted(have: &mut Vec<(PathBuf, i64)>, want: &[(PathBuf, i64)]) -> Vec<PathBuf> {
+    let mut dropped = Vec::new();
+    have.retain_mut(|(path, id)| match want.iter().find(|(p, _)| p == path) {
+        Some((_, wanted)) => {
+            *id = *wanted;
+            true
+        }
+        None => {
+            dropped.push(path.clone());
+            false
+        }
+    });
+    dropped
 }
 
 // ---- reconcile ------------------------------------------------------------

docs/BUGS.md

@@ -69,7 +69,6 @@ original wording.
 | Entry | Title | What is left |
 |---|---|---|
 | [B273](#b273--a-restore-proposes-another-computers-paths-when-the-two-share-a-hostname) | A restore proposes another computer's paths when the two share a hostname | Not fixed |
-| [B272](#b272--after-a-folder-is-removed-and-added-again-its-file-events-go-to-the-old-id) | After a folder is removed and added again, its file events go to the old id | Not fixed |
 | [B271](#b271--one-failed-listing-makes-a-large-first-sync-start-over-from-nothing) | One failed listing makes a large first sync start over from nothing | Not fixed |
 | [B270](#b270--one-file-left-in-a-mirror-folder-clears-the-wipe-guard-for-all-the-others) | One file left in a mirror folder clears the wipe guard for all the others | Not fixed |
 | [B269](#b269--a-restore-that-skipped-every-folder-reports-success) | A restore that skipped every folder reports success | Not fixed |
@@ -79,9 +78,7 @@ original wording.
 | [B265](#b265--address-keys-whose-token-is-for-an-inactive-account-key-never-open) | Address keys whose token is for an inactive account key never open | Needs an SDK change; not reproduced |
 | [B264](#b264--a-mount-that-fails-after-starting-its-workers-leaves-them-running) | A mount that fails after starting its workers leaves them running | Not fixed |
 | [B263](#b263--a-mount-that-can-never-succeed-retries-every-5-s-and-tells-only-the-journal) | A mount that can never succeed retries every 5 s and tells only the journal | Not fixed |
-| [B262](#b262--watching-a-large-synced-folder-holds-up-the-sync-engine-and-a-failed-watch-is-never-retried) | Watching a large synced folder holds up the sync engine, and a failed watch is never retried | Not fixed |
 | [B261](#b261--a-folder-that-has-never-synced-reads-up-to-date) | A folder that has never synced reads "up to date" | Not fixed |
-| [B260](#b260--the-poll-pushes-a-new-folders-first-sync-behind-every-other-folder) | The poll pushes a new folder's first sync behind every other folder | Not fixed |
 | [B134](#b134--a-file-deleted-online-while-the-drain-checks-its-write-is-kept-as-a-conflict-copy) | A file deleted online while the drain checks its write is kept as a conflict copy | Only with `local_first` off |
 | [B91](#b91--the-daemon-hung-overnight-and-nothing-could-say-what-it-was-waiting-on) | The daemon hung overnight, and nothing could say what it was waiting on | The cause is not diagnosed; the daemon now reports where it is stuck and is restarted |
 | [B67](#b67--fresh-state-restore-can-silently-select-a-new-device-high-22) | Fresh-state restore can silently select a new device (HIGH-22) | An ambiguous device after a loss of local state is not asked about |
@@ -94,6 +91,7 @@ original wording.
 
 | Entry | Title | What is left |
 |---|---|---|
+| [B262](#b262--watching-a-large-synced-folder-holds-up-the-sync-engine-and-a-failed-watch-is-never-retried) | Watching a large synced folder holds up the sync engine, and a failed watch is never retried | The first pass still waits for the watch; a failed watch is not shown in the app |
 | [B259](#b259--the-sync-engine-can-die-or-hang-and-nothing-shows-it-until-a-restart) | The sync engine can die or hang, and nothing shows it until a restart | No pass deadline; an ended engine is reported, not restarted; the trigger in #53 is not known |
 | [B199](#b199--a-stopped-share-stays-on-the-shared-by-me-page-for-minutes) | A stopped share stays on the Shared by me page for minutes | The share itself is not deleted; the SDK has no call for it |
 | [B88](#b88--pdfs-pin-rename-move-delete-and-sharing-still-cannot-name-a-path-under-a-secondary-mount) | `pdfs pin`, `rename`, `move`, `delete` and sharing still cannot name a path under a secondary mount | `pin`, `unpin`, opening a file, thumbnails, uploads, sharing and versions |
@@ -115,6 +113,8 @@ a live account since.
 
 | Entry | Title |
 |---|---|
+| [B272](#b272--after-a-folder-is-removed-and-added-again-its-file-events-go-to-the-old-id) | After a folder is removed and added again, its file events go to the old id |
+| [B260](#b260--the-poll-pushes-a-new-folders-first-sync-behind-every-other-folder) | The poll pushes a new folder's first sync behind every other folder |
 | [B258](#b258--journalctl--p-warning-shows-nothing-whatever-the-daemon-logged) | `journalctl -p warning` shows nothing, whatever the daemon logged |
 | [B256](#b256--pdfs-copy-right-after-a-save-copies-the-old-version) | `pdfs copy` right after a save copies the old version |
 | [B255](#b255--a-synced-folder-whose-files-are-all-gone-here-is-stuck-for-good) | A synced folder whose files are all gone here is stuck for good |
@@ -395,7 +395,7 @@ written before it existed.
 
 ### B272 — After a folder is removed and added again, its file events go to the old id
 
-**Status:** Open
+**Status:** Fixed, not verified live
 **Found:** 2026-10-31, code audit after issue #53.
 **Where:** `rewatch` in `crates/pdfs-fuse/src/sync/engine.rs`
 
@@ -407,8 +407,10 @@ two-minute poll, and the same goes for every later edit until the daemon restart
 path is still wanted, so the old entry stays with the removed folder's id. Its events become
 `Reconcile(old id)`, `sync_folder_get` finds no row, and the request is dropped.
 
-**Fix.** Not done. Compare `(path, id)` pairs, and update the id of a path that is still
-watched.
+**Fix.** `keep_wanted` keeps a watch whose path is still wanted and gives it the id the
+folder list has now.
+
+**Test:** `a_re_added_folder_keeps_its_watch_under_the_new_id` in `pdfs-fuse`.
 
 ---
 
@@ -613,7 +615,8 @@ show it (a state file, and `STATUS=` to systemd).
 
 ### B262 — Watching a large synced folder holds up the sync engine, and a failed watch is never retried
 
-**Status:** Open
+**Status:** Partly fixed. A watch is still set up before the first pass, and a failed watch
+is not shown in the folder's state.
 **Found:** 2026-10-31, code audit after issue #53.
 **Where:** `rewatch` and `engine_loop` in `crates/pdfs-fuse/src/sync/engine.rs`
 
@@ -631,8 +634,16 @@ line, the folder is never watched, and later changes are found only by the two-m
   and they use up watches the next folder needs.
 - Errors the watcher reports later are dropped in its callback.
 
-**Fix.** Not done. Reconcile first, unwatch after a partial failure and retry with a back-off,
-turn off symlink following, and show a watch failure in the folder's state.
+**Fix.**
+- A failed watch is unwatched, which removes the watches it had added under the folder, and
+  is tried again every 10 minutes. The warning names `fs.inotify.max_user_watches`.
+- The watcher no longer follows symlinks.
+- Errors the watcher reports are logged.
+- Not done: the watch still runs before the folder's first pass. Watching first is what keeps
+  a change made during the pass from being missed, so the order stays; the cost is one walk
+  of a new folder's tree. A failed watch is not shown in the folder's state yet.
+
+**Test:** none for the watch failure itself; it needs the inotify limit reached.
 
 ---
 
@@ -660,7 +671,7 @@ account-wide pause on the folder rows.
 
 ### B260 — The poll pushes a new folder's first sync behind every other folder
 
-**Status:** Open
+**Status:** Fixed, not verified live
 **Found:** 2026-10-31, code audit after issue #53.
 **Where:** `engine_loop` in `crates/pdfs-fuse/src/sync/engine.rs`
 
@@ -673,8 +684,11 @@ running. When a burst holds both a `ReconcileAll` and a folder's `Reconcile(id)`
 only `reconcile_all`, which goes in id order. A new folder has the highest id, so it always
 goes last.
 
-**Fix.** Not done. Run the folders asked for by id before the rest, and don't queue a poll
-while a pass runs.
+**Fix.** `run_burst` runs the folders a burst names by id first, lowest id first, and then
+every other folder when the burst also holds a `ReconcileAll`. A named folder is read again
+before its pass. A pause that lands between passes now stops the rest of the burst.
+
+**Test:** `named_folders_run_before_the_rest_of_a_burst` in `pdfs-fuse`.
 
 ---