From 6f2b4bb80aa4e00091faa256eae46c5ee5546740 Mon Sep 17 00:00:00 2001 From: Nils Pukropp Date: Sat, 31 Oct 2026 11:11:01 +0100 Subject: [PATCH] fix(fuse,cli): keep a pass going past a failed listing, report skipped restores A listing that failed anywhere in the remote walk ended the whole pass, so a first sync of a large folder on a flaky link walked the tree again and again and never downloaded anything (B271, issue #46). A failed listing below the root now records that folder as a gap: nothing under it is deleted, the rest of the pass applies, and the report names it. A restore that skipped every folder was logged as a successful activity (B269). The daemon now gets the restored and skipped folders back, logs any skip as a failed activity with the reasons, and sends the Rewatch for folders already restored when the database insert fails partway. `pdfs devices join` points to `pdfs activity` for the outcome. --- crates/pdfs-cli/src/main.rs | 6 +++- crates/pdfs-fuse/src/control.rs | 12 +++++-- crates/pdfs-fuse/src/profile.rs | 52 ++++++++++++++++++++++------ crates/pdfs-fuse/src/sync.rs | 50 +++++++++++++++++++++++--- crates/pdfs-fuse/src/sync/planner.rs | 41 ++++++++++++++++++++-- docs/BUGS.md | 28 ++++++++++----- 6 files changed, 160 insertions(+), 29 deletions(-) diff --git a/crates/pdfs-cli/src/main.rs b/crates/pdfs-cli/src/main.rs index 75f6e3b..944869b 100644 --- a/crates/pdfs-cli/src/main.rs +++ b/crates/pdfs-cli/src/main.rs @@ -1458,7 +1458,11 @@ fn cmd_sync_restore(yes: bool, device: Option) -> Result<()> { items: chosen, }, None => CtlRequest::RestoreSyncFolders { items: chosen }, - })?) + })?)?; + // The daemon acks before it works, so the outcome, and any folder it had + // to skip, only shows up later. + println!("Run `pdfs activity` to see which folders were restored."); + Ok(()) } /// Expand a leading `~` in a hand-typed path. The daemon requires an absolute diff --git a/crates/pdfs-fuse/src/control.rs b/crates/pdfs-fuse/src/control.rs index ea23938..55327c2 100644 --- a/crates/pdfs-fuse/src/control.rs +++ b/crates/pdfs-fuse/src/control.rs @@ -1526,9 +1526,17 @@ fn handle_control_conn(core: &Core, username: &str, mountpoint: &Path, stream: U let core = core.clone(); std::thread::spawn(move || { match core.restore_sync_folders(&items, device.as_deref()) { - Ok(message) => { + Ok(outcome) => { core.touch_profile(); - core.log_activity(ActivityKind::Download, &message, "", true); + if !outcome.complete() { + warn!(skipped = ?outcome.skipped, "restore skipped folders"); + } + core.log_activity( + ActivityKind::Download, + outcome.summary(), + outcome.skipped.join("; "), + outcome.complete(), + ); } Err(e) => { warn!(error = %e, "restore failed"); diff --git a/crates/pdfs-fuse/src/profile.rs b/crates/pdfs-fuse/src/profile.rs index 77f1f28..cd24a8d 100644 --- a/crates/pdfs-fuse/src/profile.rs +++ b/crates/pdfs-fuse/src/profile.rs @@ -388,7 +388,7 @@ impl Core { &self, items: &[pdfs_core::control::RestoreItem], device: Option<&str>, - ) -> CoreResult { + ) -> CoreResult { if items.is_empty() { return Err(CoreError::invalid("nothing to restore")); } @@ -434,10 +434,19 @@ impl Core { continue; } }; - let id = self - .db - .sync_folder_add(&local.to_string_lossy(), &item.remote_uid, &device.share_id) - .map_err(|e| CoreError::internal(format!("db: {e:?}")))?; + let id = match self.db.sync_folder_add( + &local.to_string_lossy(), + &item.remote_uid, + &device.share_id, + ) { + Ok(id) => id, + Err(e) => { + // The folders restored before this one still need their + // watches. + let _ = self.sync_tx.send(sync::SyncMsg::Rewatch); + return Err(CoreError::internal(format!("db: {e:?}"))); + } + }; // `ondemand` is applied through the normal request path, which // mounts the FUSE session and handles the folder being busy — the // restore does not get its own copy of that logic. @@ -454,12 +463,35 @@ impl Core { for reason in &skipped { info!(reason, "restore skipped a folder"); } - let mut message = format!("restored {restored} folder(s)"); - if !skipped.is_empty() { - message.push_str(&format!("; skipped: {}", skipped.join(", "))); - } info!(restored, skipped = skipped.len(), "restore complete"); - Ok(message) + Ok(RestoreOutcome { restored, skipped }) + } +} + +/// What a restore did: how many folders it attached, and why it left out the +/// others. +pub(crate) struct RestoreOutcome { + pub(crate) restored: usize, + pub(crate) skipped: Vec, +} + +impl RestoreOutcome { + /// The activity entry's headline. + pub(crate) fn summary(&self) -> String { + match (self.restored, self.skipped.len()) { + (n, 0) => format!("restored {n} folder(s)"), + (0, k) => format!("restored no folders; skipped {k}"), + (n, k) => format!("restored {n} folder(s); skipped {k}"), + } + } + + /// Whether the restore did all it was asked. A skipped folder is a + /// failure the user has to see: the reply went out as "restoring folders" + /// before any work, and a restore that skipped every folder used to be + /// logged as a success, so nothing anywhere said that nothing would + /// download (`docs/BUGS.md` B269). + pub(crate) fn complete(&self) -> bool { + self.skipped.is_empty() } } diff --git a/crates/pdfs-fuse/src/sync.rs b/crates/pdfs-fuse/src/sync.rs index 864e522..8ad48f2 100644 --- a/crates/pdfs-fuse/src/sync.rs +++ b/crates/pdfs-fuse/src/sync.rs @@ -1135,14 +1135,22 @@ impl Core { let uids = self .rt .block_on(self.drive.enumerate_folder_children_node_uids(folder)) - .map_err(|e| format!("enumerate {folder}: {e}"))?; + .map_err(|e| format!("enumerate {folder}: {e}")); + let uids = match unlisted_gap(walk, prefix, uids)? { + Some(uids) => uids, + None => return Ok(()), + }; if uids.is_empty() { return Ok(()); } let nodes = self .rt .block_on(self.drive.enumerate_nodes_light(&uids)) - .map_err(|e| format!("resolve nodes: {e}"))?; + .map_err(|e| format!("resolve nodes: {e}")); + let nodes = match unlisted_gap(walk, prefix, nodes)? { + Some(nodes) => nodes, + None => return Ok(()), + }; // The SDK leaves out a node it cannot decrypt, and says so only in its // log. Its name went with it, so all this can record is the folder. if nodes.len() < uids.len() { @@ -1186,10 +1194,14 @@ impl Core { let sized: HashMap)> = if stale.is_empty() { HashMap::new() } else { - self.rt + let full = self + .rt .block_on(self.drive.enumerate_nodes(&stale)) - .map_err(|e| format!("resolve nodes: {e}"))? - .iter() + .map_err(|e| format!("resolve nodes: {e}")); + let Some(full) = unlisted_gap(walk, prefix, full)? else { + return Ok(()); + }; + full.iter() .map(|n| { ( n.uid.to_string(), @@ -2007,6 +2019,34 @@ struct RemoteWalk<'a> { gaps: &'a mut RemoteGaps, } +/// A listing call's result in [`Core::walk_remote`]. A failure below the root +/// records `prefix` as unlisted and returns `None`, so the walk goes on +/// without that folder. A failure at the root still fails the pass, since +/// nothing would be left to compare. +/// +/// One listing error, after the SDK's own retries, used to fail the whole +/// walk. Nothing is applied before the walk ends, so on a link that drops a +/// request now and then a large first sync started over from nothing on +/// every pass and never got past it (`docs/BUGS.md` B271). +fn unlisted_gap( + walk: &mut RemoteWalk<'_>, + prefix: &str, + result: Result, +) -> Result, PassAbort> { + match result { + Ok(value) => Ok(Some(value)), + Err(error) if prefix.is_empty() => Err(error.into()), + Err(error) => { + warn!( + folder = prefix, + error, "sync: a folder on Drive could not be listed; leaving it out of this pass" + ); + walk.gaps.unlisted(prefix); + Ok(None) + } + } +} + /// Whether the file at `path` is no longer what the scan saw as `planned`. /// A file that is gone has nothing left to lose, so it has not changed. fn changed_since_scan(path: &Path, planned: &LocalItem) -> bool { diff --git a/crates/pdfs-fuse/src/sync/planner.rs b/crates/pdfs-fuse/src/sync/planner.rs index 988bdf6..42264ff 100644 --- a/crates/pdfs-fuse/src/sync/planner.rs +++ b/crates/pdfs-fuse/src/sync/planner.rs @@ -236,6 +236,9 @@ pub(super) struct RemoteGaps { short: HashSet, /// Paths seen, but whose entry could not be read. unread: HashSet, + /// Folders whose listing failed outright, after the SDK's retries. Nothing + /// under one is known, so all of it is left out of the pass (B271). + unlisted: HashSet, } impl RemoteGaps { @@ -249,8 +252,13 @@ impl RemoteGaps { self.unread.insert(rel); } + /// The listing of `folder` failed, so nothing under it was walked. + pub(super) fn unlisted(&mut self, folder: &str) { + self.unlisted.insert(folder.to_string()); + } + pub(super) fn is_empty(&self) -> bool { - self.short.is_empty() && self.unread.is_empty() + self.short.is_empty() && self.unread.is_empty() && self.unlisted.is_empty() } /// Whether `rel` may be missing from `remote` only because the walk could @@ -272,7 +280,7 @@ impl RemoteGaps { let mut at = parent_rel(rel); loop { if remote_dirs.contains_key(at) { - return self.short.contains(at); + return self.short.contains(at) || self.unlisted.contains(at); } if at.is_empty() { return false; @@ -295,6 +303,9 @@ impl RemoteGaps { .iter() .map(|rel| format!("{rel} (cannot be read on Proton Drive)")), ) + .chain(self.unlisted.iter().map(|rel| { + format!("{rel} (could not be listed on Proton Drive; tried again next pass)") + })) .collect(); lines.sort_unstable(); lines @@ -844,6 +855,32 @@ mod file_plan_tests { assert!(gaps.hides("top.txt", &remote, &dirs)); } + /// A folder whose listing failed was walked no further: nothing under it + /// reads as deleted on Drive, and a sibling that listed still does + /// (`docs/BUGS.md` B271). + #[test] + fn a_folder_that_failed_to_list_hides_everything_under_it() { + let remote = HashMap::from([ + ("docs".to_string(), folder()), + ("pics".to_string(), folder()), + ]); + let dirs = HashMap::from([ + (String::new(), uid("root")), + ("docs".to_string(), uid("docs")), + ("pics".to_string(), uid("pics")), + ]); + let mut gaps = RemoteGaps::default(); + gaps.unlisted("docs"); + assert!(gaps.hides("docs/a.txt", &remote, &dirs)); + assert!(gaps.hides("docs/sub/deep.txt", &remote, &dirs)); + assert!(!gaps.hides("docs", &remote, &dirs)); + assert!(!gaps.hides("pics/gone.jpg", &remote, &dirs)); + assert_eq!( + gaps.report(), + ["docs (could not be listed on Proton Drive; tried again next pass)"] + ); + } + #[test] fn a_file_whose_size_could_not_be_read_is_hidden() { let remote: HashMap = HashMap::new(); diff --git a/docs/BUGS.md b/docs/BUGS.md index 319b4ca..ed2a089 100644 --- a/docs/BUGS.md +++ b/docs/BUGS.md @@ -70,9 +70,7 @@ original wording. |---|---|---| | [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 | | [B268](#b268--pdfs-diagnose-says-no-problems-found-while-sync-is-stuck) | `pdfs diagnose` says "No problems found" while sync is stuck | Not fixed | | [B267](#b267--a-key-failure-at-a-start-from-a-warm-cache-shows-as-offline-for-good) | A key failure at a start from a warm cache shows as offline for good | Not fixed | | [B266](#b266--pin-unpin-search-and-node-loads-report-a-database-error-as-offline) | Pin, unpin, search and node loads report a database error as "offline" | Not fixed | @@ -96,6 +94,7 @@ original wording. | Entry | Title | What is left | |---|---|---| +| [B269](#b269--a-restore-that-skipped-every-folder-reports-success) | A restore that skipped every folder reports success | The app shows the result only in the activity feed; no offer to replace or reset the folder in the way | | [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 | | [B72](#b72--pdfs-trash-never-returns) | `pdfs trash` never returns | The slowness of the first refresh is unmeasured | @@ -116,6 +115,7 @@ a live account since. | Entry | Title | |---|---| +| [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 | | [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 | | [B254](#b254--an-edit-in-the-same-second-as-a-new-upload-never-arrives) | An edit in the same second as a new upload never arrives | @@ -414,7 +414,7 @@ watched. ### B271 — One failed listing makes a large first sync start over from nothing -**Status:** Open +**Status:** Fixed, not verified live **Found:** 2026-10-31, code audit after issue #46: a folder of about 19,900 items restored on a second computer sat at `scanning…` with nothing downloaded, and the journal showed `RequestTimeout (http 408)`. @@ -431,8 +431,13 @@ the walk is done. Files that did download keep their baseline rows, so only the but on a first sync that is all of it. Progress is also counted only after a folder's full listing, so a large folder shows `scanning…` with 0 checked for a long time. -**Fix.** Not done. Record a failed subtree as a gap, as undecryptable nodes already are: no -deletions under it, and the rest of the pass still applies. +**Fix.** A listing that fails below the sync root records that folder as a gap in +`RemoteGaps`, next to the folders whose listing came back short. The planner deletes nothing +under a gap, the rest of the pass still applies, and the pass report names the folder as +tried again next pass. A failed listing of the root itself still fails the pass, since there is +nothing left to apply. The progress count inside one large folder is unchanged. + +**Test:** `a_folder_that_failed_to_list_hides_everything_under_it` in `sync::planner`. --- @@ -459,7 +464,7 @@ choice to the user: delete on the other side, or sync from scratch. ### B269 — A restore that skipped every folder reports success -**Status:** Open +**Status:** Partly fixed **Found:** 2026-10-31, code audit after issue #46: the journal showed `restore complete restored=0 skipped=1` several times, and the user saw nothing. **Where:** the `RestoreSyncFolders` and `RestoreDeviceFolders` handler in @@ -474,9 +479,14 @@ result, `restored 0 folder(s); skipped: …`, is then logged as a successful act reason is only in the activity text and the journal. When the database insert fails partway, the `?` also skips the `Rewatch` for the folders already restored. -**Fix.** Not done. Log a restore that skipped folders as a failure, name the reason in the app -and the CLI, and offer to replace or reset the folder in the way. Send the `Rewatch` on every -exit path. +**Fix.** `restore_sync_folders` returns what it restored and what it skipped, with the reason +for each skip. The daemon logs a restore that skipped any folder as a failed activity, with the +reasons as its detail, and warns in the journal. A database error partway now sends the +`Rewatch` for the folders already restored before it returns. `pdfs devices join` points to +`pdfs activity` for the result. + +Left: the app shows the result only in the activity feed, and nothing offers to replace or reset +the folder in the way. ---