diff --git a/crates/pdfs-core/src/db/migrations.rs b/crates/pdfs-core/src/db/migrations.rs index f73f9f5..208f63b 100644 --- a/crates/pdfs-core/src/db/migrations.rs +++ b/crates/pdfs-core/src/db/migrations.rs @@ -11,7 +11,7 @@ use super::Db; use crate::Result; /// Current schema version. Bump on every forward migration added below. -pub(super) const SCHEMA_VERSION: i64 = 41; +pub(super) const SCHEMA_VERSION: i64 = 42; impl Db { pub(super) fn migrate(&self) -> Result<()> { @@ -424,6 +424,22 @@ impl Db { tx.execute_batch(MIGRATION_V41)?; } } + if current < 42 { + // Same guards as V26-V40. + let has_column: bool = tx.query_row( + "SELECT COUNT(*) FROM pragma_table_info('pending_op') WHERE name = 'sent_sha1'", + [], + |row| row.get::<_, i64>(0), + )? > 0; + let has_ops: bool = tx.query_row( + "SELECT COUNT(*) FROM sqlite_master WHERE type = 'table' AND name = 'pending_op'", + [], + |row| row.get::<_, i64>(0), + )? > 0; + if has_ops && !has_column { + tx.execute_batch(MIGRATION_V42)?; + } + } tx.execute( "INSERT INTO sync_state (key, value) VALUES ('schema_version', ?1) ON CONFLICT(key) DO UPDATE SET value = excluded.value", @@ -1302,3 +1318,15 @@ UPDATE nodes UPDATE nodes SET uid = 'local~' || lid WHERE lid IN (SELECT lid FROM local_v41); DROP TABLE local_v41; "; + +/// Schema v42: the SHA-1 of each blob a queued create sent, space-separated. +/// A create whose answer was lost may have made its file with any of them, +/// and the op that finds that file again may run after a restart. `NULL` for +/// an op that has sent nothing, which is every op migrated: what the daemon +/// that wrote it sent is not known. Likewise the parent and name each attempt +/// was sent under, as a JSON list of pairs: a rename since then moves the op +/// but not the file Drive may have made. +const MIGRATION_V42: &str = " +ALTER TABLE pending_op ADD COLUMN sent_sha1 TEXT; +ALTER TABLE pending_op ADD COLUMN sent_to TEXT; +"; diff --git a/crates/pdfs-core/src/db/ops.rs b/crates/pdfs-core/src/db/ops.rs index 9b509e7..f132e42 100644 --- a/crates/pdfs-core/src/db/ops.rs +++ b/crates/pdfs-core/src/db/ops.rs @@ -765,6 +765,89 @@ impl Db { }) } + /// Record that queued create `id` is sending a blob with SHA-1 `sha`. + /// + /// Drive may make the file and lose its answer. The op that next finds + /// the file, the create's retry or the trash a delete made of it, takes + /// it for its own when it holds any bytes sent so. Kept on the row, so a + /// newer blob, the turn into a trash and a restart all keep it + /// (`docs/BUGS.md` B156, B183). + pub fn note_create_sent(&self, id: i64, sha: &str) -> Result<()> { + self.conn.lock().execute( + "UPDATE pending_op + SET sent_sha1 = CASE + WHEN sent_sha1 IS NULL THEN ?2 + WHEN instr(' ' || sent_sha1 || ' ', ' ' || ?2 || ' ') > 0 THEN sent_sha1 + ELSE sent_sha1 || ' ' || ?2 END + WHERE id = ?1", + params![id, sha], + )?; + Ok(()) + } + + /// Record that queued create `id` is being sent to `parent` under `name`. + /// + /// A rename while the op waits rewrites its target, but the file a lost + /// answer left stays where it was sent. The retry and the trash look + /// there too (`docs/BUGS.md` B184). + pub fn note_create_target(&self, id: i64, parent: &str, name: &str) -> Result<()> { + let mut conn = self.conn.lock(); + let tx = conn.transaction()?; + let sent: Option = tx + .query_row("SELECT sent_to FROM pending_op WHERE id = ?1", [id], |r| { + r.get(0) + }) + .optional()? + .flatten(); + let mut targets: Vec<(String, String)> = sent + .and_then(|json| serde_json::from_str(&json).ok()) + .unwrap_or_default(); + let target = (parent.to_string(), name.to_string()); + if !targets.contains(&target) { + targets.push(target); + tx.execute( + "UPDATE pending_op SET sent_to = ?2 WHERE id = ?1", + params![id, serde_json::to_string(&targets)?], + )?; + } + tx.commit()?; + Ok(()) + } + + /// The parent and name queued op `id` was sent under as a create, in the + /// order it was ([`Db::note_create_target`]). + pub fn create_targets(&self, id: i64) -> Result> { + let sent: Option = self + .conn + .lock() + .query_row("SELECT sent_to FROM pending_op WHERE id = ?1", [id], |r| { + r.get(0) + }) + .optional()? + .flatten(); + Ok(sent + .and_then(|json| serde_json::from_str(&json).ok()) + .unwrap_or_default()) + } + + /// The SHA-1 of each blob queued op `id` sent as a create + /// ([`Db::note_create_sent`]). + pub fn create_sent(&self, id: i64) -> Result> { + let sent: Option = self + .conn + .lock() + .query_row( + "SELECT sent_sha1 FROM pending_op WHERE id = ?1", + [id], + |r| r.get(0), + ) + .optional()? + .flatten(); + Ok(sent + .map(|sent| sent.split(' ').map(str::to_string).collect()) + .unwrap_or_default()) + } + /// Replace the sidecar of a queued op, for a baseline that has moved under /// it. /// diff --git a/crates/pdfs-core/src/db/tests.rs b/crates/pdfs-core/src/db/tests.rs index 9ae448f..6c192e5 100644 --- a/crates/pdfs-core/src/db/tests.rs +++ b/crates/pdfs-core/src/db/tests.rs @@ -3848,6 +3848,69 @@ fn a_create_withdrawn_after_it_failed_queues_a_trash_that_keeps_its_blob() { assert!(db.pending_ops().unwrap().is_empty()); } +/// What a queued create sent, and where, stays on its row: through a newer +/// blob, through the turn into a trash a delete makes of it, and across a +/// reopen. The trash finds the file the create made by those bytes, after a +/// restart too, and under a name the file had before a rename +/// (`docs/BUGS.md` B183, B184). +#[test] +fn what_a_create_sent_outlives_a_newer_blob_its_trash_and_a_restart() { + let path = std::env::temp_dir().join(format!( + "pdfs-create-sent-{}-{}.db", + std::process::id(), + now_test_id() + )); + let parent = uid("parent").to_string(); + let id = { + let db = Db::open(&path).unwrap(); + let (id, _) = db + .enqueue_op(&PendingOp { + id: 0, + kind: OP_CREATE.to_string(), + uid: "local~sent".to_string(), + parent_uid: Some(parent.clone()), + name: Some("d".to_string()), + blob_path: Some("/staging/first".to_string()), + meta_json: Some("{}".to_string()), + created_at: 1, + attempts: 0, + last_error: None, + next_attempt_at: 0, + }) + .unwrap(); + assert!(db.create_sent(id).unwrap().is_empty()); + assert!(db.create_targets(id).unwrap().is_empty()); + db.note_create_target(id, &parent, "c").unwrap(); + db.note_create_target(id, &parent, "c").unwrap(); + db.note_create_sent(id, "aaaa").unwrap(); + db.note_create_sent(id, "aaaa").unwrap(); + db.record_op_failure(id, "no answer", 0).unwrap(); + db.attach_blob_to_create("local~sent", "/staging/second", "{}") + .unwrap() + .expect("the create is still queued"); + db.note_create_target(id, &parent, "d").unwrap(); + db.note_create_sent(id, "bbbb").unwrap(); + db.record_op_failure(id, "no answer", 0).unwrap(); + db.delete_ops_for_uid("local~sent").unwrap(); + id + }; + + let db = Db::open(&path).unwrap(); + let ops = db.pending_ops().unwrap(); + assert_eq!(ops.len(), 1); + assert_eq!((ops[0].id, ops[0].kind.as_str()), (id, OP_TRASH)); + assert_eq!(db.create_sent(id).unwrap(), vec!["aaaa", "bbbb"]); + assert_eq!( + db.create_targets(id).unwrap(), + vec![ + (parent.clone(), "c".to_string()), + (parent.clone(), "d".to_string()) + ] + ); + drop(db); + let _ = std::fs::remove_file(path); +} + /// A create deleted after it was sent becomes a trash in its place. The /// create's backoff, and the failure of an attempt still on the wire, are the /// create's: the trash is due at once (`docs/BUGS.md` B171). diff --git a/crates/pdfs-fuse/src/drain.rs b/crates/pdfs-fuse/src/drain.rs index 1cace40..5dac840 100644 --- a/crates/pdfs-fuse/src/drain.rs +++ b/crates/pdfs-fuse/src/drain.rs @@ -153,6 +153,19 @@ enum DrainDisposition { AuthorityUnknown(NodeUid), } +/// The node an earlier attempt at a create made before a rename moved the op +/// (`docs/BUGS.md` B184). +struct EarlierTwin { + /// The folder the attempt was sent to. + parent: NodeUid, + /// The name it was sent under. + name: String, + /// The node it made. + twin: NodeUid, + /// Whether the node holds the op's blob already. + holds_blob: bool, +} + /// Why an op went back on the clock without consuming an attempt. Only affects /// what the user is told — every variant is deferred and reported identically. #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -558,7 +571,6 @@ impl Core { self.release_claim(op); return; } - self.create_sent.lock().remove(&op.id); return; } match fetched { @@ -747,8 +759,13 @@ impl Core { let Some(real) = parse_node_uid(&op.uid) else { return Ok(()); }; - let Some(local) = self.unadopted.lock().get(&real).cloned() else { - return Ok(()); + let local = match self.unadopted.lock().get(&real).cloned() { + Some(local) => local, + // The map does not outlive a restart, which also forgets the + // stand-in: the row is under the real uid by then. A file as it was + // made here has no revision of Drive's (`docs/BUGS.md` B182). + None if op.kind == OP_REVISION && self.never_read_back(&real) => real.clone(), + None => return Ok(()), }; // A node gone since is the op's to deal with. if self.fetch_node_remote(&real)?.is_some() { @@ -759,6 +776,16 @@ impl Core { Ok(()) } + /// Whether the tree has the file `real` as it was made here: its create + /// landed, but Drive's answer was never read back. + fn never_read_back(&self, real: &NodeUid) -> bool { + let st = self.state(); + st.by_uid + .get(real) + .and_then(|ino| st.entries.get(ino)) + .is_some_and(|entry| !entry.node.is_folder() && node_revision_id(&entry.node).is_none()) + } + /// Apply a queued rename/move to the remote. /// /// The op is the desired end state, so the remote's current state decides @@ -953,7 +980,6 @@ impl Core { None => debug!(%uid, name, "withdrawn create never landed; trash op satisfied"), } self.retire_trash_op(op, &uid)?; - self.create_sent.lock().remove(&op.id); return Ok(()); } match self @@ -989,24 +1015,48 @@ impl Core { } /// The node a create that a trash withdrew made on Drive although its - /// answer was lost: in the create's folder under its name, and one the - /// create would have adopted had it been retried ([`adoptable`]). A node - /// with a change of ours queued is one the user is still working on. + /// answer was lost: in the create's folder under its name, or under one + /// an attempt was sent with before a rename (`docs/BUGS.md` B184). fn withdrawn_twin( &self, op: &PendingOp, ) -> Result, Box> { - let (Some(parent), Some(name)) = ( - op.parent_uid - .as_deref() - .filter(|parent| !is_local_uid_str(parent)) - .and_then(parse_node_uid), - op.name.as_deref(), - ) else { - return Ok(None); - }; + let mut targets: Vec<(String, String)> = op + .parent_uid + .clone() + .zip(op.name.clone()) + .into_iter() + .collect(); + for target in self.db.create_targets(op.id)? { + if !targets.contains(&target) { + targets.push(target); + } + } + for (parent, name) in targets { + if is_local_uid_str(&parent) { + continue; + } + let Some(parent) = parse_node_uid(&parent) else { + continue; + }; + if let Some(twin) = self.withdrawn_twin_in(op, &parent, &name)? { + return Ok(Some(twin)); + } + } + Ok(None) + } + + /// The node in `parent` under `name` a withdrawn create made, if it is one + /// the create would have adopted had it been retried ([`adoptable`]). A + /// node with a change of ours queued is one the user is still working on. + fn withdrawn_twin_in( + &self, + op: &PendingOp, + parent: &NodeUid, + name: &str, + ) -> Result, Box> { let uids = - match self.block_on_bounded(self.drive.enumerate_folder_children_node_uids(&parent)) { + match self.block_on_bounded(self.drive.enumerate_folder_children_node_uids(parent)) { Ok(uids) => uids, Err(e) if is_gone(&e) => return Ok(None), Err(e) => return Err(e.into()), @@ -1090,11 +1140,6 @@ impl Core { } let created = self.create_local_node(op); self.creating.lock().remove(&op.id); - // A withdrawn create goes on as a trash under the same id, which still - // looks for the bytes it sent (`Core::withdrawn_twin`). - if !self.db.op_exists(op.id).unwrap_or(true) { - self.create_sent.lock().remove(&op.id); - } created } @@ -1123,8 +1168,27 @@ impl Core { // whose trash is still queued is about to let go of it. let mut name = wanted.clone(); let mut home = parent.clone(); + let mut sent = (parent_str.to_string(), wanted.clone()); let mut blob_landed = true; - let mut real = self.create_drained_node(op, &parent, &name); + // An earlier attempt, sent before the file was renamed or moved, may + // have made it and lost the answer. That node is ours where it was + // sent, and the rename follows it (`docs/BUGS.md` B184). + let mut real = match self.earlier_twin(op, parent_str, &wanted)? { + Some(EarlierTwin { + parent: was_in, + name: was_named, + twin, + holds_blob, + }) => { + info!(%local, %twin, was_named, wanted, "an earlier attempt landed unanswered under another name; adopting it"); + sent = (was_in.to_string(), was_named.clone()); + home = was_in; + name = was_named; + blob_landed = holds_blob; + Ok(twin) + } + None => self.create_drained_node(op, &parent, &name), + }; if real.as_ref().is_err_and(|e| is_already_exists(e.as_ref())) && let Some((twin, holds_blob)) = self.adoptable_twin(op, &parent, &wanted)? { @@ -1202,7 +1266,7 @@ impl Core { let landing = CreateLanding { local: &local.to_string(), real: &real.to_string(), - sent: (parent_str, &wanted), + sent: (&sent.0, &sent.1), landed: (&home.to_string(), &name), }; let newer = match self.retire_create(op, uploaded, &landing, &local, &real)? { @@ -1335,6 +1399,38 @@ impl Core { Ok(()) } + /// The node an earlier attempt at create `op` made under a parent or name + /// the op has been moved from since ([`Core::adoptable_twin`]). + fn earlier_twin( + &self, + op: &PendingOp, + parent: &str, + name: &str, + ) -> Result, Box> { + for (was_in, was_named) in self.db.create_targets(op.id)? { + if (was_in.as_str(), was_named.as_str()) == (parent, name) { + continue; + } + let Some(was_in) = parse_node_uid(&was_in) else { + continue; + }; + match self.adoptable_twin(op, &was_in, &was_named) { + Ok(Some((twin, holds_blob))) => { + return Ok(Some(EarlierTwin { + parent: was_in, + name: was_named, + twin, + holds_blob, + })); + } + Ok(None) => {} + Err(e) if is_gone(e.as_ref()) => {} + Err(e) => return Err(e), + } + } + Ok(None) + } + /// The node already holding `name` under `parent`, if it is the one an /// earlier attempt at this op made before its answer was lost, and whether /// it holds the op's blob already. @@ -1407,12 +1503,10 @@ impl Core { /// `current`, those of the blob it holds now, and those each earlier /// attempt uploaded. fn create_sent_digests(&self, id: i64, current: Option) -> Vec { - let mut sent = self - .create_sent - .lock() - .get(&id) - .cloned() - .unwrap_or_default(); + let mut sent = self.db.create_sent(id).unwrap_or_else(|e| { + warn!(id, error = %e, "reading what a queued create sent failed"); + Vec::new() + }); sent.extend(current); sent } @@ -1428,6 +1522,8 @@ impl Core { parent: &NodeUid, name: &str, ) -> Result> { + self.db + .note_create_target(op.id, &parent.to_string(), name)?; match op.kind == OP_MKDIR { true => Ok(self.block_on_bounded(self.drive.create_folder( parent, @@ -1465,12 +1561,7 @@ impl Core { // The call may make the file and lose its answer, and the op may hold // a newer blob by the time it is retried (`Core::adoptable_twin`). let sha = staged_sha1(Path::new(blob))?; - let mut sent = self.create_sent.lock(); - let digests = sent.entry(op.id).or_default(); - if !digests.contains(&sha) { - digests.push(sha); - } - drop(sent); + self.db.note_create_sent(op.id, &sha)?; let thumbnails = self.upload_thumbnails(Path::new(blob), name); let guard = self .transfers diff --git a/crates/pdfs-fuse/src/lib.rs b/crates/pdfs-fuse/src/lib.rs index 193b330..2d9e44e 100644 --- a/crates/pdfs-fuse/src/lib.rs +++ b/crates/pdfs-fuse/src/lib.rs @@ -572,10 +572,6 @@ struct Core { /// Drive holds that name until the create lands and a rename or trash /// queued behind it frees it again. creating: Arc>>, - /// The SHA-1 of each blob an attempt at a queued create uploaded, by op - /// id. An attempt whose answer was lost may have made the file with bytes - /// a later write has since replaced in the op (`docs/BUGS.md` B156). - create_sent: Arc>>>, /// The SHA-1 of the last revision this daemon uploaded to a file and could /// not read back, so its revision id is not known (`docs/BUGS.md` B157). sealed_unread: Arc>>, diff --git a/crates/pdfs-fuse/src/mount.rs b/crates/pdfs-fuse/src/mount.rs index 828fd99..5ecd195 100644 --- a/crates/pdfs-fuse/src/mount.rs +++ b/crates/pdfs-fuse/src/mount.rs @@ -434,7 +434,6 @@ pub(crate) fn mount_with( pending: Arc::new(Mutex::new(HashMap::new())), hidden: Arc::new(Mutex::new(HashSet::new())), creating: Arc::new(Mutex::new(HashMap::new())), - create_sent: Arc::new(Mutex::new(HashMap::new())), sealed_unread: Arc::new(Mutex::new(HashMap::new())), landed: Arc::new(Mutex::new(HashMap::new())), unadopted: Arc::new(Mutex::new(HashMap::new())), diff --git a/crates/pdfs-fuse/src/sim/daemon.rs b/crates/pdfs-fuse/src/sim/daemon.rs index ff4d962..275a93d 100644 --- a/crates/pdfs-fuse/src/sim/daemon.rs +++ b/crates/pdfs-fuse/src/sim/daemon.rs @@ -1290,6 +1290,47 @@ mod tests { let _ = std::fs::remove_dir_all(&dir); } + #[test] + #[ignore = "mounts FUSE: run with `cargo test -p pdfs-fuse sim:: -- --ignored`"] + fn a_write_after_a_restart_over_a_create_never_read_back_is_not_a_conflict() { + // The create landed and the link went down before it was read back, + // then the daemon restarted. What was to read it back did not outlive + // the restart, so the next write was based on the file as it was made + // here and landed as a conflict copy of it (B182). + let drive = FakeDrive::new(); + let dir = scratch("unadopted-restart"); + 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") + && daemon.pending().is_ok_and(|items| items.is_empty()) + })); + let daemon = daemon.restart().unwrap(); + let path = daemon.mountpoint.join("f.txt"); + 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); + } + #[test] #[ignore = "mounts FUSE: run with `cargo test -p pdfs-fuse sim:: -- --ignored`"] fn a_create_whose_answer_was_lost_lands_once() { @@ -1384,6 +1425,126 @@ mod tests { let _ = std::fs::remove_dir_all(&dir); } + #[test] + #[ignore = "mounts FUSE: run with `cargo test -p pdfs-fuse sim:: -- --ignored`"] + fn a_file_deleted_after_a_restart_leaves_drive_with_what_its_create_sent() { + // Drive made the file with the bytes its create sent, but the answer + // was lost, and the file was written again. After a restart the file + // was deleted. The trash knew only the bytes the file held last, which + // were not the ones Drive had, so it left that file on Drive (B183). + let drive = FakeDrive::new(); + let dir = scratch("lost-create-reply-restart-deleted"); + let daemon = Daemon::start(&dir, drive.client(1, Faults::lan())).unwrap(); + let path = daemon.mountpoint.join("f.txt"); + + let held = daemon.client.hold_reply_to_next_create(); + daemon.client.lose_reply_to_next_create(); + std::fs::write(&path, b"first").unwrap(); + assert!(wait_until(Duration::from_secs(30), || held.reached())); + std::fs::write(&path, b"second").unwrap(); + drop(held); + assert!(wait_until(Duration::from_secs(30), || { + daemon + .pending() + .is_ok_and(|items| items.iter().any(|item| item.attempts > 0)) + })); + daemon.client.set_online(false); + let daemon = daemon.restart().unwrap(); + let path = daemon.mountpoint.join("f.txt"); + std::fs::remove_file(&path).unwrap(); + daemon.client.set_online(true); + assert!( + wait_until(Duration::from_secs(30), || { + daemon.pending().is_ok_and(|items| items.is_empty()) && drive.tree().is_empty() + }), + "{:?} {:?}", + daemon.pending(), + drive.tree() + ); + assert!(!path.exists()); + + 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_file_renamed_after_its_create_lost_its_answer_lands_once() { + // Drive made the file, but the answer was lost, and the file was + // renamed before the retry. The retry looked for the file Drive made + // under the new name only, and made a second one there (B184). + let drive = FakeDrive::new(); + let dir = scratch("lost-create-reply-renamed"); + let daemon = Daemon::start(&dir, drive.client(1, Faults::lan())).unwrap(); + let file = |bytes: &[u8]| Some(Entry::File(Arc::new(bytes.to_vec()))); + + daemon.client.lose_reply_to_next_create(); + std::fs::write(daemon.mountpoint.join("f.txt"), b"first").unwrap(); + assert!(wait_until(Duration::from_secs(30), || { + daemon + .pending() + .is_ok_and(|items| items.iter().any(|item| item.attempts > 0)) + })); + daemon.client.set_online(false); + std::fs::rename( + daemon.mountpoint.join("f.txt"), + daemon.mountpoint.join("g.txt"), + ) + .unwrap(); + daemon.client.set_online(true); + assert!( + wait_until(Duration::from_secs(30), || { + daemon.pending().is_ok_and(|items| items.is_empty()) + && drive.tree().get("g.txt").cloned() == file(b"first") + }), + "{:?} {:?}", + daemon.pending(), + drive.tree() + ); + assert_eq!(drive.tree().len(), 1, "{:?}", drive.tree()); + + 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_file_renamed_and_deleted_after_its_create_lost_its_answer_leaves_nothing() { + // As above, and the file was deleted after the rename. The trash + // looked for the file Drive made under the last name only, and left + // it on Drive under the first (B184). + let drive = FakeDrive::new(); + let dir = scratch("lost-create-reply-renamed-deleted"); + let daemon = Daemon::start(&dir, drive.client(1, Faults::lan())).unwrap(); + + daemon.client.lose_reply_to_next_create(); + std::fs::write(daemon.mountpoint.join("f.txt"), b"first").unwrap(); + assert!(wait_until(Duration::from_secs(30), || { + daemon + .pending() + .is_ok_and(|items| items.iter().any(|item| item.attempts > 0)) + })); + daemon.client.set_online(false); + std::fs::rename( + daemon.mountpoint.join("f.txt"), + daemon.mountpoint.join("g.txt"), + ) + .unwrap(); + std::fs::remove_file(daemon.mountpoint.join("g.txt")).unwrap(); + daemon.client.set_online(true); + assert!( + wait_until(Duration::from_secs(30), || { + daemon.pending().is_ok_and(|items| items.is_empty()) && drive.tree().is_empty() + }), + "{:?} {:?}", + daemon.pending(), + drive.tree() + ); + + 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_file_made_under_the_name_of_a_create_on_the_wire_waits_for_it() { diff --git a/docs/BUGS.md b/docs/BUGS.md index f88aa99..1269d0f 100644 --- a/docs/BUGS.md +++ b/docs/BUGS.md @@ -12,6 +12,71 @@ Conventions: --- +## B184 — A file renamed after its create lost its answer stays on Drive under its old name + +**Status:** Fixed (unverified). +**Found:** 2026-10-05, replaying `one_client_on_a_flaky_link` seed 11 with the B183 fix. `a.txt`'s +create reached Drive and lost its answer. The file was then rewritten, renamed to `b.txt` and +then `d`, and deleted. Drive kept `a.txt` with the bytes the create had sent. + +**Where:** `Core::withdrawn_twin` and `Core::create_local_node` in +`crates/pdfs-fuse/src/drain.rs`. + +**Cause.** A rename while a create waits rewrites the op's parent and name, but the node a lost +answer left stays where the create was sent. The trash a delete made of the create, and the +create's retry, looked for that node under the op's last name only. The trash found nothing +and left the file on Drive. The retry made a second file under the new name, so Drive had both. + +**Fix.** Each attempt records the parent and name it is sent under on the op's row, in a new +`pending_op.sent_to` column (schema 42). The trash looks for its node under each of them. A +retry first adopts a node an earlier attempt made under another name, and the rename to the +current name follows it. Sim tests `a_file_renamed_after_its_create_lost_its_answer_lands_once` +and `a_file_renamed_and_deleted_after_its_create_lost_its_answer_leaves_nothing`, and +`what_a_create_sent_outlives_a_newer_blob_its_trash_and_a_restart` in +`crates/pdfs-core/src/db/tests.rs`. + +## B183 — A file deleted after a restart stays on Drive when its create lost its answer + +**Status:** Fixed (unverified). +**Found:** 2026-10-05, by the known-bug confirmation run (`PDFS_SIM_KNOWN=fail`) on da013c2: +`one_client_on_a_flaky_link` seed 11 failed every replay. `d` was made, written again while +its create was on the wire, and deleted after several restarts. Drive kept a `d` with the bytes +the create had sent, and the next file renamed to `d` landed as `d (sync-conflict …)`. + +**Where:** `Core::withdrawn_twin` and `Core::adoptable_twin` in `crates/pdfs-fuse/src/drain.rs`. + +**Cause.** The trash a delete makes of a create that was sent looks for a node Drive made with +the bytes the create sent (B151). Those were kept in memory only (B156). After a restart the +trash knew only the blob the op held last, which a later write had replaced. It found no node of +its own and left the file on Drive. A create retried after a restart had the same gap and forked +a conflict copy. + +**Fix.** Each attempt records the SHA-1 of the blob it sends on the op's row, in a new +`pending_op.sent_sha1` column (schema 42). The row keeps it through a newer blob, through the +turn into a trash and across restarts. The in-memory map is gone. Tests +`what_a_create_sent_outlives_a_newer_blob_its_trash_and_a_restart` in +`crates/pdfs-core/src/db/tests.rs`, and sim test +`a_file_deleted_after_a_restart_leaves_drive_with_what_its_create_sent`. + +## B182 — A write after a restart over a create never read back lands as a conflict copy + +**Status:** Fixed (unverified). +**Found:** 2026-10-05, by the known-bug confirmation run on da013c2: +`one_client_on_a_flaky_link` seed 6 failed two of three replays. `y/x/c.bin`'s create landed and +the link went down before it was read back. The daemon restarted, and a write to the file landed +as `c (sync-conflict …).bin`, with Drive keeping the old bytes. + +**Where:** `Core::finish_adoption` in `crates/pdfs-fuse/src/drain.rs`. + +**Cause.** A create that landed but was not read back is adopted before the next op for the +node is sent (B143). The node is remembered for that in memory, and a restart forgot it. The +tree then still had the file as it was made here, with no revision of Drive's, and a write was +based on that. The drain found Drive's revision in its way and took it for someone else's. + +**Fix.** A queued write over a file the tree holds without a revision id is adopted first, as +B143 does, after a restart too. Sim test +`a_write_after_a_restart_over_a_create_never_read_back_is_not_a_conflict`. + ## B181 — The app shows a queued edit kept as a conflict copy in English **Status:** Fixed. @@ -673,8 +738,7 @@ the op held at the retry was hashed. The twin held the bytes an earlier attempt **Fix.** Each attempt records the SHA-1 of the blob it uploads, by op id, in memory. A twin holding any of them is ours. One holding older bytes than the op's is adopted, and the op's blob goes up -as a revision of it. After a restart such a twin still forks a conflict copy, which loses -nothing. Sim test `a_file_written_again_after_its_create_lost_its_answer_lands_once`; the fake +as a revision of it. Kept on the op's row since B183, so a restart no longer forgets them. Sim test `a_file_written_again_after_its_create_lost_its_answer_lands_once`; the fake Drive's held and lost create answers now combine. ## B155 — A write whose upload lost its answer lands as a conflict copy of itself diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 770f317..6566dc1 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -11,9 +11,10 @@ scratch (user data in `staging/` and `recovery/` is never touched by this). ## [Unreleased] -Schema: **41**. The queue table is rebuilt so an id is never given out twice; queued changes -are kept. Queued changes get a column for the reason Drive refused them, and name their file -and folder by local id as well as by Drive's id. Stored files and folders name their parent +Schema: **42**. The queue table is rebuilt so an id is never given out twice; queued changes +are kept. Queued changes get a column for the reason Drive refused them, and two for the +content a new file was sent with and where, and name their file and folder by local id as well +as by Drive's id. Stored files and folders name their parent folder by local id too. A file or folder made on this machine that Drive does not have yet goes by its local id, in the queue, the stored tree and pins alike. @@ -96,6 +97,14 @@ by its local id, in the queue, the stored tree and pins alike. - **The daemon no longer hangs when a file being read changes on Drive** (B130). - **The Activity page says in your language why an edit was kept as a conflicting copy**, instead of the daemon's English (B181). +- **A file written after a restart no longer lands as a conflict copy of itself** when the + connection dropped just as it first reached Drive (B182). +- **A file deleted after a restart is removed from Drive too** when the connection lost Drive's + answer to its upload and the file was written again meanwhile. It no longer stayed there and + turned the next file under its name into a conflict copy (B183). +- **A file renamed while the connection lost Drive's answer to its upload lands once.** It no + longer stayed on Drive under its old name too, or, when deleted, under its old name alone + (B184). - **A file renamed just as its upload finished keeps its new name.** The mount no longer showed the old name, and a second rename no longer failed with "No such file or directory" (B152, B172). diff --git a/docs/MILESTONE-3.0.0-STATUS.md b/docs/MILESTONE-3.0.0-STATUS.md index 025adb3..ac141e1 100644 --- a/docs/MILESTONE-3.0.0-STATUS.md +++ b/docs/MILESTONE-3.0.0-STATUS.md @@ -9,7 +9,7 @@ Tracks `docs/MILESTONE-3.0.0.md`. Update it with every commit that moves a phase - **Phase 2**: local ids, inode = local id, one tree for every mount (2.10.0). - **Phase 3, first part**: mkdir, create, rename, unlink and rmdir on the mount are recorded locally and sent from the queue; `"local_first": false` brings back the old path (5e0f30c). -- Fixes from the simulation and account runs since then: B125, B127, B130 and B141 to B181. +- Fixes from the simulation and account runs since then: B125, B127, B130 and B141 to B184. - **Sync issues, first cut** (§6.2): refusals sorted into issues, shown at once in the app, tray and CLI; Export button and `pdfs sync export`; `pdfs sync issues`; Discard button and `pdfs sync discard`; a warning badge on the node in the file browser. @@ -102,7 +102,12 @@ Tracks `docs/MILESTONE-3.0.0.md`. Update it with every commit that moves a phase which a68dae9 (not yet pushed then) also takes off the runtime. CI now keeps a hung run's stacks. - [ ] Known-bug list in `sim/run.rs` is empty: none open; B125, B127, B129, B130 and B132 - fixed but not yet confirmed by the runs. + fixed but not yet confirmed by the runs. The confirmation run (`PDFS_SIM_KNOWN=fail`) + on da013c2 failed `one_client_on_a_flaky_link` seeds 6 and 11 on two new bugs, both + after restarts: a write over a create never read back (B182) and a trash that forgot + what its create sent (B183). Replays with those fixed found a third: a create renamed + after it lost its answer (B184). All three fixed; no known-bug matcher fired. It was + stopped before `three_clients`. - [ ] A clean account run. The one on a6f36d5 (2026-10-04, LAN: 427 passed, 12 failed) failed B113 and B121 on every on-demand mount, two mirror-to-mirror move cases, and the journal check. B121 was a bug (B149, fixed); B113's case still expected creates