diff --git a/crates/pdfs-fuse/src/drain.rs b/crates/pdfs-fuse/src/drain.rs index 4d03938..2ef2dae 100644 --- a/crates/pdfs-fuse/src/drain.rs +++ b/crates/pdfs-fuse/src/drain.rs @@ -1752,6 +1752,22 @@ impl Core { debug!(%uid, ?remote_rev, "the remote already holds the queued write; not a conflict"); return Ok(None); } + // An upload of ours landed and its read-back went unanswered, so its + // revision id was never recorded as ours. The bytes it sent say the + // same: the remote holds them, so no other device wrote since + // (docs/BUGS.md B157). + let sealed = self.sealed_unread.lock().get(uid).cloned(); + if meta.complete + && let Some(sealed) = sealed + && let NodeKind::File { + content_sha1: Some(remote), + .. + } = &node.kind + && remote.eq_ignore_ascii_case(&sealed) + { + debug!(%uid, ?remote_rev, "queued write chains onto our own unread revision; not a conflict"); + return Ok(None); + } Ok(Some(reason)) } @@ -2178,13 +2194,11 @@ impl Core { // B119). Adopted by link, not by value: the blob is the whole file, and // a video read into a `Vec` here is an OOM. `discard_staged` below drops // the staging name only โ€” the cache keeps the inode. - self.refresh_after_upload(&uid, &blob, |node| { - if node_size(node) == meta.len + self.refresh_after_upload(&uid, &blob, |mtime, size| { + if size == meta.len && keeps_landed_upload(self.cache.is_pinned(&uid), meta.len, self.cache.budget()) { - let _ = self - .cache - .store_file(&uid, node.modification_time, meta.len, &blob); + let _ = self.cache.store_file(&uid, mtime, meta.len, &blob); } }); self.cache.discard_staged(&blob); @@ -2222,11 +2236,16 @@ impl Core { /// that conflicted with itself and left a full-size duplicate behind. /// /// `blob` is the revision that landed, which stays the pending one until - /// open write handles are rebased off it, and `adopt` gets the node read - /// back before that, so the uploaded bytes can be cached under its - /// revision first. A handle released in between fills its gaps from one - /// or the other; with the blob let go first and the cache filled last, it - /// found neither, and the next write open was refused (docs/BUGS.md B123). + /// open write handles are rebased off it, and `adopt` gets the mtime and + /// size of the node read back before that, so the uploaded bytes can be + /// cached under its revision first. A handle released in between fills its + /// gaps from one or the other; with the blob let go first and the cache + /// filled last, it found neither, and the next write open was refused + /// (docs/BUGS.md B123). When the node cannot be read back, `adopt` gets + /// the stamp the tree still has, which is the base a write opened next + /// takes: without it the blob went and nothing replaced it (B157). The + /// blob's digest then stands in for the revision id the read-back did + /// not bring, so the next write knows the revision as its own. /// /// Best effort: a failure here costs a spurious conflict copy on the next /// write, not this upload, which has already landed. Returns the node as @@ -2235,25 +2254,50 @@ impl Core { &self, uid: &NodeUid, blob: &Path, - adopt: impl FnOnce(&Node), + adopt: impl FnOnce(i64, u64), ) -> Option { let fetched = self.fetch_node_remote(uid); - if !matches!(fetched, Ok(Some(_))) { - self.release_pending(uid, blob); + if fetched.is_ok() { + self.sealed_unread.lock().remove(uid); } let node = match fetched { Ok(Some(node)) => node, // Trashed or deleted under us: the tree will hear it from the event // sync, which is better placed to unhook the inode than we are. - Ok(None) => return None, + Ok(None) => { + self.release_pending(uid, blob); + return None; + } Err(e) => { warn!(%uid, error = %e, "refreshing metadata after an upload failed; \ the next write to this file may conflict with itself"); + match staged_sha1(blob) { + Ok(sha) => { + self.sealed_unread.lock().insert(uid.clone(), sha); + } + Err(_) => { + self.sealed_unread.lock().remove(uid); + } + } + // No newer write took over, so the tree's stamp is this upload's. + if self.pending_blob(uid).as_deref() == Some(blob) { + let mut stamp = None; + self.for_each_state(|st| { + if let Some(entry) = st.by_uid.get(uid).and_then(|ino| st.entries.get(ino)) + { + stamp = Some((entry.node.modification_time, node_size(&entry.node))); + } + }); + if let Some((mtime, size)) = stamp { + adopt(mtime, size); + } + } + self.release_pending(uid, blob); return None; } }; - adopt(&node); + adopt(node.modification_time, node_size(&node)); // This function's whole job is to bring the tree level with the revision // we just sealed, so the feed's report of that revision has nothing left // to tell us. Claimed here rather than at the upload call so it is only diff --git a/crates/pdfs-fuse/src/lib.rs b/crates/pdfs-fuse/src/lib.rs index 8ab021b..0d97040 100644 --- a/crates/pdfs-fuse/src/lib.rs +++ b/crates/pdfs-fuse/src/lib.rs @@ -576,6 +576,9 @@ struct Core { /// 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>>, /// 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 602b8a2..6faf00d 100644 --- a/crates/pdfs-fuse/src/mount.rs +++ b/crates/pdfs-fuse/src/mount.rs @@ -435,6 +435,7 @@ pub(crate) fn mount_with( 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())), 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 9fadef9..a57b93a 100644 --- a/crates/pdfs-fuse/src/sim/daemon.rs +++ b/crates/pdfs-fuse/src/sim/daemon.rs @@ -358,6 +358,46 @@ 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_whose_upload_was_not_read_back_takes_two_writes_at_once() { + // The read-back after an upload went unanswered, and the drain let go of + // the uploaded bytes without caching them. A partial write had nothing + // to fill its gaps from, and the next open was refused with EIO (B157). + let drive = FakeDrive::new(); + let dir = scratch("upload-not-read-back"); + 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()))); + + std::fs::write(&path, b"first").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"first") + })); + daemon.client.lose_read_back_of_next_revision(); + std::fs::write(&path, b"0123456789").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"0123456789") + })); + write_at(&path, 2, b"ab").unwrap(); + write_at(&path, 6, b"cd").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"01ab45cd89") + }), + "{:?} {:?}", + 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 the_echo_of_our_own_rename_keeps_the_cached_bytes() { diff --git a/crates/pdfs-fuse/src/sim/fake_drive.rs b/crates/pdfs-fuse/src/sim/fake_drive.rs index 8ca1a09..5fc5a9e 100644 --- a/crates/pdfs-fuse/src/sim/fake_drive.rs +++ b/crates/pdfs-fuse/src/sim/fake_drive.rs @@ -334,6 +334,8 @@ impl FakeDrive { lose_create_reply: AtomicBool::new(false), hold_create_reply: Mutex::new(None), lose_revision_reply: AtomicBool::new(false), + lose_revision_read_back: AtomicBool::new(false), + lose_read: Mutex::new(None), })) } @@ -589,6 +591,10 @@ struct ClientInner { hold_create_reply: Mutex>>, /// Set by [`FakeClient::lose_reply_to_next_revision`]. lose_revision_reply: AtomicBool, + /// Set by [`FakeClient::lose_read_back_of_next_revision`]. + lose_revision_read_back: AtomicBool, + /// The file whose next read goes unanswered, once its revision is made. + lose_read: Mutex>, } struct HoldState { @@ -678,6 +684,13 @@ impl FakeClient { self.0.lose_revision_reply.store(true, Ordering::SeqCst); } + /// Make the next revision of a file on Drive and answer it, then lose the + /// answer to the next read of that file alone, as the read-back after an + /// upload meets on a link that drops it. + pub(crate) fn lose_read_back_of_next_revision(&self) { + self.0.lose_revision_read_back.store(true, Ordering::SeqCst); + } + /// Wait here if `uids` is the read a [`Held`] is for. async fn wait_if_held(&self, uids: &[NodeUid]) { let state = { @@ -916,6 +929,16 @@ impl DriveApi for FakeClient { async fn enumerate_nodes(&self, uids: &[NodeUid]) -> Result> { self.wait_if_held(uids).await; + { + let mut lose = self.0.lose_read.lock(); + if lose + .as_ref() + .is_some_and(|uid| uids == std::slice::from_ref(uid)) + { + *lose = None; + return Err(no_answer()); + } + } let lag = self.lag(); let nodes: Vec = self .request(false, |server| { @@ -1168,6 +1191,9 @@ impl DriveApi for FakeClient { if self.0.lose_revision_reply.swap(false, Ordering::SeqCst) { return Err(no_answer()); } + if self.0.lose_revision_read_back.swap(false, Ordering::SeqCst) { + *self.0.lose_read.lock() = Some(file_uid.clone()); + } Ok(()) } diff --git a/docs/BUGS.md b/docs/BUGS.md index 6f3d126..fcf1d82 100644 --- a/docs/BUGS.md +++ b/docs/BUGS.md @@ -12,6 +12,36 @@ Conventions: --- +## B157 โ€” A second write to a file whose upload was not read back fails with `EIO` + +**Status:** Fixed (unverified). +**Found:** 2026-10-04, by the simulation's Wi-Fi profile (`one_client_on_a_slow_link`, seed 4, +every replay on 16fca10, none on dbc4d9a: sixteen drain workers changed the timing). After an +upload of `c.bin` landed, the read-back of the node went unanswered. A write at an offset +followed, and the next write to the file failed with `EIO`. The daemon logged "refusing write +over incomplete queued revision". + +**Where:** `Core::refresh_after_upload` and `Core::revision_conflict` in +`crates/pdfs-fuse/src/drain.rs`. + +**Cause.** The uploaded bytes are cached under the revision the read-back returns (B123). With no +read-back, the drain let go of the staged blob and cached nothing. A partial write opened next had +nothing to fill its gaps from, so it queued an incomplete revision, and a write open over one is +refused. Had it been filled, the write would have landed as a conflict copy of itself: the +revision id of the upload was never recorded as ours (`own_sealed_rev`). + +**Fix.** When the read-back fails, the uploaded bytes are cached under the size and mtime the +tree still has for the file, which is the base a write opened next takes, unless a newer write +has taken over. The SHA-1 of those bytes is kept in memory, and a queued write that finds the +remote holding them chains onto it instead of forking. After a restart the write still forks a +conflict copy, which loses nothing. + +**Test:** sim test `a_file_whose_upload_was_not_read_back_takes_two_writes_at_once`, with the +fake Drive's new `lose_read_back_of_next_revision`. Without the fix the second write fails with +`EIO`; with only the cache fix, it lands as a conflict copy. + +--- + ## B156 โ€” A file written again after its create lost its answer lands as a conflict copy **Status:** Fixed (unverified). diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index f54524a..82c493f 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -42,6 +42,8 @@ by its local id, in the queue, the stored tree and pins alike. 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). +- **Writing to a file twice in a row no longer fails with "Input/output error" when the link + lost Drive's answer to reading back the file's last upload** (B157). - **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 10991eb..13de481 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 B156. +- Fixes from the simulation and account runs since then: B127 and B141 to B157. - **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`. @@ -79,7 +79,9 @@ Tracks `docs/MILESTONE-3.0.0.md`. Update it with every commit that moves a phase 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. Replays of slow-link seed 4 failed two of six times on a file written - again after its create lost its answer (B156, fixed). + again after its create lost its answer (B156, fixed). With sixteen drain workers + (16fca10), seed 4 failed every replay on a write after an upload that was not read + back (B157, 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)