diff --git a/crates/pdfs-core/src/db/ops.rs b/crates/pdfs-core/src/db/ops.rs index 603243e..ebbc91c 100644 --- a/crates/pdfs-core/src/db/ops.rs +++ b/crates/pdfs-core/src/db/ops.rs @@ -45,6 +45,12 @@ pub const OP_RENAME: &str = "rename"; #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub struct RenameMeta { pub original_parent_uid: String, + /// The node's name on Drive when the first of its queued renames was made, + /// kept across the renames that replace the row, like the parent. With it + /// the drain knows where the node should still be when the op lands. + /// `None` on a row queued before this field existed. + #[serde(default)] + pub original_name: Option, } /// The `kind` of a [`PendingOp`] that trashes a node the server knows about diff --git a/crates/pdfs-fuse/src/drain.rs b/crates/pdfs-fuse/src/drain.rs index 9b8eaad..c7b95a4 100644 --- a/crates/pdfs-fuse/src/drain.rs +++ b/crates/pdfs-fuse/src/drain.rs @@ -676,6 +676,15 @@ impl Core { return Ok(()); } }; + if let Some(meta) = op.meta_json.as_deref() { + let meta: RenameMeta = serde_json::from_str(meta)?; + if moved_elsewhere(&meta, &node, &parent, &name) { + // Someone moved or renamed it while ours was queued. Ours still + // wins, as the last change does on Drive. + info!(%uid, from = %node.name, to = %name, + "node moved remotely while its rename was queued"); + } + } let landed = if node.parent_uid.as_ref() == Some(&parent) { self.drain_rename_in_place(&uid, &node.name, &name)? } else { @@ -818,7 +827,6 @@ impl Core { Err(e) => return Err(e.into()), } self.db.complete_trash_op(op.id, &uid)?; - self.own_sealed_revs.lock().remove(&uid); if let Err(e) = self.db.clear_own_sealed_rev(&uid.to_string()) { debug!(%uid, error = %e, "clearing the sealed-revision record failed"); } @@ -1227,16 +1235,11 @@ impl Core { // instead of forking — so the finished bytes win the name rather than // being exiled to a `(sync-conflict)` copy. let remote_rev = node_revision_id(&node); - // In memory first, then the table it is written through to: the map is - // this process's cache of it, and after a restart only the table has - // the answer (which is the whole reason it is persisted). - let own_rev = match self.own_sealed_revs.lock().get(uid).cloned() { - Some(rev) => Some(rev), - None => self - .db - .own_sealed_rev(&uid.to_string(), now_millis()) - .unwrap_or_default(), - }; + // The table, so the answer survives a restart (`own_sealed_rev`). + let own_rev = self + .db + .own_sealed_rev(&uid.to_string(), now_millis()) + .unwrap_or_default(); if is_own_self_supersede(meta.complete, remote_rev.as_deref(), own_rev.as_deref()) { debug!(%uid, ?remote_rev, "queued write chains onto our own sealed revision; not a conflict"); @@ -1753,10 +1756,9 @@ impl Core { // change and does not fork (B70 layer B; consulted in // `revision_conflict`). if let Some(rev) = node_revision_id(&node) { - self.own_sealed_revs.lock().insert(uid.clone(), rev.clone()); - // Write through: losing this at shutdown reopened the fork - // window B70 layer B closed, for the first drain after a - // restart. + // In the table, not in memory: losing it at shutdown reopened + // the fork window B70 layer B closed, for the first drain + // after a restart. if let Err(e) = self .db .set_own_sealed_rev(&uid.to_string(), &rev, now_millis()) @@ -1888,6 +1890,20 @@ fn is_own_self_supersede(complete: bool, remote_rev: Option<&str>, own_rev: Opti complete && remote_rev.is_some() && remote_rev == own_rev } +/// Whether Drive has `node` neither where its queued rename found it nor where +/// the rename takes it (`parent`, `name`), so someone else moved or renamed it +/// in between. Unknown on a row without the original name. +fn moved_elsewhere(meta: &RenameMeta, node: &Node, parent: &NodeUid, name: &str) -> bool { + let Some(original_name) = meta.original_name.as_deref() else { + return false; + }; + let at = |parent: &str, name: &str| { + node.parent_uid.as_ref().map(ToString::to_string).as_deref() == Some(parent) + && node.name == name + }; + !at(&meta.original_parent_uid, original_name) && !at(&parent.to_string(), name) +} + /// What the park sweep does with one parked create whose node still exists. #[derive(Debug, PartialEq, Eq)] enum ParkVerdict { @@ -2059,6 +2075,7 @@ mod tests { op.meta_json = Some( serde_json::to_string(&RenameMeta { original_parent_uid: source.uid.to_string(), + original_name: Some(moved.name.clone()), }) .unwrap(), ); diff --git a/crates/pdfs-fuse/src/lib.rs b/crates/pdfs-fuse/src/lib.rs index 18d2c0e..6904684 100644 --- a/crates/pdfs-fuse/src/lib.rs +++ b/crates/pdfs-fuse/src/lib.rs @@ -620,18 +620,6 @@ struct Core { /// ([`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 - /// remote already moved on — but if the remote sits at a revision *we* - /// sealed, no other device touched the file: it is a single-writer - /// stall→resume (a browser download that closed and reopened its fd, then - /// rewrote the whole file). [`Core::revision_conflict`] consults this to - /// adopt our own revision as the base and supersede instead of forking - /// (docs/BUGS.md B70, layer B). One entry per written node, overwritten as - /// the node advances and dropped when it is trashed; lost on restart, which - /// only widens the (already narrow) fork window across a daemon bounce. - own_sealed_revs: Arc>>, /// Photos whose missing thumbnail is being generated right now. A tile that is /// still on screen asks for its thumbnail again every few seconds, and each of /// those downloads is a full-size photo — so an in-flight uid is never started @@ -3650,23 +3638,27 @@ impl Core { new_parent_uid: &NodeUid, new_name: &str, ) -> Result<(), Errno> { - let original_parent_uid = match self.db.pending_op_meta(&uid.to_string(), OP_RENAME) { - Ok(Some(json)) => serde_json::from_str::(&json) - .map(|meta| meta.original_parent_uid) - .map_err(|error| { - error!(%uid, %error, "queued rename authority metadata is invalid"); - Errno::EIO - })?, - Ok(None) => old_parent_uid.to_string(), + // A rename that replaces a queued one keeps where the node started. + let meta = match self.db.pending_op_meta(&uid.to_string(), OP_RENAME) { + Ok(Some(json)) => serde_json::from_str::(&json).map_err(|error| { + error!(%uid, %error, "queued rename authority metadata is invalid"); + Errno::EIO + })?, + Ok(None) => RenameMeta { + original_parent_uid: old_parent_uid.to_string(), + original_name: self + .state + .lock() + .entries + .get(&ino) + .map(|entry| entry.node.name.clone()), + }, Err(error) => { error!(%uid, %error, "reading queued rename authority failed"); return Err(Errno::EIO); } }; - let meta_json = serde_json::to_string(&RenameMeta { - original_parent_uid, - }) - .map_err(|error| { + let meta_json = serde_json::to_string(&meta).map_err(|error| { error!(%uid, %error, "serializing queued rename authority failed"); Errno::EIO })?; diff --git a/crates/pdfs-fuse/src/mount.rs b/crates/pdfs-fuse/src/mount.rs index 596c294..2dde63b 100644 --- a/crates/pdfs-fuse/src/mount.rs +++ b/crates/pdfs-fuse/src/mount.rs @@ -440,7 +440,6 @@ pub(crate) fn mount_with( trash_progress: Arc::new(tokio::sync::Notify::new()), conflict_notified: Arc::new(Mutex::new(HashSet::new())), sweep_mode, - own_sealed_revs: Arc::new(Mutex::new(HashMap::new())), self_changes: Arc::new(Mutex::new(HashMap::new())), thumb_gen: Arc::new(Mutex::new(HashSet::new())), thumb_gen_budget: Arc::new(tokio::sync::Semaphore::new(