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.
This commit is contained in:
nvrl committed 2026-10-04 06:04:20 +02:00
1 parent 6d75a56b00
commit 7269cb5118
5 files changed
+116 -9

No files matched your search

+63 -8
View File
@@ -377,9 +377,17 @@ impl Core {
// permission/quota error can become recoverable after the user // permission/quota error can become recoverable after the user
// changes account state, and deleting the row here made the blob // changes account state, and deleting the row here made the blob
// unreachable after five ordinary network failures. // unreachable after five ordinary network failures.
let backoff = DRAIN_BACKOFF_MIN //
// 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)) .saturating_mul(1u32 << attempts.min(6))
.min(DRAIN_BACKOFF_MAX); .min(DRAIN_BACKOFF_MAX)
};
warn!(uid = %op.uid, attempts, error = %err_str, "pending upload failed; will retry"); warn!(uid = %op.uid, attempts, error = %err_str, "pending upload failed; will retry");
if let Err(e) = self.db.record_op_failure( if let Err(e) = self.db.record_op_failure(
op.id, op.id,
@@ -391,6 +399,15 @@ impl Core {
self.wait_for_drain_work(); self.wait_for_drain_work();
continue; 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, // A failure to reach Drive says nothing about an earlier refusal,
// so it keeps the issue that refusal recorded. // so it keeps the issue that refusal recorded.
if let Some(issue) = refusal(e.as_ref()) { 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 // 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 // real uid (`Db::finish_create`). Until it lands, the tree keeps the
// name and folder the user gave it. // 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 // Before open handles are rebased onto the node, so one released in
// between finds what `adopt` keeps (`docs/BUGS.md` B123). // between finds what `adopt` keeps (`docs/BUGS.md` B123).
adopt(&node); adopt(&node);
@@ -1547,12 +1565,20 @@ impl Core {
st.by_uid.insert(real.clone(), ino); st.by_uid.insert(real.clone(), ino);
} }
let Some(&ino) = st.by_uid.get(real) else { let Some(&ino) = st.by_uid.get(real) else {
st.write_through(node.clone());
return; 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) { if renaming && let Some(e) = st.entries.get(&ino) {
node.name = e.node.name.clone(); node.name = e.node.name.clone();
node.parent_uid = e.node.parent_uid.clone(); node.parent_uid = e.node.parent_uid.clone();
} }
st.write_through(node.clone());
let mut landed = node.clone(); let mut landed = node.clone();
st.keep_open_write_size(ino, &mut landed); st.keep_open_write_size(ino, &mut landed);
if let Some(e) = st.entries.get_mut(&ino) { 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 // 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 // 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 // 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 /// The retryable error for an op whose name a change of ours has yet to free
/// ([`Core::name_is_held`]). /// ([`Core::name_is_held`]).
fn held_by_queued_change(name: &str) -> Box<dyn std::error::Error> { fn held_by_queued_change(name: &str) -> Box<dyn std::error::Error> {
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::<NameHeld>())
} }
/// How many attempts an op waits for a rename or a create on the wire to free /// 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))); 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] #[test]
fn a_parked_create_is_dropped_only_once_its_file_is_gone_and_closed() { fn a_parked_create_is_dropped_only_once_its_file_is_gone_and_closed() {
let now = PARK_EXPIRY_MS * 10; let now = PARK_EXPIRY_MS * 10;
+5
View File
@@ -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 { pub(crate) fn intern(&mut self, parent: u64, mut node: Node) -> u64 {
self.keep_revision_mtime(&mut node); self.keep_revision_mtime(&mut node);
self.outbox.push(DbWrite::Upsert(vec![node.clone()])); self.outbox.push(DbWrite::Upsert(vec![node.clone()]));
+40
View File
@@ -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 ## B151 — A file deleted after its create lost its answer stays on Drive
**Status:** Fixed (unverified). **Status:** Fixed (unverified).
+4
View File
@@ -33,6 +33,10 @@ folder by local id too.
for this release. for this release.
### Fixed ### 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 - **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). 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 - **A new file no longer lands twice when the link loses Drive's answer to its upload.** The
+4 -1
View File
@@ -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 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 - **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). 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, - **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`. 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 - [ ] 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 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. 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 - [ ] Known-bug list in `sim/run.rs` is empty: B125 and B130 open; B127, B129 and B132 fixed
but unconfirmed. but unconfirmed.
- [ ] A clean account run. The one on a6f36d5 (2026-10-04, LAN: 427 passed, 12 failed) - [ ] A clean account run. The one on a6f36d5 (2026-10-04, LAN: 427 passed, 12 failed)