diff --git a/crates/pdfs-core/src/db/mod.rs b/crates/pdfs-core/src/db/mod.rs index 9431c65..b26965f 100644 --- a/crates/pdfs-core/src/db/mod.rs +++ b/crates/pdfs-core/src/db/mod.rs @@ -56,8 +56,9 @@ pub use local::LocalFileHit; pub use maintenance::{DbStats, VacuumOutcome}; pub use nodes::{PublishedSharedRoot, SearchHit, StoredNode}; pub use ops::{ - AttachedBlob, 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, 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/ops.rs b/crates/pdfs-core/src/db/ops.rs index ebbc91c..a72c5ca 100644 --- a/crates/pdfs-core/src/db/ops.rs +++ b/crates/pdfs-core/src/db/ops.rs @@ -228,6 +228,28 @@ pub struct PendingCounts { /// is retrying at the slowest rate it ever will and has been for minutes. pub const FAILING_ATTEMPTS: i64 = 6; +/// A queued create that has landed, for [`Db::finish_create`]. +#[derive(Debug, Clone, Copy)] +pub struct CreateLanding<'a> { + /// The placeholder the node was known by. + pub local: &'a str, + /// The uid Drive gave it. + pub real: &'a str, + /// The parent and name the op held when the drain took it. + pub sent: (&'a str, &'a str), + /// The parent and name the node has on Drive. They differ from `sent` + /// when the name was taken or the folder was gone. + pub landed: (&'a str, &'a str), +} + +struct CreateRow { + blob: Option, + meta: Option, + parent_uid: Option, + name: Option, + created_at: i64, +} + /// The outcome of folding freshly written bytes into a queued create. #[derive(Debug, Clone)] pub struct AttachedBlob { @@ -401,27 +423,44 @@ impl Db { /// becomes a revision of `real` instead, with its sidecar passed through /// `rewrite` so it names the real node. Returns that blob and sidecar, or /// `None` when the create is simply done. + /// + /// A rename while the create is on the wire rewrites the row's target + /// ([`Db::rewrite_op_target`]), which the create no longer reads. When the + /// 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. pub fn finish_create( &self, id: i64, uploaded: Option<&str>, - local: &str, - real: &str, + landing: &CreateLanding<'_>, rewrite: impl FnOnce(&str) -> Option, ) -> Result> { + let (local, real) = (landing.local, landing.real); let mut conn = self.conn.lock(); let tx = conn.transaction()?; - let row: Option<(Option, Option)> = tx + let row: Option = tx .query_row( - "SELECT blob_path, meta_json FROM pending_op WHERE id = ?1", + "SELECT blob_path, meta_json, parent_uid, name, created_at + FROM pending_op WHERE id = ?1", params![id], - |r| Ok((r.get(0)?, r.get(1)?)), + |r| { + Ok(CreateRow { + blob: r.get(0)?, + meta: r.get(1)?, + parent_uid: r.get(2)?, + name: r.get(3)?, + created_at: r.get(4)?, + }) + }, ) .optional()?; - let newer = match row { - Some((Some(blob), Some(meta))) if uploaded != Some(blob.as_str()) => { - rewrite(&meta).map(|meta| (blob, meta)) - } + let newer = match &row { + Some(CreateRow { + blob: Some(blob), + meta: Some(meta), + .. + }) if uploaded != Some(blob.as_str()) => rewrite(meta).map(|meta| (blob.clone(), meta)), _ => None, }; match &newer { @@ -438,6 +477,28 @@ impl Db { tx.execute("DELETE FROM pending_op WHERE id = ?1", params![id])?; } } + if let Some(CreateRow { + parent_uid: Some(parent), + name: Some(name), + created_at, + .. + }) = &row + && (parent.as_str(), name.as_str()) != landing.sent + { + let meta = serde_json::to_string(&RenameMeta { + original_parent_uid: landing.landed.0.to_string(), + original_name: Some(landing.landed.1.to_string()), + })?; + tx.execute( + "DELETE FROM pending_op WHERE uid = ?1 AND kind = ?2", + params![real, OP_RENAME], + )?; + tx.execute( + "INSERT INTO pending_op (kind, uid, parent_uid, name, meta_json, created_at) + VALUES (?1, ?2, ?3, ?4, ?5, ?6)", + params![OP_RENAME, real, parent, name, meta, created_at], + )?; + } tx.execute( "UPDATE pending_op SET parent_uid = ?2 WHERE parent_uid = ?1", params![local, real], diff --git a/crates/pdfs-core/src/db/tests.rs b/crates/pdfs-core/src/db/tests.rs index d2cfe72..390fa78 100644 --- a/crates/pdfs-core/src/db/tests.rs +++ b/crates/pdfs-core/src/db/tests.rs @@ -2853,6 +2853,7 @@ fn a_write_attached_while_its_create_uploads_becomes_a_revision() { let db = Db::open_in_memory().unwrap(); let local = "local~abc"; let real = uid("real").to_string(); + let parent = uid("parent").to_string(); let create = |blob: Option<&str>| PendingOp { id: 0, kind: OP_CREATE.to_string(), @@ -2874,9 +2875,12 @@ fn a_write_attached_while_its_create_uploads_becomes_a_revision() { .expect("create is still queued"); let newer = db - .finish_create(id, None, local, &real, |json| { - Some(json.replace("local~abc", "vol~real")) - }) + .finish_create( + id, + None, + &landing(local, &real, &parent, "new.txt"), + |json| Some(json.replace("local~abc", "vol~real")), + ) .unwrap(); assert_eq!( @@ -2899,6 +2903,7 @@ fn a_create_whose_blob_landed_is_retired() { let db = Db::open_in_memory().unwrap(); let local = "local~abc"; let real = uid("real").to_string(); + let parent = uid("parent").to_string(); let (id, _) = db .enqueue_op(&PendingOp { id: 0, @@ -2930,9 +2935,12 @@ fn a_create_whose_blob_landed_is_retired() { .unwrap(); let newer = db - .finish_create(id, Some("/staging/sent"), local, &real, |_| { - panic!("an unchanged create has no sidecar to rewrite") - }) + .finish_create( + id, + Some("/staging/sent"), + &landing(local, &real, &parent, "new.txt"), + |_| panic!("an unchanged create has no sidecar to rewrite"), + ) .unwrap(); assert_eq!(newer, None); @@ -2941,6 +2949,64 @@ fn a_create_whose_blob_landed_is_retired() { assert_eq!(ops[0].parent_uid.as_deref(), Some(real.as_str())); } +/// The parent and name a create was sent with, landing where it was sent. +fn landing<'a>(local: &'a str, real: &'a str, parent: &'a str, name: &'a str) -> CreateLanding<'a> { + CreateLanding { + local, + real, + sent: (parent, name), + landed: (parent, name), + } +} + +#[test] +fn a_rename_made_while_its_create_uploads_is_queued_for_the_landed_node() { + let db = Db::open_in_memory().unwrap(); + let local = "local~abc"; + 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(uid("parent").to_string()), + name: Some("new.txt".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(); + // The drain took the create; the user moves the file into a folder that + // is itself still queued. + db.rewrite_op_target(local, "local~dir", "moved.txt") + .unwrap(); + + let mut landed = landing(local, &real, &parent, "new.txt"); + landed.landed.1 = "new (conflict).txt"; + db.finish_create(id, Some("/staging/sent"), &landed, |_| None) + .unwrap(); + + let ops = db.pending_ops().unwrap(); + assert_eq!(ops.len(), 1, "the move outlives the create"); + assert_eq!(ops[0].kind, OP_RENAME); + assert_eq!(ops[0].uid, real); + assert_eq!(ops[0].parent_uid.as_deref(), Some("local~dir")); + assert_eq!(ops[0].name.as_deref(), Some("moved.txt")); + let meta: RenameMeta = serde_json::from_str(ops[0].meta_json.as_deref().unwrap()).unwrap(); + assert_eq!(meta.original_parent_uid, uid("parent").to_string()); + assert_eq!(meta.original_name.as_deref(), Some("new (conflict).txt")); + + // The folder lands next and the move follows it. + db.remap_local_uid("local~dir", &uid("dir").to_string()) + .unwrap(); + let ops = db.pending_ops().unwrap(); + assert_eq!(ops[0].parent_uid, Some(uid("dir").to_string())); +} + #[test] fn draining_a_folder_repoints_its_queued_children() { let db = Db::open_in_memory().unwrap(); @@ -3002,8 +3068,12 @@ fn a_landed_folder_keeps_a_row_until_the_server_copy_replaces_it() { db.finish_create( id, None, - &local.to_string(), - &uid("real").to_string(), + &landing( + &local.to_string(), + &uid("real").to_string(), + &uid("root").to_string(), + "New folder", + ), |_| None, ) .unwrap(); diff --git a/crates/pdfs-fuse/src/drain.rs b/crates/pdfs-fuse/src/drain.rs index 164d4e2..835f9fd 100644 --- a/crates/pdfs-fuse/src/drain.rs +++ b/crates/pdfs-fuse/src/drain.rs @@ -33,7 +33,8 @@ use pdfs_core::batch; use pdfs_core::cache::{Baseline, StagedWrite}; use pdfs_core::control::{ActivityKind, TransferDirection}; use pdfs_core::db::{ - OP_CREATE, OP_MKDIR, OP_RENAME, OP_REVISION, OP_TRASH, PARK_EXPIRY_MS, PendingOp, RenameMeta, + CreateLanding, 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}; @@ -851,6 +852,7 @@ impl Core { // create was queued because nobody heard back, so adopt it. And a node // whose trash is still queued is about to let go of it. let mut name = wanted.clone(); + let mut home = parent.clone(); let mut adopted = false; let mut real = self.create_drained_node(op, &parent, &name); if real.as_ref().is_err_and(|e| is_already_exists(e.as_ref())) @@ -893,6 +895,7 @@ impl Core { name = conflict_name(&wanted, now_secs()); real = self.create_drained_node(op, &root, &name); } + home = root; if real.is_ok() { self.log_activity( ActivityKind::Upload, @@ -910,7 +913,13 @@ impl Core { // An adopted node was made empty, so a blob this op carries has not been // uploaded: it goes on as a revision of the node instead. let uploaded = (!adopted).then_some(op.blob_path.as_deref()).flatten(); - let newer = self.retire_create(op, uploaded, &local, &real)?; + let landing = CreateLanding { + local: &local.to_string(), + real: &real.to_string(), + sent: (parent_str, &wanted), + landed: (&home.to_string(), &name), + }; + let newer = self.retire_create(op, uploaded, &landing, &local, &real)?; // 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). @@ -1056,19 +1065,17 @@ impl Core { &self, op: &PendingOp, uploaded: Option<&str>, + landing: &CreateLanding<'_>, local: &NodeUid, real: &NodeUid, ) -> Result> { - let real_str = real.to_string(); let newer = { let mut pending = self.pending.lock(); - let newer = - self.db - .finish_create(op.id, uploaded, &local.to_string(), &real_str, |json| { - let mut meta: StagedWrite = serde_json::from_str(json).ok()?; - meta.uid = real_str.clone(); - serde_json::to_string(&meta).ok() - })?; + let newer = 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() + })?; pending.remove(local); if let Some((blob, json)) = &newer { let meta: StagedWrite = serde_json::from_str(json)?; @@ -1091,6 +1098,15 @@ impl Core { if let Some(e) = st.entries.get_mut(&ino) { e.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. + for kid in st.children.get(&ino).cloned().unwrap_or_default() { + if let Some(e) = st.entries.get_mut(&kid) + && e.node.parent_uid.as_ref() == Some(local) + { + e.node.parent_uid = Some(real.clone()); + } + } } for aw in st.active_writes.values_mut() { if aw.uid == *local { @@ -1156,9 +1172,13 @@ impl Core { adopt: impl FnOnce(&Node), ) -> Result<(), Box> { // Drive answers a create before it lists the new node. - let node = self + let mut node = self .read_back(real) .map_err(|e| self.errno_error(e, "fetch node"))?; + // A rename made while the create was on the wire is queued against the + // real uid (`Db::finish_create`). Until it lands, the tree keeps the + // name and folder the user gave it. + let renaming = self.db.has_pending_op(&real.to_string(), OP_RENAME)?; // Before open handles are rebased onto the node, so one released in // between finds what `adopt` keeps (`docs/BUGS.md` B123). adopt(&node); @@ -1174,6 +1194,10 @@ impl Core { let Some(&ino) = st.by_uid.get(real) else { return; }; + if renaming && let Some(e) = st.entries.get(&ino) { + node.name = e.node.name.clone(); + node.parent_uid = e.node.parent_uid.clone(); + } let mut landed = node.clone(); st.keep_open_write_size(ino, &mut landed); if let Some(e) = st.entries.get_mut(&ino) { diff --git a/docs/BUGS.md b/docs/BUGS.md index 02e6daa..f3b03ea 100644 --- a/docs/BUGS.md +++ b/docs/BUGS.md @@ -12,6 +12,35 @@ Conventions: --- +## B136 — A file renamed while its first upload is on the wire goes back to its old name + +**Status:** Fixed (unverified). +**Found:** 2026-10-03, by the simulation runs (`sim::run`, profile one-client-flaky, seed 3), at +the end check for lost data. A file made offline was renamed after the drain had taken its create. +The create landed under the old name and the new name was gone. Every replay of the seed. + +**Where:** `Db::finish_create` in `crates/pdfs-core/src/db/ops.rs`, `drain_local_node` and +`adopt_real_uid` in `crates/pdfs-fuse/src/drain.rs`. + +**Cause.** A rename of a node whose create is still queued rewrites the create's parent and name +(`Db::rewrite_op_target`) instead of queuing a rename. The drain copies the op when it takes it, +so a rewrite while the create was on the wire changed only the row. When the create landed, +`finish_create` deleted the row without looking at it again, and `adopt_real_uid` put the node +back where Drive had it. The move was lost on Drive and in the tree. + +The same seed showed a second fault. When a folder made offline landed, the nodes in it still +named the folder's placeholder uid as their parent in memory. The next write of one of them put +the placeholder back on its row. + +**Fix.** The drain passes where it sent the create and where it landed (a conflict name, or the +root when the folder was gone) to `finish_create`. When the row names another parent or name than +it was sent with, the same transaction queues a rename of the real uid to it, with the landed +parent and name as the original ones. `adopt_real_uid` keeps the tree's name and parent while +that rename is queued. When a node lands, its children's parent is changed to its real uid. + +**Test:** the unit test `a_rename_made_while_its_create_uploads_is_queued_for_the_landed_node`. +Seed 3 of one-client-flaky passes. + ## B135 — A file just moved is sometimes not found by its new path **Status:** Fixed (unverified). diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 9e1fe67..1d8e3bb 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -11,6 +11,10 @@ scratch (user data in `staging/` and `recovery/` is never touched by this). ## [Unreleased] +### Fixed +- **A file renamed or moved while its first upload is on the wire keeps its new name and folder.** + It landed under the old name and the rename was lost (B136). + ## [2.9.0] — 2026-10-03 Schema: **35** (unchanged). Requires `proton-drive-rs` **0.7.2**.