diff --git a/crates/pdfs-core/src/control.rs b/crates/pdfs-core/src/control.rs index 74c3533..d567c14 100644 --- a/crates/pdfs-core/src/control.rs +++ b/crates/pdfs-core/src/control.rs @@ -429,10 +429,12 @@ pub enum Request { /// Retry a backed-off queued op now instead of waiting out its backoff; /// every failed op when `id` is `None`. Replies with [`Response::Ok`]. RetryPendingOp { id: Option }, - /// List the `(sync-conflict …)` copies under My Files. Replies with + /// List the `(sync-conflict …)` copies under My Files and in synced + /// folders. Replies with /// [`Response::Conflicts`]. ListConflicts, - /// Resolve one conflict copy (`path` mountpoint-relative or absolute). + /// Resolve one conflict copy (`path` mountpoint-relative, or absolute for a + /// copy in a synced folder). /// Everything removed goes to the Proton trash. Replies with /// [`Response::Ok`]. ResolveConflict { path: String, keep: ConflictKeep }, @@ -701,9 +703,10 @@ pub struct DeviceInfo { /// [`Response::Conflicts`]). #[derive(Serialize, Deserialize, Debug, Clone)] pub struct ConflictInfo { - /// Mountpoint-relative path of the copy. + /// Mountpoint-relative path of the copy, or its absolute local path when it + /// sits in a synced folder. pub path: String, - /// Mountpoint-relative path of the file it is a copy of. + /// Path of the file it is a copy of, in the same form as `path`. pub original_path: String, /// False when the original is gone and only the copy is left. pub original_exists: bool, diff --git a/crates/pdfs-core/src/db/activity.rs b/crates/pdfs-core/src/db/activity.rs index 3998598..8d74237 100644 --- a/crates/pdfs-core/src/db/activity.rs +++ b/crates/pdfs-core/src/db/activity.rs @@ -29,6 +29,17 @@ impl Db { Ok(()) } + /// Whether the kept log already holds an entry of `kind` for `target`. Lets + /// the daemon report something once across restarts rather than once per + /// run, without keeping a second table in step with the log. + pub fn activity_has(&self, kind: ActivityKind, target: &str) -> Result { + let kind = serde_json::to_string(&kind)?; + let conn = self.read(); + let mut stmt = + conn.prepare("SELECT 1 FROM activity WHERE kind = ?1 AND target = ?2 LIMIT 1")?; + Ok(stmt.exists(params![kind, target])?) + } + /// The most recent activity, newest first, capped at `limit` entries. Rows /// whose stored `kind` no longer parses (written by an older build) are /// skipped rather than failing the whole read. diff --git a/crates/pdfs-core/src/db/tests.rs b/crates/pdfs-core/src/db/tests.rs index ec66b33..50683a8 100644 --- a/crates/pdfs-core/src/db/tests.rs +++ b/crates/pdfs-core/src/db/tests.rs @@ -1414,6 +1414,29 @@ fn activity_reads_back_newest_first() { assert_eq!(db.activity_list(1).unwrap().len(), 1); } +#[test] +fn activity_has_finds_only_matching_kind_and_target() { + let db = Db::open_in_memory().unwrap(); + db.activity_add(&activity( + "a (sync-conflict 1).txt", + ActivityKind::Conflict, + false, + )) + .unwrap(); + db.activity_add(&activity("b.txt", ActivityKind::Upload, true)) + .unwrap(); + + assert!( + db.activity_has(ActivityKind::Conflict, "a (sync-conflict 1).txt") + .unwrap() + ); + assert!( + !db.activity_has(ActivityKind::Upload, "a (sync-conflict 1).txt") + .unwrap() + ); + assert!(!db.activity_has(ActivityKind::Conflict, "b.txt").unwrap()); +} + #[test] fn activity_prunes_to_the_keep_limit() { let db = Db::open_in_memory().unwrap(); diff --git a/crates/pdfs-fuse/src/conflicts.rs b/crates/pdfs-fuse/src/conflicts.rs index 77868c1..e076928 100644 --- a/crates/pdfs-fuse/src/conflicts.rs +++ b/crates/pdfs-fuse/src/conflicts.rs @@ -14,15 +14,38 @@ //! that is open, being written, or still owed an upload, for the same reason the //! sweep does: the node is about to change under the decision. //! -//! Only copies under My Files are handled here. A copy inside a mirror folder is -//! a local file first; the sync engine owns it. +//! Copies under My Files are named by their mountpoint-relative path. Copies +//! inside a synced folder are named by their absolute local path, so a resolve +//! request routes to the on-demand mount that serves them. A copy inside a +//! mirror folder is listed too, but it is a local file first: the sync engine +//! owns it, so it is resolved by editing the folder, not through here. use super::*; use pdfs_core::control::{ConflictInfo, ConflictKeep}; use sweep::conflict_base_name; impl Core { - /// Every conflict copy under My Files, with the file it is a copy of. + /// Where the user finds a conflict copy: its mountpoint-relative path under + /// My Files, or its absolute local path inside a synced folder. `None` for a + /// node no location of this machine shows, such as one in another + /// computer's device tree. + /// + /// The sweep only flags what this can name, and the listing uses it too. A + /// flagged copy the listing cannot see reads as resolved in the activity + /// feed while it still sits in the folder (`docs/BUGS.md` B99). + pub(crate) fn conflict_path(&self, roots: &SearchRoots, uid: &NodeUid) -> Option { + let uid = uid.to_string(); + if let Ok(Some(path)) = self + .db + .path_relative_to(&self.primary_root_uid.to_string(), &uid) + { + return Some(path); + } + let drive_path = self.db.node_path(&uid).ok().flatten()?; + roots.resolve(&drive_path) + } + + /// Every conflict copy this machine shows, with the file it is a copy of. pub(crate) fn list_conflicts(&self) -> CoreResult> { let stored = self .db @@ -37,13 +60,13 @@ impl Core { for node in &nodes { by_parent_name.insert((node.parent_uid.as_ref(), node.name.as_str()), node); } - let root = self.primary_root_uid.to_string(); + let roots = self.search_roots(); let mut items = Vec::new(); for node in &nodes { let Some(base) = conflict_base_name(&node.name) else { continue; }; - let Ok(Some(path)) = self.db.path_relative_to(&root, &node.uid.to_string()) else { + let Some(path) = self.conflict_path(&roots, &node.uid) else { continue; }; let original = by_parent_name diff --git a/crates/pdfs-fuse/src/control.rs b/crates/pdfs-fuse/src/control.rs index c024281..d3d6b8e 100644 --- a/crates/pdfs-fuse/src/control.rs +++ b/crates/pdfs-fuse/src/control.rs @@ -185,6 +185,38 @@ fn route_to_mount(core: &Core, mountpoint: &Path, path: &str) -> CoreResult<(Cor rel_to_mount(mountpoint, path).map(|rel| (core.clone(), rel)) } +/// [`route_to_mount`] for a conflict copy. The listing names a copy inside a +/// synced folder by its absolute local path (`docs/BUGS.md` B99), so the +/// resolve has to run against the on-demand mount serving it. A mirror folder +/// has no mount to run against: its copy is an ordinary local file. +fn route_conflict(core: &Core, mountpoint: &Path, path: &str) -> CoreResult<(Core, PathBuf)> { + let routed = route_to_mount(core, mountpoint, path).map_err(|e| { + let p = Path::new(path); + let mirrored = core + .db + .sync_folder_list() + .unwrap_or_default() + .iter() + .any(|folder| folder.mode == "mirror" && p.starts_with(&folder.local_path)); + if mirrored { + CoreError::invalid(format!( + "{path} is in a mirrored folder; rename or delete the local file there" + )) + } else { + e + } + })?; + if routed + .1 + .components() + .all(|component| matches!(component, Component::Normal(_) | Component::CurDir)) + { + Ok(routed) + } else { + Err(CoreError::invalid(format!("{path} escapes the mountpoint"))) + } +} + /// Turn a CLI-supplied path into a mountpoint-relative path. An absolute path /// must live under `mountpoint`; a relative path is taken as already relative to /// the mount root. @@ -1123,13 +1155,15 @@ fn handle_control_conn(core: &Core, username: &str, mountpoint: &Path, stream: U Ok(items) => CtlResponse::Conflicts { items }, Err(e) => CtlResponse::error(e), }, - Ok(CtlRequest::ResolveConflict { path, keep }) => match rel_to_mount(mountpoint, &path) { - Ok(rel) => match core.resolve_conflict(&rel, &keep) { - Ok(message) => CtlResponse::Ok { message }, + Ok(CtlRequest::ResolveConflict { path, keep }) => { + match route_conflict(core, mountpoint, &path) { + Ok((core, rel)) => match core.resolve_conflict(&rel, &keep) { + Ok(message) => CtlResponse::Ok { message }, + Err(e) => CtlResponse::error(e), + }, Err(e) => CtlResponse::error(e), - }, - Err(e) => CtlResponse::error(e), - }, + } + } Ok(CtlRequest::ListPendingOps) => match core.pending_op_infos() { Ok(items) => CtlResponse::PendingOps { items }, Err(e) => CtlResponse::error(e), diff --git a/crates/pdfs-fuse/src/lib.rs b/crates/pdfs-fuse/src/lib.rs index 1f03118..90fc88d 100644 --- a/crates/pdfs-fuse/src/lib.rs +++ b/crates/pdfs-fuse/src/lib.rs @@ -558,9 +558,10 @@ struct Core { /// instead of polling or blocking for the whole run. See /// [`Core::await_trash_refresh`]. trash_progress: Arc, - /// Conflict copies the sweep has already flagged as needing attention this - /// run, so a divergent `(sync-conflict …)` file is logged once rather than - /// on every sweep pass. See [`Core::run_conflict_sweep_loop`]. + /// Conflict copies the sweep has already checked against the activity log + /// this run, so each pass does not ask the database again. The log itself + /// is what keeps a copy from being flagged again after a restart. See + /// [`Core::run_conflict_sweep_loop`] and `docs/BUGS.md` B99. conflict_notified: Arc>>, /// Whether the conflict sweep may actually trash the duplicates it finds, or /// only report them. Resolved once at mount from config + environment diff --git a/crates/pdfs-fuse/src/sweep.rs b/crates/pdfs-fuse/src/sweep.rs index 3ed5bb1..5e1422b 100644 --- a/crates/pdfs-fuse/src/sweep.rs +++ b/crates/pdfs-fuse/src/sweep.rs @@ -109,6 +109,7 @@ impl Core { by_parent_name.insert((parent, node.name.as_str()), node); } + let roots = self.search_roots(); let online = self.online.load(std::sync::atomic::Ordering::Relaxed); for node in &nodes { if node.trashed || node.is_folder() { @@ -132,7 +133,16 @@ impl Core { self.remove_conflict_copy(node, &base); } } - SweepAction::Flag(detail) => self.flag_conflict(&node.uid, &node.name, detail), + // Only flag what the conflict listing can show. Anything else + // would sit in the feed as "resolved" with no way to act on + // it (`docs/BUGS.md` B99). + SweepAction::Flag(detail) => { + if self.conflict_path(&roots, &node.uid).is_some() { + self.flag_conflict(&node.uid, &node.name, detail); + } else { + debug!(uid = %node.uid, name = %node.name, "conflict sweep: copy is outside every location; not flagged"); + } + } } } } @@ -273,12 +283,21 @@ impl Core { busy } - /// Surface a conflict copy that needs the user's attention, at most once per - /// daemon run so the activity feed is not spammed each pass. + /// Surface a conflict copy that needs the user's attention, once. The + /// in-memory set spares the database each pass; the activity log itself is + /// what keeps a restart from logging the same copy again (`docs/BUGS.md` + /// B99). A copy's name carries its own timestamp, so the name identifies it. fn flag_conflict(&self, uid: &NodeUid, name: &str, detail: String) { if !self.conflict_notified.lock().insert(uid.clone()) { return; } + match self.db.activity_has(ActivityKind::Conflict, name) { + Ok(false) => {} + Ok(true) => return, + // Cannot tell: a repeat entry is harmless, a missing one hides a + // conflict. + Err(e) => warn!(%uid, name, error = ?e, "conflict sweep: activity check failed"), + } debug!(%uid, name, detail, "conflict sweep: flagged divergent conflict copy"); self.log_activity(ActivityKind::Conflict, name, detail, false); } diff --git a/docs/BUGS.md b/docs/BUGS.md index 5bc7129..7731f55 100644 --- a/docs/BUGS.md +++ b/docs/BUGS.md @@ -12,6 +12,42 @@ Conventions: --- +## B99 — Conflict copies in synced folders are logged again on every start and shown as "Resolved" + +**Status:** Fixed (unverified). No daemon carrying the change has been restarted against the +affected copies yet. +**Found:** 2026-09-27. The Activity page listed the same six conflict copies every day, e.g. +`Conflict [EMBER] Oshi no Ko S2 - 12 (sync-conflict 1786815864).mkv` with `Differs from … · 24 times +since Sep 20, 13:13 · Resolved`. `find` showed all six copies still on disk, in `~/Videos` and +`~/Documents`. Both are on-demand sync folders. `pdfs conflicts` answered `No conflicts.`. The +journal showed the daemon mounting at 17:29:33, and the entries were stamped 17:30. That is the +sweep's 30-second warmup. + +**Where:** `crates/pdfs-fuse/src/sweep.rs` (`flag_conflict`, `sweep_conflicts_once`), +`crates/pdfs-fuse/src/conflicts.rs` (`list_conflicts`), `crates/pdfs-fuse/src/control.rs` +(`ResolveConflict`). + +**Cause.** There were two faults. + +- The sweep kept its "already flagged" set only in memory. Every daemon start forgot it, so the + first pass logged every open conflict again. +- The sweep flags copies anywhere in the account's own tree. `list_conflicts` only reported copies + below My Files, and resolving only accepted paths under the primary mount. A copy in a synced + folder was logged but never listed, so the GUI read it as resolved. It also could not be + resolved from the GUI or the CLI. + +**Fix.** `Core::conflict_path` names a copy by its mountpoint-relative path under My Files, or +by its absolute local path inside a synced folder. It reuses the roots that search already +resolves hits through. The listing uses it, and the sweep flags only copies it can name. So the +feed never holds a conflict the listing cannot answer for. `ResolveConflict` routes an absolute +path to the on-demand mount that serves it. For a copy in a mirror folder, it says to edit the +local file instead. Before logging, `flag_conflict` checks the activity log +(`Db::activity_has`), so a restart does not log the same copy again. The copy's name carries its +own timestamp, so the name identifies the copy. No schema change. +Regression test: `activity_has_finds_only_matching_kind_and_target`. + +--- + ## B98 — A superseded upload spins its drain thread forever and shows as "Uploading" at 0 bytes **Status:** Fixed (unverified) — the daemon carrying the change has not been driven against a