From 85ec91d6b3003060427b6d9b27a367fefc6bc376 Mon Sep 17 00:00:00 2001 From: Nils Pukropp Date: Sun, 4 Oct 2026 13:14:29 +0200 Subject: [PATCH] fix(fuse): adopt a twin holding bytes an earlier attempt of the create sent (B156) --- crates/pdfs-fuse/src/drain.rs | 100 +++++++++++++++++-------- crates/pdfs-fuse/src/lib.rs | 4 + crates/pdfs-fuse/src/mount.rs | 1 + crates/pdfs-fuse/src/sim/daemon.rs | 35 +++++++++ crates/pdfs-fuse/src/sim/fake_drive.rs | 6 +- docs/BUGS.md | 20 +++++ docs/CHANGELOG.md | 2 + docs/MILESTONE-3.0.0-STATUS.md | 5 +- 8 files changed, 135 insertions(+), 38 deletions(-) diff --git a/crates/pdfs-fuse/src/drain.rs b/crates/pdfs-fuse/src/drain.rs index 98b446d..d7b985a 100644 --- a/crates/pdfs-fuse/src/drain.rs +++ b/crates/pdfs-fuse/src/drain.rs @@ -927,6 +927,7 @@ 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 @@ -1018,11 +1019,12 @@ impl Core { }, }; // A blob gone from staging matches nothing, and leaves the node be. - let sent = match op.blob_path.as_deref() { + let current = 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)) + let sent = self.create_sent_digests(op.id, current); + Ok(adoptable(is_dir, &twin, op.created_at, &sent).then_some(twin.uid)) } /// Record the name the remote actually gave a node, after a conflict forced @@ -1062,6 +1064,11 @@ 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 } @@ -1346,15 +1353,32 @@ impl Core { }, }; // 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() { + // answer leaves holds them (docs/BUGS.md B127): the ones the op holds + // now, or ones a write has replaced since (B156), which still go up. + let current = 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())), - ) + let holds_blob = current + .as_deref() + .is_some_and(|sha| holds_bytes(&twin, sha)); + let sent = self.create_sent_digests(op.id, current); + Ok(adoptable(op.kind == OP_MKDIR, &twin, op.created_at, &sent) + .then_some((twin.uid, holds_blob))) + } + + /// The SHA-1s of the bytes queued create `id` may have made its file with: + /// `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(); + sent.extend(current); + sent } /// Make one queued `create`/`mkdir` real under a given name, and hand back @@ -1402,6 +1426,15 @@ impl Core { if !meta.complete { return Err("queued create holds an incomplete blob".into()); } + // 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); let thumbnails = self.upload_thumbnails(Path::new(blob), name); let guard = self .transfers @@ -2503,30 +2536,26 @@ 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, 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 { +/// create leaves it, or holding exactly bytes the op uploaded, whose SHA-1 is +/// one of `sent`. +fn adoptable(is_dir: bool, twin: &Node, op_created_ms: i64, sent: &[String]) -> 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, - 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)) - })) + NodeKind::File { claimed_size, .. } => { + !is_dir && (*claimed_size == Some(0) || sent.iter().any(|sha| holds_bytes(twin, sha))) } } } +/// Whether `node` is a file whose content has SHA-1 `sha`. +fn holds_bytes(node: &Node, sha: &str) -> bool { + matches!(&node.kind, NodeKind::File { content_sha1: Some(have), .. } + if have.eq_ignore_ascii_case(sha)) +} + /// 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)?)?) @@ -3033,20 +3062,20 @@ mod tests { 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, - None + &[] )); // Our create leaves a file empty; one with other content is someone // else's. @@ -3054,20 +3083,20 @@ mod tests { 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, - None + &[] )); assert!(!adoptable( false, &made(folder_node("d", None), 990), queued, - None + &[] )); } @@ -3080,10 +3109,15 @@ mod tests { 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"))); + let sent = |shas: &[&str]| shas.iter().map(|sha| sha.to_string()).collect::>(); + assert!(adoptable(false, &twin, queued, &sent(&["aa55"]))); + assert!(!adoptable(false, &twin, queued, &sent(&["bb66"]))); + assert!(!adoptable(false, &twin, queued, &[])); + assert!(!adoptable(true, &twin, queued, &sent(&["aa55"]))); + // An attempt sent these bytes before a write replaced them (B156). + assert!(adoptable(false, &twin, queued, &sent(&["aa55", "bb66"]))); + assert!(holds_bytes(&twin, "aa55")); + assert!(!holds_bytes(&twin, "bb66")); } #[test] diff --git a/crates/pdfs-fuse/src/lib.rs b/crates/pdfs-fuse/src/lib.rs index 7cdd86f..1c704de 100644 --- a/crates/pdfs-fuse/src/lib.rs +++ b/crates/pdfs-fuse/src/lib.rs @@ -573,6 +573,10 @@ 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>>>, /// 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. diff --git a/crates/pdfs-fuse/src/mount.rs b/crates/pdfs-fuse/src/mount.rs index c87c318..602b8a2 100644 --- a/crates/pdfs-fuse/src/mount.rs +++ b/crates/pdfs-fuse/src/mount.rs @@ -434,6 +434,7 @@ 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())), unadopted: Arc::new(Mutex::new(HashMap::new())), drain_wake: Arc::new((Mutex::new(false), Condvar::new())), shutdown: Arc::new(crate::shutdown::Shutdown::default()), diff --git a/crates/pdfs-fuse/src/sim/daemon.rs b/crates/pdfs-fuse/src/sim/daemon.rs index 6ae2eab..9fadef9 100644 --- a/crates/pdfs-fuse/src/sim/daemon.rs +++ b/crates/pdfs-fuse/src/sim/daemon.rs @@ -504,6 +504,41 @@ 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_written_again_after_its_create_lost_its_answer_lands_once() { + // Drive made the file, but its answer was lost, and the file was + // written again before the retry. The retry found the name taken by a + // node holding the bytes the first attempt sent, not the ones the op + // held now, and uploaded the file as a conflict copy (B156). + let drive = FakeDrive::new(); + let dir = scratch("lost-create-reply-rewritten"); + let daemon = Daemon::start(&dir, drive.client(1, Faults::lan())).unwrap(); + let path = daemon.mountpoint.join("f.txt"); + let file = |bytes: &[u8]| Some(Entry::File(Arc::new(bytes.to_vec()))); + + 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.is_empty()) + && drive.tree().get("f.txt").cloned() == file(b"second") + }), + "{:?} {:?}", + daemon.pending(), + drive.tree() + ); + assert_eq!(drive.tree().len(), 1, "{:?}", drive.tree()); + assert_eq!(std::fs::read(&path).unwrap(), b"second"); + + 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() { diff --git a/crates/pdfs-fuse/src/sim/fake_drive.rs b/crates/pdfs-fuse/src/sim/fake_drive.rs index 9d70e6e..8ca1a09 100644 --- a/crates/pdfs-fuse/src/sim/fake_drive.rs +++ b/crates/pdfs-fuse/src/sim/fake_drive.rs @@ -1099,15 +1099,15 @@ 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()); - } let held = self.0.hold_create_reply.lock().take(); if made.is_ok() && let Some(state) = held { wait_until_released(&state).await; } + if made.is_ok() && self.0.lose_create_reply.swap(false, Ordering::SeqCst) { + return Err(no_answer()); + } made } diff --git a/docs/BUGS.md b/docs/BUGS.md index 8327295..6f3d126 100644 --- a/docs/BUGS.md +++ b/docs/BUGS.md @@ -12,6 +12,26 @@ Conventions: --- +## B156 — A file written again after its create lost its answer lands as a conflict copy + +**Status:** Fixed (unverified). +**Found:** 2026-10-04, by the simulation's Wi-Fi profile (`one_client_on_a_slow_link`, seed 4, +two of six replays on e5f3f61). The create of `a.txt` timed out twice, but Drive had made the file +with the bytes of the first write. A later write replaced the blob the queued create carried. The +third attempt found the name taken and created `a (sync-conflict …).txt`; `a.txt` kept the old +bytes. + +**Where:** `Core::adoptable_twin` and `Core::withdrawn_twin` in `crates/pdfs-fuse/src/drain.rs`. + +**Cause.** A twin is adopted when it holds the bytes the create uploads (B127), and only the blob +the op held at the retry was hashed. The twin held the bytes an earlier attempt sent. + +**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 +Drive's held and lost create answers now combine. + ## B155 — A write whose upload lost its answer lands as a conflict copy of itself **Status:** Fixed (unverified). diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 66f3d58..7d8759e 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -38,6 +38,8 @@ by its local id, in the queue, the stored tree and pins alike. no longer lands as a conflict copy, and the mount no longer shows that copy's name (B154). - **A change to a file no longer lands as a conflict copy of itself when the link loses Drive's answer to its upload** (B155). +- **A file written again while its upload was being retried no longer lands as a conflict copy + of itself when the link lost Drive's answer to the first try** (B156). - **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). - **A change that waits for a name to be freed no longer waits seconds after it is.** A rename or diff --git a/docs/MILESTONE-3.0.0-STATUS.md b/docs/MILESTONE-3.0.0-STATUS.md index 4f4dd25..310e2de 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: B127 and B141 to B155. +- Fixes from the simulation and account runs since then: B127 and B141 to B156. - **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`. @@ -64,7 +64,8 @@ Tracks `docs/MILESTONE-3.0.0.md`. Update it with every commit that moves a phase 7269cb5 failed LAN seed 9, a regression from the B153 fix (B154), and Wi-Fi seed 2, a write whose answer was lost (B155). Both fixed. One slow-link run of seed 2 failed once with a rename answering `ENOENT` as a folder move landed. Not reproduced since; - it may be B152. + it may be B152. Replays of slow-link seed 4 failed two of six times on a file written + again after its create lost its answer (B156, fixed). - [ ] 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)