diff --git a/crates/pdfs-core/src/cache.rs b/crates/pdfs-core/src/cache.rs index 82425c0..18b00ff 100644 --- a/crates/pdfs-core/src/cache.rs +++ b/crates/pdfs-core/src/cache.rs @@ -365,6 +365,14 @@ pub struct StagedWrite { pub based_on: Option, } +impl StagedWrite { + /// Whether `authored` ranges cover a file of `len` bytes, so the staged + /// bytes are its complete new content. An empty file has nothing to cover. + pub fn covers(authored: &[(u64, u64)], len: u64) -> bool { + len == 0 || authored == [(0, len)] + } +} + /// The identity of a remote revision, as far as we can observe it. /// /// `revision_id` is the authoritative identity when present: the server assigns a @@ -2056,6 +2064,17 @@ mod tests { NodeUid::new(VolumeId::from("vol"), LinkId::from(link)) } + #[test] + fn an_empty_write_covers_the_file_and_a_gap_does_not() { + assert!( + StagedWrite::covers(&[], 0), + "an empty file has nothing to cover" + ); + assert!(StagedWrite::covers(&[(0, 7)], 7)); + assert!(!StagedWrite::covers(&[(0, 3), (4, 7)], 7)); + assert!(!StagedWrite::covers(&[], 7)); + } + /// A unique temp directory removed on drop; avoids a dev-dependency. struct TempDir(PathBuf); impl TempDir { diff --git a/crates/pdfs-fuse/src/background.rs b/crates/pdfs-fuse/src/background.rs index ad734a0..d1cca87 100644 --- a/crates/pdfs-fuse/src/background.rs +++ b/crates/pdfs-fuse/src/background.rs @@ -233,6 +233,48 @@ fn acknowledge_applied_event( Ok(id) } +/// How many applied node-update ids [`RecentEvents`] remembers. Far more than a +/// replay can reach back: one comes from a batch whose last event was not its +/// newest, so it repeats at most what that batch held. +const RECENT_EVENTS: usize = 4096; + +/// The node updates applied lately, by event id, so one that comes round again +/// is skipped rather than applied twice. +/// +/// The feed can deliver the same event twice in a batch, and a batch whose +/// events come out of order leaves the cursor short of its newest event, so the +/// next poll replays what lay past it. An update applied twice is not harmless: +/// the first copy of our own change claims the echo [`Core::note_self_change`] +/// recorded, and the second, unclaimed, reads as someone else's edit and evicts +/// the bytes a partial write is about to gap-fill from (docs/BUGS.md B123). +/// +/// Only node updates: a delete is idempotent, and a continuity loss names the +/// cursor it lost, which is an id already applied. +#[derive(Default)] +struct RecentEvents { + ids: HashSet, + order: VecDeque, +} + +impl RecentEvents { + /// Record `event` as applied, reporting whether it was already. + fn repeat(&mut self, event: &DriveEvent) -> bool { + let DriveEvent::NodeUpdated { id, .. } = event else { + return false; + }; + if !self.ids.insert(id.clone()) { + return true; + } + self.order.push_back(id.clone()); + if self.order.len() > RECENT_EVENTS + && let Some(old) = self.order.pop_front() + { + self.ids.remove(&old); + } + false + } +} + /// The folders one batch of events changed, collected so the listings are /// dropped once at the end of the batch rather than once per event. /// @@ -377,6 +419,7 @@ pub(super) async fn run_event_sync( }; info!(?cursor, "event sync started"); + let mut recent = RecentEvents::default(); loop { core.next_poll(POLL_INTERVAL).await; let events = match link::bounded(client.enumerate_events(&scope, cursor.as_ref())).await { @@ -397,6 +440,11 @@ pub(super) async fn run_event_sync( // did apply. let mut dirty = DirtyParents::default(); for event in &events { + if recent.repeat(event) { + debug!(id = %event.id().as_str(), "skipping an event already applied"); + cursor = Some(event.id().clone()); + continue; + } // Converge the SDK's own caches (folder keys, entity cache) on the // server before applying the event to our tree. Without this, a node // re-keyed/moved by another client keeps a stale key in the SDK for @@ -717,6 +765,43 @@ mod tests { } } + #[test] + fn a_node_update_seen_again_is_skipped_but_other_events_are_not() { + let update = |id: &str| DriveEvent::NodeUpdated { + id: DriveEventId::from(id), + node_uid: uid("file"), + parent_node_uid: None, + is_trashed: false, + is_shared: false, + }; + let mut recent = RecentEvents::default(); + assert!(!recent.repeat(&update("e1"))); + assert!(!recent.repeat(&update("e2"))); + assert!(recent.repeat(&update("e1")), "a duplicate or replay"); + let lost = DriveEvent::ContinuityLost { + id: DriveEventId::from("e2"), + }; + assert!(!recent.repeat(&lost), "names the cursor, which was applied"); + } + + #[test] + fn recent_events_forget_the_oldest_past_their_bound() { + let update = |n: usize| DriveEvent::NodeUpdated { + id: DriveEventId::from(format!("e{n}")), + node_uid: uid("file"), + parent_node_uid: None, + is_trashed: false, + is_shared: false, + }; + let mut recent = RecentEvents::default(); + for n in 0..=RECENT_EVENTS { + assert!(!recent.repeat(&update(n))); + } + assert_eq!(recent.ids.len(), RECENT_EVENTS); + assert!(!recent.repeat(&update(0)), "the oldest was forgotten"); + assert!(recent.repeat(&update(RECENT_EVENTS))); + } + #[test] fn foreign_delete_invalidates_an_earlier_refresh_generation() { let own_volume = VolumeId::from("own"); diff --git a/crates/pdfs-fuse/src/drain.rs b/crates/pdfs-fuse/src/drain.rs index c7b95a4..164d4e2 100644 --- a/crates/pdfs-fuse/src/drain.rs +++ b/crates/pdfs-fuse/src/drain.rs @@ -29,16 +29,14 @@ use std::time::{Duration, Instant}; use parking_lot::Mutex; -use futures::StreamExt; use pdfs_core::batch; use pdfs_core::cache::{Baseline, StagedWrite}; use pdfs_core::control::{ActivityKind, TransferDirection}; use pdfs_core::db::{ OP_CREATE, OP_MKDIR, OP_RENAME, OP_REVISION, OP_TRASH, PARK_EXPIRY_MS, PendingOp, RenameMeta, }; -use proton_drive_rs::proton_sdk::error::ProtonError; use proton_drive_rs::proton_sdk::ids::NodeUid; -use proton_drive_rs::{Node, NodeKind, NodeMoveItem}; +use proton_drive_rs::{Node, NodeKind}; use tracing::{debug, error, info, warn}; use super::state::{Intervals, PendingRevision}; @@ -693,7 +691,7 @@ impl Core { // no window for the requirements to go stale between two calls — // the race B46 queued this op to get away from. let target = (node.name != name).then_some(name.as_str()); - match self.move_renaming(&uid, &parent, target) { + match self.move_rename_remote(&uid, &parent, target) { Ok(()) => name.clone(), // The destination holds that name already. Landing under a // *different* name is the non-destructive resolution: it neither @@ -704,7 +702,7 @@ impl Core { } let alt = conflict_name(&name, now_secs()); warn!(%uid, name, alt, "destination already holds that name; using a conflict name"); - self.move_renaming(&uid, &parent, Some(&alt))?; + self.move_rename_remote(&uid, &parent, Some(&alt))?; self.adopt_drained_name(&uid, &alt); self.log_activity( ActivityKind::Rename, @@ -741,7 +739,6 @@ impl Core { } }; self.db.delete_op(op.id)?; - self.note_self_change(&uid); info!(%uid, name = %landed, "pending rename landed"); Ok(()) } @@ -780,33 +777,6 @@ impl Core { } } - /// Move `uid` under `parent` in one request, renaming it to `target_name` on - /// the way when one is given. - fn move_renaming( - &self, - uid: &NodeUid, - parent: &NodeUid, - target_name: Option<&str>, - ) -> Result<(), ProtonError> { - let item = NodeMoveItem { - uid: uid.clone(), - target_name: target_name.map(str::to_string), - }; - self.rt - .block_on(async { - let mut outcomes = - std::pin::pin!(self.drive.move_nodes_streaming(vec![item], parent.clone())); - match outcomes.next().await { - Some(Ok((_, outcome))) => outcome, - Some(Err(e)) => Err(e), - None => Err(ProtonError::invalid_operation(format!( - "move of {uid} reported no outcome" - ))), - } - }) - .inspect(|()| self.forget_sdk_node(uid)) - } - /// Apply a queued trash to the remote. /// /// A node that is already gone is a success, not a failure: the outcome the @@ -941,7 +911,23 @@ impl Core { // uploaded: it goes on as a revision of the node instead. let uploaded = (!adopted).then_some(op.blob_path.as_deref()).flatten(); let newer = self.retire_create(op, uploaded, &local, &real)?; - self.adopt_real_uid(&local, &real)?; + // The uploaded blob is the new file's content, so it becomes the cached + // content, as for a revision. A partial write fills its gaps from it; + // without it the next write open was refused (`docs/BUGS.md` B123). + let landed: Option = uploaded + .filter(|_| !newer) + .and(op.meta_json.as_deref()) + .and_then(|json| serde_json::from_str(json).ok()); + self.adopt_real_uid(&local, &real, |node| { + if let (Some(blob), Some(meta)) = (uploaded, &landed) + && node_size(node) == meta.len + && keeps_landed_upload(self.cache.is_pinned(&real), meta.len, self.cache.budget()) + { + let _ = + self.cache + .store_file(&real, node.modification_time, meta.len, Path::new(blob)); + } + })?; // The feed will report this create back to us; the tree already has it // under its real uid, so that event is ours to ignore (`Core::self_changes`). self.note_self_change(&real); @@ -1096,6 +1082,9 @@ impl Core { } newer.is_some() }; + if !newer && let Some(blob) = uploaded { + self.cache_created(op, local, real, blob); + } self.for_each_state(|st| { if let Some(ino) = st.by_uid.remove(local) { st.by_uid.insert(real.clone(), ino); @@ -1112,6 +1101,38 @@ impl Core { Ok(newer) } + /// Cache a landed create's uploaded `blob` under `real`, keyed as the tree + /// has the file. + /// + /// A write handle open on the file is repointed at `real` right after this, + /// and a release fills its gaps from the cache by the size and mtime it + /// opened over. Drive lists the new node a moment after the create, so + /// `adopt_real_uid` caches the bytes only once it can read the node back. + /// A release in between found nothing to fill from, and the next write open + /// was refused (docs/BUGS.md B131). + fn cache_created(&self, op: &PendingOp, local: &NodeUid, real: &NodeUid, blob: &str) { + let Some(meta) = op + .meta_json + .as_deref() + .and_then(|json| serde_json::from_str::(json).ok()) + else { + return; + }; + let mut mtime = None; + self.for_each_state(|st| { + if let Some(entry) = st.by_uid.get(local).and_then(|ino| st.entries.get(ino)) { + mtime = Some(entry.node.modification_time); + } + }); + if let Some(mtime) = mtime + && keeps_landed_upload(self.cache.is_pinned(real), meta.len, self.cache.budget()) + { + let _ = self + .cache + .store_file(real, mtime, meta.len, Path::new(blob)); + } + } + /// Swap a placeholder uid for the real one across everything that keyed off /// it: queued children, the DB, the in-memory tree, and the caches. /// @@ -1125,14 +1146,22 @@ impl Core { /// its `local~` uid, which reads as an empty file for as long as the daemon /// runs (`docs/BUGS.md` B74). A uid is unique across mounts, so at most one /// of them matches and the others are no-ops. + /// + /// `adopt` sees the node as Drive made it, before anything is rebased onto + /// it. pub(crate) fn adopt_real_uid( &self, local: &NodeUid, real: &NodeUid, + adopt: impl FnOnce(&Node), ) -> Result<(), Box> { + // Drive answers a create before it lists the new node. let node = self - .fetch_node(real) + .read_back(real) .map_err(|e| self.errno_error(e, "fetch node"))?; + // Before open handles are rebased onto the node, so one released in + // between finds what `adopt` keeps (`docs/BUGS.md` B123). + adopt(&node); // Repoints queued children and node rows, and readdresses the placeholder row. // A no-op after `retire_create`, which has already done both. self.db @@ -1667,10 +1696,7 @@ impl Core { // an orphaned file (harmless), whereas the reverse would leave a queued // op pointing at nothing. self.db.delete_op(op.id)?; - self.release_pending(&uid, &blob); self.evict_reader(&uid); - let landed = self.refresh_after_upload(&uid); - // The staged blob now matches the sealed revision, so it becomes the // file's cached content, stamped like the revision just read back. // Reading the file again then needs no network, which an editor or a @@ -1678,14 +1704,15 @@ 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. - if let Some(node) = landed - && node_size(&node) == 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); - } + self.refresh_after_upload(&uid, &blob, |node| { + if node_size(node) == 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); + } + }); self.cache.discard_staged(&blob); self.log_activity(ActivityKind::Upload, &name, "uploaded", true); // Both times, so a slow drain shows whether the upload itself or the @@ -1720,11 +1747,27 @@ impl Core { /// we just sealed. Skipping both, as this used to, is what produced a file /// 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). + /// /// 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 /// the server now has it, or `None` when it could not be read back. - pub(crate) fn refresh_after_upload(&self, uid: &NodeUid) -> Option { - let node = match self.fetch_node_remote(uid) { + pub(crate) fn refresh_after_upload( + &self, + uid: &NodeUid, + blob: &Path, + adopt: impl FnOnce(&Node), + ) -> Option { + let fetched = self.fetch_node_remote(uid); + if !matches!(fetched, Ok(Some(_))) { + self.release_pending(uid, blob); + } + 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. @@ -1736,6 +1779,7 @@ impl Core { return None; } }; + adopt(&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 @@ -1768,6 +1812,7 @@ impl Core { } self.rebase_open_writes(uid, &node); } + self.release_pending(uid, blob); // Ordered so that a write queued *during* the fetch above is still // caught: it took its baseline from the node's optimistic stamp, and // this overwrites it with the revision the server actually holds. diff --git a/crates/pdfs-fuse/src/filesystem.rs b/crates/pdfs-fuse/src/filesystem.rs index 6cecdd4..6fae51b 100644 --- a/crates/pdfs-fuse/src/filesystem.rs +++ b/crates/pdfs-fuse/src/filesystem.rs @@ -1435,7 +1435,7 @@ impl ProtonFs { len, base_size, base_mtime, - complete: authored == [(0, len)], + complete: StagedWrite::covers(&authored, len), authored, based_on: self .core @@ -1944,12 +1944,14 @@ impl ProtonFs { // now and this handle's is stale. A retry resolves it. Ok(false) => { warn!(%uid, name, newname, "queued create vanished under a rename"); - self.core.restore_replaced(victim.as_ref(), newname); + self.core + .restore_replaced(victim.as_ref(), &new_parent_uid, newname); reply.error(Errno::EBUSY); } Err(e) => { error!(%uid, error = %e, "rewriting a queued create's target failed"); - self.core.restore_replaced(victim.as_ref(), newname); + self.core + .restore_replaced(victim.as_ref(), &new_parent_uid, newname); reply.error(Errno::EIO); } } @@ -2001,7 +2003,8 @@ impl ProtonFs { } } Err(e) => { - self.core.restore_replaced(victim.as_ref(), newname); + self.core + .restore_replaced(victim.as_ref(), &new_parent_uid, newname); reply.error(e); } } @@ -2015,8 +2018,6 @@ impl ProtonFs { queue_it(reply); return; } - // Rename first if both halves change; a move that then fails leaves the - // node renamed in its source folder, which the tree is told below. // A failure past this point has already trashed any node the rename was // replacing, so put it back rather than leaving the caller with neither // the source moved nor the destination intact. @@ -2026,48 +2027,37 @@ impl ProtonFs { &old_parent_uid, &new_parent_uid, ) { - self.core.restore_replaced(victim.as_ref(), newname); + self.core + .restore_replaced(victim.as_ref(), &new_parent_uid, newname); reply.error(error); return; } - if newname != name - && let Err(e) = self.core.rename_remote(&uid, newname) - { + // A move that also renames is one request, so the name only has to be + // free in the destination, and the reply never stands for half of it. + let renamed = if newparent != parent { + let target = (newname != name).then_some(newname); + self.core.move_rename_remote(&uid, &new_parent_uid, target) + } else if newname != name { + self.core.rename_remote(&uid, newname) + } else { + Ok(()) + }; + if let Err(e) = renamed { if self.core.lost_link(&e, "rename") { queue_it(reply); return; } error!(%uid, error = %e, "rename failed"); - self.core.restore_replaced(victim.as_ref(), newname); - reply.error(Errno::EIO); - return; - } - // A successful reply must mean both remote halves landed, not merely - // that the second half was queued. - if newparent != parent - && let Err(e) = self.core.move_remote(&uid, &new_parent_uid) - { - if self.core.lost_link(&e, "move") { - queue_it(reply); - return; - } - error!(%uid, error = %e, "move after rename failed"); - // The rename half landed in the source directory. - let mut state = self.core.state(); - let old_parent_uid = state - .entries - .get(&parent) - .map(|entry| entry.uid.clone()) - .unwrap_or_else(|| uid.clone()); - state.rename_in_place(ino, parent, &old_parent_uid, newname); - drop(state); - self.core.restore_replaced(victim.as_ref(), newname); + self.core + .restore_replaced(victim.as_ref(), &new_parent_uid, newname); reply.error(Errno::EIO); return; } + // Through the guard, so the database has both folders unlisted before + // the reply. The next lookup rebuilds a folder still marked listed from + // the database, which did not have the move yet (docs/BUGS.md B135). self.core - .state - .lock() + .state() .relocate(ino, parent, newparent, &new_parent_uid, newname); reply.ok(); } diff --git a/crates/pdfs-fuse/src/lib.rs b/crates/pdfs-fuse/src/lib.rs index 6904684..3ddf1c0 100644 --- a/crates/pdfs-fuse/src/lib.rs +++ b/crates/pdfs-fuse/src/lib.rs @@ -74,7 +74,7 @@ use proton_drive_rs::proton_sdk::api::ResponseCode; use proton_drive_rs::proton_sdk::error::ProtonError; use proton_drive_rs::proton_sdk::ids::{DriveEventId, LinkId, NodeUid, ShareId, VolumeId}; use proton_drive_rs::{ - DriveEvent, DriveEventScopeId, MemberRole, Node, NodeKind, ProtonDriveClient, + DriveEvent, DriveEventScopeId, MemberRole, Node, NodeKind, NodeMoveItem, ProtonDriveClient, ProtonPhotosClient, SharedWithMeItem, ThumbnailType, }; @@ -1477,7 +1477,7 @@ fn merge_over_pending( .filter(|&(_, _, authored)| authored) .map(|(s, e, _)| (s, e)) .collect(); - meta.complete = meta.authored == [(0, meta.len)]; + meta.complete = StagedWrite::covers(&meta.authored, meta.len); Ok(()) } @@ -1627,6 +1627,38 @@ impl Core { }) } + /// Move `uid` into the folder `parent` on Drive, renaming it to + /// `target_name` on the way when one is given, in one request. + /// + /// Done as a rename and then a move, a sibling in the source folder that + /// held the new name made Drive refuse the first half, although the + /// destination was free (docs/BUGS.md B122). One request also leaves no + /// half-applied state between the two. + pub(crate) fn move_rename_remote( + &self, + uid: &NodeUid, + parent: &NodeUid, + target_name: Option<&str>, + ) -> Result<(), ProtonError> { + self.change_remote(uid, || { + let item = NodeMoveItem { + uid: uid.clone(), + target_name: target_name.map(str::to_string), + }; + self.block_on_bounded(async { + let mut outcomes = + std::pin::pin!(self.drive.move_nodes_streaming(vec![item], parent.clone())); + match outcomes.next().await { + Some(Ok((_, outcome))) => outcome, + Some(Err(e)) => Err(e), + None => Err(ProtonError::invalid_operation(format!( + "move of {uid} reported no outcome" + ))), + } + }) + }) + } + /// Run a rename or move of `uid`, then drop the SDK's now stale copy of it. fn change_remote( &self, @@ -1637,7 +1669,16 @@ impl Core { self.forget_sdk_node(uid); std::thread::sleep(OUT_OF_DATE_RETRY_DELAY); }) - .inspect(|()| self.forget_sdk_node(uid)) + .inspect(|()| self.changed_remote(uid)) + } + + /// Bookkeeping after a rename or move of `uid` landed: the SDK's copy of it + /// is stale, and the feed will echo the change back. Without the second, + /// that echo reads as someone else's edit and evicts the cached bytes a + /// partial write is about to gap-fill from (docs/BUGS.md B123). + pub(crate) fn changed_remote(&self, uid: &NodeUid) { + self.forget_sdk_node(uid); + self.note_self_change(uid); } /// Drop the SDK's cached copy of `uid` after this daemon renamed or moved it. @@ -3047,19 +3088,32 @@ impl Core { /// drain finds the node by name and adopts it: the application gets its /// file or folder either way, never an `ENOENT` for something that exists. fn fetch_minted_node(&self, uid: &NodeUid, name: &str) -> Option { + // The feed echoes the create. Unclaimed, it would take the echo the + // upload of the file's bytes records, and that one would then evict + // the bytes just uploaded (docs/BUGS.md B123). + self.note_self_change(uid); + let node = self.read_back(uid).ok(); + if node.is_none() { + warn!(%uid, name, "Drive made the node but did not hand it back"); + } + node + } + + /// [`Core::fetch_node`] for a node Drive has just made, asked again for a + /// moment while Drive does not list it yet. + pub(crate) fn read_back(&self, uid: &NodeUid) -> Result { let mut delays = MINTED_READ_BACK_DELAYS.iter(); loop { match self.fetch_node(uid) { - Ok(node) => return Some(node), - Err(e) if e == Errno::ENOENT => match delays.next() { - Some(delay) => std::thread::sleep(*delay), - None => break, - }, - Err(_) => break, + Err(e) + if e == Errno::ENOENT + && let Some(delay) = delays.next() => + { + std::thread::sleep(*delay); + } + fetched => return fetched, } } - warn!(%uid, name, "Drive made the node but did not hand it back"); - None } /// [`Core::fetch_node`] without the collapse to an `Errno`, for the drain: @@ -3160,7 +3214,7 @@ impl Core { len: h.len, base_size: h.base_size, base_mtime: h.base_mtime, - complete: authored == [(0, h.len)], + complete: StagedWrite::covers(&authored, h.len), authored, based_on: self.remote_baseline( &h.uid, @@ -3193,7 +3247,7 @@ impl Core { len: h.len, base_size: h.base_size, base_mtime: h.base_mtime, - complete: authored == [(0, h.len)], + complete: StagedWrite::covers(&authored, h.len), authored, based_on: self.remote_baseline( &h.uid, @@ -4803,7 +4857,11 @@ impl Core { /// further to try, and the node is still in the trash where `pdfs restore` /// can reach it. Says so loudly in that case, because the alternative is a /// file the user believes was only renamed quietly sitting in the trash. - fn restore_replaced(&self, victim: Option<&(u64, NodeUid)>, name: &str) { + /// + /// A restored node comes back to the mount as the ordinary restore brings + /// it: out of `hidden`, and through a fresh listing of `parent` in every + /// mount that shows it (`docs/BUGS.md` B124). + fn restore_replaced(&self, victim: Option<&(u64, NodeUid)>, parent: &NodeUid, name: &str) { let Some((_, uid)) = victim else { return }; if is_local_uid(uid) { // Its queued create was discarded and cannot be reconstructed from @@ -4817,6 +4875,13 @@ impl Core { .and_then(batch::into_unit) { Ok(()) => { + self.hidden.lock().remove(uid); + self.for_each_mount(|st, notify| { + if let Some(&ino) = st.by_uid.get(parent) { + st.invalidate_listing(ino); + notify.inval_entry(ino, name.to_string()); + } + }); self.invalidate_trash(); debug!(%uid, name, "restored the node a failed rename had replaced"); } @@ -6301,7 +6366,7 @@ mod merge_over_pending_tests { len, base_size, base_mtime: 100, - complete: authored == [(0, len)], + complete: StagedWrite::covers(authored, len), authored: authored.to_vec(), based_on: Some(Baseline { mtime: 100, @@ -8458,6 +8523,20 @@ mod tests { assert_eq!(replaced.matches("self.withdraw_queued_ops(uid)").count(), 2); } + #[test] + fn a_replaced_node_put_back_shows_in_the_mount_again() { + // The restore after a failed rename left the node hidden and out of + // the listing, so a create by its name failed with ENOENT (B124). + let restore = function_source(include_str!("lib.rs"), "fn restore_replaced("); + assert_before( + restore, + "self.drive.restore_nodes(", + "self.hidden.lock().remove(uid)", + ); + assert!(restore.contains("st.invalidate_listing(ino)")); + assert!(restore.contains("notify.inval_entry(ino, name.to_string())")); + } + #[test] fn a_size_upgrade_leaves_the_page_cache_alone() { // Reads resolve provisional sizes inline, while the kernel holds the @@ -8540,21 +8619,32 @@ mod tests { usize::from(source.contains("fn move_remote(")), "call Core::move_remote instead of the SDK move directly" ); + assert_eq!( + source + .matches(concat!("drive.move_nodes_streaming", "(")) + .count(), + usize::from(source.contains("fn move_rename_remote(")), + "call Core::move_rename_remote instead of the SDK move directly" + ); } } #[test] - fn a_rename_or_move_drops_the_sdk_copy_of_the_node() { + fn a_rename_or_move_drops_the_sdk_copy_and_expects_its_echo() { // The SDK would send the old name hash with the node's next rename or - // move until the event poll caught up (B121). + // move until the event poll caught up (B121), and the echo of the + // change, taken for someone else's, evicted the cached bytes (B123). let change = function_source(include_str!("lib.rs"), "fn change_remote("); - assert_eq!( - change.matches("self.forget_sdk_node(uid)").count(), - 2, - "forget the node before each retry and once the change landed" + assert!( + change.contains("self.forget_sdk_node(uid)"), + "forget the node before each retry" ); - let drained = function_source(include_str!("drain.rs"), "fn move_renaming("); - assert!(drained.contains("self.forget_sdk_node(uid)")); + assert!(change.contains("self.changed_remote(uid)")); + let moved = function_source(include_str!("lib.rs"), "fn move_rename_remote("); + assert!(moved.contains("self.change_remote(uid")); + let changed = function_source(include_str!("lib.rs"), "fn changed_remote("); + assert!(changed.contains("self.forget_sdk_node(uid)")); + assert!(changed.contains("self.note_self_change(uid)")); } #[test] @@ -8632,6 +8722,18 @@ mod tests { assert_before(upload, "self.db.op_exists(op.id)", "File::open(&blob)"); } + #[test] + fn an_online_rename_writes_the_move_through_before_it_answers() { + // Through the raw lock, the move stayed in the state's outbox, and the + // next lookup rebuilt the destination from a database without it + // (B135). + let rename: String = function_source(include_str!("filesystem.rs"), "fn serve_rename(") + .split_whitespace() + .collect(); + assert!(rename.contains("self.core.state().relocate(")); + assert!(!rename.contains(".lock().relocate(")); + } + #[test] fn a_landed_upload_stays_pending_until_its_bytes_are_cached() { let drain = function_source(include_str!("drain.rs"), "pub(crate) fn drain_revision("); diff --git a/crates/pdfs-fuse/src/sim/daemon.rs b/crates/pdfs-fuse/src/sim/daemon.rs index 05f9ca2..563b6bd 100644 --- a/crates/pdfs-fuse/src/sim/daemon.rs +++ b/crates/pdfs-fuse/src/sim/daemon.rs @@ -309,4 +309,115 @@ mod tests { assert!(matches!(daemon.stop(), Ok(MountOutcome::Shutdown))); let _ = std::fs::remove_dir_all(&dir); } + + /// Write `bytes` at `offset` into an existing file, without truncating it. + fn write_at(path: &Path, offset: u64, bytes: &[u8]) -> io::Result<()> { + use std::os::unix::fs::FileExt; + std::fs::OpenOptions::new() + .write(true) + .open(path)? + .write_all_at(bytes, offset) + } + + #[test] + #[ignore = "mounts FUSE: run with `cargo test -p pdfs-fuse sim:: -- --ignored`"] + fn a_file_closed_empty_takes_a_second_write_at_once() { + // The empty revision never counted as complete, so the next write + // open before it drained was refused with EIO (B123). + let drive = FakeDrive::new(); + let dir = scratch("empty-rewrite"); + let daemon = Daemon::start(&dir, drive.client(1, Faults::lan())).unwrap(); + + let path = daemon.mountpoint.join("empty.txt"); + std::fs::write(&path, b"old").unwrap(); + assert!(wait_until(Duration::from_secs(30), || { + drive.tree().contains_key("empty.txt") + })); + std::fs::write(&path, b"").unwrap(); + write_at(&path, 0, b"filled").unwrap(); + assert!( + wait_until(Duration::from_secs(30), || { + drive.tree().get("empty.txt") == Some(&Entry::File(Arc::new(b"filled".to_vec()))) + }), + "the second write landed: {:?}", + 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() { + // The echo read as someone else's change and evicted the bytes, so a + // partial write could not gap-fill, and the next write open on the + // file was refused with EIO (B123). + let drive = FakeDrive::new(); + let dir = scratch("rename-echo"); + let daemon = Daemon::start(&dir, drive.client(1, Faults::lan())).unwrap(); + + let before = daemon.mountpoint.join("a.txt"); + std::fs::write(&before, b"0123456789").unwrap(); + assert!(wait_until(Duration::from_secs(30), || { + drive.tree().contains_key("a.txt") + })); + assert_eq!(std::fs::read(&before).unwrap(), b"0123456789"); + let after = daemon.mountpoint.join("b.txt"); + std::fs::rename(&before, &after).unwrap(); + // Long enough for the next event poll to bring the echo. + std::thread::sleep(crate::POLL_INTERVAL + Duration::from_secs(3)); + + write_at(&after, 3, b"x").unwrap(); + write_at(&after, 5, b"y").unwrap(); + assert!( + wait_until(Duration::from_secs(30), || { + drive.tree().get("b.txt") == Some(&Entry::File(Arc::new(b"012x4y6789".to_vec()))) + }), + "both writes landed: {:?}", + 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_rename_across_folders_ignores_the_new_name_in_the_old_folder() { + // The rename went to Drive as a rename in the old folder and then a + // move, so a sibling there with the new name failed it with EIO + // (B122). + let drive = FakeDrive::new(); + let dir = scratch("rename-across"); + let daemon = Daemon::start(&dir, drive.client(1, Faults::lan())).unwrap(); + + let mount = &daemon.mountpoint; + std::fs::create_dir(mount.join("x")).unwrap(); + std::fs::create_dir(mount.join("z")).unwrap(); + std::fs::write(mount.join("x/f.txt"), b"moved").unwrap(); + std::fs::write(mount.join("x/d"), b"sibling").unwrap(); + std::fs::write(mount.join("z/d"), b"replaced").unwrap(); + let file = |bytes: &[u8]| Some(Entry::File(Arc::new(bytes.to_vec()))); + assert!(wait_until(Duration::from_secs(30), || { + let tree = drive.tree(); + [ + ("x/f.txt", b"moved".as_slice()), + ("x/d", b"sibling"), + ("z/d", b"replaced"), + ] + .iter() + .all(|(path, bytes)| tree.get(*path).cloned() == file(bytes)) + })); + + std::fs::rename(mount.join("x/f.txt"), mount.join("z/d")).unwrap(); + assert_eq!(std::fs::read(mount.join("z/d")).unwrap(), b"moved"); + let tree = drive.tree(); + assert_eq!(tree.get("z/d").cloned(), file(b"moved")); + assert_eq!(tree.get("x/d").cloned(), file(b"sibling")); + assert!(!tree.contains_key("x/f.txt")); + + assert!(matches!(daemon.stop(), Ok(MountOutcome::Shutdown))); + let _ = std::fs::remove_dir_all(&dir); + } } diff --git a/crates/pdfs-fuse/src/sim/fake_drive.rs b/crates/pdfs-fuse/src/sim/fake_drive.rs index ea853da..5ffd8b0 100644 --- a/crates/pdfs-fuse/src/sim/fake_drive.rs +++ b/crates/pdfs-fuse/src/sim/fake_drive.rs @@ -537,6 +537,17 @@ impl Faults { } } + /// A good link whose event feed is slow and untidy: every echo of our own + /// changes comes late, often twice, often out of order. + pub(crate) fn echoes() -> Self { + Self { + event_delay: Duration::from_secs(1), + duplicate_events: 0.3, + reorder_events: 0.3, + ..Self::lan() + } + } + /// The Wi-Fi of the 2026-10 account runs: slow and lossy, with listings /// and events well behind. pub(crate) fn wifi() -> Self { diff --git a/crates/pdfs-fuse/src/sim/run.rs b/crates/pdfs-fuse/src/sim/run.rs index f20883d..a94d705 100644 --- a/crates/pdfs-fuse/src/sim/run.rs +++ b/crates/pdfs-fuse/src/sim/run.rs @@ -65,25 +65,6 @@ struct Known { } const KNOWN: &[Known] = &[ - // A rename across folders renames in the source folder first, so a - // sibling there with the new name refuses it. - Known { - bug: "B122", - is: |model, op, errno, _, _| { - let FsOp::Rename { from, to } = op else { - return false; - }; - errno == libc::EIO - && parent_of(from) != parent_of(to) - && name_of(from) != name_of(to) - && model - .tree() - .contains_key(&join(parent_of(from), name_of(to))) - }, - // The destination it was replacing is restored on Drive but stays - // gone from the mount (B124). - ends_seed: |model, op| matches!(op, FsOp::Rename { to, .. } if model.tree().contains_key(to)), - }, // A replacing rename trashes what it replaces first, so Drive refuses the // name only when it holds a node the mount has forgotten: the create of // the replaced file landed after its op was dropped. @@ -99,20 +80,10 @@ const KNOWN: &[Known] = &[ .iter() .any(|line| line.starts_with("rename failed") && line.contains("AlreadyExists")) }, - // The file it was replacing is gone from the mount (B124). + // The model made the rename, and Drive holds a node the mount does + // not know. ends_seed: |_, _| true, }, - // A write over a queued revision whose gaps are not filled yet is refused. - Known { - bug: "B123", - is: |_, _, errno, logged, _| { - errno == libc::EIO - && logged - .iter() - .any(|line| line.starts_with("refusing write over incomplete queued revision")) - }, - ends_seed: |_, _| false, - }, // A folder made offline is listed on Drive once its listing has been // invalidated, and Drive does not know it, or does not list it yet. Only // the client writes its folder, so a folder of its own is never really @@ -649,13 +620,6 @@ impl Run { /// - B128: the file's queued create ran into the name of the file it /// replaced and took that over, trash and all. /// - /// A conflict copy of an earlier version, which the model no longer - /// holds, takes the drain saying it kept a queued write as that copy - /// after a landed create could not be read back: - /// - /// - B131: the tree kept the create's placeholder, so a write based on it - /// found Drive's copy changed. - /// /// A conflict copy of a file the client deleted takes the drain saying it /// kept that file's write as the copy, because Drive had the file in the /// trash while the tree still placed it: @@ -693,16 +657,6 @@ impl Run { }) }) }; - let unread_create = self.logged.iter().any(|line| { - line.starts_with("pending upload failed; will retry uid=local~") - && line.ends_with("error=fetch node: no such file or folder") - }); - let kept_write = |copy: &str| { - let alt = format!("alt={:?}", name_of(copy)); - self.logged.iter().any(|line| { - line.starts_with("queued write landed as a conflict copy") && line.ends_with(&alt) - }) - }; let kept_deleted = |copy: &str| { let alt = format!(" alt={:?}", name_of(copy)); self.logged.iter().any(|line| { @@ -724,15 +678,11 @@ impl Run { if !is_conflict_copy(copy) || model.contains_key(copy) { continue; } - let bug = if unread_create && kept_write(copy) { - "B131" - } else if kept_deleted(copy) { - "B134" - } else { + if !kept_deleted(copy) { continue; - }; + } damage.push(KnownDamage { - bug, + bug: "B134", path: copy.clone(), copy: Some(copy.clone()), change: FsOp::Create { @@ -1068,6 +1018,19 @@ mod tests { drain_budget: Duration::from_secs(20), }; + static ONE_CLIENT_ECHOES: Profile = Profile { + name: "one-client-echoes", + clients: 1, + steps: 60, + faults: Faults::echoes, + link_flaps: false, + restarts: false, + deletes_mid_upload: false, + settle_budget: Some(Duration::from_secs(6)), + syscall_budget: Duration::from_secs(2), + drain_budget: Duration::from_secs(20), + }; + static ONE_CLIENT_FLAKY: Profile = Profile { name: "one-client-flaky", clients: 1, @@ -1106,6 +1069,12 @@ mod tests { run_seeds(&ONE_CLIENT_DELETES, 3); } + #[test] + #[ignore = "mounts FUSE: run with `cargo test -p pdfs-fuse --lib sim::run -- --ignored`"] + fn one_client_with_stale_hashes_and_reordered_echoes() { + run_seeds(&ONE_CLIENT_ECHOES, 6); + } + #[test] #[ignore = "mounts FUSE: run with `cargo test -p pdfs-fuse --lib sim::run -- --ignored`"] fn one_client_on_a_flaky_link() { diff --git a/crates/pdfs-fuse/src/state.rs b/crates/pdfs-fuse/src/state.rs index ce5bdf7..a43336a 100644 --- a/crates/pdfs-fuse/src/state.rs +++ b/crates/pdfs-fuse/src/state.rs @@ -825,11 +825,54 @@ impl State { } } - pub(crate) fn intern(&mut self, parent: u64, node: Node) -> u64 { + pub(crate) fn intern(&mut self, parent: u64, mut node: Node) -> u64 { + self.keep_revision_mtime(&mut node); self.outbox.push(DbWrite::Upsert(vec![node.clone()])); self.intern_mem(parent, node) } + /// Keep what is already known of a file whose content has not changed. + /// + /// Drive stamps a link's `ModifyTime` on a rename or a move as well, while + /// POSIX leaves a file's mtime alone there. The content cache is keyed by + /// it, too: taking the new stamp made the cached bytes unreachable, so the + /// next partial write could not fill its gaps (docs/BUGS.md B123). The + /// active revision id says whether the content is still the same, and so + /// whether the size and digest a listing left out are still the ones known. + fn keep_revision_mtime(&self, node: &mut Node) { + let Some(entry) = self + .by_uid + .get(&node.uid) + .and_then(|ino| self.entries.get(ino)) + else { + return; + }; + if let ( + NodeKind::File { + active_revision_id: Some(now), + claimed_size, + content_sha1, + .. + }, + NodeKind::File { + active_revision_id: Some(was), + claimed_size: size_was, + content_sha1: sha1_was, + .. + }, + ) = (&mut node.kind, &entry.node.kind) + && now == was + { + node.modification_time = entry.node.modification_time; + if claimed_size.is_none() { + *claimed_size = *size_was; + } + if content_sha1.is_none() { + content_sha1.clone_from(sha1_was); + } + } + } + /// Allocate (or reuse) a stable inode for a node, updating the hot-cache maps /// only. Every caller owes the DB a write-through — see the callers below; /// the split exists so a batch can pay for one transaction instead of `n`. @@ -915,7 +958,10 @@ impl State { /// /// This is what keeps `ls` on a large folder quick: one commit for the /// listing, rather than one autocommit — and one fsync — per child. - pub(crate) fn intern_batch(&mut self, parent: u64, nodes: Vec) -> Vec { + pub(crate) fn intern_batch(&mut self, parent: u64, mut nodes: Vec) -> Vec { + for node in &mut nodes { + self.keep_revision_mtime(node); + } self.outbox.push(DbWrite::Upsert(nodes.clone())); nodes .into_iter() @@ -1937,6 +1983,51 @@ mod tests { assert_eq!(st.entries[&shared].access, Access::Viewer); } + fn at_revision(mut node: Node, revision: &str, mtime: i64) -> Node { + node.modification_time = mtime; + if let NodeKind::File { + active_revision_id, .. + } = &mut node.kind + { + *active_revision_id = Some(revision.into()); + } + node + } + + #[test] + fn a_renamed_file_keeps_its_mtime_until_its_content_changes() { + let (mut st, _dir) = state(); + let root = st.intern(0, node("root", "none", "My Files", true)); + let mut file = node("file", "root", "a.txt", false); + if let NodeKind::File { + claimed_size, + total_size_on_storage, + .. + } = &mut file.kind + { + *claimed_size = Some(7); + *total_size_on_storage = 519; + } + let ino = st.intern(root, at_revision(file.clone(), "r1", 100)); + // Drive stamps the link on a rename; the content is still r1. + let mut renamed = at_revision(file.clone(), "r1", 160); + renamed.name = "b.txt".into(); + st.intern_batch(root, vec![renamed]); + assert_eq!(st.entries[&ino].node.modification_time, 100); + assert_eq!(st.entries[&ino].node.name, "b.txt"); + // A listing that leaves the size out still describes revision r1. + let mut bare = at_revision(file.clone(), "r1", 170); + assert_eq!(crate::node_size(&st.entries[&ino].node), 7); + if let NodeKind::File { claimed_size, .. } = &mut bare.kind { + *claimed_size = None; + } + st.intern(root, bare); + assert_eq!(st.entries[&ino].node.modification_time, 100); + assert_eq!(crate::node_size(&st.entries[&ino].node), 7); + st.intern(root, at_revision(file, "r2", 200)); + assert_eq!(st.entries[&ino].node.modification_time, 200); + } + fn handle(len: u64, base_size: u64) -> WriteHandle { let path = std::env::temp_dir().join(format!("pdfs-handle-test-{}", std::process::id())); let file = File::create(&path).unwrap(); diff --git a/docs/BUGS.md b/docs/BUGS.md index 3fe93d3..02e6daa 100644 --- a/docs/BUGS.md +++ b/docs/BUGS.md @@ -12,6 +12,30 @@ Conventions: --- +## B135 — A file just moved is sometimes not found by its new path + +**Status:** Fixed (unverified). +**Found:** 2026-10-03, by the simulation runs (`sim::run`, profile three-clients, seed 1), once +B131 was fixed. `rename a.txt -> y/f.txt` succeeded, and the `rename y/f.txt -> y/a.txt` two +milliseconds later failed with `ENOENT`. Once in about four replays of the seed. + +**Where:** the online path of `serve_rename` in `crates/pdfs-fuse/src/filesystem.rs`. + +**Cause.** After the move lands on Drive, `serve_rename` settles the tree with `State::relocate`. +It took the inode lock directly (`state.lock()`) instead of through `Core::state`, whose guard +writes what a mutation owes the database once the lock is released (DB1). So the writes stayed in +the state's outbox: both folders no longer listed, and the node's new parent. Whichever thread +next released a guard applied them, after it had let go of the lock. The lookup of `y/f.txt` +found `y` unlisted in memory and asked the database, which still marked `y` as listed and placed +the file in the root, so it rebuilt `y` without the file. It took another thread taking the lock +in between, so it happened only now and then. + +**Fix.** `serve_rename` relocates through `Core::state`, so the database has the move before the +reply. + +**Test:** the unit test `an_online_rename_writes_the_move_through_before_it_answers`. With the +fix, seed 1 of three-clients passes repeatedly with `PDFS_SIM_KNOWN=fail`. + ## B134 — A file deleted online while the drain checks its write is kept as a conflict copy **Status:** Open. @@ -87,7 +111,7 @@ one-client-flaky fails on it when the mkdir is still queued, which depends on ti ## B131 — A write to a file whose create could not be read back lands as a conflict copy -**Status:** Open. +**Status:** Fixed (unverified). **Found:** 2026-10-03, by the simulation runs (`sim::run`, profile three-clients, seed 1). `a.txt` was created while the link was down, and the link came back right after. The create landed, and the drain logged "pending upload failed; will retry" for its placeholder with @@ -106,9 +130,21 @@ write opened afterwards takes that node as its base. Drive stamped the file when landed, and if that was in a later second than the placeholder's mtime, the drain sees the remote revision changed and keeps the client's own write as a conflict copy. -**Test:** the simulation runs count a conflict copy that the drain kept for a queued write, when -a landed create could not be read back. With `PDFS_SIM_KNOWN=fail`, seed 1 of three-clients -fails on it when the create lands in a later second than it was made, which depends on timing. +Once the read-back succeeded, the same seed found a second gap. A write handle open on the file +is repointed at the real uid when the op retires, and its release fills its gaps from the content +cache. `adopt_real_uid` cached the create's bytes only after the read-back, which waits for Drive +to list the node. A release in between found nothing to fill from and queued an incomplete +revision, and the next write open was refused with `EIO` (as in B123). + +**Fix.** `adopt_real_uid` reads the node back with `Core::read_back`, which asks again for a moment +while Drive does not list it yet, as the online create does (B118). Before the op retires, +`Core::cache_created` caches the uploaded bytes under the real uid, keyed by the size and mtime +the tree has for the file, which is what an open handle has as its base. `adopt_real_uid` caches +them again, keyed as Drive has the node, before it rebases anything onto it. + +**Test:** seed 1 of three-clients passes with `PDFS_SIM_KNOWN=fail`. Before the fix it failed on +the conflict copy when the create landed in a later second than it was made, which depends on +timing, and then on the refused write. ## B130 — A read of a file that a remote change invalidates can hang the daemon @@ -250,7 +286,7 @@ fails on it. ## B124 — After a replacing rename fails, the file it was replacing is gone from the mount -**Status:** Open. +**Status:** Fixed (unverified). **Found:** 2026-10-03, by the simulation runs (`sim::run`, profile one-client-lan, seed 1). A rename `x/f.txt -> z/d` over an existing `z/d` failed with `EIO` (B122). Then `create z/d` failed with `ENOENT`, although `z/d` was back on Drive. @@ -263,40 +299,66 @@ every mount's state and adds it to `hidden`, which filters listings. When the re in the state, as the ordinary restore does. Until the daemon restarts, the file is on Drive but missing from the mount, and a create by its name fails. -**Test:** the simulation runs end a seed on it. With `PDFS_SIM_KNOWN=fail`, seed 1 of -`cargo test -p pdfs-fuse --lib sim::run::tests::one_client_on_a_good_link -- --ignored ---test-threads=1` fails on it. +**Fix.** `restore_replaced` takes the node out of `hidden`, and drops the listing of the folder +it is restored to, and the kernel's entry for its name, in every mount that shows the folder. The +next lookup lists the folder again and finds the node. + +**Test:** the unit test `a_replaced_node_put_back_shows_in_the_mount_again`. Since B122 is fixed, +the simulation runs no longer reach it through a rename across folders. ## B123 — A second write open on a file just written fails with `EIO` -**Status:** Open. +**Status:** Fixed (unverified). **Found:** 2026-10-03, by the simulation runs (`sim::run`, profile one-client-lan, seeds 5 and 9). A write at an offset (seed 5) or a create over an existing file (seed 9) failed with `EIO`. -The daemon logged "refusing write over incomplete queued revision". +The daemon logged "refusing write over incomplete queued revision". The one-client-echoes profile, +whose events come late, twice and out of order, found it in four more seeds. -**Where:** the write open in `crates/pdfs-fuse/src/filesystem.rs`, and the `complete` flag that -`queue_revision` in `crates/pdfs-fuse/src/lib.rs` stamps on a queued revision. +**Where:** the write open in `crates/pdfs-fuse/src/filesystem.rs`, the `complete` flag that +`queue_revision` in `crates/pdfs-fuse/src/lib.rs` stamps on a queued revision, and everything that +leaves the content cache without the bytes of the file's base. **Cause.** A write open over a queued revision that is not complete is refused by design, because -a second partial edit on top of one with gaps cannot be represented. Two ordinary cases leave a +a second partial edit on top of one with gaps cannot be represented. Ordinary cases left a revision incomplete, though: -- A file closed empty never counts as complete. `queue_revision` compares the authored ranges with - `[(0, len)]`, which for a length of 0 is `[(0, 0)]`, while the authored ranges are empty. The - truncate path already treats a size of 0 as complete. -- The gaps of a partial edit are filled from the content cache, and the cache can be empty - then. One way is our own rename: `rename_remote` and `move_remote` do not call - `note_self_change`, so the echo of the rename evicts the cached bytes. +- A file closed empty never counted as complete. `queue_revision` compared the authored ranges + with `[(0, len)]`, which for a length of 0 is `[(0, 0)]`, while the authored ranges are empty. + The truncate path already treated a size of 0 as complete. +- The gaps of a partial edit are filled from the content cache, and the cache could be empty or + out of reach then: + - The echo of our own change evicted it, taken for someone else's. `rename_remote`, + `move_remote` and the drain's `move_renaming` did not record an echo, and neither did a + create made online, so the create's echo took the one the upload recorded. The feed can also + deliver an event twice, or replay one after a batch out of order, and the second copy found + the echo already taken. + - Drive stamps a link's `ModifyTime` on a rename or a move too. The listing after it moved the + file's mtime, and the cache, keyed by mtime and size, no longer matched. + - When an upload landed, the drain let go of the staged blob, read the node back, rebased open + write handles onto it, and only then cached the blob. A handle released in between found + neither the blob nor the cached bytes. Either way, the next write open on the file within the drain debounce (`DRAIN_REVISION_DEBOUNCE`, -2 s) fails, until the revision has landed. +2 s) failed, until the revision had landed. -**Test:** the simulation runs count it and go on. With `PDFS_SIM_KNOWN=fail`, seeds 5 and 9 of -one-client-lan fail on it. +**Fix.** `StagedWrite::covers` counts an empty file as complete, at every site that sets the flag. +`Core::changed_remote` records the echo of every rename and move, and `fetch_minted_node` the echo +of every create made online. The event sync skips a node update whose id it applied lately +(`RecentEvents`). `State::intern` keeps a file's mtime, and the size a listing left out, while its +active revision id stays the same. `refresh_after_upload` caches the uploaded bytes before it +rebases open handles, and lets go of the staged blob only after. + +**Test:** the unit tests `an_empty_write_covers_the_file_and_a_gap_does_not`, +`a_node_update_seen_again_is_skipped_but_other_events_are_not`, +`a_renamed_file_keeps_its_mtime_until_its_content_changes` and +`a_rename_or_move_drops_the_sdk_copy_and_expects_its_echo`, and the FUSE tests +`a_file_closed_empty_takes_a_second_write_at_once` and +`the_echo_of_our_own_rename_keeps_the_cached_bytes` in `sim::daemon`. The simulation runs no +longer tolerate it. ## B122 — A rename across folders fails with `EIO` when the source folder holds the new name -**Status:** Open. +**Status:** Fixed (unverified). **Found:** 2026-10-03, by the simulation runs (`sim::run`, profile one-client-lan, seeds 1, 7 and 10). A `rename(2)` like `x/f.txt -> z/d` failed with `EIO` while `x/d` existed. The daemon logged "rename failed" with Drive's `AlreadyExists`. @@ -309,8 +371,12 @@ the source folder with the new name makes Drive refuse the first call, although is free. B46's follow-up already notes that this path was not converted to the single `move-multiple` request the drain uses. If the rename was replacing a file, B124 follows. -**Test:** the simulation runs count it and go on. With `PDFS_SIM_KNOWN=fail`, seed 1 of -one-client-lan fails on it. +**Fix.** `Core::move_rename_remote` moves and renames in one `move-multiple` request, so the name +only has to be free in the destination. `serve_rename` uses it for every rename across folders, +and the drain uses it in place of its own `move_renaming`. + +**Test:** the FUSE test `a_rename_across_folders_ignores_the_new_name_in_the_old_folder` in +`sim::daemon`. The simulation runs no longer tolerate it. ## B121 — A move right after a rename fails with `EIO`, and a rename across folders takes seconds @@ -340,7 +406,7 @@ the cached node. Every rename, and every move but the drain's, goes through `Cor or `Core::move_remote`. Both drop the entry once the change has landed, and before each retry of a request Drive called out of date. The drain's `move_renaming` drops it once its move lands. -**Test:** the unit tests `a_rename_or_move_drops_the_sdk_copy_of_the_node` and +**Test:** the unit tests `a_rename_or_move_drops_the_sdk_copy_and_expects_its_echo` and `every_remote_rename_and_move_rides_out_a_stale_hash`. On the account run, "regression B111" and "regression B113" should show no `status=422` warnings.