mirror of
https://github.com/narrrl/proton-drive-linux.git
synced 2026-11-03 16:41:26 +01:00
fix(fuse): keep parked creates, renames and freed names from racing the drain
- B144: when Drive says a name is taken but nothing local holds it any more, the rename, move or create is sent once more under the wanted name before falling back to a conflict name. The trash that freed it could land between Drive's answer and the check. - B145: the park sweep no longer drops a parked create whose node row is not written yet. It waits a minute, and keeps open files, before taking a missing row for a deleted file. The account run's B70 check failed with EBUSY on this. - B146: a rename queued while an upload's read-back is on the wire is kept; the tree no longer takes back the name Drive still had.
This commit is contained in:
1 parent
5e0f30cc2c
commit
473842eefc
3 files changed
+149
-15
No files matched your search
@@ -745,6 +745,11 @@ impl Core {
|
||||
if self.name_is_held(op, Some(&parent), &name)? {
|
||||
return Err(held_by_queued_change(&name));
|
||||
}
|
||||
// What held it may have let go since Drive answered
|
||||
// (docs/BUGS.md B144).
|
||||
match self.move_rename_remote(&uid, &parent, target) {
|
||||
Ok(()) => name.clone(),
|
||||
Err(e) if is_already_exists(&e) => {
|
||||
let alt = conflict_name(&name, now_secs());
|
||||
warn!(%uid, name, alt, "destination already holds that name; using a conflict name");
|
||||
self.move_rename_remote(&uid, &parent, Some(&alt))?;
|
||||
@@ -757,6 +762,9 @@ impl Core {
|
||||
);
|
||||
alt
|
||||
}
|
||||
Err(e) => return Err(e.into()),
|
||||
}
|
||||
}
|
||||
// The destination folder is gone. Leaving the node in its current
|
||||
// parent is the honest outcome: it is not where the user asked for
|
||||
// it, but it exists and it is where it has always been. The name
|
||||
@@ -821,6 +829,13 @@ impl Core {
|
||||
if self.name_is_held(op, parent, name)? {
|
||||
return Err(held_by_queued_change(name));
|
||||
}
|
||||
// What held it may have let go since Drive answered
|
||||
// (docs/BUGS.md B144).
|
||||
match self.rename_remote(uid, name) {
|
||||
Ok(()) => return Ok(name.to_string()),
|
||||
Err(e) if is_already_exists(&e) => {}
|
||||
Err(e) => return Err(e.into()),
|
||||
}
|
||||
let alt = conflict_name(name, now_secs());
|
||||
warn!(%uid, name, alt, "rename target name is taken; using a conflict name");
|
||||
self.rename_remote(uid, &alt)?;
|
||||
@@ -949,6 +964,11 @@ impl Core {
|
||||
{
|
||||
return Err(held_by_queued_change(&wanted));
|
||||
}
|
||||
// What held it may have let go since Drive answered (docs/BUGS.md
|
||||
// B144).
|
||||
if real.as_ref().is_err_and(|e| is_already_exists(e.as_ref())) {
|
||||
real = self.create_drained_node(op, &parent, &wanted);
|
||||
}
|
||||
if real.as_ref().is_err_and(|e| is_already_exists(e.as_ref())) {
|
||||
name = conflict_name(&wanted, now_secs());
|
||||
warn!(%local, wanted, name, "name is taken remotely; creating under a conflict name");
|
||||
@@ -1688,7 +1708,7 @@ impl Core {
|
||||
continue;
|
||||
};
|
||||
let name = op.name.as_deref().unwrap_or("?");
|
||||
let known = match self.db.node_by_uid(&op.uid) {
|
||||
let listed = match self.db.node_by_uid(&op.uid) {
|
||||
Ok(known) => known.is_some(),
|
||||
// Never infer "deleted" from a failed read: that would discard
|
||||
// the bytes over a transient database error.
|
||||
@@ -1697,7 +1717,9 @@ impl Core {
|
||||
continue;
|
||||
}
|
||||
};
|
||||
if !known {
|
||||
match park_verdict(op.created_at, now, self.uid_is_open(&uid), listed) {
|
||||
ParkVerdict::Keep => continue,
|
||||
ParkVerdict::Drop => {
|
||||
if let Err(error) = self.discard_queued_ops(&uid) {
|
||||
warn!(%uid, name, ?error, "dropping a deleted transient's parked create failed");
|
||||
continue;
|
||||
@@ -1706,8 +1728,7 @@ impl Core {
|
||||
debug!(%uid, name, "dropped a parked create whose file was deleted");
|
||||
continue;
|
||||
}
|
||||
if park_verdict(op.created_at, now, self.uid_is_open(&uid)) != ParkVerdict::Release {
|
||||
continue;
|
||||
ParkVerdict::Release => {}
|
||||
}
|
||||
match self.db.set_create_hold(&op.uid, false) {
|
||||
Ok(true) => {
|
||||
@@ -2033,18 +2054,26 @@ impl Core {
|
||||
// A rename queued behind the upload has not reached Drive yet, and
|
||||
// until it lands the tree keeps the name and folder the user gave the
|
||||
// file, as `adopt_real_uid` does. Taking Drive's put the file back
|
||||
// under its old name.
|
||||
// under its old name. A rename queued after the check below has
|
||||
// already moved the entry when the tree is updated, so a name that
|
||||
// changed in between is the user's too (docs/BUGS.md B146).
|
||||
let placed = |st: &crate::state::State| {
|
||||
let entry = st.by_uid.get(uid).and_then(|ino| st.entries.get(ino))?;
|
||||
Some((entry.node.name.clone(), entry.node.parent_uid.clone()))
|
||||
};
|
||||
let before = placed(&self.state());
|
||||
let renaming = self
|
||||
.db
|
||||
.has_pending_op(&uid.to_string(), OP_RENAME)
|
||||
.unwrap_or(false);
|
||||
self.for_each_state(|st| {
|
||||
let moved = placed(st) != before;
|
||||
let Some(entry) = st.by_uid.get(uid).and_then(|ino| st.entries.get(ino)) else {
|
||||
return;
|
||||
};
|
||||
let parent = entry.parent;
|
||||
let mut node = node.clone();
|
||||
if renaming {
|
||||
if renaming || moved {
|
||||
node.name = entry.node.name.clone();
|
||||
node.parent_uid = entry.node.parent_uid.clone();
|
||||
}
|
||||
@@ -2169,23 +2198,41 @@ fn moved_elsewhere(meta: &RenameMeta, node: &Node, parent: &NodeUid, name: &str)
|
||||
!at(&meta.original_parent_uid, original_name) && !at(&parent.to_string(), name)
|
||||
}
|
||||
|
||||
/// What the park sweep does with one parked create whose node still exists.
|
||||
/// What the park sweep does with one parked create.
|
||||
#[derive(Debug, PartialEq, Eq)]
|
||||
enum ParkVerdict {
|
||||
/// Leave it parked: the rename it waits for can still come.
|
||||
Keep,
|
||||
/// Un-park it: nothing will rename it now, so the bytes go up as they are.
|
||||
Release,
|
||||
/// Drop it: the file was deleted, so the create is no longer wanted.
|
||||
Drop,
|
||||
}
|
||||
|
||||
/// Judge one parked create by age alone. An open node is kept whatever its age
|
||||
/// — it is still being written, and the rename is the writer's last step.
|
||||
fn park_verdict(created_at: i64, now: i64, open: bool) -> ParkVerdict {
|
||||
if open || now - created_at < PARK_EXPIRY_MS {
|
||||
return ParkVerdict::Keep;
|
||||
/// How long a parked create whose node is not `listed` is kept anyway. The
|
||||
/// create is queued before its node is written, and a sweep in between took the
|
||||
/// file for deleted (docs/BUGS.md B145).
|
||||
const PARK_UNLISTED_GRACE_MS: i64 = 60 * 1000;
|
||||
|
||||
/// Judge one parked create by its age and whether its node is `listed`. An open
|
||||
/// node is kept whatever its age — it is still being written, and the rename is
|
||||
/// the writer's last step.
|
||||
fn park_verdict(created_at: i64, now: i64, open: bool, listed: bool) -> ParkVerdict {
|
||||
let age = now - created_at;
|
||||
if open {
|
||||
ParkVerdict::Keep
|
||||
} else if !listed {
|
||||
if age < PARK_UNLISTED_GRACE_MS {
|
||||
ParkVerdict::Keep
|
||||
} else {
|
||||
ParkVerdict::Drop
|
||||
}
|
||||
} else if age < PARK_EXPIRY_MS {
|
||||
ParkVerdict::Keep
|
||||
} else {
|
||||
ParkVerdict::Release
|
||||
}
|
||||
}
|
||||
|
||||
/// The retryable error for an op whose name a change of ours has yet to free
|
||||
/// ([`Core::name_is_held`]).
|
||||
@@ -2264,21 +2311,41 @@ mod tests {
|
||||
let now = PARK_EXPIRY_MS * 10;
|
||||
// Fresh: the writer may still rename it.
|
||||
assert_eq!(
|
||||
park_verdict(now - PARK_EXPIRY_MS + 1, now, false),
|
||||
park_verdict(now - PARK_EXPIRY_MS + 1, now, false, true),
|
||||
ParkVerdict::Keep
|
||||
);
|
||||
// Old, but still open: a slow write is not an abandoned one.
|
||||
assert_eq!(
|
||||
park_verdict(now - PARK_EXPIRY_MS, now, true),
|
||||
park_verdict(now - PARK_EXPIRY_MS, now, true, true),
|
||||
ParkVerdict::Keep
|
||||
);
|
||||
// Old and closed: no rename is coming.
|
||||
assert_eq!(
|
||||
park_verdict(now - PARK_EXPIRY_MS, now, false),
|
||||
park_verdict(now - PARK_EXPIRY_MS, now, false, true),
|
||||
ParkVerdict::Release
|
||||
);
|
||||
}
|
||||
|
||||
/// A transient create is queued a moment before its node is written. A
|
||||
/// sweep in between dropped it, and the rename to the finished name then
|
||||
/// failed with `EBUSY` (B145).
|
||||
#[test]
|
||||
fn a_parked_create_is_dropped_only_once_its_file_is_gone_and_closed() {
|
||||
let now = PARK_EXPIRY_MS * 10;
|
||||
// Just queued, its node not written yet.
|
||||
assert_eq!(park_verdict(now - 3, now, false, false), ParkVerdict::Keep);
|
||||
// Still open: the writer has not let go of it.
|
||||
assert_eq!(
|
||||
park_verdict(now - PARK_UNLISTED_GRACE_MS, now, true, false),
|
||||
ParkVerdict::Keep
|
||||
);
|
||||
// Gone for a while and closed: the file was deleted.
|
||||
assert_eq!(
|
||||
park_verdict(now - PARK_UNLISTED_GRACE_MS, now, false, false),
|
||||
ParkVerdict::Drop
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn drain_authorities_match_each_operation_kind() {
|
||||
for kind in [OP_REVISION, OP_TRASH] {
|
||||
|
||||
@@ -12,6 +12,67 @@ Conventions:
|
||||
|
||||
---
|
||||
|
||||
## B146 — A file renamed just as its upload lands can go back to its old name
|
||||
|
||||
**Status:** Fixed (unverified).
|
||||
**Found:** 2026-10-04, by the CI simulation run (`sim::run`, profile one-client-echoes, seed 4).
|
||||
Client 0 renamed `a.txt` to `b.txt`, and a millisecond later renaming `b.txt` to `e.md` failed
|
||||
with `ENOENT`. Not reproduced locally.
|
||||
|
||||
**Where:** `Core::refresh_after_upload` in `crates/pdfs-fuse/src/drain.rs`.
|
||||
|
||||
**Cause.** After an upload lands, the drain reads the node back and puts Drive's metadata in the
|
||||
tree. It keeps the tree's name only while a rename is queued, and it asked the queue before taking
|
||||
the tree's lock. A rename queued between the two had already renamed the entry, and Drive's
|
||||
metadata, which still had the old name, put it back.
|
||||
|
||||
**Fix.** The drain notes the entry's name and folder before it asks the queue. If either changed
|
||||
by the time it holds the lock, a rename came in between, and the tree keeps it.
|
||||
|
||||
**Test:** none. The window is between two statements and is not hit on purpose.
|
||||
|
||||
## B145 — Renaming a just-made download to its final name can fail with "Device or resource busy"
|
||||
|
||||
**Status:** Fixed (unverified).
|
||||
**Found:** 2026-10-04, by the account acceptance run (B70 check). The browser-style rename
|
||||
`b70-download.zip.crdownload` → `b70-download.zip` failed with `EBUSY`. The journal shows
|
||||
`swept parked creates released=0 dropped=1` 3 ms after the create was queued, then
|
||||
`queued create vanished under a rename`.
|
||||
|
||||
**Where:** `Core::sweep_parked_creates` and `park_verdict` in `crates/pdfs-fuse/src/drain.rs`.
|
||||
|
||||
**Cause.** A transient file's create is parked until the rename that gives it its final name.
|
||||
`queue_local_node` queues the create before the caller writes the node's row. The park sweep
|
||||
reads a parked create with no row as a file that was deleted and drops it. A sweep that ran
|
||||
between the two steps dropped a live file's create, and the rename found nothing to rename.
|
||||
|
||||
**Fix.** A parked create with no row is dropped only once it is a minute old and not open.
|
||||
Before that it is kept, like any other park.
|
||||
|
||||
**Test:** the unit test `a_parked_create_is_dropped_only_once_its_file_is_gone_and_closed`. The
|
||||
window is a few milliseconds wide and is not hit on purpose.
|
||||
|
||||
## B144 — A rename can land under a conflict name although its target was freed
|
||||
|
||||
**Status:** Fixed (unverified).
|
||||
**Found:** 2026-10-04, by the simulation runs (`sim::run`, profile one-client-echoes, seed 4).
|
||||
Client 0 renamed `d` to `c.bin` while the trash of the old `c.bin` was queued. The rename landed
|
||||
as `c (sync-conflict …).bin`, and the client's tree and Drive disagreed.
|
||||
|
||||
**Where:** `Core::drain_rename_in_place`, `Core::drain_rename` and `Core::create_local_node` in
|
||||
`crates/pdfs-fuse/src/drain.rs`.
|
||||
|
||||
**Cause.** When Drive answers that the name is taken, the drain asks whether a queued trash or
|
||||
rename still holds it, and if so waits for that to land. The trash landed between Drive's answer
|
||||
and the check. Nothing held the name any more, so the drain took the answer as a real clash and
|
||||
fell back to a conflict name.
|
||||
|
||||
**Fix.** When nothing holds the name, the drain sends the request once more under the wanted name
|
||||
before falling back to a conflict name.
|
||||
|
||||
**Test:** seed 4 of one-client-echoes passes repeatedly. The window is too narrow to hit on
|
||||
purpose.
|
||||
|
||||
## B143 — A write to a file whose create landed as the link went down lands as a conflict copy
|
||||
|
||||
**Status:** Fixed (unverified).
|
||||
|
||||
@@ -38,6 +38,12 @@ are kept.
|
||||
busy".** The rename goes to the file Drive made (B142).
|
||||
- **A file whose upload landed just as the connection dropped no longer gets a conflict copy at
|
||||
its next save.** The daemon reads the file back once the connection returns (B143).
|
||||
- **A rename whose target name was just freed no longer lands as a conflict copy.** The rename
|
||||
is sent once more under the name it asked for (B144).
|
||||
- **Saving a download under its final name no longer fails with "Device or resource busy".**
|
||||
A file made a moment before was taken for deleted and dropped from the queue (B145).
|
||||
- **A file renamed just as its upload lands keeps its new name.** The tree could take back the
|
||||
name Drive still had (B146).
|
||||
|
||||
## [2.10.0] — 2026-10-03
|
||||
|
||||
|
||||
Reference in new issue
Block a user