From 6d75a56b00a2e83751d6f71710786ab4456d3359 Mon Sep 17 00:00:00 2001 From: Nils Pukropp Date: Sun, 4 Oct 2026 05:41:57 +0200 Subject: [PATCH] fix(fuse,core): adopt a twin holding the create's bytes (B127); trash the node of a withdrawn create that lost its answer (B151) A retried create now adopts the file its lost answer made when Drive's SHA-1 matches the staged blob, not only when the file is empty. A delete after a sent create keeps the op as a trash that finds and trashes the node that create may have made; a new create of that name waits for it. The simulation's settle now reads the log, so known damage is explained. --- crates/pdfs-core/src/db/nodes.rs | 15 +- crates/pdfs-core/src/db/ops.rs | 41 ++++- crates/pdfs-core/src/db/tests.rs | 53 ++++++ crates/pdfs-fuse/src/drain.rs | 233 ++++++++++++++++++++++--- crates/pdfs-fuse/src/lib.rs | 8 +- crates/pdfs-fuse/src/sim/daemon.rs | 59 +++++++ crates/pdfs-fuse/src/sim/fake_drive.rs | 12 ++ crates/pdfs-fuse/src/sim/run.rs | 6 +- crates/pdfs-fuse/src/takeout.rs | 2 +- docs/BUGS.md | 29 ++- docs/CHANGELOG.md | 4 + docs/MILESTONE-3.0.0-STATUS.md | 14 +- 12 files changed, 425 insertions(+), 51 deletions(-) diff --git a/crates/pdfs-core/src/db/nodes.rs b/crates/pdfs-core/src/db/nodes.rs index b0a75f5..4672594 100644 --- a/crates/pdfs-core/src/db/nodes.rs +++ b/crates/pdfs-core/src/db/nodes.rs @@ -271,18 +271,21 @@ impl Db { /// 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<()> { + /// + /// Returns the staged blob of the create the trash withdrew, if it kept + /// one, for the caller to discard. + 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 + let (name, blob): (Option, Option) = tx .query_row( - "SELECT name FROM pending_op WHERE id = ?1", + "SELECT name, blob_path FROM pending_op WHERE id = ?1", params![op_id], - |row| row.get(0), + |row| Ok((row.get(0)?, row.get(1)?)), ) .optional()? - .flatten(); + .unwrap_or_default(); let rowid: Option = tx .query_row( "SELECT rowid FROM nodes WHERE uid = ?1", @@ -299,7 +302,7 @@ impl Db { wake_ops_waiting_for_tx(&tx, &name)?; } tx.commit()?; - Ok(()) + Ok(blob) } /// Check if a folder node has any non-trashed children in the database. diff --git a/crates/pdfs-core/src/db/ops.rs b/crates/pdfs-core/src/db/ops.rs index 2f45577..28446b5 100644 --- a/crates/pdfs-core/src/db/ops.rs +++ b/crates/pdfs-core/src/db/ops.rs @@ -101,6 +101,12 @@ pub fn op_supersedes(kind: &str) -> bool { /// 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). +/// +/// So does a create that was sent before and failed. Its answer may have been +/// lost after Drive made the node, which would then keep the deleted file's +/// name and bytes (`docs/BUGS.md` B151). Such a trash keeps the create's +/// parent and blob, which the drain looks for the node by, and which it +/// discards once the trash is done ([`Db::complete_trash_op`]). fn drop_doomed_ops(tx: &rusqlite::Transaction<'_>, uid: &str) -> Result> { const DOOMED: &str = " WITH RECURSIVE @@ -123,11 +129,13 @@ fn drop_doomed_ops(tx: &rusqlite::Transaction<'_>, uid: &str) -> Result = { let mut stmt = tx.prepare(&format!( "{DOOMED} SELECT blob_path FROM pending_op - WHERE id IN (SELECT id FROM doomed) AND blob_path IS NOT NULL" + WHERE id IN (SELECT id FROM doomed) AND blob_path IS NOT NULL + AND NOT ({SENT})" ))?; let rows = stmt.query_map(params![uid], |r| r.get::<_, String>(0))?; rows.collect::, _>>()? @@ -136,9 +144,10 @@ fn drop_doomed_ops(tx: &rusqlite::Transaction<'_>, uid: &str) -> Result 0" + SET kind = 'trash', issue = NULL, + parent_uid = CASE WHEN parent_uid LIKE '{LOCAL_VOLUME}~%' THEN NULL + ELSE parent_uid END + WHERE id IN (SELECT id FROM doomed) AND {SENT}" ), params![uid], )?; @@ -147,7 +156,7 @@ fn drop_doomed_ops(tx: &rusqlite::Transaction<'_>, uid: &str) -> Result 0 AND uid LIKE '{LOCAL_VOLUME}~%')" + AND NOT (kind = 'trash' AND uid LIKE '{LOCAL_VOLUME}~%')" ), params![uid], )?; @@ -358,6 +367,28 @@ impl Db { .map_err(Into::into) } + /// Whether the trash of a create a delete withdrew after it was sent is + /// queued for `name` in `parent_uid` ([`drop_doomed_ops`]). + /// + /// The drain looks for the node that create may have made by its folder + /// and name, so a create of that name there waits for the trash: sent + /// first, it made a node the trash took for the withdrawn one. + pub fn has_withdrawn_create(&self, parent_uid: &str, name: &str) -> Result { + let conn = self.read(); + conn.query_row( + &format!( + "SELECT EXISTS( + SELECT 1 FROM pending_op + WHERE kind = ?1 AND parent_uid = ?2 AND name = ?3 + AND uid LIKE '{LOCAL_VOLUME}~%' + )" + ), + params![OP_TRASH, parent_uid, name], + |row| row.get(0), + ) + .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 diff --git a/crates/pdfs-core/src/db/tests.rs b/crates/pdfs-core/src/db/tests.rs index d4cac1e..25872c8 100644 --- a/crates/pdfs-core/src/db/tests.rs +++ b/crates/pdfs-core/src/db/tests.rs @@ -3581,6 +3581,59 @@ fn a_create_withdrawn_while_it_uploads_queues_a_trash_of_the_landed_node() { ); } +/// A file deleted after its create was sent and failed may still have been +/// made: the answer can be lost after Drive made it. Its trash keeps the +/// create's folder and blob to find that node by, and holds the name in the +/// folder until it is done (`docs/BUGS.md` B151). A create never sent goes, +/// blob and all. +#[test] +fn a_create_withdrawn_after_it_failed_queues_a_trash_that_keeps_its_blob() { + let db = Db::open_in_memory().unwrap(); + let parent = uid("parent").to_string(); + let create = |local: &str, blob: &str| { + db.enqueue_op(&PendingOp { + id: 0, + kind: OP_CREATE.to_string(), + uid: local.to_string(), + parent_uid: Some(parent.clone()), + name: Some("f.txt".to_string()), + blob_path: Some(blob.to_string()), + meta_json: Some("{}".to_string()), + created_at: 1, + attempts: 0, + last_error: None, + next_attempt_at: 0, + }) + .unwrap() + .0 + }; + let sent = create("local~sent", "/staging/sent"); + create("local~unsent", "/staging/unsent"); + db.record_op_failure(sent, "no answer", 0).unwrap(); + + assert!(db.delete_ops_for_uid("local~sent").unwrap().is_empty()); + assert_eq!( + db.delete_ops_for_uid("local~unsent").unwrap(), + vec!["/staging/unsent".to_string()] + ); + + let ops = db.pending_ops().unwrap(); + assert_eq!(ops.len(), 1); + assert_eq!(ops[0].kind, OP_TRASH); + assert_eq!(ops[0].uid, "local~sent"); + assert_eq!(ops[0].parent_uid.as_deref(), Some(parent.as_str())); + assert_eq!(ops[0].blob_path.as_deref(), Some("/staging/sent")); + assert!(db.has_withdrawn_create(&parent, "f.txt").unwrap()); + assert!(!db.has_withdrawn_create(&parent, "g.txt").unwrap()); + + let local = NodeUid::new(VolumeId::from("local"), LinkId::from("sent")); + assert_eq!( + db.complete_trash_op(sent, &local).unwrap().as_deref(), + Some("/staging/sent") + ); + assert!(db.pending_ops().unwrap().is_empty()); +} + #[test] fn a_landed_placeholder_never_overwrites_the_server_copy() { let db = Db::open_in_memory().unwrap(); diff --git a/crates/pdfs-fuse/src/drain.rs b/crates/pdfs-fuse/src/drain.rs index b31d4ee..44a97ba 100644 --- a/crates/pdfs-fuse/src/drain.rs +++ b/crates/pdfs-fuse/src/drain.rs @@ -42,6 +42,7 @@ use tracing::{debug, error, info, warn}; use super::link::{self, is_network_error}; use super::state::{Intervals, PendingRevision}; +use super::takeout::sha1_of; use super::transfers::CountingReader; use super::{ Core, DRAIN_BACKOFF_MAX, DRAIN_BACKOFF_MIN, DRAIN_IDLE_POLL, DRAIN_REVISION_DEBOUNCE, @@ -206,9 +207,13 @@ fn pending_op_authorities(op: &PendingOp) -> Result, Box Ok(Vec::new()), + // What a withdrawn create leaves: nothing on Drive to ask about but + // the folder it was sent to, where the node it may have made is + // trashed (`Core::drain_trash`). + OP_TRASH if is_local_uid_str(&op.uid) => match op.parent_uid.as_deref() { + Some(folder) if !is_local_uid_str(folder) => Ok(vec![parent()?]), + _ => Ok(Vec::new()), + }, OP_REVISION | OP_TRASH => Ok(vec![uid()?]), OP_CREATE | OP_MKDIR => Ok(vec![parent()?]), OP_RENAME => { @@ -879,11 +884,28 @@ 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`). + // The trash a delete left of a create that was sent, on the wire or + // before, and failed (`Db::delete_ops_for_uid`). A create that failed + // may still have made the node and lost only its answer, and the node + // would keep the deleted file's name and bytes (docs/BUGS.md B151). 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)?; + match self.withdrawn_twin(op)? { + Some(twin) => { + self.hidden.lock().insert(twin.clone()); + match self + .block_on_bounded(self.drive.trash_nodes(std::slice::from_ref(&twin))) + .and_then(batch::into_unit) + { + Ok(()) => {} + Err(e) if is_gone(&e) => {} + Err(e) => return Err(e.into()), + } + self.invalidate_trash(); + info!(%uid, %twin, name, "a withdrawn create had landed unanswered; trashed it"); + } + None => debug!(%uid, name, "withdrawn create never landed; trash op satisfied"), + } + self.retire_trash_op(op, &uid)?; return Ok(()); } match self @@ -896,7 +918,7 @@ impl Core { } Err(e) => return Err(e.into()), } - self.db.complete_trash_op(op.id, &uid)?; + self.retire_trash_op(op, &uid)?; if let Err(e) = self.db.clear_own_sealed_rev(&uid.to_string()) { debug!(%uid, error = %e, "clearing the sealed-revision record failed"); } @@ -906,6 +928,82 @@ impl Core { Ok(()) } + /// Remove a trash op that landed, and the blob of a create it withdrew. + fn retire_trash_op( + &self, + op: &PendingOp, + uid: &NodeUid, + ) -> Result<(), Box> { + if let Some(blob) = self.db.complete_trash_op(op.id, uid)? { + self.cache.discard_staged(Path::new(&blob)); + } + Ok(()) + } + + /// 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. + 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 uids = + 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()), + }; + let children = self.block_on_bounded(self.drive.enumerate_nodes_light(&uids))?; + let Some(twin) = children + .into_iter() + .find(|node| node.name == name && !node.trashed) + else { + return Ok(None); + }; + let queued = twin.uid.to_string(); + for kind in [OP_RENAME, OP_REVISION, OP_TRASH] { + if self.db.has_pending_op(&queued, kind)? { + return Ok(None); + } + } + let is_dir = match twin.kind { + // A folder made by a create that never answered has nothing in + // it: what was queued inside waited for its uid. + NodeKind::Folder => { + let inside = self + .block_on_bounded(self.drive.enumerate_folder_children_node_uids(&twin.uid))?; + if !inside.is_empty() { + return Ok(None); + } + true + } + NodeKind::File { .. } => false, + }; + let twin = match is_dir { + true => twin, + false => match self.fetch_node_remote(&twin.uid)? { + Some(node) => node, + None => return Ok(None), + }, + }; + // A blob gone from staging matches nothing, and leaves the node be. + let sent = match op.blob_path.as_deref() { + Some(blob) if node_size(&twin) > 0 => staged_sha1(Path::new(blob)).ok(), + _ => None, + }; + Ok(adoptable(is_dir, &twin, op.created_at, sent.as_deref()).then_some(twin.uid)) + } + /// Record the name the remote actually gave a node, after a conflict forced /// it away from the one the user asked for. Best effort: the event sync /// would correct the tree anyway, but not before the user has looked at it. @@ -956,6 +1054,9 @@ impl Core { } let parent = parse_node_uid(parent_str).ok_or("create op has an unparseable parent")?; let wanted = op.name.clone().ok_or("create op has no name")?; + if self.db.has_withdrawn_create(parent_str, &wanted)? { + return Err(held_by_queued_change(&wanted)); + } // Someone else may have taken the name while this sat in the queue — // reachable only because the op waited, which is the whole point of the @@ -968,14 +1069,14 @@ 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 adopted = false; + let mut blob_landed = true; let mut real = self.create_drained_node(op, &parent, &name); if real.as_ref().is_err_and(|e| is_already_exists(e.as_ref())) - && let Some(twin) = self.adoptable_twin(op, &parent, &wanted)? + && let Some((twin, holds_blob)) = self.adoptable_twin(op, &parent, &wanted)? { info!(%local, %twin, wanted, "the name is held by our own unanswered create; adopting it"); real = Ok(twin); - adopted = true; + blob_landed = holds_blob; } if real.as_ref().is_err_and(|e| is_already_exists(e.as_ref())) && self.name_is_held(op, Some(&parent), &wanted)? @@ -1036,9 +1137,9 @@ impl Core { // Retire the op before touching anything else: if we crash here the node // exists remotely and the local placeholder is reconciled by the event // sync, whereas a surviving op would create the file a second time. - // An adopted node was made empty, so a blob this op carries has not been + // An adopted node made empty has not had a blob this op carries // uploaded: it goes on as a revision of the node instead. - let uploaded = (!adopted).then_some(op.blob_path.as_deref()).flatten(); + let uploaded = blob_landed.then_some(op.blob_path.as_deref()).flatten(); let landing = CreateLanding { local: &local.to_string(), real: &real.to_string(), @@ -1167,7 +1268,8 @@ impl Core { } /// The node already holding `name` under `parent`, if it is the one an - /// earlier attempt at this op made before its answer was lost. + /// earlier attempt at this op made before its answer was lost, and whether + /// it holds the op's blob already. /// /// A create that timed out may still have landed; the op was queued because /// the caller never heard back. Without this, its replay forked a @@ -1177,7 +1279,7 @@ impl Core { op: &PendingOp, parent: &NodeUid, name: &str, - ) -> Result, Box> { + ) -> Result, Box> { // A second file queued under the same name, after the first was // renamed away, finds the first one's node fresh and empty while its // create is on the wire. Taking it put both files on one node, and the @@ -1205,8 +1307,7 @@ impl Core { if self.state().by_uid.contains_key(&twin.uid) { return Ok(None); } - // The light listing carries no file size, and an adoptable file is - // exactly an empty one. + // The light listing carries no file size or digest. let twin = match twin.kind { NodeKind::Folder => twin, NodeKind::File { .. } => match self.fetch_node_remote(&twin.uid)? { @@ -1214,7 +1315,16 @@ impl Core { None => return Ok(None), }, }; - Ok(adoptable(op.kind == OP_MKDIR, &twin, op.created_at).then_some(twin.uid)) + // A create uploads its bytes in the same call, so the node a lost + // answer leaves holds them (docs/BUGS.md B127). + let sent = match op.blob_path.as_deref() { + Some(blob) if node_size(&twin) > 0 => Some(staged_sha1(Path::new(blob))?), + _ => None, + }; + Ok( + adoptable(op.kind == OP_MKDIR, &twin, op.created_at, sent.as_deref()) + .then(|| (twin.uid, sent.is_some())), + ) } /// Make one queued `create`/`mkdir` real under a given name, and hand back @@ -2323,17 +2433,35 @@ const ADOPT_WINDOW_MS: i64 = 2 * 60 * 1000; /// Whether `twin`, found holding the name a queued create wants, is the node an /// earlier, unanswered attempt at that create made: the same kind, created /// around when the op was queued, and for a file still empty, as the mount's -/// create leaves it. -fn adoptable(is_dir: bool, twin: &Node, op_created_ms: i64) -> bool { +/// create leaves it, or holding exactly the bytes the op uploads, whose SHA-1 +/// is `sent`. +fn adoptable(is_dir: bool, twin: &Node, op_created_ms: i64, sent: Option<&str>) -> bool { if twin.creation_time.saturating_mul(1000) < op_created_ms - ADOPT_WINDOW_MS { return false; } match &twin.kind { NodeKind::Folder => is_dir, - NodeKind::File { claimed_size, .. } => !is_dir && *claimed_size == Some(0), + NodeKind::File { + claimed_size, + content_sha1, + .. + } => { + !is_dir + && (*claimed_size == Some(0) + || sent.is_some_and(|sent| { + content_sha1 + .as_deref() + .is_some_and(|sha| sha.eq_ignore_ascii_case(sent)) + })) + } } } +/// The lowercase-hex SHA-1 of a staged blob, as Drive reports a file's. +fn staged_sha1(blob: &Path) -> Result> { + Ok(sha1_of(&mut File::open(blob)?)?) +} + /// What share of the cache budget an unpinned upload may take and still be kept /// as cached content once it lands: a sixteenth, 320 MiB of the default 5 GiB. /// A larger file would push out much of what the user read lately, for bytes @@ -2484,6 +2612,13 @@ mod tests { uid: "local~never-landed".to_string(), ..pending(OP_TRASH) }; + let authorities = pending_op_authorities(&withdrawn).unwrap(); + assert_eq!(authorities.len(), 1); + assert_eq!(authorities[0].to_string(), "v~parent"); + let withdrawn = PendingOp { + parent_uid: None, + ..withdrawn + }; assert!(pending_op_authorities(&withdrawn).unwrap().is_empty()); } @@ -2812,21 +2947,63 @@ mod tests { node.creation_time = secs; node }; - assert!(adoptable(false, &made(file_node(0, 0, None), 990), queued)); - assert!(adoptable(true, &made(folder_node("d", None), 990), queued)); + assert!(adoptable( + false, + &made(file_node(0, 0, None), 990), + queued, + None + )); + assert!(adoptable( + true, + &made(folder_node("d", None), 990), + queued, + None + )); // Made long before the op: someone else's file, not our lost create. - assert!(!adoptable(false, &made(file_node(0, 0, None), 800), queued)); - // Our create leaves a file empty; one with content is someone else's. - assert!(!adoptable(false, &made(file_node(0, 5, None), 990), queued)); + assert!(!adoptable( + false, + &made(file_node(0, 0, None), 800), + queued, + None + )); + // Our create leaves a file empty; one with other content is someone + // else's. + assert!(!adoptable( + false, + &made(file_node(0, 5, None), 990), + queued, + None + )); // The kind has to match what the op makes. - assert!(!adoptable(true, &made(file_node(0, 0, None), 990), queued)); + assert!(!adoptable( + true, + &made(file_node(0, 0, None), 990), + queued, + None + )); assert!(!adoptable( false, &made(folder_node("d", None), 990), - queued + queued, + None )); } + #[test] + fn a_twin_holding_the_bytes_the_create_sent_is_adopted() { + // The create uploaded its bytes and lost the answer (B127). + let queued = 1_000_000; + let mut twin = file_node(0, 5, None); + twin.creation_time = 990; + if let NodeKind::File { content_sha1, .. } = &mut twin.kind { + *content_sha1 = Some("AA55".into()); + } + assert!(adoptable(false, &twin, queued, Some("aa55"))); + assert!(!adoptable(false, &twin, queued, Some("bb66"))); + assert!(!adoptable(false, &twin, queued, None)); + assert!(!adoptable(true, &twin, queued, Some("aa55"))); + } + #[test] fn a_landed_upload_stays_cached_unless_it_would_crowd_the_cache_out() { const GIB: u64 = 1 << 30; diff --git a/crates/pdfs-fuse/src/lib.rs b/crates/pdfs-fuse/src/lib.rs index 407aa48..3b5c757 100644 --- a/crates/pdfs-fuse/src/lib.rs +++ b/crates/pdfs-fuse/src/lib.rs @@ -1851,9 +1851,11 @@ impl Core { restored += 1; // Only a revision must have a blob. A create carries none until // something is written to it (`touch` offline is a legitimate op - // with nothing to serve), and a rename or trash never has one. All - // still have to be replayed, so only the blob — if any — is checked. - if op.blob_path.is_none() && op.kind != OP_REVISION { + // with nothing to serve), and a rename never has one. A trash keeps + // the blob of a create it withdrew only to find the node by, not + // to serve. All still have to be replayed, so only the blob — if + // any — is checked. + if op.kind == OP_TRASH || (op.blob_path.is_none() && op.kind != OP_REVISION) { continue; } let parsed = op diff --git a/crates/pdfs-fuse/src/sim/daemon.rs b/crates/pdfs-fuse/src/sim/daemon.rs index d33b90b..d234343 100644 --- a/crates/pdfs-fuse/src/sim/daemon.rs +++ b/crates/pdfs-fuse/src/sim/daemon.rs @@ -476,6 +476,65 @@ mod tests { 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() { + // Drive made the file, but its answer was lost. The retry found the + // name taken by a node holding the file's bytes, which was not empty + // and so not adopted, and uploaded the file again as a conflict copy + // (B127). + let drive = FakeDrive::new(); + let dir = scratch("lost-create-reply"); + 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"once").unwrap(); + assert!( + wait_until(Duration::from_secs(30), || { + daemon.pending().is_ok_and(|items| items.is_empty()) + && drive.tree().get("f.txt").cloned() == file(b"once") + }), + "{:?}", + 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_deleted_after_its_create_lost_its_answer_leaves_drive() { + // Drive made the file, but its answer was lost, and the file was + // deleted before the retry. The delete dropped the queued create and + // left the node Drive had made in place (B151). + let drive = FakeDrive::new(); + let dir = scratch("lost-create-reply-deleted"); + let daemon = Daemon::start(&dir, drive.client(1, Faults::lan())).unwrap(); + let path = daemon.mountpoint.join("f.txt"); + + daemon.client.lose_reply_to_next_create(); + std::fs::write(&path, b"gone").unwrap(); + assert!(wait_until(Duration::from_secs(30), || { + drive.tree().contains_key("f.txt") + })); + std::fs::remove_file(&path).unwrap(); + 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_pdfs_rename_after_a_queued_rename_is_not_undone() { diff --git a/crates/pdfs-fuse/src/sim/fake_drive.rs b/crates/pdfs-fuse/src/sim/fake_drive.rs index e9ed5c2..17feb05 100644 --- a/crates/pdfs-fuse/src/sim/fake_drive.rs +++ b/crates/pdfs-fuse/src/sim/fake_drive.rs @@ -331,6 +331,7 @@ impl FakeDrive { requests: AtomicU64::new(0), hold: Mutex::new(None), drop_after_create: AtomicBool::new(false), + lose_create_reply: AtomicBool::new(false), })) } @@ -580,6 +581,8 @@ struct ClientInner { hold: Mutex>>, /// Set by [`FakeClient::drop_link_after_next_create`]. drop_after_create: AtomicBool, + /// Set by [`FakeClient::lose_reply_to_next_create`]. + lose_create_reply: AtomicBool, } struct HoldState { @@ -625,6 +628,12 @@ impl FakeClient { self.0.drop_after_create.store(true, Ordering::SeqCst); } + /// Make the next file create on Drive and lose its answer, as a link that + /// drops it on the way back does. + pub(crate) fn lose_reply_to_next_create(&self) { + self.0.lose_create_reply.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 { @@ -1061,6 +1070,9 @@ impl DriveApi for FakeClient { if made.is_ok() && self.0.drop_after_create.swap(false, Ordering::SeqCst) { self.set_online(false); } + if made.is_ok() && self.0.lose_create_reply.swap(false, Ordering::SeqCst) { + return Err(no_answer()); + } made } diff --git a/crates/pdfs-fuse/src/sim/run.rs b/crates/pdfs-fuse/src/sim/run.rs index 7f901b8..374c2a7 100644 --- a/crates/pdfs-fuse/src/sim/run.rs +++ b/crates/pdfs-fuse/src/sim/run.rs @@ -363,7 +363,11 @@ impl Run { fn settle(&mut self, budget: Duration) -> Result<(), String> { self.note("settle".into()); let start = Instant::now(); - if !wait_until(budget, || self.landed()) { + // What the drain logs meanwhile is what explains a known bug's damage. + if !wait_until(budget, || { + self.logged.extend(take_logged()); + self.landed() + }) { return Err(format!( "liveness: the queues did not drain within {budget:?}\n{}", self.difference() diff --git a/crates/pdfs-fuse/src/takeout.rs b/crates/pdfs-fuse/src/takeout.rs index f94f1a4..43e8a5b 100644 --- a/crates/pdfs-fuse/src/takeout.rs +++ b/crates/pdfs-fuse/src/takeout.rs @@ -530,7 +530,7 @@ fn open_archive( /// The lowercase-hex SHA-1 of everything `reader` yields — the digest Proton's /// photo duplicate detection is keyed on. -fn sha1_of(reader: &mut (dyn Read + Send)) -> CoreResult { +pub(crate) fn sha1_of(reader: &mut (dyn Read + Send)) -> CoreResult { let mut hasher = Sha1::new(); let mut buffer = vec![0u8; 256 * 1024]; loop { diff --git a/docs/BUGS.md b/docs/BUGS.md index 4fa4a45..c6d1bd0 100644 --- a/docs/BUGS.md +++ b/docs/BUGS.md @@ -12,6 +12,29 @@ Conventions: --- +## B151 — A file deleted after its create lost its answer stays on Drive + +**Status:** Fixed (unverified). +**Found:** 2026-10-04, by the simulation's Wi-Fi profile in CI (`one_client_on_a_slow_link`, +seed 2). `x/a.txt` was made, written and deleted within a third of a second. The queues never +drained, and Drive kept `x/a.txt` with the bytes of its create. + +**Where:** `drop_doomed_ops` in `crates/pdfs-core/src/db/ops.rs`, and `Core::drain_trash` in +`crates/pdfs-fuse/src/drain.rs`. + +**Cause.** A create that times out may still have made the file; only its answer was lost (as in +B127). The create stays queued with a backoff. A delete before the retry dropped the queued create +as if nothing had been sent, so the node Drive had made kept the deleted file's name and bytes. +Only a create on the wire at the time of the delete was turned into a trash (B129), and when that +create failed, the trash took it as never made. + +**Fix.** A create that was sent, on the wire or before, becomes a trash that keeps the create's +folder and blob. The drain looks in that folder for a node under the create's name that the +create would have adopted (B127): made around when it was queued, and empty or holding the +blob's bytes. It trashes that node, and then discards the blob. A new create of the same name in +that folder waits for the trash. Sim test +`a_file_deleted_after_its_create_lost_its_answer_leaves_drive`. + ## B150 — A conflict copy whose folder is gone retries forever **Status:** Fixed (unverified). @@ -559,7 +582,7 @@ profile fails on it when it comes up, which depends on timing. ## B127 — A file created offline lands twice when the answer to its upload is lost -**Status:** Open. +**Status:** Fixed (unverified). **Found:** 2026-10-03, by the simulation runs (`sim::run`, profile one-client-flaky, seed 1). The upload of a queued create timed out after Drive had made the file. On the next attempt the drain logged "name is taken remotely; creating under a conflict name". Drive ended with the file @@ -573,6 +596,10 @@ and, for a file, be empty. A queued create uploads its bytes in the same call, t (`upload_created_file`). So the node a lost answer leaves behind holds those bytes, is not adopted, and the file is uploaded a second time under a conflict name. +**Fix.** A file is also adopted when Drive's SHA-1 of its content is that of the op's blob. The +blob then counts as uploaded. Sim test `a_create_whose_answer_was_lost_lands_once`. A write that +replaced the blob after the lost upload still forks a copy: the node holds the older bytes. + **Test:** the simulation runs count it and go on. With `PDFS_SIM_KNOWN=fail` the flaky profile fails on it when it comes up, which depends on timing. diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 4363f9e..0a5c792 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -33,6 +33,10 @@ folder by local id too. for this release. ### Fixed +- **A file deleted right after it was made no longer stays on Drive when the link lost Drive's + answer.** The node Drive made is trashed (B151). +- **A new file no longer lands twice when the link loses Drive's answer to its upload.** The + file Drive made is taken as it is, instead of a second upload as a conflict copy (B127). - **A change to a file deleted elsewhere, together with its folder, no longer retries forever.** The conflict copy that keeps it now lands in the location's root (B150). - **`pdfs rename` and `pdfs move` right after a change through the mount are no longer undone.** diff --git a/docs/MILESTONE-3.0.0-STATUS.md b/docs/MILESTONE-3.0.0-STATUS.md index 408c2f5..25e7110 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: B141 to B150. +- Fixes from the simulation and account runs since then: B127 and B141 to B151. - **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`. @@ -43,12 +43,14 @@ Tracks `docs/MILESTONE-3.0.0.md`. Update it with every commit that moves a phase and 100 folders on Wi-Fi against LAN (target: within 10 %). - [x] The measurement: `a_thousand_files_drain_as_fast_on_wifi_as_on_lan` in `sim/daemon.rs`, run with `PDFS_SIM_MEASURE=1`. It prints the times and asserts nothing yet. - - [ ] Baseline numbers, taken once the account run is done. + - [x] Baseline on eb1c105, three drain threads: the syscalls take 3.7 s on LAN and 4.3 s on + Wi-Fi; the drain takes 138 s on LAN and 938 s on Wi-Fi, 6.8 times as long. 4. **Done-when checks**: - - [ ] Simulation passes on the Wi-Fi profile: `one_client_on_a_slow_link` is added, not yet - run. - - [ ] Known-bug list in `sim/run.rs` is empty: B125 and B130 open; B129 and B132 fixed but - unconfirmed. + - [ ] Simulation passes on the Wi-Fi profile: `one_client_on_a_slow_link` failed in CI on + seeds 2 and 3. Two bugs (B127, B151) and a harness gap: a settle did not read the log, so + it could not explain a known bug's damage. All three fixed; not yet confirmed in CI. + - [ ] Known-bug list in `sim/run.rs` is empty: B125 and B130 open; B127, B129 and B132 fixed + but unconfirmed. - [ ] 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