diff --git a/crates/pdfs-core/src/cache.rs b/crates/pdfs-core/src/cache.rs index 225992c..bfa84da 100644 --- a/crates/pdfs-core/src/cache.rs +++ b/crates/pdfs-core/src/cache.rs @@ -621,10 +621,20 @@ impl ContentCache { for entry in rd.flatten() { let name = entry.file_name(); let Some(name) = name.to_str() else { continue }; + // A `.meta` tags the blob named like it without the suffix. One + // whose blob is gone is what an eviction leaves between its two + // removals, by a crash or by a store of the same uid landing in + // between, and nothing else would ever remove it. + if let Some(blob) = name.strip_suffix(".meta") { + if !dir.join(blob).exists() { + let _ = std::fs::remove_file(entry.path()); + } + continue; + } // `.geom` is a block-geometry sidecar (see `GeometryMeta`), tens // of bytes and not a cached block; like `.meta` it is neither // budgeted nor evictable on its own. - if name.ends_with(".meta") || name.ends_with(".geom") { + if name.ends_with(".geom") { continue; } // A `.tmp` is a staging file from a store that never completed @@ -2250,6 +2260,28 @@ mod tests { assert!(total <= 8, "budget enforced after reconcile, total={total}"); } + /// An eviction removes the blob, then its tag. A crash in between, or a + /// store of the same uid landing in between, left the tag on its own, and + /// nothing removed it. + #[test] + fn a_tag_without_its_blob_is_swept_at_open() { + let dir = TempDir::new(); + let content = dir.path().join("content"); + let pins = dir.path().join("pins.json"); + let db = Arc::new(Db::open_in_memory().unwrap()); + { + let c = ContentCache::open(content.clone(), pins.clone(), 0, db.clone()).unwrap(); + c.store(&uid("kept"), 1, 4, b"kept").unwrap(); + c.store(&uid("gone"), 1, 4, b"gone").unwrap(); + std::fs::remove_file(c.blob_path(&uid("gone"))).unwrap(); + } + + let c = ContentCache::open(content, pins, 0, db).unwrap(); + assert!(!c.meta_path(&uid("gone")).exists()); + assert!(c.meta_path(&uid("kept")).exists()); + assert_eq!(c.read_range(&uid("kept"), 1, 4, 0, 4).unwrap(), b"kept"); + } + #[test] fn stale_metadata_is_a_miss() { let (c, _d) = cache(); diff --git a/crates/pdfs-fuse/src/drain.rs b/crates/pdfs-fuse/src/drain.rs index e530787..4af8e75 100644 --- a/crates/pdfs-fuse/src/drain.rs +++ b/crates/pdfs-fuse/src/drain.rs @@ -1281,6 +1281,13 @@ impl Core { sent: (&sent.0, &sent.1), landed: (&home.to_string(), &name), }; + // Counted from the op's retirement until the bytes are cached and the + // tree has the real uid, as an upload is. Uncounted, the queue read + // empty while that still ran: a delete in between took an open file + // for closed and dropped what its handle reads, and a script that + // waited for an empty queue read the file before it was cached + // (`docs/BUGS.md` B257). + let _landing = LandingUpload::begin(&self.landing_uploads); let newer = match self.retire_create(op, uploaded, &landing, &local, &real)? { CreateRetired::Withdrawn { trash } => { return self.trash_withdrawn_create(op, trash, &local, &real, &name); diff --git a/crates/pdfs-fuse/src/filesystem.rs b/crates/pdfs-fuse/src/filesystem.rs index 5d3e443..52d8751 100644 --- a/crates/pdfs-fuse/src/filesystem.rs +++ b/crates/pdfs-fuse/src/filesystem.rs @@ -2368,7 +2368,7 @@ impl ProtonFs { // regardless — the file is gone from their point of view the moment the // command returns. The same when the network goes // away under the remote call below. - let queue_it = |reply: ReplyEmpty| match self.core.queue_trash(&uid, name) { + let queue_it = |reply: ReplyEmpty| match self.core.queue_trash(&uid, name, open_now) { Ok(()) => reply.ok(), Err(e) => reply.error(e), }; diff --git a/crates/pdfs-fuse/src/lib.rs b/crates/pdfs-fuse/src/lib.rs index d9bbb0e..cc5c887 100644 --- a/crates/pdfs-fuse/src/lib.rs +++ b/crates/pdfs-fuse/src/lib.rs @@ -3997,7 +3997,12 @@ impl Core { /// the cached ones, as when the trash is sent at once: a partial write /// fills its gaps from them, offline too (`docs/BUGS.md` B186). The final /// close or the trash landing, whichever comes last, lets go of them. - fn queue_trash(&self, uid: &NodeUid, name: &str) -> Result<(), Errno> { + /// + /// `open` is whether the caller found the file open. A create that landed + /// under a delete is trashed by its real uid before the tree has it, so + /// asking the tree alone took an open file for closed (`docs/BUGS.md` + /// B257). + fn queue_trash(&self, uid: &NodeUid, name: &str, open: bool) -> Result<(), Errno> { self.require_uid_writable(uid)?; // The bytes below are about to be discarded, and a drain worker may be // reading them onto the wire right now. Stop it first: with several @@ -4012,7 +4017,7 @@ impl Core { error!(%uid, error = %e, "queueing trash failed"); Errno::EIO })?; - let open_now = self.is_open_anywhere(uid); + let open_now = open || self.is_open_anywhere(uid); if open_now { let read_from = self.pending_blob(uid); self.release_dropped_below( @@ -5381,9 +5386,12 @@ impl Core { /// `unlink` under an open handle. Its queued ops go now, or the drain would /// upload to the node after it was trashed (`docs/BUGS.md` B106). fn remove_replaced(&self, ino: u64, uid: &NodeUid, name: &str) -> Result<(), Errno> { + // Asked under the uid the tree has now: a landed create's real uid is + // not in the tree yet (`docs/BUGS.md` B257). + let open = self.is_open_anywhere(uid); let landed; let uid = if is_local_uid(uid) { - if self.is_open_anywhere(uid) { + if open { self.withdraw_queued_ops(uid)?; } else { self.discard_queued_ops(uid)?; @@ -5408,7 +5416,7 @@ impl Core { // Local-first or offline, or the network goes away under the call: // queue the trash. The rename that follows queues as well, behind it. if !self.sends_inline() { - return self.queue_trash(uid, name); + return self.queue_trash(uid, name, open); } self.require_uid_writable(uid)?; if let Err(e) = self @@ -5416,7 +5424,7 @@ impl Core { .and_then(batch::into_unit) { if self.lost_link(&e, "trash a replaced node") { - return self.queue_trash(uid, name); + return self.queue_trash(uid, name, open); } error!(%uid, name, error = %e, "trashing the node a rename replaces failed"); self.log_activity(ActivityKind::Trash, name, e.to_string(), false); @@ -9255,7 +9263,7 @@ mod tests { assert!(!replaced.contains("st.forget(uid)")); assert_before( replaced, - "if self.is_open_anywhere(uid)", + "let open = self.is_open_anywhere(uid)", "self.discard_queued_ops(uid)", ); assert_before(replaced, "if open_now", "self.cache.evict(uid)"); diff --git a/crates/pdfs-fuse/src/sim/daemon.rs b/crates/pdfs-fuse/src/sim/daemon.rs index 52c6080..3f352dc 100644 --- a/crates/pdfs-fuse/src/sim/daemon.rs +++ b/crates/pdfs-fuse/src/sim/daemon.rs @@ -130,6 +130,22 @@ impl Daemon { } } + /// Whether nothing is queued and no upload is still landing, as `pdfs + /// status` counts it. An empty [`Self::pending`] is not enough before a + /// read that needs the cache: the op goes before the uploaded bytes are + /// cached, so a test that went offline at once raced the drain. + pub(crate) fn drained(&self) -> bool { + self.pending().is_ok_and(|items| items.is_empty()) + && matches!( + self.request(&Request::Status), + Ok(Response::Status { + pending_uploads: 0, + pending_changes: 0, + .. + }) + ) + } + /// Whether the mount believes Proton is reachable, as `pdfs status` shows /// it. pub(crate) fn online(&self) -> io::Result { @@ -565,7 +581,7 @@ mod tests { std::fs::write(&path, b"0123456789").unwrap(); assert!(wait_until(Duration::from_secs(30), || { - daemon.pending().is_ok_and(|items| items.is_empty()) + daemon.drained() && drive.tree().get("f.txt") == Some(&Entry::File(Arc::new(b"0123456789".to_vec()))) })); daemon.client.set_online(false); @@ -684,6 +700,55 @@ mod tests { let _ = std::fs::remove_dir_all(&dir); } + #[test] + #[ignore = "mounts FUSE: run with `cargo test -p pdfs-fuse sim:: -- --ignored`"] + fn a_new_file_deleted_while_open_as_its_create_lands_still_reads() { + // The create's op was retired and the tree still had the placeholder. + // The queue read empty, and the delete trashed the node by its real + // uid, which the tree did not have yet: it took the open file for + // closed and dropped the bytes its handle reads (B257). + use std::io::{Read, Seek, SeekFrom, Write}; + let drive = FakeDrive::new(); + let dir = scratch("delete-open-as-create-lands"); + let daemon = Daemon::start(&dir, drive.client(1, Faults::lan())).unwrap(); + let path = daemon.mountpoint.join("f.txt"); + + let landing = daemon.client.hold_next_landing_of_any(); + std::fs::write(&path, b"0123456789").unwrap(); + assert!(wait_until(Duration::from_secs(30), || landing.reached())); + assert!(!daemon.drained(), "the landing create is counted"); + let mut file = std::fs::OpenOptions::new() + .read(true) + .write(true) + .open(&path) + .unwrap(); + drop(landing); + assert!(wait_until(Duration::from_secs(30), || daemon.drained())); + + daemon.client.set_online(false); + std::fs::remove_file(&path).unwrap(); + file.seek(SeekFrom::End(0)).unwrap(); + file.write_all(b"ab").unwrap(); + let mut read = Vec::new(); + file.seek(SeekFrom::Start(0)).unwrap(); + file.read_to_end(&mut read).unwrap(); + assert_eq!(read, b"0123456789ab"); + drop(file); + + daemon.client.set_online(true); + assert!( + wait_until(Duration::from_secs(30), || { + daemon.drained() && 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_restart_does_not_bring_back_the_write_of_a_file_deleted_while_open() { @@ -698,7 +763,7 @@ mod tests { std::fs::write(&path, b"0123456789").unwrap(); assert!(wait_until(Duration::from_secs(30), || { - daemon.pending().is_ok_and(|items| items.is_empty()) + daemon.drained() && drive.tree().get("f.txt") == Some(&Entry::File(Arc::new(b"0123456789".to_vec()))) })); daemon.client.set_online(false); diff --git a/docs/BUGS.md b/docs/BUGS.md index e701f5a..82a9423 100644 --- a/docs/BUGS.md +++ b/docs/BUGS.md @@ -100,8 +100,7 @@ a live account since. | Entry | Title | |---|---| -| [B256](#b256--pdfs-copy-right-after-a-save-copies-the-old-version) | `pdfs copy` right after a save copies the old version | -| [B255](#b255--a-synced-folder-whose-files-are-all-gone-here-is-stuck-for-good) | A synced folder whose files are all gone here is stuck for good | +| [B257](#b257--deleting-a-new-file-while-it-is-open-can-drop-the-bytes-its-handle-reads) | Deleting a new file while it is open can drop the bytes its handle reads | | [B254](#b254--an-edit-in-the-same-second-as-a-new-upload-never-arrives) | An edit in the same second as a new upload never arrives | | [B253](#b253--the-app-shows-the-sign-in-page-when-the-keyring-is-locked) | The app shows the sign-in page when the keyring is locked | | [B252](#b252--computers-sharing-a-device-overwrite-each-others-profile) | Computers sharing a device overwrite each other's profile | @@ -276,6 +275,8 @@ a live account since. | Entry | Title | |---|---| +| [B256](#b256--pdfs-copy-right-after-a-save-copies-the-old-version) | `pdfs copy` right after a save copies the old version | +| [B255](#b255--a-synced-folder-whose-files-are-all-gone-here-is-stuck-for-good) | A synced folder whose files are all gone here is stuck for good | | [B194](#b194--on-a-fresh-install-the-tray-exits-and-the-takeout-page-cannot-remember-its-archives) | On a fresh install the tray exits and the Takeout page cannot remember its archives | | [B192](#b192--a-new-file-closed-empty-reaches-drive-only-half-a-minute-later) | A new file closed empty reaches Drive only half a minute later | | [B181](#b181--the-app-shows-a-queued-edit-kept-as-a-conflict-copy-in-english) | The app shows a queued edit kept as a conflict copy in English | @@ -359,9 +360,46 @@ a live account since. Newest first. -### B256 — `pdfs copy` right after a save copies the old version +### B257 — Deleting a new file while it is open can drop the bytes its handle reads **Status:** Fixed, not verified live +**Found:** 2026-10-27, the simulation +`a_file_deleted_offline_while_open_still_reads_and_takes_writes` failing once on CI with a cache +tag left behind, then on every run with a 50 ms pause added to `ContentCache::store_file`. +**Where:** `retire_create` in `crates/pdfs-fuse/src/drain.rs`; `queue_trash` and +`remove_replaced` in `crates/pdfs-fuse/src/lib.rs`; `trash_child` in +`crates/pdfs-fuse/src/filesystem.rs`; `ContentCache::reconcile` in `crates/pdfs-core/src/cache.rs` + +**Repro:** Create a file in the mount, wait until `pdfs status` shows nothing queued, open it, +and delete it while the link is down. If the create was still landing, a partial write or a read +through the open handle fails with "Input/output error", and the cache can keep a tag with no +bytes until the next start. + +**Cause.** A create that carries the file's bytes retires its op, then reads the node back, +caches the bytes and only then gives the tree the real uid. Nothing counted that window, so the +queue read empty while it ran, which B119 promises means the file reads without the network. A +delete in that window found the placeholder uid in the tree, and trashed the node by its real +uid. `queue_trash` asked the tree whether that uid was open, found no entry for it, and dropped +the cached bytes the open handle reads, which B186 had kept. When the cache store was running at +the same moment, its tag was written after the eviction, and nothing removed it. + +**Fix.** The create's landing is counted like an upload's, from the op's retirement until the +tree has the real uid. The delete and the replace pass whether they found the file open, so +`queue_trash` no longer depends on the tree having the new uid. Opening the cache removes a tag +whose bytes are gone, which also covers a crash between an eviction's two removals. + +**Test:** The simulation `a_new_file_deleted_while_open_as_its_create_lands_still_reads` holds a +create's landing and fails without the count. The simulation above passes again with the 50 ms +pause, since it now waits for `Daemon::drained`, which counts the landing. Unit test +`a_tag_without_its_blob_is_swept_at_open` in `crates/pdfs-core/src/cache.rs`. No test hits the +delete inside the cache store itself: the window is too short to hold from the simulation. + +--- + +### B256 — `pdfs copy` right after a save copies the old version + +**Status:** Fixed, checked on 2026-10-27 by the acceptance case "regression B256" on a live +account **Found:** 2026-10-26, the simulation `a_pdfs_copy_copies_on_drive_and_waits_for_unsynced_changes` failing on most runs. The feature was not released yet. **Where:** `release` in `crates/pdfs-fuse/src/filesystem.rs`; `copy_to` in @@ -385,7 +423,8 @@ A move checks the same set. ### B255 — A synced folder whose files are all gone here is stuck for good -**Status:** Fixed, not verified live +**Status:** Fixed, checked on 2026-10-27 by the acceptance case "regression B255" on a live +account **Found:** 2026-10-26, issue #46: a folder restored from another computer never downloaded. **Where:** `push_pass` in `crates/pdfs-fuse/src/sync.rs`; `reset_sync_folder` in `crates/pdfs-fuse/src/devices.rs` diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 89ce68b..1d70bc8 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -11,6 +11,15 @@ scratch (user data in `staging/` and `recovery/` is never touched by this). ## [Unreleased] +### Fixed + +- **A new file reads without the network as soon as `pdfs status` shows nothing queued** + (B257). A new file's upload counted as done before its bytes were cached, so a script that + waited for an empty queue and then went offline could not read it. A new file deleted while + open in that moment could also lose the bytes its open handle reads, which made a write or + read through that handle fail with "Input/output error". The cache also removes leftover + tags of evicted files when it opens. + ## [3.6.0] — 2026-10-27 Schema: **44**, unchanged.