mirror of
https://github.com/narrrl/proton-drive-linux.git
synced 2026-11-03 16:41:26 +01:00
fix(fuse): do not send a file removed with its folder again (B168)
A delete of a file whose create was on the wire withdraws the create into a trash. When its folder was removed too and that trash landed first, Drive refused the create, and the drain re-homed the deleted file in the root; only its missing bytes stopped it. The withdrawn create's trash then read the trashed folder back into the cache before finding nothing to trash. A withdrawn create is no longer re-homed, and its trash is done once the folder it was sent to is trashed or gone.
This commit is contained in:
1 parent
9bbc994a4f
commit
8e54ed0517
6 files changed
+148
-3
No files matched your search
@@ -534,7 +534,32 @@ impl Core {
|
||||
/// and `pdfs recover` is how they get them back.
|
||||
/// * we could not ask. Ordinary deferral; the network is the network.
|
||||
fn resolve_unknown_authority(&self, op: &PendingOp, authority: &NodeUid) {
|
||||
match self.fetch_node_remote(authority) {
|
||||
let fetched = self.fetch_node_remote(authority);
|
||||
// The trash a withdrawn create left looks for the node it may have made
|
||||
// in the folder it was sent to. A folder trashed or gone took that node
|
||||
// with it, so there is nothing left to trash. Most often it is gone
|
||||
// because the user removed it with the file, and its trash landed first
|
||||
// (`docs/BUGS.md` B168).
|
||||
let folder_gone = match &fetched {
|
||||
Ok(None) => true,
|
||||
Ok(Some(node)) => node.trashed,
|
||||
Err(_) => false,
|
||||
};
|
||||
if op.kind == OP_TRASH
|
||||
&& is_local_uid_str(&op.uid)
|
||||
&& folder_gone
|
||||
&& let Some(uid) = parse_node_uid(&op.uid)
|
||||
{
|
||||
debug!(uid = %op.uid, %authority, "a withdrawn create's folder is gone; trash op satisfied");
|
||||
if let Err(error) = self.retire_trash_op(op, &uid) {
|
||||
error!(uid = %op.uid, %error, "retiring a withdrawn create's trash failed");
|
||||
self.release_claim(op);
|
||||
return;
|
||||
}
|
||||
self.create_sent.lock().remove(&op.id);
|
||||
return;
|
||||
}
|
||||
match fetched {
|
||||
Ok(Some(node)) => {
|
||||
if let Err(error) = self.db.upsert_node(&node) {
|
||||
error!(uid = %op.uid, %authority, %error,
|
||||
@@ -1139,8 +1164,13 @@ impl Core {
|
||||
// succeed as written and retrying it forever wedges the queue behind a
|
||||
// file that has bytes to save. Re-home it to the root: not where the
|
||||
// user put it, but it exists, it is visible, and the bytes are intact.
|
||||
//
|
||||
// Not for a create a delete withdrew while it was on the wire: the
|
||||
// folder is most often gone because the user removed it, with this
|
||||
// file, and the file would come back in the root (`docs/BUGS.md` B168).
|
||||
if let Some(root) = self.root_uid()
|
||||
&& real.is_err()
|
||||
&& !self.db.has_pending_op(&op.uid, OP_TRASH)?
|
||||
&& self.parent_is_gone(&parent)
|
||||
{
|
||||
warn!(%local, name, "parent folder is gone remotely; creating in the root instead");
|
||||
|
||||
@@ -649,6 +649,48 @@ mod tests {
|
||||
let _ = std::fs::remove_dir_all(&dir);
|
||||
}
|
||||
|
||||
#[test]
|
||||
#[ignore = "mounts FUSE: run with `cargo test -p pdfs-fuse sim:: -- --ignored`"]
|
||||
fn a_file_removed_with_its_folder_while_uploading_stays_removed() {
|
||||
// A folder removed while a file in it was on its way to Drive: the
|
||||
// folder's trash landed first, Drive refused the file's create, and
|
||||
// the drain took the folder for trashed elsewhere and made the deleted
|
||||
// file again in the root, failing only because its bytes were gone.
|
||||
// Its queued trash then re-read the folder it had been sent to (B168).
|
||||
let drive = FakeDrive::new();
|
||||
let dir = scratch("removed-while-uploading");
|
||||
let daemon = Daemon::start(&dir, drive.client(1, Faults::lan())).unwrap();
|
||||
let mnt = daemon.mountpoint.clone();
|
||||
std::fs::create_dir(mnt.join("x")).unwrap();
|
||||
assert!(wait_until(Duration::from_secs(30), || {
|
||||
daemon.pending().is_ok_and(|items| items.is_empty()) && drive.lookup("x").is_some()
|
||||
}));
|
||||
let folder = drive.lookup("x").unwrap();
|
||||
|
||||
let create = daemon.client.hold_next_create();
|
||||
std::fs::write(mnt.join("x/removed-while-uploading"), b"abc").unwrap();
|
||||
assert!(wait_until(Duration::from_secs(30), || create.reached()));
|
||||
std::fs::remove_file(mnt.join("x/removed-while-uploading")).unwrap();
|
||||
std::fs::remove_dir(mnt.join("x")).unwrap();
|
||||
assert!(wait_until(Duration::from_secs(30), || drive
|
||||
.lookup("x")
|
||||
.is_none()));
|
||||
drop(create);
|
||||
assert!(wait_until(Duration::from_secs(60), || {
|
||||
daemon.pending().is_ok_and(|items| items.is_empty())
|
||||
}));
|
||||
|
||||
assert_eq!(drive.names_ever_in_root(), ["x"]);
|
||||
assert!(drive.tree().is_empty());
|
||||
let rehomed = take_logged().into_iter().find(|line| {
|
||||
line.contains("creating in the root") && line.contains("removed-while-uploading")
|
||||
});
|
||||
assert_eq!(rehomed, None);
|
||||
assert!(matches!(daemon.stop(), Ok(MountOutcome::Shutdown)));
|
||||
let db = Db::open(&dir.join("pdfs.db")).unwrap();
|
||||
assert!(db.node_by_uid(&folder.to_string()).unwrap().is_none());
|
||||
}
|
||||
|
||||
#[test]
|
||||
#[ignore = "mounts FUSE: run with `cargo test -p pdfs-fuse sim:: -- --ignored`"]
|
||||
fn a_folder_made_again_while_the_old_one_is_trashed_is_a_new_folder() {
|
||||
|
||||
@@ -38,7 +38,8 @@
|
||||
//! also have a new file's next reads answered as Drive lists it before its
|
||||
//! create is committed ([`FakeClient::answer_reads_unrevised`]; B163), and
|
||||
//! hold a folder's listing on its way back with one of its files listed that
|
||||
//! way ([`FakeClient::hold_answer_to_next_listing`]).
|
||||
//! way ([`FakeClient::hold_answer_to_next_listing`]), or hold a new file's
|
||||
//! create on its way to Drive ([`FakeClient::hold_next_create`]).
|
||||
|
||||
use std::collections::{BTreeMap, HashMap};
|
||||
use std::io::{Read, Write};
|
||||
@@ -340,6 +341,7 @@ impl FakeDrive {
|
||||
drop_after_create: AtomicBool::new(false),
|
||||
lose_create_reply: AtomicBool::new(false),
|
||||
hold_create_reply: Mutex::new(None),
|
||||
hold_create: Mutex::new(None),
|
||||
lose_revision_reply: AtomicBool::new(false),
|
||||
lose_revision_read_back: AtomicBool::new(false),
|
||||
lose_read: Mutex::new(None),
|
||||
@@ -405,6 +407,23 @@ impl FakeDrive {
|
||||
tree
|
||||
}
|
||||
|
||||
/// Every name a node has had in the root, trashed or not, as far back as
|
||||
/// Drive keeps its versions.
|
||||
pub(crate) fn names_ever_in_root(&self) -> Vec<String> {
|
||||
let server = self.server.lock();
|
||||
let mut names: Vec<String> = server
|
||||
.nodes
|
||||
.values()
|
||||
.flatten()
|
||||
.filter_map(|(_, remote)| remote.as_ref())
|
||||
.filter(|remote| remote.parent.as_ref() == Some(&server.root))
|
||||
.map(|remote| remote.name.clone())
|
||||
.collect();
|
||||
names.sort();
|
||||
names.dedup();
|
||||
names
|
||||
}
|
||||
|
||||
/// The uid at `path` below the root, as Drive has it now.
|
||||
pub(crate) fn lookup(&self, path: &str) -> Option<NodeUid> {
|
||||
let server = self.server.lock();
|
||||
@@ -602,6 +621,8 @@ struct ClientInner {
|
||||
lose_create_reply: AtomicBool,
|
||||
/// The create [`FakeClient::hold_reply_to_next_create`] is waiting for.
|
||||
hold_create_reply: Mutex<Option<Arc<HoldState>>>,
|
||||
/// The create [`FakeClient::hold_next_create`] is waiting for.
|
||||
hold_create: Mutex<Option<Arc<HoldState>>>,
|
||||
/// Set by [`FakeClient::lose_reply_to_next_revision`].
|
||||
lose_revision_reply: AtomicBool,
|
||||
/// Set by [`FakeClient::lose_read_back_of_next_revision`].
|
||||
@@ -743,6 +764,18 @@ impl FakeClient {
|
||||
Held(state)
|
||||
}
|
||||
|
||||
/// Hold the next file create before it reaches Drive, until the answer
|
||||
/// is dropped.
|
||||
pub(crate) fn hold_next_create(&self) -> Held {
|
||||
let state = Arc::new(HoldState {
|
||||
uid: self.0.drive.root(),
|
||||
reached: AtomicBool::new(false),
|
||||
released: AtomicBool::new(false),
|
||||
});
|
||||
*self.0.hold_create.lock() = Some(state.clone());
|
||||
Held(state)
|
||||
}
|
||||
|
||||
/// Make the next revision of a file on Drive and lose its answer.
|
||||
pub(crate) fn lose_reply_to_next_revision(&self) {
|
||||
self.0.lose_revision_reply.store(true, Ordering::SeqCst);
|
||||
@@ -1225,6 +1258,10 @@ impl DriveApi for FakeClient {
|
||||
_aead: bool,
|
||||
) -> Result<NodeUid> {
|
||||
let content = read_all(reader)?;
|
||||
let held = self.0.hold_create.lock().take();
|
||||
if let Some(state) = held {
|
||||
wait_until_released(&state).await;
|
||||
}
|
||||
let made = self
|
||||
.request(true, |server| {
|
||||
server.new_file(
|
||||
|
||||
@@ -12,6 +12,38 @@ Conventions:
|
||||
|
||||
---
|
||||
|
||||
## B168 — A file removed with its folder while uploading is sent again, to the root
|
||||
|
||||
**Status:** Fixed.
|
||||
**Found:** 2026-10-04, by the journal check of the account run on the working tree after B165:
|
||||
"re-interned an authority missing from the local tree; retrying uid=local~151607". The wide
|
||||
directory case removed its folder while three of its files were still on their way to Drive.
|
||||
The folder's trash landed first, and Drive refused the three creates (422). The drain then
|
||||
logged "parent folder is gone remotely; creating in the root instead" for each, and failed
|
||||
only because the deleted files' bytes were gone. The trashes the deletes had queued then
|
||||
found the folder missing from the tree and read it back from Drive.
|
||||
|
||||
**Where:** `create_local_node` and `resolve_unknown_authority` in
|
||||
`crates/pdfs-fuse/src/drain.rs`.
|
||||
|
||||
**Cause.** A delete of a file whose create is on the wire turns the create's op into a trash,
|
||||
which later looks in the create's folder for a node the create may have made (B151). The
|
||||
create's own failure path did not look at that: a create refused because its folder is gone
|
||||
is re-homed in the root (for a folder trashed on another device), deleted file or not. And the
|
||||
trash's access check asks about that folder, whose row the folder's own trash had removed, so
|
||||
it read the trashed folder back into the cache, deferred, and only then found nothing to trash.
|
||||
|
||||
**Fix.** A create a delete withdrew is not re-homed. A withdrawn create's trash whose folder
|
||||
is trashed or gone is done: Drive took the node it was looking for along with the folder. It
|
||||
used to record a sync issue when the folder was gone for good.
|
||||
|
||||
**Test:** `a_file_removed_with_its_folder_while_uploading_stays_removed` in `sim/daemon.rs`
|
||||
holds the file's create on its way to Drive until the folder's trash has landed, then checks
|
||||
that nothing was sent to the root and the folder was not read back (each fails without its
|
||||
half of the fix). `FakeClient::hold_next_create` is new for it.
|
||||
|
||||
---
|
||||
|
||||
## B167 — The mount takes seconds to come up on a large Drive
|
||||
|
||||
**Status:** Fixed.
|
||||
|
||||
@@ -64,6 +64,8 @@ by its local id, in the queue, the stored tree and pins alike.
|
||||
empty (B166).
|
||||
- **The mount comes up faster on a large Drive.** Rebuilding the folder listings from the cache
|
||||
took close to 2 s with 37,000 files at every start (B167).
|
||||
- **Files deleted with their folder while uploading stay deleted.** The mount no longer tries
|
||||
to upload them to the top of your Drive instead (B168).
|
||||
- **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
|
||||
|
||||
@@ -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 B167.
|
||||
- Fixes from the simulation and account runs since then: B127 and B141 to B168.
|
||||
- **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`.
|
||||
|
||||
@@ -108,6 +108,8 @@ Tracks `docs/MILESTONE-3.0.0.md`. Update it with every commit that moves a phase
|
||||
last restart, before the new daemon had bound it: `pdfs status` reports that as not
|
||||
running, which the run's queue wait takes for an empty queue (the script's side).
|
||||
Looking into it found the start spending 1.7 s rebuilding listings (B167, fixed).
|
||||
Its journal check caught a file removed with its folder while uploading being
|
||||
sent again, to the root (B168, fixed).
|
||||
5. **Release**: 2.10.0 is tagged; the 2.9.0 tag still waits for the user's go-ahead.
|
||||
|
||||
## After 3.0.0
|
||||
|
||||
Reference in new issue
Block a user