diff --git a/Cargo.lock b/Cargo.lock index f7fa4b4..7a0bdeb 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3518,7 +3518,7 @@ checksum = "35fb2e5f958ec131621fdd531e9fc186ed768cbe395337403ae56c17a74c68ec" [[package]] name = "pdfs-cli" -version = "1.0.0" +version = "1.1.0" dependencies = [ "anyhow", "clap 4.6.4", @@ -3533,7 +3533,7 @@ dependencies = [ [[package]] name = "pdfs-core" -version = "1.0.0" +version = "1.1.0" dependencies = [ "directories", "ignore", @@ -3553,7 +3553,7 @@ dependencies = [ [[package]] name = "pdfs-fuse" -version = "1.0.0" +version = "1.1.0" dependencies = [ "fuser", "image", @@ -3569,7 +3569,7 @@ dependencies = [ [[package]] name = "pdfs-gui" -version = "1.0.0" +version = "1.1.0" dependencies = [ "anyhow", "async-channel", diff --git a/Cargo.toml b/Cargo.toml index acbc5f8..9b2dcae 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -3,7 +3,7 @@ resolver = "2" members = ["crates/pdfs-core", "crates/pdfs-fuse", "crates/pdfs-cli", "crates/pdfs-gui"] [workspace.package] -version = "1.0.0" +version = "1.1.0" edition = "2024" license = "MIT" authors = ["Nils Pukropp "] diff --git a/crates/pdfs-cli/src/main.rs b/crates/pdfs-cli/src/main.rs index 6797f8c..ebbd61d 100644 --- a/crates/pdfs-cli/src/main.rs +++ b/crates/pdfs-cli/src/main.rs @@ -1291,7 +1291,10 @@ fn mount_once(mountpoint: Option) -> Result { cache, &control_socket, db, - username, + pdfs_fuse::MountOptions { + username, + sweep_mode: config.resolved_conflict_sweep(), + }, ); // Clean unmount check (as a best-effort final backup). diff --git a/crates/pdfs-core/src/config.rs b/crates/pdfs-core/src/config.rs index df73b2d..768eda0 100644 --- a/crates/pdfs-core/src/config.rs +++ b/crates/pdfs-core/src/config.rs @@ -20,6 +20,59 @@ pub const KEYRING_SERVICE: &str = "proton-drive-linux"; /// blobs back under this; pinned files are exempt. See [`crate::cache`]. pub const DEFAULT_CACHE_BUDGET_BYTES: u64 = 5 * 1024 * 1024 * 1024; +/// Environment override for [`AppConfig::resolved_conflict_sweep`]. Exists so a +/// support case can disable the sweep without editing the config file or +/// reinstalling — it wins over the stored setting. +pub const CONFLICT_SWEEP_ENV: &str = "PDFS_CONFLICT_SWEEP"; + +/// What the background conflict sweep (`pdfs-fuse`'s `pdfs-conflict-sweep` +/// thread) is allowed to do about a `(sync-conflict …)` copy it believes is a +/// redundant duplicate. +/// +/// Defaults to [`SweepMode::Report`]: the sweep's whole reason to exist is that +/// the *previous* revision-identity check was wrong (B69), so the replacement is +/// not trusted with deletion until it has a release cycle of field evidence +/// behind it. See `docs/BUGS.md` B71. +#[derive(Serialize, Deserialize, Debug, Clone, Copy, PartialEq, Eq, Default)] +#[serde(rename_all = "lowercase")] +pub enum SweepMode { + /// Do not run the sweep at all — the thread is never spawned. + Off, + /// Run, and log what it *would* remove, but never trash anything. Divergent + /// and orphaned copies are still surfaced to the activity feed as usual. + #[default] + Report, + /// Trash copies proven identical to their live sibling. + Enforce, +} + +impl SweepMode { + /// Parse an operator-supplied value ([`CONFLICT_SWEEP_ENV`] or a CLI flag). + /// `None` for anything unrecognised, so a typo falls back to the stored + /// setting rather than silently picking a mode the operator did not ask for. + pub fn parse(value: &str) -> Option { + match value.trim().to_ascii_lowercase().as_str() { + "off" | "no" | "false" | "0" | "disabled" => Some(Self::Off), + "report" | "dry-run" | "dryrun" | "observe" => Some(Self::Report), + "enforce" | "on" | "yes" | "true" | "1" => Some(Self::Enforce), + _ => None, + } + } + + /// Whether this mode is allowed to trash a duplicate. + pub fn is_enforcing(self) -> bool { + matches!(self, Self::Enforce) + } +} + +/// Precedence for [`AppConfig::resolved_conflict_sweep`], split out from the +/// environment read so it can be unit-tested without mutating process state. +fn resolve_sweep_mode(env: Option<&str>, stored: Option) -> SweepMode { + env.and_then(SweepMode::parse) + .or(stored) + .unwrap_or_default() +} + /// Configuration structure allowing the user to customize client identification headers. #[derive(Serialize, Deserialize, Debug, Clone)] pub struct AppConfig { @@ -48,6 +101,11 @@ pub struct AppConfig { /// by hostname". Defaulted for configs predating the field. #[serde(default)] pub device_uid: Option, + /// What the background conflict sweep may do. `None` means "use + /// [`SweepMode::default`]" (report-only). Defaulted for configs predating + /// the field, and overridable at runtime via [`CONFLICT_SWEEP_ENV`]. + #[serde(default)] + pub conflict_sweep: Option, } impl Default for AppConfig { @@ -59,6 +117,7 @@ impl Default for AppConfig { mountpoint: None, ignore_patterns: None, device_uid: None, + conflict_sweep: None, } } } @@ -70,6 +129,20 @@ impl AppConfig { self.cache_budget.unwrap_or(DEFAULT_CACHE_BUDGET_BYTES) } + /// The effective conflict-sweep mode: the [`CONFLICT_SWEEP_ENV`] override if + /// it parses, then the user's stored choice, then [`SweepMode::default`]. + /// + /// The environment wins so a support case can stop a destructive background + /// loop without editing config or reinstalling (`docs/BUGS.md` B71). An + /// unparseable value is ignored rather than treated as "off", so a typo + /// cannot silently change behaviour in either direction. + pub fn resolved_conflict_sweep(&self) -> SweepMode { + resolve_sweep_mode( + std::env::var(CONFLICT_SWEEP_ENV).ok().as_deref(), + self.conflict_sweep, + ) + } + /// The effective global sync ignore patterns: the user's explicit list, or /// [`DEFAULT_IGNORE_PATTERNS`] when unset. An explicit empty list is /// honoured as "no global patterns", which is why this is not a @@ -374,6 +447,7 @@ mod tests { mountpoint: Some("/tmp/x".to_string()), ignore_patterns: Some(vec!["build/".to_string()]), device_uid: Some("dev-uid".to_string()), + conflict_sweep: Some(SweepMode::Enforce), }; let json = serde_json::to_string(&config).unwrap(); let decoded: AppConfig = serde_json::from_str(&json).unwrap(); @@ -388,6 +462,62 @@ mod tests { assert_eq!(default_config.user_agent, USER_AGENT); } + #[test] + fn sweep_mode_defaults_to_report_only() { + // The sweep trashes user files, so an installed config that has never + // heard of the setting must not get the enforcing behaviour. + assert_eq!(SweepMode::default(), SweepMode::Report); + assert!(!SweepMode::default().is_enforcing()); + assert_eq!(resolve_sweep_mode(None, None), SweepMode::Report); + } + + #[test] + fn sweep_mode_parses_operator_spellings() { + for s in ["off", "OFF", " no ", "false", "0", "disabled"] { + assert_eq!(SweepMode::parse(s), Some(SweepMode::Off), "{s}"); + } + for s in ["report", "dry-run", "dryrun", "observe"] { + assert_eq!(SweepMode::parse(s), Some(SweepMode::Report), "{s}"); + } + for s in ["enforce", "on", "yes", "true", "1"] { + assert_eq!(SweepMode::parse(s), Some(SweepMode::Enforce), "{s}"); + } + // A typo must not resolve to anything, so it falls through to the config. + assert_eq!(SweepMode::parse("offf"), None); + assert_eq!(SweepMode::parse(""), None); + } + + #[test] + fn sweep_mode_env_overrides_config_but_a_typo_does_not() { + let stored = Some(SweepMode::Enforce); + // The point of the override: kill a destructive loop without editing config. + assert_eq!(resolve_sweep_mode(Some("off"), stored), SweepMode::Off); + // Unrecognised value falls back to the stored choice rather than guessing. + assert_eq!(resolve_sweep_mode(Some("nope"), stored), SweepMode::Enforce); + assert_eq!(resolve_sweep_mode(None, stored), SweepMode::Enforce); + // No env, no stored value: report-only. + assert_eq!(resolve_sweep_mode(Some("nope"), None), SweepMode::Report); + } + + #[test] + fn a_config_predating_conflict_sweep_still_loads() { + let json = r#"{"app_version":"v","user_agent":"u"}"#; + let decoded: AppConfig = serde_json::from_str(json).unwrap(); + assert!(decoded.conflict_sweep.is_none()); + } + + #[test] + fn sweep_mode_round_trips_through_the_config_file() { + let config = AppConfig { + conflict_sweep: Some(SweepMode::Enforce), + ..Default::default() + }; + let json = serde_json::to_string(&config).unwrap(); + assert!(json.contains(r#""conflict_sweep":"enforce""#), "{json}"); + let decoded: AppConfig = serde_json::from_str(&json).unwrap(); + assert_eq!(decoded.conflict_sweep, Some(SweepMode::Enforce)); + } + #[test] fn a_config_predating_ignore_patterns_still_loads() { // Every installed config on disk looks like this; the field must not be diff --git a/crates/pdfs-core/src/db/ops.rs b/crates/pdfs-core/src/db/ops.rs index f18c9d0..b3acab6 100644 --- a/crates/pdfs-core/src/db/ops.rs +++ b/crates/pdfs-core/src/db/ops.rs @@ -280,6 +280,22 @@ impl Db { Ok(count > 0) } + /// Check whether *any* op of any kind is queued against `uid`. + /// + /// Used as a safety interlock by callers that are about to remove a node + /// they only know about from a stale snapshot: a queued op means the user's + /// bytes are still owed an upload, and dropping it would discard the staged + /// blob that holds them (`docs/BUGS.md` B71). + pub fn has_any_op(&self, uid: &str) -> Result { + let conn = self.conn.lock(); + let count: i64 = conn.query_row( + "SELECT COUNT(*) FROM pending_op WHERE uid = ?1", + params![uid], + |row| row.get(0), + )?; + Ok(count > 0) + } + /// Drop every op targeting a node **or anything queued beneath it**, /// returning the staged blobs they held so the caller can delete them. /// diff --git a/crates/pdfs-fuse/src/lib.rs b/crates/pdfs-fuse/src/lib.rs index 31702dc..e5c3879 100644 --- a/crates/pdfs-fuse/src/lib.rs +++ b/crates/pdfs-fuse/src/lib.rs @@ -53,7 +53,7 @@ use fuser::{ ReplyEntry, ReplyIoctl, ReplyOpen, ReplyWrite, Request, Session, TimeOrNow, WriteFlags, }; use pdfs_core::cache::{BLOCK_SIZE, Baseline, ContentCache, StagedWrite}; -use pdfs_core::config::AppDirs; +use pdfs_core::config::{AppDirs, SweepMode}; use pdfs_core::control::{ ActivityEntry, ActivityKind, DirEntry, ErrorKind, LocalHit, PhotoKind, PublicLinkInfo, SearchFilters, SearchHit, SearchSource, SyncFolderInfo, SyncPhase, SyncProgress, @@ -95,7 +95,7 @@ mod upload; mod workers; use background::{run_event_sync, run_local_index}; pub(crate) use mount::is_stale_mount; -pub use mount::{MountOutcome, mount}; +pub use mount::{MountOptions, MountOutcome, mount}; use mount::{SecondaryMount, clear_stale_mount, fuse_connection_id, umount_session_unblocked}; use reads::{ReaderSlot, STREAM_BYPASS_MIN, StreamRing}; use state::{Entry, Intervals, PendingRevision, State, WriteHandle}; @@ -243,6 +243,11 @@ struct Core { /// run, so a divergent `(sync-conflict …)` file is logged once rather than /// on every sweep pass. See [`Core::run_conflict_sweep_loop`]. 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 + /// ([`AppConfig::resolved_conflict_sweep`]); [`SweepMode::Off`] means the + /// sweep thread is never spawned at all. See `docs/BUGS.md` B71. + sweep_mode: SweepMode, /// The latest server revision id this daemon has itself sealed, keyed by /// node. A queued write whose baseline names an *earlier* revision would /// normally fork into a `(sync-conflict)` copy when the drain finds the diff --git a/crates/pdfs-fuse/src/mount.rs b/crates/pdfs-fuse/src/mount.rs index d4b111c..7f55288 100644 --- a/crates/pdfs-fuse/src/mount.rs +++ b/crates/pdfs-fuse/src/mount.rs @@ -143,6 +143,15 @@ fn fetch_or_recall_root( } } +/// Per-mount settings resolved from [`pdfs_core::config::AppConfig`] by the +/// caller, so `mount` itself never reads config or the environment. +pub struct MountOptions { + /// Proton account the session belongs to, for display and per-user paths. + pub username: String, + /// What the background conflict sweep may do. See `docs/BUGS.md` B71. + pub sweep_mode: SweepMode, +} + /// Mount the filesystem at `mountpoint` and block until it is unmounted or the /// daemon is asked to stop. /// @@ -162,8 +171,12 @@ pub fn mount( cache: ContentCache, control_socket: &Path, db: Arc, - username: String, + options: MountOptions, ) -> std::io::Result { + let MountOptions { + username, + sweep_mode, + } = options; let (root, online) = fetch_or_recall_root(&client, &rt, &db)?; let scope = root.tree_event_scope_id(); @@ -197,6 +210,7 @@ pub fn mount( timeline_refreshing: Arc::new(AtomicBool::new(false)), trash_refreshing: Arc::new(AtomicBool::new(false)), conflict_notified: Arc::new(Mutex::new(HashSet::new())), + sweep_mode, own_sealed_revs: Arc::new(Mutex::new(HashMap::new())), thumb_gen: Arc::new(Mutex::new(HashSet::new())), no_thumbnail: Arc::new(Mutex::new(HashMap::new())), @@ -231,7 +245,12 @@ pub fn mount( // Reconcile leftover `(sync-conflict …)` copies: drop the ones that turned // out identical to the live file, surface the ones that genuinely diverge. // Its own thread — it enumerates and trashes, and must never block a FUSE call. - { + // `Off` skips the thread entirely rather than starting an idle one, so the + // setting is verifiable from the outside (`ls /proc//task/*/comm`). + if sweep_mode == SweepMode::Off { + info!("conflict sweep disabled by configuration"); + } else { + info!(mode = ?sweep_mode, "conflict sweep enabled"); let core = core.clone(); std::thread::Builder::new() .name("pdfs-conflict-sweep".into()) diff --git a/crates/pdfs-fuse/src/sweep.rs b/crates/pdfs-fuse/src/sweep.rs index 5967a8f..71eab9d 100644 --- a/crates/pdfs-fuse/src/sweep.rs +++ b/crates/pdfs-fuse/src/sweep.rs @@ -21,11 +21,12 @@ use std::collections::HashMap; use std::time::Duration; +use pdfs_core::config::SweepMode; use pdfs_core::control::ActivityKind; use proton_drive_rs::proton_sdk::ids::NodeUid; use tracing::{debug, info, warn}; -use super::{Core, node_content_sha1, node_size}; +use super::{Core, node_content_sha1, node_revision_id, node_size}; /// Time between sweeps. Conflicts are rare and non-urgent; a slow cadence keeps /// the sweep off the hot path while still cleaning up within a few minutes. @@ -112,29 +113,35 @@ impl Core { .unwrap_or_default(); let sibling = by_parent_name.get(&(parent, base.as_str())).copied(); - match sibling { - Some(sib) if is_identical(node, sib) => { - // Redundant duplicate — remove it, but only while online (the - // remote trash is the point). Offline, it waits for a later pass. + match decide(node, sibling, &base, self.sweep_mode) { + SweepAction::Remove => { + // Only while online — the remote trash is the point. Offline, + // it waits for a later pass. if online { - self.remove_conflict_copy(&node.uid, &node.name, &base); + self.remove_conflict_copy(node, &base); } } - Some(_) => { - self.flag_conflict(&node.uid, &node.name, format!("differs from {base}")) - } - None => self.flag_conflict( - &node.uid, - &node.name, - format!("no original {base} to reconcile against"), - ), + SweepAction::Flag(detail) => self.flag_conflict(&node.uid, &node.name, detail), } } } /// Trash a conflict copy proven identical to its live sibling, and unhook it /// locally — the same sequence the interactive unlink path uses after a trash. - fn remove_conflict_copy(&self, uid: &NodeUid, name: &str, base: &str) { + /// + /// `snapshot` is the node as the pass saw it, which by now is stale: the pass + /// enumerated every node up front and has been doing network I/O since. Before + /// touching anything remote this re-reads the node and refuses to act unless + /// it is *still* the same revision, still untrashed, still unwritten and still + /// unopened. Without that, a user edit landing inside the window is destroyed + /// — `discard_queued_ops` below drops the queued write *and its staged blob*, + /// which may hold the only copy of those bytes (`docs/BUGS.md` B71). + fn remove_conflict_copy(&self, snapshot: &proton_drive_rs::Node, base: &str) { + let uid = &snapshot.uid; + let name = snapshot.name.as_str(); + if !self.duplicate_still_removable(snapshot) { + return; + } match self .rt .block_on(self.client.trash_nodes(std::slice::from_ref(uid))) @@ -164,6 +171,75 @@ impl Core { info!(%uid, name, base, "conflict sweep: removed identical conflict copy"); } + /// Re-check, immediately before the destructive call, everything the pass + /// decided from its stale snapshot. Any doubt means "leave it alone": a + /// conflict copy that survives to the next pass costs nothing, a wrongly + /// removed one costs the user data. + fn duplicate_still_removable(&self, snapshot: &proton_drive_rs::Node) -> bool { + let uid = &snapshot.uid; + let name = snapshot.name.as_str(); + + // A queued op means bytes are still owed an upload. `discard_queued_ops` + // would throw those away along with the staged blob holding them. + match self.db.has_any_op(&uid.to_string()) { + Ok(false) => {} + Ok(true) => { + debug!(%uid, name, "conflict sweep: skipping duplicate with queued ops"); + return false; + } + // Cannot prove the queue is empty — assume it is not. + Err(e) => { + warn!(%uid, name, error = ?e, "conflict sweep: op check failed; skipping"); + return false; + } + } + + // Someone is writing to it, or holds it open. Trashing now would pull the + // node out from under a live handle. + if self.is_busy(uid) { + debug!(%uid, name, "conflict sweep: skipping duplicate that is open or being written"); + return false; + } + + // Finally, the node itself must not have moved on since the snapshot. + match self.db.node_by_uid(&uid.to_string()) { + Ok(Some(fresh)) => { + if fresh.trashed { + return false; + } + if fresh.name != snapshot.name || fresh.parent_uid != snapshot.parent_uid { + debug!(%uid, name, "conflict sweep: duplicate was renamed or moved; skipping"); + return false; + } + if node_revision_id(&fresh) != node_revision_id(snapshot) + || node_size(&fresh) != node_size(snapshot) + || node_content_sha1(&fresh) != node_content_sha1(snapshot) + { + debug!(%uid, name, "conflict sweep: duplicate changed since the pass began"); + return false; + } + true + } + // Already gone locally, or unreadable — either way, not ours to remove. + Ok(None) => false, + Err(e) => { + warn!(%uid, name, error = ?e, "conflict sweep: re-read failed; skipping"); + false + } + } + } + + /// Whether a node has a write in flight or a file handle open against it. + /// Read-only opens use fh 0 and are not tracked, so this is specifically the + /// writer/holder check. + fn is_busy(&self, uid: &NodeUid) -> bool { + let state = self.state.lock(); + let Some(&ino) = state.by_uid.get(uid) else { + return false; + }; + state.active_writes.contains_key(&ino) || state.handles.values().any(|&held| held == ino) + } + /// 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. fn flag_conflict(&self, uid: &NodeUid, name: &str, detail: String) { @@ -175,6 +251,46 @@ impl Core { } } +/// What the sweep concluded about one `(sync-conflict …)` copy. +#[derive(Debug, Clone, PartialEq, Eq)] +enum SweepAction { + /// Provably redundant: trash it, subject to the runtime interlocks in + /// [`Core::duplicate_still_removable`]. + Remove, + /// Only the user can resolve this one; surface it with this detail. + Flag(String), +} + +/// Decide what to do about conflict copy `node` given its live sibling (if any) +/// and the configured `mode`. +/// +/// Pure, so the policy that governs deletion is testable without a mount, a +/// runtime, or an account — the same split used for `revision_changed` and +/// `is_own_self_supersede`. Everything that can fail or race lives in the caller. +fn decide( + node: &proton_drive_rs::Node, + sibling: Option<&proton_drive_rs::Node>, + base: &str, + mode: SweepMode, +) -> SweepAction { + match sibling { + Some(sib) if is_identical(node, sib) => { + if mode.is_enforcing() { + SweepAction::Remove + } else { + // Report-only: say what would happen, remove nothing. The point is + // to accumulate field evidence that the identity check is right + // before it is trusted with deletion (docs/BUGS.md B71). + SweepAction::Flag(format!( + "identical to {base}; would be removed (sweep is report-only)" + )) + } + } + Some(_) => SweepAction::Flag(format!("differs from {base}")), + None => SweepAction::Flag(format!("no original {base} to reconcile against")), + } +} + /// Whether two file nodes provably hold the same bytes: equal size and equal, /// present content SHA-1. A missing digest on either side is not proof, so the /// sweep treats it as divergent rather than risk removing a real difference. @@ -191,6 +307,113 @@ fn is_identical(a: &proton_drive_rs::Node, b: &proton_drive_rs::Node) -> bool { #[cfg(test)] mod tests { use super::*; + use proton_drive_rs::NodeKind; + use proton_drive_rs::proton_sdk::ids::{LinkId, VolumeId}; + + /// A file node with the two fields the sweep's identity check reads. + fn file(name: &str, size: i64, sha1: Option<&str>) -> proton_drive_rs::Node { + proton_drive_rs::Node { + uid: NodeUid::new(VolumeId::from("v"), LinkId::from(name)), + parent_uid: None, + kind: NodeKind::File { + media_type: "text/plain".into(), + total_size_on_storage: size, + active_revision_state: None, + active_revision_id: Some("rev".into()), + claimed_size: Some(size), + claimed_modification_time: None, + content_sha1: sha1.map(String::from), + }, + name: name.into(), + creation_time: 0, + modification_time: 0, + trashed: false, + is_shared: false, + is_shared_publicly: false, + signature_email: None, + verification: Default::default(), + } + } + + /// The only input shape that may ever produce a removal: same size, same + /// present digest, live sibling, enforcing mode. + #[test] + fn removes_only_a_provably_identical_copy_when_enforcing() { + let copy = file("f (sync-conflict 1).txt", 100, Some("aaa")); + let live = file("f.txt", 100, Some("aaa")); + assert_eq!( + decide(©, Some(&live), "f.txt", SweepMode::Enforce), + SweepAction::Remove + ); + } + + #[test] + fn report_only_never_removes_but_still_says_what_it_would_do() { + let copy = file("f (sync-conflict 1).txt", 100, Some("aaa")); + let live = file("f.txt", 100, Some("aaa")); + // Same inputs as the removal case above; only the mode differs. + let SweepAction::Flag(detail) = decide(©, Some(&live), "f.txt", SweepMode::Report) + else { + panic!("report mode must never remove"); + }; + assert!(detail.contains("would be removed"), "{detail}"); + assert!(detail.contains("report-only"), "{detail}"); + } + + #[test] + fn a_differing_size_is_a_real_conflict() { + let copy = file("f (sync-conflict 1).txt", 101, Some("aaa")); + let live = file("f.txt", 100, Some("aaa")); + assert_eq!( + decide(©, Some(&live), "f.txt", SweepMode::Enforce), + SweepAction::Flag("differs from f.txt".into()) + ); + } + + #[test] + fn a_missing_digest_is_never_proof_of_identity() { + // Equal sizes are not enough: two different 100-byte files are common. + // A copy the sweep cannot fingerprint must survive, in either direction. + for (a, b) in [(None, Some("aaa")), (Some("aaa"), None), (None, None)] { + let copy = file("f (sync-conflict 1).txt", 100, a); + let live = file("f.txt", 100, b); + assert_eq!( + decide(©, Some(&live), "f.txt", SweepMode::Enforce), + SweepAction::Flag("differs from f.txt".into()), + "{a:?} vs {b:?}" + ); + } + } + + #[test] + fn a_differing_digest_at_equal_size_is_a_real_conflict() { + let copy = file("f (sync-conflict 1).txt", 100, Some("aaa")); + let live = file("f.txt", 100, Some("bbb")); + assert_eq!( + decide(©, Some(&live), "f.txt", SweepMode::Enforce), + SweepAction::Flag("differs from f.txt".into()) + ); + } + + #[test] + fn an_orphaned_copy_is_surfaced_not_removed() { + // No live original left to reconcile against — the copy may be the only + // surviving version of the file, so it is never the sweep's to remove. + let copy = file("f (sync-conflict 1).txt", 100, Some("aaa")); + assert_eq!( + decide(©, None, "f.txt", SweepMode::Enforce), + SweepAction::Flag("no original f.txt to reconcile against".into()) + ); + } + + #[test] + fn identity_needs_both_size_and_digest_to_match() { + let a = file("a", 100, Some("aaa")); + assert!(is_identical(&a, &file("b", 100, Some("aaa")))); + assert!(!is_identical(&a, &file("b", 101, Some("aaa")))); + assert!(!is_identical(&a, &file("b", 100, Some("bbb")))); + assert!(!is_identical(&a, &file("b", 100, None))); + } #[test] fn parses_conflict_names_and_leaves_others_alone() { diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 9a9d328..e4c7c3a 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -225,6 +225,7 @@ The background daemon relies on the following thread topology: 3. **IPC listener Thread:** Listens on the Unix socket and admits at most 64 concurrent, timeout-bound handlers for front-end status/configuration requests. 4. **Sync Engine Loop Thread:** Serializes sync runs. Wakes on debounced local inotify filesystem changes, remote polling intervals, or manual user requests. 5. **Drain Queue Worker Thread:** Processes staged writes (`PendingOp`) sequentially, uploading revisions and retrying with exponential backoff on failures. +6. **Conflict Sweep Thread (`pdfs-conflict-sweep`, optional):** Reconciles leftover `(sync-conflict …)` copies — 30 s warmup, then one pass every 5 minutes. A copy proven identical to its live sibling (equal size **and** equal `content_sha1`) is redundant; anything it cannot prove identical is surfaced to the activity feed and left alone. Governed by `AppConfig.conflict_sweep` / `PDFS_CONFLICT_SWEEP` (`SweepMode`): **report-only by default**, `off` skips the thread entirely, and only `enforce` lets it trash. Because it deletes, the enforcing path re-verifies revision id, size, digest, name, parent, queued ops and open handles immediately before acting rather than trusting its own pass-start snapshot. See `docs/BUGS.md` B69 and B71. --- diff --git a/docs/BUGS.md b/docs/BUGS.md index b71bc08..bf27317 100644 --- a/docs/BUGS.md +++ b/docs/BUGS.md @@ -2115,7 +2115,7 @@ byte-identical fresh-state recovery on a dedicated account. ## B69 — Spurious `(sync-conflict)` copies from mtime-keyed revision identity (B16/B25 root fix) -**Status:** Fixed in-tree 2026-07-25 — offline-tested; **live validation pending** (needs an SDK release, see below) +**Status:** Fixed — offline-tested 2026-07-25, **live-validated 2026-07-26** (see below) **Found:** 2026-07-24, user reported `test_export (sync-conflict 1784898786).xml` created after a normal save from an application into the on-demand mount. A full-mount scan turned up ~156 conflict copies; 151 were byte-identical to their live sibling. **Where:** `crates/pdfs-fuse/src/{drain,filesystem,lib,state,sweep,sync,sync/planner}.rs`, `crates/pdfs-core/src/cache.rs`, `crates/pdfs-core/src/control.rs`; SDK `proton-drive-rs` `node.rs`/`client.rs`/`public_link.rs`. @@ -2131,6 +2131,19 @@ byte-identical fresh-state recovery on a dedicated account. **Verified:** `proton-sdk-rs` 50/61 lib tests + clippy clean; client `cargo fmt`/`clippy -D warnings`/`cargo test --workspace --locked` all green (258 tests, incl. new `revision_changed`, planner `AdoptBaseline`, and `conflict_base_name` cases). The 151 identical copies were removed manually during triage; the 5 divergent ones are left for the user. +**Live validation (2026-07-26):** 1.1.0 installed over the production account (6 +FUSE mounts of real data), daemon restarted, sweep observed across its warmup pass +and a full 5-minute interval. Pre-run snapshot: exactly 2 `(sync-conflict)` copies +left on the account, both divergent in size *and* `content_sha1`, so the correct +behaviour was to remove nothing. Result: **nothing removed** — the conflict +manifest was byte-identical before and after, the activity feed gained zero +`auto-removed duplicate` entries, and the copies were flagged `differs from …` as +intended. (The 40 `trash` entries in the feed for that window are +`trashed from the mount` — a previous FUSE-acceptance run's queued cleanup ops +replaying through the drain, unrelated to the sweep.) The sweep's *removal* path +therefore remains unexercised against a real account; it is now report-only by +default (B71) precisely so that stays true until there is field evidence. + **Shipping note (resolved 2026-07-25):** the client's revision-id/sha1 use depends on new SDK surface (`proton-sdk`/`proton-drive-rs` **0.2.2**). That version is now **published to crates.io**; the client's workspace deps were bumped `"0.2"` → `"0.2.2"`, the `Cargo.lock` re-resolved to the registry crates, and the local `[patch.crates-io]` shim removed. No local patch remains. (Live validation of the fix against a real account is still pending.) ## B70 — Browser in-flight temp files uploaded and conflict-forked on the on-demand mount @@ -2217,3 +2230,135 @@ sealing; B stops a single-writer resume forking against its own seal):** **Triage of the copies already made is done (2026-07-25):** all completed files were promoted back to their canonical names (metadata-only rename, no re-upload) and the abandoned `.crdownload` stubs trashed. + +## B71 — Conflict sweep is not production-ready (auto-trash loop, CRIT-13) + +**Status:** Blockers 1–4 fixed in-tree 2026-07-26; items 5–10 and the remaining +cleanup still open. The sweep is safe to leave running (it is report-only by +default and cannot trash anything until explicitly switched to `enforce`). +**Where:** `crates/pdfs-fuse/src/sweep.rs`, spawn site `crates/pdfs-fuse/src/mount.rs`, +config surface `crates/pdfs-core/src/config.rs`. + +The B69 fix (see above) shipped a `pdfs-conflict-sweep` thread that **trashes user +files on the real Drive** from a background loop. The identity check itself is +conservative (equal size *and* equal `content_sha1`, missing digest treated as +divergent), but the machinery around it is not yet safe to run unattended. Review +found the following, ordered by severity. Each is a separate defect; they are +grouped because they gate the same feature. + +**Blockers (all fixed 2026-07-26 — the sweep may not trash anything without them):** + +1. **Data-loss race — trash-then-discard (CRIT).** `remove_conflict_copy` + (`sweep.rs:137-155`) trashes the node remotely and *then* calls + `discard_queued_ops(uid)`, which deletes the node's queued ops **and discards + their staged blobs** (`lib.rs:1544-1553`). The decision was made from the + `load_all()` snapshot taken at `sweep.rs:77`, with network round trips in + between. A user edit landing in that window is destroyed — the queued write is + dropped and its staged bytes, which may be the only copy, are discarded. This + violates the invariant that `staging/` is never purged. *Fix:* re-read the node + from the DB and re-verify `active_revision_id` immediately before trashing; + skip the node outright if `Core.pending` or the op table has any entry for it; + never let the sweep discard staged bytes. +2. **No kill-switch (CRIT).** `mount.rs:234-239` spawns the loop unconditionally. + There is no `AppConfig` field and no environment override, so the only way to + stop a destructive background loop is to uninstall. *Fix:* add + `AppConfig.conflict_sweep: Option` (same `Option` idiom as `cache_budget` + / `ignore_patterns`), a `PDFS_CONFLICT_SWEEP=off` override for support, and a + Settings toggle. +3. **Open handles ignored (HIGH).** Nothing checks for a live file handle before + `forget_or_unlink` + `evict_reader`, so the sweep can trash a node a reader + currently has open. *Fix:* skip nodes with open handles. +4. **Enforcing on day one (HIGH, process).** The sweep exists because B69 proved + the previous revision-identity check was wrong; betting deletion on the + *replacement* check in the same release, with no field evidence, is the wrong + order. *Fix:* ship report-only (log `would-trash` plus the existing + `ActivityKind::Conflict`), collect a release cycle of real logs, then flip to + enforcing. + +**Fix for 1–4 (landed 2026-07-26):** + +- **`SweepMode`** (`pdfs-core/src/config.rs`) — `Off` / `Report` / `Enforce`, + serialised into `AppConfig.conflict_sweep` (`#[serde(default)]`, so configs + predating the field still load). `Default` is **`Report`**, which is blocker 4: + an existing install that has never heard of the setting cannot get the + enforcing behaviour on upgrade. `AppConfig::resolved_conflict_sweep` layers + `PDFS_CONFLICT_SWEEP` over the stored value; the precedence is a pure + `resolve_sweep_mode(env, stored)` so it is testable without mutating process + state. An unparseable value falls through to the config rather than guessing a + mode in either direction. +- **Spawn gate** (`mount.rs`) — `SweepMode::Off` skips the thread entirely rather + than starting an idle one, so the setting is verifiable from outside the + process (`ls /proc//task/*/comm`). Both branches log the resolved mode at + startup. `mount` grew past clippy's argument limit, so `username` + + `sweep_mode` moved into a `MountOptions` struct resolved by the caller — + `mount` still reads neither config nor environment. +- **Interlocks** (`sweep.rs::duplicate_still_removable`) — re-checks, immediately + before the destructive call, everything the pass decided from its stale + snapshot: no queued op (new `Db::has_any_op`, a cheap `COUNT` — a queued op + means bytes are owed an upload and `discard_queued_ops` would throw away the + staged blob holding them), no open write handle or shared scratch file + (`Core::is_busy`, checking `active_writes` + `handles` under one `State` lock), + and the node still at the same revision id, size, digest, name, parent and + untrashed state. Any doubt — including a failed DB read — means leave it alone: + a copy that survives to the next pass costs nothing, a wrongly removed one + costs data. This is blockers 1 and 3. +- **Testability** — the decision moved into a pure + `decide(node, sibling, base, mode) -> SweepAction`, the same extraction used + for `revision_changed` and `is_own_self_supersede`; everything that can fail or + race stays in the caller. Tests now cover removal-when-enforcing, + report-mode-never-removes, size-differs, digest-differs, missing-digest in both + directions, and orphaned copies. Gate: `cargo fmt` / `clippy -D warnings` clean, + `cargo test --workspace` 282 passed (was 270). + +**Should fix:** + +5. **`load_all()` every pass (MED).** Each 300 s pass pulls the entire node table + into a `Vec` and builds a full `(parent, name)` `HashMap`. *Fix:* + query only conflict-named rows (`name LIKE '% (sync-conflict %'`) and look up + siblings per parent — O(conflicts), not O(drive). +6. **No batching or per-pass cap (MED).** The B69 incident produced 151 copies; + that is 151 sequential `trash_nodes` calls in one pass with no cap and no + backoff. `trash_nodes` takes a slice — batch it and cap per pass. +7. **`online` sampled once per pass (MED).** Read at `sweep.rs:100`, ahead of a + loop that does network I/O; going offline mid-pass yields a long run of failing + calls. *Fix:* bail on the first transport error. +8. **Ambiguous sibling index (MED).** `by_parent_name.insert` (`sweep.rs:97`) is + last-write-wins, so with duplicate `(parent, name)` rows the surviving entry + depends on `load_all` row order. *Fix:* make the choice deterministic or skip + ambiguous parents. +9. **Trashed ancestors (LOW).** A copy under a trashed folder finds no live + sibling and is flagged "orphaned", spamming the activity feed. *Fix:* skip + nodes with a trashed ancestor. +10. **SHA-1 provenance unverified (MED).** `is_identical` (`sweep.rs:181`) + compares digests without asserting they belong to the current + `active_revision_id`. A stale `XAttr` digest from an earlier revision plus a + size collision would delete real data. (The *missing* digest case is already + handled correctly.) + +**Also required to close:** + +- **Tests.** The *policy* is now covered via the pure `decide` (see above). Still + untested are the *interlocks*, which need a `Core`: pending-op→skip, + open-handle→skip, revision-moved→skip, offline→skip. Those want a test harness + that can build a `Core` against a temp DB and a stub client. +- **`conflict_notified` is unbounded.** `HashSet` that only grows except + on trash (`sweep.rs:157,170`). +- **Docs.** `docs/ARCHITECTURE.md` thread map does not list `pdfs-conflict-sweep`; + `docs/RECOVERY.md` needs a "the sweep trashed a file, recover it from Proton + trash" path. + +**Mitigating context from the 2026-07-26 pre-run snapshot:** the account had only +2 conflict copies left, both divergent in size *and* `content_sha1`, so the +correct behaviour for the first live pass is to trash nothing. + +## B72 — `pdfs trash` never returns + +**Status:** Open — found 2026-07-26. +**Found:** capturing a trash baseline before the B69/B70 live validation. `pdfs trash` +produced no output and was still running when killed at 120 s. Daemon was healthy +and every other CLI call (`pdfs sync list`, `pdfs --version`) returned promptly. +**Where:** unconfirmed — either the `Request` handler in `crates/pdfs-fuse/src/control.rs` +or the CLI's response read in `crates/pdfs-cli/src/main.rs`. +**Impact:** the trash listing is the user's recovery path after any destructive +operation, including the B71 sweep. It needs to work before the sweep is allowed +to trash anything. diff --git a/packaging/PKGBUILD b/packaging/PKGBUILD index 97f4b20..512ec2d 100644 --- a/packaging/PKGBUILD +++ b/packaging/PKGBUILD @@ -9,7 +9,7 @@ # Build + install: cd packaging && makepkg -fi pkgname=proton-drive-linux -pkgver=1.0.0 +pkgver=1.1.0 pkgrel=1 pkgdesc="Proton Drive client for Linux: FUSE files-on-demand mount, CLI, GTK4 app + tray" arch=('x86_64')