diff --git a/crates/pdfs-fuse/src/drain.rs b/crates/pdfs-fuse/src/drain.rs index a41f36b..757e2a4 100644 --- a/crates/pdfs-fuse/src/drain.rs +++ b/crates/pdfs-fuse/src/drain.rs @@ -729,12 +729,12 @@ impl Core { if current == name { return Ok(current.to_string()); } - match self.rt.block_on(self.client.rename_node(uid, name, None)) { + match self.rename_remote(uid, name) { Ok(()) => Ok(name.to_string()), Err(e) if is_already_exists(&e) => { let alt = conflict_name(name, now_secs()); warn!(%uid, name, alt, "rename target name is taken; using a conflict name"); - self.rt.block_on(self.client.rename_node(uid, &alt, None))?; + self.rename_remote(uid, &alt)?; self.adopt_drained_name(uid, &alt); self.log_activity( ActivityKind::Rename, diff --git a/crates/pdfs-fuse/src/filesystem.rs b/crates/pdfs-fuse/src/filesystem.rs index 2cf2268..fa22e7d 100644 --- a/crates/pdfs-fuse/src/filesystem.rs +++ b/crates/pdfs-fuse/src/filesystem.rs @@ -2039,10 +2039,7 @@ impl ProtonFs { return; } if newname != name - && let Err(e) = self - .core - .rt - .block_on(self.core.client.rename_node(&uid, newname, None)) + && let Err(e) = self.core.rename_remote(&uid, newname) { error!(%uid, error = %e, "rename failed"); self.core.restore_replaced(victim.as_ref(), newname); diff --git a/crates/pdfs-fuse/src/lib.rs b/crates/pdfs-fuse/src/lib.rs index a493db6..94f8a4f 100644 --- a/crates/pdfs-fuse/src/lib.rs +++ b/crates/pdfs-fuse/src/lib.rs @@ -1563,6 +1563,15 @@ fn prepare_shared_roots( } impl Core { + /// Rename `uid` on Drive, riding out a stale "out of date" answer that + /// follows an earlier rename of the same node. See [`retry_stale_rename`]. + pub(crate) fn rename_remote(&self, uid: &NodeUid, name: &str) -> Result<(), ProtonError> { + retry_stale_rename( + || self.rt.block_on(self.client.rename_node(uid, name, None)), + || std::thread::sleep(RENAME_RETRY_DELAY), + ) + } + /// Record that this daemon just changed `uid` on the remote, so the event /// feed's echo of that change is recognised instead of re-applied. Called /// from the drain once the remote has actually accepted the change and the @@ -2728,9 +2737,15 @@ impl Core { // ciphertext size for up to 30 s even though the daemon has had the // real one all along. Notify *after* the DB write, so a re-`getattr` // provoked by the invalidation cannot race the persistence. + // + // Attributes only (offset -1): the content did not change, and a + // page-cache invalidation deadlocks when this runs inside a `read` + // (B100 resolves sizes there). The kernel holds the page lock of + // that read until we reply, and the invalidation waits for the + // same lock, so the read never answered (docs/BUGS.md B110). if let Some(notifier) = core.notifier.get() { for ino in changed { - let _ = notifier.inval_inode(INodeNo(ino), 0, 0); + let _ = notifier.inval_inode(INodeNo(ino), -1, 0); } } debug!(folder_ino, files = updated.len(), "filled in listing sizes"); @@ -4368,8 +4383,7 @@ impl Core { &old_parent_uid, ) .map_err(|error| self.errno_error(error, "rename access"))?; - self.rt - .block_on(self.client.rename_node(&uid, new_name, None)) + self.rename_remote(&uid, new_name) .map_err(|e| CoreError::from_api(&e, "rename"))?; // Every mount, so a fork showing the same node re-interns it under the // new name instead of keeping the old one (`docs/BUGS.md` B74). @@ -5285,6 +5299,37 @@ fn api_code(e: &(dyn std::error::Error + 'static)) -> Option { } } +/// How often a rename that Drive calls out of date is retried, and how long +/// apart. Twenty half-second waits match the move retry in the FUSE `rename`. +const RENAME_RETRIES: u32 = 20; +const RENAME_RETRY_DELAY: Duration = Duration::from_millis(500); + +/// Run a remote rename, retrying while Drive answers `InvalidRequirements`. +/// +/// A rename sends the node's current name hash, read from the server just +/// before. Right after an earlier rename of the same node that read can still +/// return the old hash, and Drive rejects the request as "out of date" until +/// its reads catch up (docs/BUGS.md B111). Any other error returns at once. +fn retry_stale_rename( + mut rename: impl FnMut() -> Result<(), ProtonError>, + mut wait: impl FnMut(), +) -> Result<(), ProtonError> { + let mut attempts = 0u32; + loop { + match rename() { + Err(e) + if api_code(&e) == Some(ResponseCode::InvalidRequirements) + && attempts < RENAME_RETRIES => + { + attempts += 1; + debug!(attempts, error = %e, "rename was out of date; retrying"); + wait(); + } + result => return result, + } + } +} + /// Whether a call failed because the name it asked for is already in use. /// /// The queue makes this reachable in a way the synchronous path never was: a @@ -8031,7 +8076,7 @@ mod tests { ("fn serve_create(", "require_uid_writable", "upload_file"), ("fn serve_mkdir(", "require_uid_writable", "create_folder"), ("fn trash_child(", "require_uid_writable", "trash_nodes"), - ("fn serve_rename(", "require_rename_access", "rename_node"), + ("fn serve_rename(", "require_rename_access", "rename_remote"), ] { let function = function_source(filesystem, signature); assert_before(function, gate, remote_call); @@ -8046,7 +8091,7 @@ mod tests { ( "fn rename(&self, rel: &Path", "require_rename_access", - ".rename_node", + ".rename_remote", ), ( "fn move_to(&self, rel: &Path", @@ -8092,6 +8137,85 @@ mod tests { assert_eq!(replaced.matches("self.withdraw_queued_ops(uid)").count(), 2); } + #[test] + fn a_size_upgrade_leaves_the_page_cache_alone() { + // Reads resolve provisional sizes inline, while the kernel holds the + // page lock of that read; dropping pages here waited on it forever. + let upgrade = function_source(include_str!("lib.rs"), "fn apply_size_upgrade("); + assert!(upgrade.contains("notifier.inval_inode(INodeNo(ino), -1, 0)")); + assert!(!upgrade.contains("notifier.inval_inode(INodeNo(ino), 0, 0)")); + let read = function_source(include_str!("filesystem.rs"), " fn read("); + assert!(read.contains("core.resolve_provisional_size(ino.0, &uid)")); + } + + fn api_error(code: super::ResponseCode) -> super::ProtonError { + super::ProtonError::Api(proton_drive_rs::proton_sdk::error::ProtonApiError { + code, + http_status: 422, + message: "This file or folder was out of date, rename failed.".into(), + details: None, + }) + } + + #[test] + fn a_rename_called_out_of_date_is_retried_until_it_lands() { + let mut calls = 0; + let mut waits = 0; + let result = super::retry_stale_rename( + || { + calls += 1; + if calls < 3 { + Err(api_error(super::ResponseCode::InvalidRequirements)) + } else { + Ok(()) + } + }, + || waits += 1, + ); + assert!(result.is_ok()); + assert_eq!((calls, waits), (3, 2)); + } + + #[test] + fn a_rename_gives_up_on_other_errors_and_after_its_retries() { + let mut calls = 0; + let taken = super::retry_stale_rename( + || { + calls += 1; + Err(api_error(super::ResponseCode::AlreadyExists)) + }, + || {}, + ); + assert!(taken.is_err()); + assert_eq!(calls, 1, "a taken name is not a stale read"); + + let mut calls = 0; + let stale = super::retry_stale_rename( + || { + calls += 1; + Err(api_error(super::ResponseCode::InvalidRequirements)) + }, + || {}, + ); + assert!(stale.is_err()); + assert_eq!(calls, super::RENAME_RETRIES + 1); + } + + #[test] + fn every_remote_rename_rides_out_a_stale_read() { + for source in [ + include_str!("lib.rs"), + include_str!("filesystem.rs"), + include_str!("drain.rs"), + ] { + assert_eq!( + source.matches(concat!("client.rename_node", "(")).count(), + usize::from(source.contains("fn rename_remote(")), + "call Core::rename_remote instead of the SDK rename directly" + ); + } + } + #[test] fn a_remote_read_gives_up_at_its_deadline() { // The SDK bounds requests, not reads; a stalled link held one `read` diff --git a/docs/BUGS.md b/docs/BUGS.md index 093b65f..52b5488 100644 --- a/docs/BUGS.md +++ b/docs/BUGS.md @@ -12,6 +12,49 @@ Conventions: --- +## B111 — Renaming a file back right after renaming it fails with EIO + +**Status:** Fixed (unverified). +**Found:** 2026-09-29, by the acceptance case "unusual but legal names", on My files and on the +on-demand folder. The case renames each file to `renamed-N` and back at once. One rename back per +run failed with `EIO`, and the journal showed `rename failed ... proton api error +InvalidRequirements (http 422): This file or folder was out of date, rename failed.` + +**Where:** every remote rename: `serve_rename` in `crates/pdfs-fuse/src/filesystem.rs`, +`drain_rename_in_place` in `crates/pdfs-fuse/src/drain.rs`, and the control `rename` in +`crates/pdfs-fuse/src/lib.rs`. + +**Cause.** The SDK's `rename_node` reads the node's details from the server and sends its current +name hash as `OriginalHash`. Right after a rename of the same node, that read can still return the +old hash. Drive then refuses the request as out of date. The daemon reported this at once as +`EIO`, although the same request succeeds a moment later. + +**Fix.** All remote renames go through `Core::rename_remote`. It retries a rename that fails with +`InvalidRequirements` up to 20 times, half a second apart, the same window the move after a +rename already uses. Any other error still returns at once. + +## B110 — A read that settles a provisional size deadlocks on its own page lock + +**Status:** Fixed (unverified). +**Found:** 2026-09-29, by the acceptance case "application workloads", which timed out on My +files and on the on-demand folder on a fast network. The journal showed `a fuse worker has held +the same job for a long time worker=pdfs-fuse-8 job=read` growing past 700 s with no network +error. `/proc//task/*/wchan` put that worker, and one more, in `__folio_lock`, while the +daemon had no file on any mount open. + +**Where:** `crates/pdfs-fuse/src/lib.rs` (`Core::apply_size_upgrade`). + +**Cause.** B100 made `read` settle a provisional size before it fetches, through +`resolve_provisional_size` and `apply_size_upgrade`. That function tells the kernel about the new +size with `inval_inode(ino, 0, 0)`, which also drops the file's page cache. The kernel holds the +page lock of the read being served until the daemon replies. The invalidation waits for the same +lock, on the thread that has to send the reply, so neither ever finishes. The process that +issued the read hangs, and so does anything else that touches those pages. + +**Fix.** The size upgrade invalidates attributes only (`offset -1`). A size upgrade does not +change content, so the page cache has nothing stale to drop. A daemon already stuck this way +needs a restart. + ## B109 — A re-listing shrinks a file that is open for writing, and SQLite reads it as malformed **Status:** Fixed (unverified). diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index b35946e..6f7779b 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -40,6 +40,12 @@ scratch (user data in `staging/` and `recovery/` is never touched by this). - **Editors can save over a file several times in a row.** The fix for B104 could leave the upload of a replaced file queued, and the next save onto the same name failed with `Input/output error` (B106). +- **Renaming a file back right after renaming it no longer fails.** Drive can briefly call the + second rename out of date, and the daemon now waits it out instead of returning an I/O error + (B111). +- **Reading a freshly listed file no longer hangs.** When a read had to fetch the file's real + size first, the daemon could wait on the read it was serving, and the program froze until + the daemon restarted (B110). - **Databases and other files kept open while written no longer read back empty.** When the folder was listed again while a file was still open, its size dropped to what Drive held, often 0 bytes, and SQLite reported `database disk image is malformed` (B109).