From 7269cb511824a5825d030aa81250a8e54aa51096 Mon Sep 17 00:00:00 2001 From: Nils Pukropp Date: Sun, 4 Oct 2026 06:04:20 +0200 Subject: [PATCH] fix(fuse): keep a rename made as its create lands (B152); wake an op once the name it waits for is free (B153) Adoption asks again under the tree lock whether a rename is queued, and writes the node row through the lock's outbox, so a rename made while the create landed is no longer undone in the tree and the row. An op that finds its name held fails as NameHeld: it backs off by the minimum without doubling, and wakes the waiters for its name again once its failure is recorded, so a trash that landed in between is not missed. --- crates/pdfs-fuse/src/drain.rs | 73 +++++++++++++++++++++++++++++----- crates/pdfs-fuse/src/state.rs | 5 +++ docs/BUGS.md | 40 +++++++++++++++++++ docs/CHANGELOG.md | 4 ++ docs/MILESTONE-3.0.0-STATUS.md | 5 ++- 5 files changed, 117 insertions(+), 10 deletions(-) diff --git a/crates/pdfs-fuse/src/drain.rs b/crates/pdfs-fuse/src/drain.rs index 44a97ba..f347672 100644 --- a/crates/pdfs-fuse/src/drain.rs +++ b/crates/pdfs-fuse/src/drain.rs @@ -377,9 +377,17 @@ impl Core { // permission/quota error can become recoverable after the user // changes account state, and deleting the row here made the blob // unreachable after five ordinary network failures. - let backoff = DRAIN_BACKOFF_MIN - .saturating_mul(1u32 << attempts.min(6)) - .min(DRAIN_BACKOFF_MAX); + // + // A name a change of ours is about to free is a wait, not a + // failure: the trash or rename holding it lands in moments. + let held = waits_for_name(e.as_ref()); + let backoff = if held { + DRAIN_BACKOFF_MIN + } else { + DRAIN_BACKOFF_MIN + .saturating_mul(1u32 << attempts.min(6)) + .min(DRAIN_BACKOFF_MAX) + }; warn!(uid = %op.uid, attempts, error = %err_str, "pending upload failed; will retry"); if let Err(e) = self.db.record_op_failure( op.id, @@ -391,6 +399,15 @@ impl Core { self.wait_for_drain_work(); continue; } + // What held the name wakes the ops waiting for it as it lets go + // (`Db::wake_ops_waiting_for`). Letting go between our check and + // the record above woke nothing, and the op sat out its backoff. + if held + && let Some(name) = op.name.as_deref() + && let Err(e) = self.db.wake_ops_waiting_for(name) + { + debug!(uid = %op.uid, error = %e, "waking the ops waiting for a name failed"); + } // A failure to reach Drive says nothing about an earlier refusal, // so it keeps the issue that refusal recorded. if let Some(issue) = refusal(e.as_ref()) { @@ -1526,7 +1543,8 @@ impl Core { // A rename made while the create was on the wire is queued against the // real uid (`Db::finish_create`). Until it lands, the tree keeps the // name and folder the user gave it. - let renaming = self.db.has_pending_op(&real.to_string(), OP_RENAME)?; + let real_key = real.to_string(); + let renaming = self.db.has_pending_op(&real_key, OP_RENAME)?; // Before open handles are rebased onto the node, so one released in // between finds what `adopt` keeps (`docs/BUGS.md` B123). adopt(&node); @@ -1547,12 +1565,20 @@ impl Core { st.by_uid.insert(real.clone(), ino); } let Some(&ino) = st.by_uid.get(real) else { + st.write_through(node.clone()); return; }; + // Asked again under the lock: `Core::queue_rename` queues its op + // before it renames the entry under this lock. Asked only before, + // a rename made since was overwritten with the name the create was + // sent with, here and in the node row (`docs/BUGS.md` B152). + let renaming = + renaming || matches!(self.db.has_pending_op(&real_key, OP_RENAME), Ok(true)); if renaming && let Some(e) = st.entries.get(&ino) { node.name = e.node.name.clone(); node.parent_uid = e.node.parent_uid.clone(); } + st.write_through(node.clone()); let mut landed = node.clone(); st.keep_open_write_size(ino, &mut landed); if let Some(e) = st.entries.get_mut(&ino) { @@ -1582,10 +1608,6 @@ impl Core { } } }); - // Write the real node through, now that nothing points at the old uid. - if let Err(e) = self.db.upsert_node(&node) { - warn!(%real, error = %e, "db upsert_node failed after remap"); - } // A handle still open on the placeholder was based on no revision at // all: a local-clock mtime and no revision id. Released as it is, its // write would land as a conflict copy of the empty file this create just @@ -2416,7 +2438,28 @@ fn park_verdict(created_at: i64, now: i64, open: bool, listed: bool) -> ParkVerd /// The retryable error for an op whose name a change of ours has yet to free /// ([`Core::name_is_held`]). fn held_by_queued_change(name: &str) -> Box { - format!("{name} is still held by a node whose trash or rename is queued").into() + Box::new(NameHeld(name.to_string())) +} + +/// An op waits for a queued change of ours to free the name it wants. +#[derive(Debug)] +struct NameHeld(String); + +impl std::fmt::Display for NameHeld { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!( + f, + "{} is still held by a node whose trash or rename is queued", + self.0 + ) + } +} + +impl std::error::Error for NameHeld {} + +/// Whether a failed attempt only waited for a name ([`NameHeld`]). +fn waits_for_name(e: &(dyn std::error::Error + 'static)) -> bool { + std::iter::successors(Some(e), |e| e.source()).any(|e| e.is::()) } /// How many attempts an op waits for a rename or a create on the wire to free @@ -2577,6 +2620,18 @@ mod tests { assert_eq!(refusal(&wrapped), Some(Some(SyncIssue::Quota))); } + /// An op that waited for a queued trash to free its name retried only + /// after the doubled backoff, past the moment the trash landed. + #[test] + fn a_held_name_is_a_wait_and_other_failures_are_not() { + assert!(waits_for_name(held_by_queued_change("c.bin").as_ref())); + assert_eq!( + held_by_queued_change("c.bin").to_string(), + "c.bin is still held by a node whose trash or rename is queued" + ); + assert!(!waits_for_name(&std::io::Error::other("disk"))); + } + #[test] fn a_parked_create_is_dropped_only_once_its_file_is_gone_and_closed() { let now = PARK_EXPIRY_MS * 10; diff --git a/crates/pdfs-fuse/src/state.rs b/crates/pdfs-fuse/src/state.rs index e38ebe6..dad6208 100644 --- a/crates/pdfs-fuse/src/state.rs +++ b/crates/pdfs-fuse/src/state.rs @@ -884,6 +884,11 @@ impl State { } } + /// Write `node` through as it is, in order with the tree's other changes. + pub(crate) fn write_through(&mut self, node: Node) { + self.outbox.push(DbWrite::Upsert(vec![node])); + } + 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()])); diff --git a/docs/BUGS.md b/docs/BUGS.md index c6d1bd0..039c503 100644 --- a/docs/BUGS.md +++ b/docs/BUGS.md @@ -12,6 +12,46 @@ Conventions: --- +## B153 โ€” An op waiting for a name sat out its backoff after the name was free + +**Status:** Fixed (unverified). +**Found:** 2026-10-04, by the simulation's LAN profile (`one_client_on_a_good_link`, seed 4, +about one run in five, also before the B151 change). `d` was renamed over `c.bin` while the +create of the old `c.bin` was on the wire. The queues did not drain within 4 s: the rename still +owed `c.bin is still held by a node whose trash or rename is queued`. + +**Where:** the failure path of `Core::run_pending_drain` in `crates/pdfs-fuse/src/drain.rs`. + +**Cause.** The rename found the name held by the queued trash of the old `c.bin` and failed. The +trash landed a millisecond later and woke the ops waiting for the name +(`Db::wake_ops_waiting_for`), before the rename had recorded its failure, so there was nothing to +wake. The failure then set the doubled backoff of a network error, 4 s on the first attempt and +more after it. + +**Fix.** A held name fails as `NameHeld`, which backs off by the minimum (2 s) without doubling, +and the op wakes the waiters for its name again once its failure is recorded. Unit test +`a_held_name_is_a_wait_and_other_failures_are_not`; the seed is the regression check. + +## B152 โ€” A rename made as its create landed was undone in the tree + +**Status:** Fixed (unverified). +**Found:** 2026-10-04, by the simulation's LAN profile (`one_client_on_a_good_link`, seed 4, +about one run in ten). `a.txt` was renamed to `b.txt` 3 ms before its create finished landing. +The next rename, `b.txt` to `e.md`, answered `ENOENT`, and the node row kept `a.txt` although +Drive had `b.txt`. + +**Where:** `Core::adopt_real_uid` in `crates/pdfs-fuse/src/drain.rs`. + +**Cause.** Adoption puts the node Drive made into the tree, but keeps the tree's name and folder +while a rename is queued. It asked whether one was queued before it wrote the remote uid into the +queue and took the tree lock. A rename between the two was overwritten with the name the create +was sent with, in the tree and in the node row, which was written after the lock. + +**Fix.** Adoption asks again under the tree lock. `Core::queue_rename` queues its op before it +renames the entry under that lock, so a renamed entry always has its op. The node row is written +through the lock's outbox, in order with the rename's own write. No test: the window is inside +one call and no fake hook reaches it; the seed is the regression check. + ## B151 โ€” A file deleted after its create lost its answer stays on Drive **Status:** Fixed (unverified). diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 0a5c792..6ccd4b5 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -33,6 +33,10 @@ folder by local id too. for this release. ### Fixed +- **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 + upload over a file being deleted goes out as soon as the delete lands (B153). - **A file deleted right after it was made no longer stays on Drive when the link lost Drive's answer.** The node Drive made is trashed (B151). - **A new file no longer lands twice when the link loses Drive's answer to its upload.** The diff --git a/docs/MILESTONE-3.0.0-STATUS.md b/docs/MILESTONE-3.0.0-STATUS.md index 25e7110..aa26126 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 B151. +- Fixes from the simulation and account runs since then: B127 and B141 to B153. - **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`. @@ -49,6 +49,9 @@ Tracks `docs/MILESTONE-3.0.0.md`. Update it with every commit that moves a phase - [ ] Simulation passes on the Wi-Fi profile: `one_client_on_a_slow_link` failed in CI on seeds 2 and 3. Two bugs (B127, B151) and a harness gap: a settle did not read the log, so it could not explain a known bug's damage. All three fixed; not yet confirmed in CI. + Replays then found two races on the LAN profile too (B152, B153), both fixed. One + slow-link run of seed 2 failed once with a rename answering `ENOENT` as a folder move + landed. Not reproduced in 8 runs since; it may be B152. - [ ] 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)