diff --git a/crates/pdfs-cli/src/main.rs b/crates/pdfs-cli/src/main.rs index 6b26f0b..fcd73f5 100644 --- a/crates/pdfs-cli/src/main.rs +++ b/crates/pdfs-cli/src/main.rs @@ -1983,6 +1983,7 @@ fn mount_once(mountpoint: Option) -> Result { sweep_mode: config.resolved_conflict_sweep(), upload_limit: config.upload_limit.unwrap_or(0), download_limit: config.download_limit.unwrap_or(0), + local_first: config.local_first.unwrap_or(true), }, ); diff --git a/crates/pdfs-core/src/config.rs b/crates/pdfs-core/src/config.rs index 60ea86e..42d8a55 100644 --- a/crates/pdfs-core/src/config.rs +++ b/crates/pdfs-core/src/config.rs @@ -154,6 +154,13 @@ pub struct AppConfig { /// configs predating the field. #[serde(default)] pub online_map: bool, + /// Record a new folder, a new file, a rename or a delete through the mount + /// locally and send it from the queue, instead of waiting for Drive in the + /// syscall. `None` means on. `false` brings back the old path for one + /// release (docs/MILESTONE-3.0.0.md §10). Defaulted for configs predating + /// the field. + #[serde(default)] + pub local_first: Option, } /// The My Files page's view choices, remembered between runs. @@ -228,6 +235,7 @@ impl Default for AppConfig { tray_hidden: false, language: None, online_map: false, + local_first: None, } } } @@ -584,6 +592,7 @@ mod tests { tray_hidden: true, language: Some("de".to_string()), online_map: true, + local_first: Some(false), }; let json = serde_json::to_string(&config).unwrap(); let decoded: AppConfig = serde_json::from_str(&json).unwrap(); diff --git a/crates/pdfs-core/src/db/mod.rs b/crates/pdfs-core/src/db/mod.rs index b26965f..32a1701 100644 --- a/crates/pdfs-core/src/db/mod.rs +++ b/crates/pdfs-core/src/db/mod.rs @@ -56,9 +56,9 @@ pub use local::LocalFileHit; pub use maintenance::{DbStats, VacuumOutcome}; pub use nodes::{PublishedSharedRoot, SearchHit, StoredNode}; pub use ops::{ - AttachedBlob, CreateLanding, FAILING_ATTEMPTS, LOCAL_VOLUME, OP_CREATE, OP_MKDIR, OP_RENAME, - OP_REVISION, OP_TRASH, PARK_EXPIRY_MS, PARK_UNTIL, PendingCounts, PendingOp, RenameMeta, - op_supersedes, + AttachedBlob, CreateLanding, CreateRetired, FAILING_ATTEMPTS, LOCAL_VOLUME, OP_CREATE, + OP_MKDIR, OP_RENAME, OP_REVISION, OP_TRASH, PARK_EXPIRY_MS, PARK_UNTIL, PendingCounts, + PendingOp, RenameMeta, op_supersedes, }; pub use photos::{StoredPhoto, StoredPlace, THUMB_HAVE, THUMB_NONE, THUMB_UNKNOWN, TimelineRow}; pub use pins::PinRow; diff --git a/crates/pdfs-core/src/db/nodes.rs b/crates/pdfs-core/src/db/nodes.rs index 7c13d1e..766349c 100644 --- a/crates/pdfs-core/src/db/nodes.rs +++ b/crates/pdfs-core/src/db/nodes.rs @@ -6,6 +6,7 @@ use std::collections::HashSet; use rusqlite::{OptionalExtension, Transaction, params}; use super::Db; +use super::ops::{OP_CREATE, OP_MKDIR, OP_RENAME, wake_ops_waiting_for_tx}; use crate::{Access, Result}; use proton_drive_rs::proton_sdk::ids::{LinkId, NodeUid, VolumeId}; use proton_drive_rs::{Node, NodeKind}; @@ -266,10 +267,22 @@ impl Db { /// resolve its shared-tree access. Removing the op first could resurrect /// that row after a crash; removing the row first could strand a now /// unauthorizable op. One transaction closes both windows. + /// + /// An op that found the name still held by this trash failed with a + /// backoff, and the name is free now. It is made due at once + /// ([`Db::wake_ops_waiting_for`]). pub fn complete_trash_op(&self, op_id: i64, uid: &NodeUid) -> Result<()> { let uid = uid.to_string(); let mut conn = self.conn.lock(); let tx = conn.transaction()?; + let name: Option = tx + .query_row( + "SELECT name FROM pending_op WHERE id = ?1", + params![op_id], + |row| row.get(0), + ) + .optional()? + .flatten(); let rowid: Option = tx .query_row( "SELECT rowid FROM nodes WHERE uid = ?1", @@ -282,6 +295,9 @@ impl Db { tx.execute("DELETE FROM nodes_fts WHERE rowid = ?1", params![rowid])?; } tx.execute("DELETE FROM pending_op WHERE id = ?1", params![op_id])?; + if let Some(name) = name { + wake_ops_waiting_for_tx(&tx, &name)?; + } tx.commit()?; Ok(()) } @@ -549,6 +565,18 @@ impl Db { } } + /// The uid of the node with local id `lid`: `None` when it has no row, or + /// is not on Drive yet. + pub fn uid_of_lid(&self, lid: i64) -> Result> { + let conn = self.read(); + Ok(conn + .query_row("SELECT uid FROM nodes WHERE lid = ?1", params![lid], |r| { + r.get(0) + }) + .optional()? + .flatten()) + } + /// The local id of the node stored under `uid`, or `None` when it has no /// row. It stays the same for as long as the row exists, including when a /// create lands and the row takes the uid Drive gave it. @@ -636,6 +664,40 @@ impl Db { Ok(out) } + /// What a queued change does to `parent`'s listing, which Drive does not + /// show until it lands: the children made or moved in by a queued create, + /// mkdir or rename, as the DB holds them, and the nodes a queued rename + /// moves elsewhere. + /// + /// A listing read from Drive is laid over with these, or a folder listed + /// while a create in it is queued, after a restart say, loses the file + /// until the create lands (`docs/BUGS.md` B132). + pub fn queued_children(&self, parent: &NodeUid) -> Result<(Vec, HashSet)> { + let conn = self.read(); + let parent = parent.to_string(); + let mut stmt = conn.prepare( + "SELECT n.node_json FROM nodes n + WHERE n.parent_uid = ?1 AND n.node_json IS NOT NULL AND n.trashed = 0 + AND EXISTS (SELECT 1 FROM pending_op p + WHERE p.uid = n.uid AND p.kind IN (?2, ?3, ?4))", + )?; + let rows = stmt.query_map(params![parent, OP_CREATE, OP_MKDIR, OP_RENAME], |r| { + r.get::<_, String>(0) + })?; + let mut here = Vec::new(); + for json in rows { + here.push(serde_json::from_str(&json?)?); + } + let mut stmt = conn.prepare( + "SELECT DISTINCT p.uid FROM pending_op p JOIN nodes n ON n.uid = p.uid + WHERE p.kind = ?2 AND n.parent_uid IS NOT ?1", + )?; + let gone = stmt + .query_map(params![parent, OP_RENAME], |r| r.get::<_, String>(0))? + .collect::, _>>()?; + Ok((here, gone)) + } + // --- Content-cache LRU index (P4) ------------------------------------- // // Replaces the per-eviction `read_dir` scans in `ContentCache`. Each cached @@ -671,9 +733,18 @@ pub(super) fn adopt_placeholder_row_tx( local: &str, real: &str, ) -> Result<()> { + // The node a row holds names its parent too, and is what a restart builds + // the tree from: a folder that landed with no write to its children after + // left them under a parent no row has (`docs/BUGS.md` B140). + let real_json = parse_node_uid(real) + .map(|uid| serde_json::to_string(&uid)) + .transpose()?; tx.execute( - "UPDATE nodes SET parent_uid = ?2 WHERE parent_uid = ?1", - params![local, real], + "UPDATE nodes SET parent_uid = ?2, + node_json = CASE WHEN ?3 IS NULL OR node_json IS NULL THEN node_json + ELSE json_set(node_json, '$.parent_uid', json(?3)) END + WHERE parent_uid = ?1", + params![local, real, real_json], )?; let row: Option<(i64, Option)> = tx .query_row( diff --git a/crates/pdfs-core/src/db/ops.rs b/crates/pdfs-core/src/db/ops.rs index a72c5ca..0681ddd 100644 --- a/crates/pdfs-core/src/db/ops.rs +++ b/crates/pdfs-core/src/db/ops.rs @@ -92,6 +92,13 @@ pub fn op_supersedes(kind: &str) -> bool { /// Other ops are taken from the node tree only for revisions. A queued rename /// that moved a node *into* the folder has to land, or the node would stay /// where it was on the server instead of going to the trash with the folder. +/// +/// A create already on the wire cannot be stopped, so its row is not deleted +/// but turned into a trash of the node it is making, still claimed by the +/// upload. [`Db::finish_create`] points it at the landed node. Until then it +/// holds the name, as any queued trash does ([`Db::has_pending_trash_named`]): +/// a rename over the file sent first found the name taken and landed under a +/// conflict name (`docs/BUGS.md` B129). fn drop_doomed_ops(tx: &rusqlite::Transaction<'_>, uid: &str) -> Result> { const DOOMED: &str = " WITH RECURSIVE @@ -124,12 +131,48 @@ fn drop_doomed_ops(tx: &rusqlite::Transaction<'_>, uid: &str) -> Result, _>>()? }; tx.execute( - &format!("{DOOMED} DELETE FROM pending_op WHERE id IN (SELECT id FROM doomed)"), + &format!( + "{DOOMED} + UPDATE pending_op + SET kind = 'trash', parent_uid = NULL, blob_path = NULL, meta_json = NULL + WHERE id IN (SELECT id FROM doomed) + AND kind IN ('create', 'mkdir') AND claimed_at <> 0" + ), + params![uid], + )?; + tx.execute( + &format!( + "{DOOMED} + DELETE FROM pending_op + WHERE id IN (SELECT id FROM doomed) + AND NOT (kind = 'trash' AND claimed_at <> 0 AND uid LIKE '{LOCAL_VOLUME}~%')" + ), params![uid], )?; Ok(blobs) } +/// The `pending_op` rows, `?1` being [`OP_RENAME`], that move a node away from +/// the name `?2`. +const MOVES_FROM: &str = "kind = ?1 + AND json_extract(meta_json, '$.original_name') = ?2 + AND (name <> ?2 OR parent_uid <> json_extract(meta_json, '$.original_parent_uid'))"; + +/// [`Db::wake_ops_waiting_for`] within a transaction. +pub(super) fn wake_ops_waiting_for_tx(tx: &rusqlite::Transaction<'_>, name: &str) -> Result<()> { + tx.execute( + &format!( + "UPDATE pending_op SET next_attempt_at = 0 + WHERE name = ?2 AND kind IN (?3, ?4, ?1) AND attempts > 0 + AND next_attempt_at < ?5 + AND NOT EXISTS (SELECT 1 FROM pending_op WHERE kind = ?6 AND name = ?2) + AND NOT EXISTS (SELECT 1 FROM pending_op WHERE {MOVES_FROM})" + ), + params![OP_RENAME, name, OP_CREATE, OP_MKDIR, PARK_UNTIL, OP_TRASH], + )?; + Ok(()) +} + /// The volume id given to a node that exists only on this machine, so far. A /// real [`NodeUid`] is `{volume}~{link}`, so a placeholder is `local~` and /// round-trips through the same `Display`/parse path as any other uid. @@ -242,6 +285,20 @@ pub struct CreateLanding<'a> { pub landed: (&'a str, &'a str), } +/// What became of a queued create when it landed, from [`Db::finish_create`]. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum CreateRetired { + /// Its row is gone and nothing of it is left queued. + Landed, + /// A newer write attached to it on the wire and is now queued as a + /// revision of the real node: its blob and its rewritten sidecar. + Newer { blob: String, meta: String }, + /// Its row had been dropped while it was on the wire: the node was deleted, + /// or a rename replaced it. The node Drive just made is unwanted, and a + /// trash of it is queued under this id, claimed by the caller. + Withdrawn { trash: i64 }, +} + struct CreateRow { blob: Option, meta: Option, @@ -278,6 +335,32 @@ impl Db { .map_err(Into::into) } + /// Whether a queued rename moves a node away from `name`. + /// + /// Like a queued trash ([`Db::has_pending_trash_named`]) it frees the name + /// once it lands, so an op that wants the name can wait for it. A create + /// renamed while it was on the wire queues one (`Db::finish_create`). + pub fn has_pending_move_from(&self, name: &str) -> Result { + let conn = self.read(); + conn.query_row( + &format!("SELECT EXISTS(SELECT 1 FROM pending_op WHERE {MOVES_FROM})"), + params![OP_RENAME, name], + |row| row.get(0), + ) + .map_err(Into::into) + } + + /// Make the ops that failed because `name` was held due at once, now that + /// what held it let go. Nothing changes while another queued trash or + /// rename still holds it, or for a parked op. + pub fn wake_ops_waiting_for(&self, name: &str) -> Result<()> { + let mut conn = self.conn.lock(); + let tx = conn.transaction()?; + wake_ops_waiting_for_tx(&tx, name)?; + tx.commit()?; + Ok(()) + } + /// Whether a specific desired-state operation is still queued for a node. pub fn has_pending_op(&self, uid: &str, kind: &str) -> Result { let conn = self.read(); @@ -304,21 +387,29 @@ impl Db { .map_err(Into::into) } + /// Queue `op`, returning its id and the staged blob of the op it replaced. + /// + /// A rename a drain worker holds is not replaced but queued behind: the + /// claim keeps the newer one from being sent until it retires, so the two + /// land in the order they were made (`docs/BUGS.md` B141). Should it fail, + /// [`Db::record_op_failure`] drops it. pub fn enqueue_op(&self, op: &PendingOp) -> Result<(i64, Option)> { let mut conn = self.conn.lock(); let tx = conn.transaction()?; let superseded: Option = if op_supersedes(&op.kind) { let blob: Option = tx .query_row( - "SELECT blob_path FROM pending_op WHERE uid = ?1 AND kind = ?2", - params![op.uid, op.kind], + "SELECT blob_path FROM pending_op + WHERE uid = ?1 AND kind = ?2 AND (kind <> ?3 OR claimed_at = 0)", + params![op.uid, op.kind, OP_RENAME], |r| r.get(0), ) .optional()? .flatten(); tx.execute( - "DELETE FROM pending_op WHERE uid = ?1 AND kind = ?2", - params![op.uid, op.kind], + "DELETE FROM pending_op + WHERE uid = ?1 AND kind = ?2 AND (kind <> ?3 OR claimed_at = 0)", + params![op.uid, op.kind, OP_RENAME], )?; blob } else { @@ -429,21 +520,28 @@ impl Db { /// row names another parent or name than `landing.sent`, a rename of `real` /// to that target is queued in the same transaction, or the move would be /// lost with the row. + /// + /// A delete while the create is on the wire cannot stop the upload. It + /// turns the row into a trash ([`Db::delete_ops_for_uid`]), which is + /// pointed at `real` here, still claimed by the caller, so the node is not + /// left on Drive holding the name (`docs/BUGS.md` B129). A row gone + /// altogether is queued as such a trash. The placeholder row is left to + /// the delete that withdrew it. pub fn finish_create( &self, id: i64, uploaded: Option<&str>, landing: &CreateLanding<'_>, rewrite: impl FnOnce(&str) -> Option, - ) -> Result> { + ) -> Result { let (local, real) = (landing.local, landing.real); let mut conn = self.conn.lock(); let tx = conn.transaction()?; let row: Option = tx .query_row( "SELECT blob_path, meta_json, parent_uid, name, created_at - FROM pending_op WHERE id = ?1", - params![id], + FROM pending_op WHERE id = ?1 AND kind <> ?2", + params![id, OP_TRASH], |r| { Ok(CreateRow { blob: r.get(0)?, @@ -455,6 +553,28 @@ impl Db { }, ) .optional()?; + let withdrawn = tx.execute( + "UPDATE pending_op SET uid = ?2, name = ?3 WHERE id = ?1 AND kind = ?4", + params![id, real, landing.landed.1, OP_TRASH], + )?; + if withdrawn > 0 { + tx.commit()?; + return Ok(CreateRetired::Withdrawn { trash: id }); + } + if row.is_none() { + let now = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map_or(1, |d| d.as_millis() as i64); + tx.execute( + "INSERT INTO pending_op + (kind, uid, parent_uid, name, created_at, next_attempt_at, claimed_at) + VALUES (?1, ?2, NULL, ?3, ?4, 0, ?4)", + params![OP_TRASH, real, landing.landed.1, now], + )?; + let trash = tx.last_insert_rowid(); + tx.commit()?; + return Ok(CreateRetired::Withdrawn { trash }); + } let newer = match &row { Some(CreateRow { blob: Some(blob), @@ -505,7 +625,10 @@ impl Db { )?; adopt_placeholder_row_tx(&tx, local, real)?; tx.commit()?; - Ok(newer) + Ok(match newer { + Some((blob, meta)) => CreateRetired::Newer { blob, meta }, + None => CreateRetired::Landed, + }) } /// Replace the sidecar of a queued op, for a baseline that has moved under @@ -539,12 +662,17 @@ impl Db { /// *is* the rename. Returns false when the create has already drained, in /// which case the node has a real uid and the caller must rename it there /// instead (offline.md Phase 3b). + /// + /// A create backing off is made due: what it failed on may have been the + /// name it no longer wants, such as one held by a queued change. A parked + /// create stays parked. pub fn rewrite_op_target(&self, uid: &str, parent_uid: &str, name: &str) -> Result { let conn = self.conn.lock(); let n = conn.execute( - "UPDATE pending_op SET parent_uid = ?2, name = ?3 + "UPDATE pending_op SET parent_uid = ?2, name = ?3, + next_attempt_at = CASE WHEN next_attempt_at < ?6 THEN 0 ELSE next_attempt_at END WHERE uid = ?1 AND kind IN (?4, ?5)", - params![uid, parent_uid, name, OP_CREATE, OP_MKDIR], + params![uid, parent_uid, name, OP_CREATE, OP_MKDIR, PARK_UNTIL], )?; Ok(n > 0) } @@ -978,8 +1106,22 @@ impl Db { /// Clears any access-deferral window: whatever the op was waiting for has /// now been reported, so the next uncleared deferral starts its own window /// rather than escalating again on the following recheck. + /// + /// A rename with a newer one queued behind it ([`Db::enqueue_op`]) is + /// dropped instead: retried, it could land after the newer one and undo it. pub fn record_op_failure(&self, id: i64, error: &str, next_attempt_at: i64) -> Result<()> { let conn = self.conn.lock(); + let dropped = conn.execute( + "DELETE FROM pending_op + WHERE id = ?1 AND kind = ?2 + AND EXISTS (SELECT 1 FROM pending_op AS newer + WHERE newer.uid = pending_op.uid AND newer.kind = ?2 + AND newer.id > ?1)", + params![id, OP_RENAME], + )?; + if dropped > 0 { + return Ok(()); + } conn.execute( "UPDATE pending_op SET attempts = attempts + 1, last_error = ?2, next_attempt_at = ?3, @@ -990,6 +1132,27 @@ impl Db { Ok(()) } + /// Make a create due again if it was renamed or moved + /// ([`Db::rewrite_op_target`]) while the attempt that just failed was on + /// the wire, `parent_uid` and `name` being what that attempt sent. What it + /// failed on may have been the target it no longer has. Returns whether it + /// was. + pub fn retry_if_retargeted( + &self, + id: i64, + parent_uid: Option<&str>, + name: Option<&str>, + ) -> Result { + let conn = self.conn.lock(); + let n = conn.execute( + "UPDATE pending_op SET next_attempt_at = 0 + WHERE id = ?1 AND kind IN (?4, ?5) AND next_attempt_at < ?6 + AND (parent_uid IS NOT ?2 OR name IS NOT ?3)", + params![id, parent_uid, name, OP_CREATE, OP_MKDIR, PARK_UNTIL], + )?; + Ok(n > 0) + } + /// Defer a due operation without recording a failed remote attempt. /// /// Access checks happen before the drain calls Proton, so a locally denied diff --git a/crates/pdfs-core/src/db/tests.rs b/crates/pdfs-core/src/db/tests.rs index 5d3d178..12e432f 100644 --- a/crates/pdfs-core/src/db/tests.rs +++ b/crates/pdfs-core/src/db/tests.rs @@ -2474,6 +2474,125 @@ fn a_queued_trash_holds_its_name_until_it_lands() { assert!(!db.has_pending_trash_named("report.txt").unwrap()); } +/// A create that found its name held by a queued trash backed off. The trash +/// landing frees the name, so the create is due at once instead of after the +/// backoff; a parked create stays parked. +#[test] +fn a_landed_trash_makes_the_ops_waiting_for_its_name_due() { + let db = Db::open_in_memory().unwrap(); + let old = uid("old"); + db.upsert_node(&file("old", "root", "report.txt", 1)) + .unwrap(); + let (trash_id, _) = db + .replace_ops_with_trash(&old.to_string(), "report.txt", 1) + .unwrap(); + let create = |name: &str, next_attempt_at: i64| { + let (id, _) = db + .enqueue_op(&PendingOp { + id: 0, + kind: OP_CREATE.to_string(), + uid: format!("local~{name}-{next_attempt_at}"), + parent_uid: Some(uid("root").to_string()), + name: Some(name.to_string()), + blob_path: None, + meta_json: None, + created_at: 2, + attempts: 0, + last_error: None, + next_attempt_at, + }) + .unwrap(); + db.record_op_failure(id, "report.txt is still held", next_attempt_at) + .unwrap(); + id + }; + let waiting = create("report.txt", 9_000); + let parked = create("report.txt", PARK_UNTIL); + let other = create("other.txt", 9_000); + + db.complete_trash_op(trash_id, &old).unwrap(); + + let due = |id| { + db.pending_ops() + .unwrap() + .into_iter() + .find(|op| op.id == id) + .unwrap() + .next_attempt_at + }; + assert_eq!(due(waiting), 0); + assert_eq!(due(parked), PARK_UNTIL); + assert_eq!(due(other), 9_000); +} + +#[test] +fn a_queued_rename_away_from_a_name_holds_it_until_it_lands() { + let db = Db::open_in_memory().unwrap(); + let root = uid("root").to_string(); + let rename = |node: &str, from: &str, to: &str| { + let meta = serde_json::to_string(&RenameMeta { + original_parent_uid: root.clone(), + original_name: Some(from.to_string()), + }) + .unwrap(); + db.enqueue_op(&PendingOp { + id: 0, + kind: OP_RENAME.to_string(), + uid: uid(node).to_string(), + parent_uid: Some(root.clone()), + name: Some(to.to_string()), + blob_path: None, + meta_json: Some(meta), + created_at: 1, + attempts: 0, + last_error: None, + next_attempt_at: 0, + }) + .unwrap() + .0 + }; + let away = rename("away", "c.bin", "d"); + rename("back", "e.md", "e.md"); + let (waiting, _) = db + .enqueue_op(&PendingOp { + id: 0, + kind: OP_CREATE.to_string(), + uid: "local~waiting".to_string(), + parent_uid: Some(root.clone()), + name: Some("c.bin".to_string()), + blob_path: None, + meta_json: None, + created_at: 2, + attempts: 0, + last_error: None, + next_attempt_at: 0, + }) + .unwrap(); + db.record_op_failure(waiting, "c.bin is still held", 9_000) + .unwrap(); + + assert!(db.has_pending_move_from("c.bin").unwrap()); + assert!(!db.has_pending_move_from("d").unwrap()); + assert!( + !db.has_pending_move_from("e.md").unwrap(), + "a rename that keeps the name in its folder frees nothing" + ); + db.wake_ops_waiting_for("c.bin").unwrap(); + let due = || { + db.pending_ops() + .unwrap() + .into_iter() + .find(|op| op.id == waiting) + .unwrap() + .next_attempt_at + }; + assert_eq!(due(), 9_000, "the rename still holds the name"); + + db.delete_op(away).unwrap(); + db.wake_ops_waiting_for("c.bin").unwrap(); + assert_eq!(due(), 0); +} + #[test] fn failed_atomic_trash_insert_keeps_prior_revision_and_blob_ownership() { let db = Db::open_in_memory().unwrap(); @@ -2796,6 +2915,106 @@ fn renaming_a_queued_create_rewrites_its_target() { ); } +/// A create that failed on a name held by a queued change waits out a +/// backoff. Renamed away from that name, it is due again at once; parked, it +/// stays parked. +#[test] +fn renaming_a_backing_off_create_makes_it_due() { + let db = Db::open_in_memory().unwrap(); + let create = |local: &str| PendingOp { + id: 0, + kind: OP_CREATE.to_string(), + uid: local.to_string(), + parent_uid: Some(uid("dir").to_string()), + name: Some("a.txt".to_string()), + blob_path: Some(format!("/staging/{local}")), + meta_json: Some("{}".to_string()), + created_at: 1, + attempts: 0, + last_error: None, + next_attempt_at: 0, + }; + let (failed, _) = db.enqueue_op(&create("local~failed")).unwrap(); + db.record_op_failure(failed, "a.txt is still held", 10_000) + .unwrap(); + db.enqueue_op(&create("local~parked")).unwrap(); + db.set_create_hold("local~parked", true).unwrap(); + + db.rewrite_op_target("local~failed", &uid("dir").to_string(), "d") + .unwrap(); + db.rewrite_op_target("local~parked", &uid("dir").to_string(), "e.tmp") + .unwrap(); + + let ops = db.pending_ops().unwrap(); + let op = |local: &str| ops.iter().find(|o| o.uid == local).unwrap(); + assert_eq!(op("local~failed").next_attempt_at, 0); + assert_eq!(op("local~failed").attempts, 1); + assert_eq!(op("local~parked").next_attempt_at, PARK_UNTIL); + + // Renamed while an attempt was on the wire, which then failed: the + // failure is recorded after the rename, and the create is still due. + let sent = op("local~failed").clone(); + db.rewrite_op_target("local~failed", &uid("dir").to_string(), "f") + .unwrap(); + db.record_op_failure(sent.id, "d is still held", 10_000) + .unwrap(); + assert!( + db.retry_if_retargeted(sent.id, sent.parent_uid.as_deref(), sent.name.as_deref()) + .unwrap() + ); + let ops = db.pending_ops().unwrap(); + assert_eq!( + ops.iter() + .find(|o| o.id == sent.id) + .unwrap() + .next_attempt_at, + 0 + ); + assert!( + !db.retry_if_retargeted(sent.id, sent.parent_uid.as_deref(), Some("f")) + .unwrap() + ); +} + +/// A rename queued while an older one is on the wire waits for it. The older +/// one, if it fails, is dropped rather than retried after the newer one. +#[test] +fn a_rename_on_the_wire_lands_before_the_one_that_replaces_it() { + let db = Db::open_in_memory().unwrap(); + let file = uid("file").to_string(); + let rename = |parent: &str| PendingOp { + id: 0, + kind: OP_RENAME.to_string(), + uid: file.clone(), + parent_uid: Some(uid(parent).to_string()), + name: Some("e.md".to_string()), + blob_path: None, + meta_json: None, + created_at: 1, + attempts: 0, + last_error: None, + next_attempt_at: 0, + }; + let (first, _) = db.enqueue_op(&rename("x")).unwrap(); + assert_eq!(db.claim_next_due_op(1).unwrap().unwrap().id, first); + + let (second, _) = db.enqueue_op(&rename("c0")).unwrap(); + assert!(db.claim_next_due_op(1).unwrap().is_none()); + + db.record_op_failure(first, "original hash is out of date", 10_000) + .unwrap(); + db.release_op_claim(first).unwrap(); + let ops = db.pending_ops().unwrap(); + assert_eq!(ops.iter().map(|o| o.id).collect::>(), [second]); + assert_eq!(db.claim_next_due_op(1).unwrap().unwrap().id, second); + + // Unclaimed, a queued rename is replaced as before. + db.release_op_claim(second).unwrap(); + let (third, _) = db.enqueue_op(&rename("x")).unwrap(); + let ops = db.pending_ops().unwrap(); + assert_eq!(ops.iter().map(|o| o.id).collect::>(), [third]); +} + #[test] fn a_write_folds_into_a_queued_create_instead_of_superseding_it() { let db = Db::open_in_memory().unwrap(); @@ -2885,10 +3104,10 @@ fn a_write_attached_while_its_create_uploads_becomes_a_revision() { assert_eq!( newer, - Some(( - "/staging/written".to_string(), - "{\"uid\":\"vol~real\"}".to_string() - )) + CreateRetired::Newer { + blob: "/staging/written".to_string(), + meta: "{\"uid\":\"vol~real\"}".to_string() + } ); let ops = db.pending_ops().unwrap(); assert_eq!(ops.len(), 1, "the write is still queued"); @@ -2943,7 +3162,7 @@ fn a_create_whose_blob_landed_is_retired() { ) .unwrap(); - assert_eq!(newer, None); + assert_eq!(newer, CreateRetired::Landed); let ops = db.pending_ops().unwrap(); assert_eq!(ops.len(), 1, "only the child is left"); assert_eq!(ops[0].parent_uid.as_deref(), Some(real.as_str())); @@ -3102,6 +3321,60 @@ fn a_landed_folder_keeps_a_row_until_the_server_copy_replaces_it() { assert_eq!(found[0].path, "New folder/inside.txt"); } +/// A file deleted while its create was on the wire still lands: nothing stops +/// an upload in flight. The node it made is unwanted, so a trash of it is +/// queued in the same transaction, already claimed by the drain that landed it +/// (`docs/BUGS.md` B129). +#[test] +fn a_create_withdrawn_while_it_uploads_queues_a_trash_of_the_landed_node() { + let db = Db::open_in_memory().unwrap(); + let local = "local~gone"; + let real = uid("real").to_string(); + let parent = uid("parent").to_string(); + let (id, _) = db + .enqueue_op(&PendingOp { + id: 0, + kind: OP_CREATE.to_string(), + uid: local.to_string(), + parent_uid: Some(parent.clone()), + name: Some("d".to_string()), + blob_path: Some("/staging/sent".to_string()), + meta_json: Some("{}".to_string()), + created_at: 1, + attempts: 0, + last_error: None, + next_attempt_at: 0, + }) + .unwrap(); + db.claim_next_due_op(10) + .unwrap() + .expect("the drain takes the create"); + db.delete_ops_for_uid(local).unwrap(); + + let retired = db + .finish_create( + id, + Some("/staging/sent"), + &landing(local, &real, &parent, "d"), + |_| None, + ) + .unwrap(); + + let CreateRetired::Withdrawn { trash } = retired else { + panic!("a withdrawn create is not landed: {retired:?}"); + }; + let ops = db.pending_ops().unwrap(); + assert_eq!(ops.len(), 1); + assert_eq!(ops[0].id, trash); + assert_eq!(ops[0].kind, OP_TRASH); + assert_eq!(ops[0].uid, real); + assert_eq!(ops[0].name.as_deref(), Some("d")); + assert!( + db.claim_next_due_op(i64::MAX / 2).unwrap().is_none(), + "the trash is the landing drain's to send" + ); +} + #[test] fn a_landed_placeholder_never_overwrites_the_server_copy() { let db = Db::open_in_memory().unwrap(); @@ -3194,6 +3467,7 @@ fn a_landed_placeholder_keeps_its_lid() { assert_eq!(db.lid_of(&local.to_string()).unwrap(), None); assert_eq!(db.lid_of(&real.to_string()).unwrap(), Some(lid)); + assert_eq!(db.uid_of_lid(lid).unwrap(), Some(real.to_string())); let name = if known { "New folder (1)" } else { @@ -3207,6 +3481,85 @@ fn a_landed_placeholder_keeps_its_lid() { } } +/// Drive lists none of what is queued for a folder, so a listing read from +/// it is laid over with what the queue made, moved in and moved away (B132). +#[test] +fn queued_children_are_what_drive_does_not_list_yet() { + let db = Db::open_in_memory().unwrap(); + db.upsert_node(&folder("root", None, "My Files")).unwrap(); + db.upsert_node(&folder("dir", Some("root"), "dir")).unwrap(); + db.upsert_node(&folder("other", Some("root"), "other")) + .unwrap(); + let op = |kind: &str, uid: &str, parent: &str, name: &str| PendingOp { + id: 0, + kind: kind.to_string(), + uid: uid.to_string(), + parent_uid: Some(parent.to_string()), + name: Some(name.to_string()), + blob_path: None, + meta_json: None, + created_at: 1, + attempts: 0, + last_error: None, + next_attempt_at: 0, + }; + let dir = uid("dir").to_string(); + let other = uid("other").to_string(); + + let local = NodeUid::new(VolumeId::from("local"), LinkId::from("new")); + let mut made = file("x", "dir", "made.txt", 3); + made.uid = local.clone(); + db.upsert_node(&made).unwrap(); + db.enqueue_op(&op(OP_CREATE, &local.to_string(), &dir, "made.txt")) + .unwrap(); + db.upsert_node(&file("in", "dir", "in.txt", 3)).unwrap(); + db.enqueue_op(&op(OP_RENAME, &uid("in").to_string(), &dir, "in.txt")) + .unwrap(); + db.upsert_node(&file("out", "other", "out.txt", 3)).unwrap(); + db.enqueue_op(&op(OP_RENAME, &uid("out").to_string(), &other, "out.txt")) + .unwrap(); + db.upsert_node(&file("still", "dir", "still.txt", 3)) + .unwrap(); + + let (here, gone) = db.queued_children(&uid("dir")).unwrap(); + let mut names: Vec<_> = here.iter().map(|n| n.name.as_str()).collect(); + names.sort(); + assert_eq!(names, ["in.txt", "made.txt"]); + assert_eq!( + gone, + std::collections::HashSet::from([uid("out").to_string()]) + ); +} + +/// A restart builds the tree from the node each row holds. A folder that +/// landed handed its children to the real uid in the row's column only, so +/// they came back under a parent no row has (B140). +#[test] +fn a_landed_folder_hands_its_children_over_in_the_stored_node_too() { + let db = Db::open_in_memory().unwrap(); + db.upsert_node(&folder("root", None, "My Files")).unwrap(); + let local = NodeUid::new(VolumeId::from("local"), LinkId::from("dir")); + let mut placeholder = folder("x", Some("root"), "dir"); + placeholder.uid = local.clone(); + db.upsert_node(&placeholder).unwrap(); + let mut child = file("child", "x", "a.txt", 3); + child.parent_uid = Some(local.clone()); + db.upsert_node(&child).unwrap(); + + db.remap_local_uid(&local.to_string(), &uid("dir").to_string()) + .unwrap(); + + let stored = db.node_by_uid(&uid("child").to_string()).unwrap().unwrap(); + assert_eq!(stored.parent_uid, Some(uid("dir"))); + let names: Vec<_> = db + .known_children(&uid("dir")) + .unwrap() + .into_iter() + .map(|n| n.name) + .collect(); + assert_eq!(names, ["a.txt"]); +} + #[test] fn migration_v36_gives_every_node_its_rowid_as_lid() { let path = std::env::temp_dir().join(format!( diff --git a/crates/pdfs-fuse/src/drain.rs b/crates/pdfs-fuse/src/drain.rs index b2cf5ee..cfffc46 100644 --- a/crates/pdfs-fuse/src/drain.rs +++ b/crates/pdfs-fuse/src/drain.rs @@ -33,8 +33,8 @@ use pdfs_core::batch; use pdfs_core::cache::{Baseline, StagedWrite}; use pdfs_core::control::{ActivityKind, TransferDirection}; use pdfs_core::db::{ - CreateLanding, OP_CREATE, OP_MKDIR, OP_RENAME, OP_REVISION, OP_TRASH, PARK_EXPIRY_MS, - PendingOp, RenameMeta, + CreateLanding, CreateRetired, OP_CREATE, OP_MKDIR, OP_RENAME, OP_REVISION, OP_TRASH, + PARK_EXPIRY_MS, PendingOp, RenameMeta, }; use proton_drive_rs::proton_sdk::ids::NodeUid; use proton_drive_rs::{Node, NodeKind}; @@ -203,6 +203,9 @@ fn pending_op_authorities(op: &PendingOp) -> Result, Box Ok(Vec::new()), OP_REVISION | OP_TRASH => Ok(vec![uid()?]), OP_CREATE | OP_MKDIR => Ok(vec![parent()?]), OP_RENAME => { @@ -380,6 +383,19 @@ impl Core { self.wait_for_drain_work(); continue; } + match self.db.retry_if_retargeted( + op.id, + op.parent_uid.as_deref(), + op.name.as_deref(), + ) { + Ok(true) => { + debug!(uid = %op.uid, "create was renamed while it failed; retrying") + } + Ok(false) => {} + Err(e) => { + debug!(uid = %op.uid, error = %e, "checking a failed create's target failed") + } + } } // Unconditionally, on every path: a handler that retired its own row // leaves nothing to release, and one that returned without retiring @@ -639,6 +655,7 @@ impl Core { /// Perform one queued op and retire it. pub(crate) fn drain_op(&self, op: &PendingOp) -> Result<(), Box> { + self.finish_adoption(op)?; match op.kind.as_str() { OP_REVISION => self.drain_revision(op), OP_CREATE | OP_MKDIR => self.drain_local_node(op), @@ -648,6 +665,29 @@ impl Core { } } + /// Finish adopting the node `op` is for, if its create landed but could + /// not be read back. + /// + /// Until it is, the tree has the node as it was made locally, and a write + /// queued over it is based on that rather than on what Drive holds. Sent + /// so, the write found our own create's revision in its way and landed as + /// a conflict copy (`docs/BUGS.md` B143). + fn finish_adoption(&self, op: &PendingOp) -> Result<(), Box> { + let Some(real) = parse_node_uid(&op.uid) else { + return Ok(()); + }; + let Some(local) = self.unadopted.lock().get(&real).cloned() else { + return Ok(()); + }; + // A node gone since is the op's to deal with. + if self.fetch_node_remote(&real)?.is_some() { + self.adopt_real_uid(&local, &real, |_| {})?; + } + self.unadopted.lock().remove(&real); + debug!(%local, %real, "adopted a landed create"); + Ok(()) + } + /// Apply a queued rename/move to the remote. /// /// The op is the desired end state, so the remote's current state decides @@ -675,17 +715,21 @@ 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 meta: Option = op + .meta_json + .as_deref() + .map(serde_json::from_str) + .transpose()?; + if let Some(meta) = &meta + && 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)? + self.drain_rename_in_place(op, &uid, Some(&parent), &node.name, &name)? } else { // Move and rename land as one request (`move-multiple` takes a // target name), so there is no half-applied state between them and @@ -698,8 +742,8 @@ impl Core { // *different* name is the non-destructive resolution: it neither // clobbers their file nor drops ours, and it is visible. Err(e) if is_already_exists(&e) => { - if self.db.has_pending_trash_named(&name)? { - return Err(held_by_queued_trash(&name)); + if self.name_is_held(op, Some(&parent), &name)? { + return Err(held_by_queued_change(&name)); } let alt = conflict_name(&name, now_secs()); warn!(%uid, name, alt, "destination already holds that name; using a conflict name"); @@ -721,7 +765,13 @@ impl Core { // wedge the queue. Err(e) if is_gone(&e) => { warn!(%uid, name, %parent, "move destination is gone; leaving the node where it is"); - let landed = match self.drain_rename_in_place(&uid, &node.name, &name) { + let landed = match self.drain_rename_in_place( + op, + &uid, + node.parent_uid.as_ref(), + &node.name, + &name, + ) { Ok(landed) => landed, // The node went with it; nothing is left to rename. Err(e) if is_gone(e.as_ref()) => node.name.clone(), @@ -740,6 +790,13 @@ impl Core { } }; self.db.delete_op(op.id)?; + // An op that wanted the name this node let go of waits no longer. + if let Some(from) = meta.and_then(|m| m.original_name) + && from != landed + { + self.db.wake_ops_waiting_for(&from)?; + self.wake_drain(); + } info!(%uid, name = %landed, "pending rename landed"); Ok(()) } @@ -749,7 +806,9 @@ impl Core { /// offline; the node then lands under a conflict name instead. fn drain_rename_in_place( &self, + op: &PendingOp, uid: &NodeUid, + parent: Option<&NodeUid>, current: &str, name: &str, ) -> Result> { @@ -759,8 +818,8 @@ impl Core { match self.rename_remote(uid, name) { Ok(()) => Ok(name.to_string()), Err(e) if is_already_exists(&e) => { - if self.db.has_pending_trash_named(name)? { - return Err(held_by_queued_trash(name)); + if self.name_is_held(op, parent, name)? { + return Err(held_by_queued_change(name)); } let alt = conflict_name(name, now_secs()); warn!(%uid, name, alt, "rename target name is taken; using a conflict name"); @@ -786,6 +845,13 @@ impl Core { pub(crate) fn drain_trash(&self, op: &PendingOp) -> Result<(), Box> { let uid = parse_node_uid(&op.uid).ok_or("trash op has an unparseable uid")?; let name = op.name.clone().unwrap_or_else(|| op.uid.clone()); + // The trash a delete left of a create on the wire, which then failed: + // nothing was made, so nothing is trashed (`Db::delete_ops_for_uid`). + if is_local_uid_str(&op.uid) { + debug!(%uid, name, "withdrawn create never landed; trash op satisfied"); + self.db.complete_trash_op(op.id, &uid)?; + return Ok(()); + } match self .rt .block_on(self.drive.trash_nodes(std::slice::from_ref(&uid))) @@ -832,6 +898,22 @@ impl Core { &self, op: &PendingOp, ) -> Result<(), Box> { + // Drive holds the name the create is sent with once it lands, whatever + // became of the file meanwhile, until the rename or trash queued + // behind it frees it (`Core::name_is_held`). + if let (Some(parent), Some(name)) = (op.parent_uid.as_deref(), op.name.as_deref()) + && let Some(parent) = parse_node_uid(parent) + { + self.creating + .lock() + .insert(op.id, (parent, name.to_string())); + } + let created = self.create_local_node(op); + self.creating.lock().remove(&op.id); + created + } + + fn create_local_node(&self, op: &PendingOp) -> Result<(), Box> { let local = parse_node_uid(&op.uid).ok_or("pending op has an unparseable uid")?; let parent_str = op.parent_uid.as_deref().ok_or("create op has no parent")?; // `run_pending_drain` will not offer an op whose parent is still a @@ -863,9 +945,9 @@ impl Core { adopted = true; } if real.as_ref().is_err_and(|e| is_already_exists(e.as_ref())) - && self.db.has_pending_trash_named(&wanted)? + && self.name_is_held(op, Some(&parent), &wanted)? { - return Err(held_by_queued_trash(&wanted)); + return Err(held_by_queued_change(&wanted)); } if real.as_ref().is_err_and(|e| is_already_exists(e.as_ref())) { name = conflict_name(&wanted, now_secs()); @@ -906,6 +988,12 @@ impl Core { } } let real = real?; + // Retiring the op makes the ops behind it due: a child created in a new + // folder, a rename made while the create was on the wire. Drive answers + // a create before it lists the node, and those ops sent before then + // found no folder or dropped the rename as gone. Best effort: + // `adopt_real_uid` reads it back again and reports the failure. + let _ = self.read_back(&real); // Retire the op before touching anything else: if we crash here the node // exists remotely and the local placeholder is reconciled by the event @@ -919,7 +1007,12 @@ impl Core { sent: (parent_str, &wanted), landed: (&home.to_string(), &name), }; - let newer = self.retire_create(op, uploaded, &landing, &local, &real)?; + let newer = match self.retire_create(op, uploaded, &landing, &local, &real)? { + CreateRetired::Withdrawn { trash } => { + return self.trash_withdrawn_create(op, trash, &local, &real, &name); + } + retired => matches!(retired, CreateRetired::Newer { .. }), + }; // The uploaded blob is the new file's content, so it becomes the cached // content, as for a revision. A partial write fills its gaps from it; // without it the next write open was refused (`docs/BUGS.md` B123). @@ -927,7 +1020,10 @@ impl Core { .filter(|_| !newer) .and(op.meta_json.as_deref()) .and_then(|json| serde_json::from_str(json).ok()); - self.adopt_real_uid(&local, &real, |node| { + // The op is gone, so the create has landed whatever the read-back + // says. One that fails is finished before anything else is sent for + // the node (`docs/BUGS.md` B143). + if let Err(e) = self.adopt_real_uid(&local, &real, |node| { if let (Some(blob), Some(meta)) = (uploaded, &landed) && node_size(node) == meta.len && keeps_landed_upload(self.cache.is_pinned(&real), meta.len, self.cache.budget()) @@ -936,7 +1032,10 @@ impl Core { self.cache .store_file(&real, node.modification_time, meta.len, Path::new(blob)); } - })?; + }) { + warn!(%local, %real, error = %e, "reading back a landed create failed; adopting it later"); + self.unadopted.lock().insert(real.clone(), local.clone()); + } // The feed will report this create back to us; the tree already has it // under its real uid, so that event is ours to ignore (`Core::self_changes`). self.note_self_change(&real); @@ -954,6 +1053,81 @@ impl Core { Ok(()) } + /// Whether `name` is held on Drive by a change of ours that is to free it: + /// a queued trash, a queued rename away from it, or another create on the + /// wire that was sent with it in `parent`, which a rename or trash follows + /// (`Db::finish_create`). The op that wants the name waits for it rather + /// than land under a conflict name. + /// + /// A trash is waited for as long as it takes. The rest only for + /// [`NAME_HOLD_ATTEMPTS`]: two files swapping names each hold the name the + /// other wants. + fn name_is_held( + &self, + op: &PendingOp, + parent: Option<&NodeUid>, + name: &str, + ) -> Result> { + if self.db.has_pending_trash_named(name)? { + return Ok(true); + } + if op.attempts >= NAME_HOLD_ATTEMPTS { + return Ok(false); + } + let creating = self + .creating + .lock() + .iter() + .any(|(id, (p, n))| *id != op.id && Some(p) == parent && n == name); + Ok(creating || self.db.has_pending_move_from(name)?) + } + + /// Trash the node a create made after the file was deleted, or replaced by + /// a rename, while its upload was on the wire (`docs/BUGS.md` B129). + /// + /// `Db::finish_create` queued the trash, claimed, in the transaction that + /// found the create gone. It goes out now rather than behind the queue: + /// until it lands the node holds its name on Drive, and a file created + /// there again forks a conflict copy. A failure leaves it queued. + fn trash_withdrawn_create( + &self, + op: &PendingOp, + trash: i64, + local: &NodeUid, + real: &NodeUid, + name: &str, + ) -> Result<(), Box> { + self.hidden.lock().insert(real.clone()); + self.note_self_change(real); + let trash = PendingOp { + id: trash, + kind: OP_TRASH.to_string(), + uid: real.to_string(), + parent_uid: None, + name: Some(name.to_string()), + blob_path: None, + meta_json: None, + created_at: op.created_at, + attempts: 0, + last_error: None, + next_attempt_at: 0, + }; + info!(%local, %real, name, "a create landed after its file was deleted; trashing it"); + if let Err(e) = self.drain_trash(&trash) { + warn!(%real, name, error = %e, "trashing a withdrawn create failed; queued"); + if let Err(e) = self.db.record_op_failure( + trash.id, + &e.to_string(), + now_millis() + DRAIN_BACKOFF_MIN.as_millis() as i64, + ) { + error!(%real, error = %e, "recording a drain failure failed"); + } + self.release_claim(&trash); + self.wake_drain(); + } + Ok(()) + } + /// The node already holding `name` under `parent`, if it is the one an /// earlier attempt at this op made before its answer was lost. /// @@ -1051,9 +1225,8 @@ impl Core { Ok(uid) } - /// Retire a landed create's op and move its queued state to the real uid. - /// True when a newer write attached to the create while it was uploading, - /// which is now queued as a revision of `real`. + /// Retire a landed create's op and move its queued state to the real uid, + /// unless the create was withdrawn while it was on the wire. /// /// Runs under the `pending` lock, which `Core::enqueue_staged_write` also /// holds from attaching a blob to a create until it records the pending @@ -1068,17 +1241,22 @@ impl Core { landing: &CreateLanding<'_>, local: &NodeUid, real: &NodeUid, - ) -> Result> { - let newer = { + ) -> Result> { + let retired = { let mut pending = self.pending.lock(); - let newer = self.db.finish_create(op.id, uploaded, landing, |json| { + let retired = self.db.finish_create(op.id, uploaded, landing, |json| { let mut meta: StagedWrite = serde_json::from_str(json).ok()?; meta.uid = landing.real.to_string(); serde_json::to_string(&meta).ok() })?; + // A withdrawn create's placeholder is the delete's to clean up, and + // an open handle may still read its pending entry. + if let CreateRetired::Withdrawn { .. } = retired { + return Ok(retired); + } pending.remove(local); - if let Some((blob, json)) = &newer { - let meta: StagedWrite = serde_json::from_str(json)?; + if let CreateRetired::Newer { blob, meta } = &retired { + let meta: StagedWrite = serde_json::from_str(meta)?; pending.insert( real.clone(), PendingRevision { @@ -1087,16 +1265,21 @@ impl Core { }, ); } - newer.is_some() + retired }; - if !newer && let Some(blob) = uploaded { + if retired == CreateRetired::Landed + && let Some(blob) = uploaded + { self.cache_created(op, local, real, blob); } self.for_each_state(|st| { if let Some(ino) = st.by_uid.remove(local) { st.by_uid.insert(real.clone(), ino); + // The node too: the next write of it upserts the row by the + // node's uid, and would bring the placeholder row back. if let Some(e) = st.entries.get_mut(&ino) { e.uid = real.clone(); + e.node.uid = real.clone(); } // Its children still name the placeholder as their parent, and // the next write of one would put it back on the row. @@ -1114,7 +1297,7 @@ impl Core { } } }); - Ok(newer) + Ok(retired) } /// Cache a landed create's uploaded `blob` under `real`, keyed as the tree @@ -1179,6 +1362,13 @@ impl Core { // Before open handles are rebased onto the node, so one released in // between finds what `adopt` keeps (`docs/BUGS.md` B123). adopt(&node); + // What Drive holds is the base open handles and queued writes are + // rebased onto below. The tree shows a write queued over it, as a + // listing does: a write open sized from the landed node over a shorter + // queued blob left a gap that refused the next open (`docs/BUGS.md` + // B123). + let remote = node.clone(); + self.stamp_pending_sizes(std::slice::from_mut(&mut node)); // Repoints queued children and node rows, and readdresses the placeholder row. // A no-op after `retire_create`, which has already done both. self.db @@ -1233,10 +1423,22 @@ impl Core { // write would land as a conflict copy of the empty file this create just // made (`docs/BUGS.md` B113). Rebased before the restamp below, so a // release that races this is caught there. - self.rebase_open_writes(real, &node); + // + // A release that took its base before the rebase and queues after the + // restamp is caught by neither. The revision this create sealed is + // ours, as one a revision upload sealed is, so the drain chains that + // write onto it instead of forking a conflict copy (B70 layer B). + if let Some(rev) = node_revision_id(&remote) + && let Err(e) = self + .db + .set_own_sealed_rev(&real.to_string(), &rev, now_millis()) + { + warn!(%real, error = %e, "persisting our sealed revision failed"); + } + self.rebase_open_writes(real, &remote); // A write carried over from the create was made against no revision at // all; the one just created is what it now replaces. - self.rebaseline_pending(real, &node); + self.rebaseline_pending(real, &remote); if node.is_folder() { // It was recorded as listed while local (it was empty and had nothing // to enumerate). That still holds: its queued children re-intern under @@ -1828,16 +2030,25 @@ impl Core { if self.rebaseline_pending(uid, &node) { return Some(node); } + // A rename queued behind the upload has not reached Drive yet, and + // until it lands the tree keeps the name and folder the user gave the + // file, as `adopt_real_uid` does. Taking Drive's put the file back + // under its old name. + let renaming = self + .db + .has_pending_op(&uid.to_string(), OP_RENAME) + .unwrap_or(false); self.for_each_state(|st| { - let Some(parent) = st - .by_uid - .get(uid) - .and_then(|ino| st.entries.get(ino)) - .map(|e| e.parent) - else { + let Some(entry) = st.by_uid.get(uid).and_then(|ino| st.entries.get(ino)) else { return; }; - st.intern(parent, node.clone()); + let parent = entry.parent; + let mut node = node.clone(); + if renaming { + node.name = entry.node.name.clone(); + node.parent_uid = entry.node.parent_uid.clone(); + } + st.intern(parent, node); }); Some(node) } @@ -1976,11 +2187,17 @@ fn park_verdict(created_at: i64, now: i64, open: bool) -> ParkVerdict { ParkVerdict::Release } -/// The retryable error for an op whose name a queued trash has yet to free. -fn held_by_queued_trash(name: &str) -> Box { - format!("{name} is still held by a node whose trash is queued").into() +/// The retryable error for an op whose name a change of ours has yet to free +/// ([`Core::name_is_held`]). +fn held_by_queued_change(name: &str) -> Box { + format!("{name} is still held by a node whose trash or rename is queued").into() } +/// How many attempts an op waits for a rename or a create on the wire to free +/// the name it wants. Two files swapping names each wait for the other, so the +/// wait has to end; a conflict name ends it. +const NAME_HOLD_ATTEMPTS: i64 = 4; + /// How much older than its op a remote node may be and still be the one an /// unanswered call made. The op is queued the moment the call gives up, so the /// node is at most one call's deadline older; the rest is clock skew between @@ -2076,6 +2293,11 @@ mod tests { assert_eq!(authorities.len(), 2); assert_eq!(authorities[0].to_string(), "v~node"); assert_eq!(authorities[1].to_string(), "v~parent"); + let withdrawn = PendingOp { + uid: "local~never-landed".to_string(), + ..pending(OP_TRASH) + }; + assert!(pending_op_authorities(&withdrawn).unwrap().is_empty()); } fn folder_node(link: &str, parent: Option<&str>) -> Node { diff --git a/crates/pdfs-fuse/src/filesystem.rs b/crates/pdfs-fuse/src/filesystem.rs index a53ad08..4fa8419 100644 --- a/crates/pdfs-fuse/src/filesystem.rs +++ b/crates/pdfs-fuse/src/filesystem.rs @@ -1514,7 +1514,7 @@ impl ProtonFs { // // A create that set out online and lost the network on the way lands in // the queue too, so a Wi-Fi drop costs the caller a wait, never an EIO. - let minted = if !transient && self.core.is_online() && !is_local_uid(&parent_uid) { + let minted = if !transient && self.core.sends_inline() && !is_local_uid(&parent_uid) { if let Err(error) = self.core.require_uid_writable(&parent_uid) { reply.error(error); return; @@ -1620,7 +1620,7 @@ impl ProtonFs { // queued — the folder becomes a placeholder that the drain turns into a // real one (offline.md Phase 3b). // Losing the network on the way queues it the same way. - let minted = if self.core.is_online() && !is_local_uid(&parent_uid) { + let minted = if self.core.sends_inline() && !is_local_uid(&parent_uid) { if let Err(error) = self.core.require_uid_writable(&parent_uid) { reply.error(error); return; @@ -1770,10 +1770,13 @@ impl ProtonFs { self.core.cache.clear_scratch_durable(&h.path); reply.ok(); } - (Some(h), false) => match self.core.queue_revision(&h) { - Ok(()) => reply.ok(), - Err(e) => reply.error(e), - }, + (Some(mut h), false) => { + self.core.follow_landed_create(&mut h); + match self.core.queue_revision(&h) { + Ok(()) => reply.ok(), + Err(e) => reply.error(e), + } + } (None, _) => reply.ok(), } } @@ -1796,7 +1799,7 @@ impl ProtonFs { reply.error(error); return; } - let (ino, uid) = match self.core.lookup_child(parent, name) { + let (ino, mut uid) = match self.core.lookup_child(parent, name) { Ok(x) => x, Err(e) => { reply.error(e); @@ -1921,6 +1924,8 @@ impl ProtonFs { // to rename: the queued op *is* the node, so rewriting its target is the // whole rename. Nothing reaches the API, which is why this works offline // (offline.md Phase 3b). + // A create that lands meanwhile leaves no op to rewrite; the node is + // then renamed by the uid Drive gave it (docs/BUGS.md B142). if is_local_uid(&uid) { match self.core.db.rewrite_op_target( &uid.to_string(), @@ -1931,6 +1936,8 @@ impl ProtonFs { self.core .state() .rename_in_place(ino, newparent, &new_parent_uid, newname); + // A create that failed on its old name is due again. + self.core.wake_drain(); // Finalize: a transient scratch file (its create parked, bytes // held back) renamed to a finished name is the moment the // completed file is meant to reach Drive. Un-park the create so @@ -1949,23 +1956,29 @@ impl ProtonFs { } debug!(%uid, newname, "renamed a node whose create is still queued"); reply.ok(); + return; } - // The create drained underneath us, so the node has a real uid - // now and this handle's is stale. A retry resolves it. - Ok(false) => { - warn!(%uid, name, newname, "queued create vanished under a rename"); - self.core - .restore_replaced(victim.as_ref(), &new_parent_uid, newname); - reply.error(Errno::EBUSY); - } + Ok(false) => match self.core.landed_row_uid(ino) { + Some(real) => { + debug!(local = %uid, %real, newname, "create landed under a rename"); + uid = real; + } + None => { + warn!(%uid, name, newname, "queued create vanished under a rename"); + self.core + .restore_replaced(victim.as_ref(), &new_parent_uid, newname); + reply.error(Errno::EBUSY); + return; + } + }, Err(e) => { error!(%uid, error = %e, "rewriting a queued create's target failed"); self.core .restore_replaced(victim.as_ref(), &new_parent_uid, newname); reply.error(Errno::EIO); + return; } } - return; } // Offline, or into a folder that does not exist remotely yet: queue the // rename rather than 404 or fail. Online moves into a real parent still @@ -2020,7 +2033,7 @@ impl ProtonFs { } }; if rename_needs_queue( - self.core.is_online(), + self.core.sends_inline(), is_local_uid(&new_parent_uid), newparent != parent, newname != name, @@ -2133,7 +2146,7 @@ impl ProtonFs { Ok(()) => reply.ok(), Err(e) => reply.error(e), }; - if !self.core.is_online() { + if !self.core.sends_inline() { queue_it(reply); return; } diff --git a/crates/pdfs-fuse/src/lib.rs b/crates/pdfs-fuse/src/lib.rs index dbb0f85..a948035 100644 --- a/crates/pdfs-fuse/src/lib.rs +++ b/crates/pdfs-fuse/src/lib.rs @@ -569,6 +569,14 @@ struct Core { /// Nodes removed through this daemon which a briefly stale remote listing /// may still return. Uids are not reused; an explicit restore clears one. hidden: Arc>>, + /// The folder and name each create on the wire was sent with, by op id. + /// Drive holds that name until the create lands and a rename or trash + /// queued behind it frees it again. + creating: Arc>>, + /// Creates that landed but could not be read back, by the uid Drive gave + /// them, with the placeholder's. The drain adopts one before it sends + /// anything else for it. + unadopted: Arc>>, /// Nudges the drain workers: set true and notify to have them re-examine /// the queue instead of waiting out their backoff. drain_wake: Arc<(Mutex, Condvar)>, @@ -624,6 +632,10 @@ 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, + /// A mkdir, create, rename or delete through the mount is recorded and + /// queued, and the drain sends it; the syscall never waits for Drive. Off, + /// they go to Drive inline while online, as before 3.0. + local_first: bool, /// 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 @@ -2485,6 +2497,14 @@ impl Core { return Err(Errno::EIO); } }; + // Drive shows none of what is still queued for the folder (B132). + // A shared folder's listing is published as Drive has it. + if !foreign { + match self.db.queued_children(&folder_uid) { + Ok((queued, gone)) => overlay_queued(&mut nodes, queued, &gone), + Err(e) => warn!(%folder_uid, error = %e, "db queued_children failed"), + } + } // Same as the DB path above: the remote's size for a file with a write // still queued is the pre-write one (B11). self.stamp_pending_sizes(&mut nodes); @@ -3102,6 +3122,33 @@ impl Core { } } + /// Point a handle closed while its file's create landed at the uid Drive + /// gave the node. + /// + /// `Core::retire_create` moves the row to that uid before it repoints the + /// open handles. A handle closed in between still named the placeholder, + /// which no row has any more, and its write was refused as if access had + /// changed. The inode is the row's local id, which finds it either way. + pub(crate) fn follow_landed_create(&self, h: &mut WriteHandle) { + if !is_local_uid(&h.uid) { + return; + } + if let Some(real) = self.landed_row_uid(h.ino) { + debug!(local = %h.uid, %real, "write handle closed as its create landed"); + h.uid = real; + } + } + + /// The uid Drive gave the node at `ino`, once its create has landed, or + /// `None` while it is still queued. Read from the row, which has it before + /// the tree does. + pub(crate) fn landed_row_uid(&self, ino: u64) -> Option { + state::ino_lid(ino) + .and_then(|lid| self.db.uid_of_lid(lid).ok().flatten()) + .and_then(|uid| parse_node_uid(&uid)) + .filter(|uid| !is_local_uid(uid)) + } + /// Accept a released write handle's bytes and queue their upload /// (offline.md Phase 3). /// @@ -4758,9 +4805,9 @@ impl Core { debug!(%uid, name, "replaced a node whose create was still queued"); return Ok(()); } - // Offline, or the network goes away under the call: queue the trash. - // The rename that follows queues as well, behind it. - if !self.is_online() { + // Local-first or offline, or the network goes away under the call: + // queue the trash. The rename that follows queues as well, behind it. + if !self.sends_inline() { return self.queue_trash(uid, name); } self.require_uid_writable(uid)?; @@ -6223,17 +6270,17 @@ fn copy_pending_for_truncate(pending: &PendingRevision, destination: &File) -> s } /// Whether a rename must be represented as a durable desired end state. -/// Offline operations and operations targeting a not-yet-uploaded directory -/// cannot use the API immediately. Online remote-to-remote operations stay -/// synchronous so a successful kernel reply means the remote namespace has -/// already reached the same end state. +/// Local-first, offline, or into a not-yet-uploaded directory, the API is not +/// called in the syscall. Only remote-to-remote renames sent inline +/// ([`Core::sends_inline`]) stay synchronous, so a successful kernel reply means +/// the remote namespace has already reached the same end state. fn rename_needs_queue( - online: bool, + inline: bool, destination_is_local: bool, _parent_changed: bool, _name_changed: bool, ) -> bool { - !online || destination_is_local + !inline || destination_is_local } /// Convert one kernel pathname component into the UTF-8 name accepted by Drive. @@ -6254,6 +6301,18 @@ fn node_visible(node: &Node, folder_uid: &NodeUid, hidden: &HashSet) -> !node.trashed && node.uid != *folder_uid && !hidden.contains(&node.uid) } +/// Lay what is queued for a folder over Drive's listing of it, from +/// `Db::queued_children`: a node a queued rename moved away is dropped, and a +/// node a queued create, mkdir or rename put there is listed as the DB has it. +fn overlay_queued(nodes: &mut Vec, queued: Vec, gone: &HashSet) { + let here: HashSet = queued.iter().map(|node| node.uid.to_string()).collect(); + nodes.retain(|node| { + let uid = node.uid.to_string(); + !gone.contains(&uid) && !here.contains(&uid) + }); + nodes.extend(queued); +} + /// Bytes rendered with a binary unit and one decimal place (e.g. `"1.2 GB"`), /// for the activity log. Uses 1024-based steps but the shorter SI labels. fn human_bytes(bytes: u64) -> String { diff --git a/crates/pdfs-fuse/src/link.rs b/crates/pdfs-fuse/src/link.rs index a908365..884ba56 100644 --- a/crates/pdfs-fuse/src/link.rs +++ b/crates/pdfs-fuse/src/link.rs @@ -113,6 +113,15 @@ impl Core { self.online.load(Ordering::Relaxed) } + /// Whether a namespace change through the mount goes to Drive inside the + /// syscall. Only with [`AppConfig::local_first`] off, and only online; + /// otherwise it is queued and the drain sends it. + /// + /// [`AppConfig::local_first`]: pdfs_core::config::AppConfig::local_first + pub(crate) fn sends_inline(&self) -> bool { + !self.local_first && self.is_online() + } + /// Run a remote call a caller is waiting on, giving up after /// [`INTERACTIVE_CALL_TIMEOUT`]. The timer is built inside the runtime: a /// FUSE worker has no reactor of its own. diff --git a/crates/pdfs-fuse/src/mount.rs b/crates/pdfs-fuse/src/mount.rs index cc4d330..c87c318 100644 --- a/crates/pdfs-fuse/src/mount.rs +++ b/crates/pdfs-fuse/src/mount.rs @@ -244,6 +244,9 @@ pub struct MountOptions { /// Upload and download caps in bytes per second (`0` = no cap). pub upload_limit: u64, pub download_limit: u64, + /// Namespace changes through the mount are queued, not sent in the + /// syscall. See [`pdfs_core::config::AppConfig::local_first`]. + pub local_first: bool, } /// Spawn one FUSE session rooted at an arbitrary remote node. @@ -385,6 +388,7 @@ pub(crate) fn mount_with( sweep_mode, upload_limit, download_limit, + local_first, } = options; // Start the uptime clock here rather than at the first `pdfs diagnostics`, // so the age it reports is the daemon's own. @@ -429,6 +433,8 @@ pub(crate) fn mount_with( sync_paused_until: Arc::new(AtomicI64::new(paused_until)), pending: Arc::new(Mutex::new(HashMap::new())), hidden: Arc::new(Mutex::new(HashSet::new())), + creating: Arc::new(Mutex::new(HashMap::new())), + unadopted: Arc::new(Mutex::new(HashMap::new())), drain_wake: Arc::new((Mutex::new(false), Condvar::new())), shutdown: Arc::new(crate::shutdown::Shutdown::default()), upload_times: Arc::new(Mutex::new(HashMap::new())), @@ -442,6 +448,7 @@ pub(crate) fn mount_with( trash_progress: Arc::new(tokio::sync::Notify::new()), conflict_notified: Arc::new(Mutex::new(HashSet::new())), sweep_mode, + local_first, 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( diff --git a/crates/pdfs-fuse/src/sim/daemon.rs b/crates/pdfs-fuse/src/sim/daemon.rs index 563b6bd..c8a33a1 100644 --- a/crates/pdfs-fuse/src/sim/daemon.rs +++ b/crates/pdfs-fuse/src/sim/daemon.rs @@ -91,6 +91,7 @@ impl Daemon { sweep_mode: AppConfig::default().resolved_conflict_sweep(), upload_limit: 0, download_limit: 0, + local_first: AppConfig::default().local_first.unwrap_or(true), }, Host::Simulation(stopped), ) @@ -412,10 +413,54 @@ mod tests { std::fs::rename(mount.join("x/f.txt"), mount.join("z/d")).unwrap(); assert_eq!(std::fs::read(mount.join("z/d")).unwrap(), b"moved"); - let tree = drive.tree(); - assert_eq!(tree.get("z/d").cloned(), file(b"moved")); - assert_eq!(tree.get("x/d").cloned(), file(b"sibling")); - assert!(!tree.contains_key("x/f.txt")); + // The rename is recorded locally and sent from the queue. + assert!( + wait_until(Duration::from_secs(30), || { + let tree = drive.tree(); + tree.get("z/d").cloned() == file(b"moved") + && tree.get("x/d").cloned() == file(b"sibling") + && !tree.contains_key("x/f.txt") + }), + "{:?}", + drive.tree().keys().collect::>() + ); + + assert!(matches!(daemon.stop(), Ok(MountOutcome::Shutdown))); + let _ = std::fs::remove_dir_all(&dir); + } + + #[test] + #[ignore = "mounts FUSE: run with `cargo test -p pdfs-fuse sim:: -- --ignored`"] + fn a_write_over_a_create_whose_read_back_failed_is_not_a_conflict() { + // The create landed and the link went down before it was read back. + // The next write was based on the file as it was made locally, and + // landed as a conflict copy of it (B143). + let drive = FakeDrive::new(); + let dir = scratch("unadopted"); + let daemon = Daemon::start(&dir, drive.client(1, Faults::lan())).unwrap(); + let file = |bytes: &[u8]| Some(Entry::File(Arc::new(bytes.to_vec()))); + let path = daemon.mountpoint.join("f.txt"); + + // Drive stamps a create with the second it arrives, so it has to land + // a second after the file was made here for the two to differ. + daemon.client.set_online(false); + std::fs::write(&path, b"first").unwrap(); + std::thread::sleep(Duration::from_millis(1100)); + daemon.client.drop_link_after_next_create(); + daemon.client.set_online(true); + assert!(wait_until(Duration::from_secs(30), || { + drive.tree().get("f.txt").cloned() == file(b"first") + })); + write_at(&path, 5, b" and second").unwrap(); + daemon.client.set_online(true); + assert!( + wait_until(Duration::from_secs(30), || { + drive.tree().get("f.txt").cloned() == file(b"first and second") + }), + "{:?}", + drive.tree() + ); + assert_eq!(drive.tree().len(), 1, "{:?}", drive.tree()); assert!(matches!(daemon.stop(), Ok(MountOutcome::Shutdown))); let _ = std::fs::remove_dir_all(&dir); diff --git a/crates/pdfs-fuse/src/sim/fake_drive.rs b/crates/pdfs-fuse/src/sim/fake_drive.rs index 5ffd8b0..e9ed5c2 100644 --- a/crates/pdfs-fuse/src/sim/fake_drive.rs +++ b/crates/pdfs-fuse/src/sim/fake_drive.rs @@ -330,6 +330,7 @@ impl FakeDrive { online: AtomicBool::new(true), requests: AtomicU64::new(0), hold: Mutex::new(None), + drop_after_create: AtomicBool::new(false), })) } @@ -577,6 +578,8 @@ struct ClientInner { requests: AtomicU64, /// The read [`FakeClient::hold_next_read`] is waiting for. hold: Mutex>>, + /// Set by [`FakeClient::drop_link_after_next_create`]. + drop_after_create: AtomicBool, } struct HoldState { @@ -616,6 +619,12 @@ impl FakeClient { *self.0.faults.lock() = faults; } + /// Take the link down as soon as the next file create has been answered, + /// so whatever the daemon asks next goes unanswered. + pub(crate) fn drop_link_after_next_create(&self) { + self.0.drop_after_create.store(true, Ordering::SeqCst); + } + /// Hold the next read of `uid` alone, before it reaches Drive, until the /// answer is dropped. Only one read is held at a time. pub(crate) fn hold_next_read(&self, uid: &NodeUid) -> Held { @@ -1038,16 +1047,21 @@ impl DriveApi for FakeClient { _aead: bool, ) -> Result { let content = read_all(reader)?; - self.request(true, |server| { - server.new_file( - parent_uid, - name, - media_type, - content, - last_modification_time, - ) - }) - .await + let made = self + .request(true, |server| { + server.new_file( + parent_uid, + name, + media_type, + content, + last_modification_time, + ) + }) + .await; + if made.is_ok() && self.0.drop_after_create.swap(false, Ordering::SeqCst) { + self.set_online(false); + } + made } async fn upload_file_replacing_draft_from_dyn( diff --git a/crates/pdfs-fuse/src/state.rs b/crates/pdfs-fuse/src/state.rs index 79b9aa9..e38ebe6 100644 --- a/crates/pdfs-fuse/src/state.rs +++ b/crates/pdfs-fuse/src/state.rs @@ -212,6 +212,12 @@ pub(crate) fn lid_ino(lid: i64) -> u64 { lid.unsigned_abs() + crate::ROOT_INO } +/// The local id of the node at inode `ino`, the inverse of [`lid_ino`]: +/// `None` for the root and a fallback inode. +pub(crate) fn ino_lid(ino: u64) -> Option { + (ino > crate::ROOT_INO && ino < FALLBACK_INO).then(|| (ino - crate::ROOT_INO) as i64) +} + /// Mutable inode bookkeeping, guarded by a mutex because fuser drives the /// `Filesystem` trait through `&self`. /// @@ -1325,6 +1331,13 @@ mod tests { use proton_drive_rs::ShareMembership; use proton_drive_rs::proton_sdk::ids::{LinkId, ShareId, ShareMembershipId, VolumeId}; + #[test] + fn an_inode_gives_back_the_lid_it_was_made_from() { + assert_eq!(ino_lid(lid_ino(7)), Some(7)); + assert_eq!(ino_lid(crate::ROOT_INO), None); + assert_eq!(ino_lid(FALLBACK_INO + 3), None); + } + fn uid(link: &str) -> NodeUid { NodeUid::new(VolumeId::from("vol"), LinkId::from(link)) } diff --git a/docs/BUGS.md b/docs/BUGS.md index ffd1c6d..75cedb4 100644 --- a/docs/BUGS.md +++ b/docs/BUGS.md @@ -12,6 +12,145 @@ Conventions: --- +## B143 — A write to a file whose create landed as the link went down lands as a conflict copy + +**Status:** Fixed (unverified). +**Found:** 2026-10-04, by the simulation runs (`sim::run`, profile three-clients, seed 1). +Client 0 made `y/z/b.txt` while the link was down. When the link came back, the create landed and +the link went down again right after it. A write to the file was then kept as +`b (sync-conflict …).txt`: "expected 56 bytes at mtime 1791068288, found 56 at 1791068289". Only +client 0 writes that file. + +**Where:** `Core::drain_local_node`, `Core::drain_op` and `Core::finish_adoption` in +`crates/pdfs-fuse/src/drain.rs`. + +**Cause.** The gap B131 closed, for a link that drops instead of a lookup that is late. +`Core::read_back` asks again only while Drive answers that it does not list the node yet. When +the link is down it gives up, and `adopt_real_uid` fails after the create's op has retired. The +tree keeps the placeholder's node under the real uid, and nothing reads it back later. A write +opened after that takes the local clock's mtime as its base. Drive stamped the create with the +second it arrived, so the drain saw a revision it did not know and kept the client's own write as +a conflict copy. + +**Fix.** A create whose read-back fails is remembered, under the uid Drive gave it. Before the +drain sends the next op for that uid, it reads the node back and rebases handles and queued writes +onto it, as `adopt_real_uid` would have done. If the read-back fails again, the op is retried +later, as for any other failed request. + +**Test:** the daemon test `a_write_over_a_create_whose_read_back_failed_is_not_a_conflict`. The +fake Drive takes the link down as soon as it has answered the create. Without the fix, the test +fails on the conflict copy. + +## B142 — Renaming a new file as its upload lands can fail with "Device or resource busy" + +**Status:** Fixed (unverified). +**Found:** 2026-10-04, by the simulation runs (`sim::run`, profile three-clients, seed 3). +Client 1 renamed `c.bin`, a file whose create was still queued, to `f.txt`, and got `EBUSY`. + +**Where:** `serve_rename` in `crates/pdfs-fuse/src/filesystem.rs`. + +**Cause.** A file whose create is queued is renamed by rewriting the name in the create op. When +the create landed between the lookup and the rewrite, the op was gone, and the rename answered +`EBUSY` as if the create had vanished. + +**Fix.** When the op is gone, the rename looks up the node's row. If the create has landed, the +row already holds the uid Drive gave the file, and the rename is queued against that uid, as for +any other file. + +**Test:** seed 3 of three-clients passes repeatedly. The window is too narrow to hit on purpose. + +## B141 — A file renamed twice in a row can end up under the first name + +**Status:** Fixed (unverified). +**Found:** 2026-10-04, by the simulation runs (`sim::run`, profile one-client-echoes, seed 2). +`e.md` was moved into `x`, and right after that into the client folder. Drive kept it in `x`, +and the liveness check found it in a different place on Drive than in the mount. In about one of +three replays. + +**Where:** `Db::enqueue_op` and `Db::claim_next_due_op` in `crates/pdfs-core/src/db/ops.rs`. + +**Cause.** A rename replaces the rename queued before it, by deleting that row. When a drain +worker had already taken the older row, deleting it also lifted the rule that one node has one op +in flight. A second worker sent the newer rename at once, while the first was still retrying its +own after Drive called it out of date (B111). The older one landed last. + +**Fix.** A rename a worker holds stays queued, and the newer one waits behind it. If the older one +fails, it is dropped instead of being retried after the newer one. + +**Test:** the unit test `a_rename_on_the_wire_lands_before_the_one_that_replaces_it`. Seed 2 of +one-client-echoes passes repeatedly. + +## B140 — Files in a folder that lands as the daemon stops are gone after a restart + +**Status:** Fixed (unverified). +**Found:** 2026-10-04, by the simulation runs (`sim::run`, profile one-client-flaky, seed 4), +replaying B132. The client folder `c0` was made with files in it, and its mkdir landed after the +mount had stopped. Right after the restart, a write to `c0/a.txt` failed with `ENOENT`, and the +listing of `c0` was empty. In about half of the replays. + +**Where:** `adopt_placeholder_row_tx` in `crates/pdfs-core/src/db/nodes.rs` and `Core::hydrate` in +`crates/pdfs-fuse/src/lib.rs`. + +**Cause.** When a folder lands, the rows below it are handed to the real uid by their +`parent_uid` column only. The node each row holds (`node_json`) still named the placeholder. A +running daemon rewrites the rows from its tree on the next change, so this did not show. A restart +builds the tree from `node_json`, found no row for the placeholder and left the children out of the +folder. + +**Fix.** The landing rewrites the parent in `node_json` as well, in the same statement. + +**Test:** the unit test `a_landed_folder_hands_its_children_over_in_the_stored_node_too`. Seed 4 +of one-client-flaky passes ten replays in a row. + +## B139 — A file closed while its first upload lands keeps its last write in staging + +**Status:** Fixed (unverified). +**Found:** 2026-10-03, by the simulation runs (`sim::run`, profile three-clients, seed 1), at the +end check for lost data. A file made through the mount was written and renamed while its create +was queued. Its handle was closed just as the create landed. The daemon logged "cannot queue +write; bytes kept in staging" with "access changed before the write could be queued", and Drive +kept the bytes from before the last write. + +**Where:** `Core::queue_revision` in `crates/pdfs-fuse/src/lib.rs` and `release` in +`crates/pdfs-fuse/src/filesystem.rs`. + +**Cause.** A write handle keeps the uid the node had when it was opened. When the create lands, +`finish_create` retires the op and the placeholder row becomes the real node's, before the tree +moves the open handle over. A handle closed in that gap still named the placeholder, which no +longer had a row, so the access check refused it and the bytes went to staging instead of the +queue. + +**Fix.** On close, a handle that names a placeholder looks up the node by its inode's local id, +which survives the landing, and queues the write against the real uid when the node has one. + +**Test:** the unit tests `a_landed_placeholder_keeps_its_lid` and +`an_inode_gives_back_the_lid_it_was_made_from`. Seed 1 of three-clients passes. + +## B138 — A file created where another is leaving lands as a conflict copy + +**Status:** Fixed (unverified). +**Found:** 2026-10-03, by the simulation runs (`sim::run`, profile one-client-lan, seed 9). +`c.bin` was created and renamed to `d` while its create was on the wire, and a new `c.bin` was +made at once. The new file landed on Drive under a conflict name. + +**Where:** `drain_local_node`, `drain_rename` and `drain_rename_in_place` in +`crates/pdfs-fuse/src/drain.rs`. + +**Cause.** When Drive refuses a name, the drain waits only if a queued trash holds it +(`has_pending_trash_named`); otherwise it lands the op under a conflict name. A create on the wire +holds the name it was sent with until the rename queued behind it lands, and a queued rename away +from a name holds it until it lands. Several workers drain at once, so the new create could reach +Drive first. + +**Fix.** The drain also waits while another create on the wire was sent with that name in that +folder, or a queued rename moves a node away from it. Those waits end after a few attempts, as two +files swapping names each hold the name the other wants. When a rename or trash frees a name, the +ops that failed on it are made due at once, and so is a create renamed away from a name it failed +on, whether the rename came before the failure was recorded or after. + +**Test:** the unit test `a_queued_rename_away_from_a_name_holds_it_until_it_lands`. Seed 9 of +one-client-lan passes. + ## B137 — A queued change can be deleted by an older one landing **Status:** Fixed (unverified). @@ -135,7 +274,7 @@ fails on it in every seed. ## B132 — A folder whose mkdir is still queued is gone after a restart -**Status:** Open. +**Status:** Fixed (unverified). **Found:** 2026-10-03, by the simulation runs (`sim::run`, profile one-client-flaky, seed 4). The first listing of the Drive root failed with the link down, and the client's folder `c0` was made offline. The link came back, and the client restarted with 8 ops still queued, `c0`'s @@ -154,9 +293,13 @@ records it as complete, without the queued node's placeholder. Every path throug with `ENOENT` until something lists the folder again. The same may happen without a restart whenever a folder holding a queued create is listed on Drive. -**Test:** the simulation runs count an `ENOENT` after a restart that restored queued ops, in a -seed that lost the link, and end the seed there. With `PDFS_SIM_KNOWN=fail`, seed 4 of -one-client-flaky fails on it when the mkdir is still queued, which depends on timing. +**Fix.** A listing read from Drive is laid over with what the queue holds for the folder +(`Db::queued_children`): a node a queued create, mkdir or rename put there is listed as the DB has +it, and one a queued rename moved away is left out. Replays of the seed after this fix turned up a +second cause, B140. + +**Test:** the unit test `queued_children_are_what_drive_does_not_list_yet`. Seed 4 of +one-client-flaky passes ten replays in a row. ## B131 — A write to a file whose create could not be read back lands as a conflict copy @@ -224,7 +367,7 @@ state `D` inside `NotifyBatch::flush`), count it and end the seed there. With ## B129 — A file deleted or replaced while its create is on the wire stays on Drive -**Status:** Open. +**Status:** Fixed (unverified). **Found:** 2026-10-03, by the simulation runs (`sim::run`, profile one-client-lan, seed 4). `a.txt` was created, renamed to `b.txt` and then to `e.md` while its create was still queued. A moment later `rename f.txt -> e.md` failed with `EIO`, and the daemon logged "rename failed" @@ -242,6 +385,13 @@ retire, and the tree has already forgotten the local uid, so nothing trashes the Drive keeps a file that the mount no longer shows. Its name stays taken: an online rename onto it, as here, fails with `EIO`, and a later create with that name lands as a conflict copy. +**Fix.** A delete or replacing rename no longer drops a create the drain has taken. It turns +the row into a trash of the placeholder, still claimed by the upload, which holds the name as any +queued trash does. When the create lands, `finish_create` points that trash at the landed node in +the same transaction, and the drain sends it at once; if that fails it stays queued with the usual +backoff. A create whose row is gone altogether gets such a trash too. A trash of a placeholder +whose create never landed is done without asking Drive. + **Test:** the simulation runs count a rename it fails and end the seed there. With `PDFS_SIM_KNOWN=fail`, seed 4 of one-client-lan fails on it when it comes up, which depends on timing. diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index a7b94a7..e154396 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -14,9 +14,30 @@ scratch (user data in `staging/` and `recovery/` is never touched by this). Schema: **37**. The queue table is rebuilt so an id is never given out twice; queued changes are kept. +### Changed +- **Making, renaming and deleting through the mount no longer waits for Drive.** A new folder, + a new file, a rename or a delete is recorded locally and the call returns at once; the queue + sends it, as it does offline. `"local_first": false` in `config.json` brings back the old way + for this release. + ### Fixed +- **A file deleted while its first upload is on the wire no longer stays on Drive.** The node + the upload made is trashed as soon as it lands, so its name is free again (B129). - **A delete queued while a rename is sent is no longer lost.** The rename landing could remove the delete from the queue, leaving the file on Drive (B137). +- **A file made where another one is leaving no longer lands as a conflict copy.** Its upload + waits for the rename or trash that frees the name (B138). +- **A file closed just as its first upload lands keeps its last write.** The write was refused + and left in staging (B139). +- **Files made in a new folder are still there after a restart.** A folder that landed as the + daemon stopped, or one listed from Drive while files in it were queued, came back empty + (B132, B140). +- **A file renamed twice in quick succession ends up under the second name.** The first rename + could land after the second and undo it (B141). +- **Renaming a new file just as its upload lands no longer fails with "Device or resource + busy".** The rename goes to the file Drive made (B142). +- **A file whose upload landed just as the connection dropped no longer gets a conflict copy at + its next save.** The daemon reads the file back once the connection returns (B143). ## [2.10.0] — 2026-10-03 diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index 6b7b472..8817875 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -80,6 +80,7 @@ fix it. | `ignore_patterns` | list | see below | — | Global ignore rules for synced folders | | `device_uid` | string | none | Computers → Continue This Backup Here… | The computer this machine continues. Unset means "match by hostname" | | `conflict_sweep` | `off` / `report` / `enforce` | `report` | — | See [`conflict_sweep`](#conflict_sweep) | +| `local_first` | bool | `true` | — | See [`local_first`](#local_first) | | `language` | gettext code | system | General → Appearance | Language of the app, tray and prompt, such as `de` or `pt_BR` | | `proton_theme` | bool | follow system | General → Appearance | Proton's colours instead of the system theme | | `online_map` | bool | `false` | General → Appearance | Street map for Places from OpenFreeMap | @@ -116,6 +117,15 @@ were copied from. `PDFS_CONFLICT_SWEEP` overrides the setting for one run of the service. +### `local_first` + +A new folder, a new file, a rename or a delete made through the mount is recorded locally and the +call returns at once. The change goes to Drive from the queue, the same way it does offline, and +`pdfs status` counts it until it lands. + +Set it to `false` to have these calls wait for Drive again, as before 3.0.0. The switch is there +for one release, in case the queued path misbehaves for you; please report it if it does. + ### `open_with` Decides how files picked in the search launcher or the app are opened. Rules are tried in order;