mirror of
https://github.com/narrrl/proton-drive-linux.git
synced 2026-11-03 16:41:26 +01:00
fix(fuse): keep what an open file reads when its delete is queued (B186)
This commit is contained in:
1 parent
d26dc8ada4
commit
c271a1897c
6 files changed
+109
-5
No files matched your search
@@ -994,6 +994,17 @@ impl Core {
|
|||||||
Err(e) => return Err(e.into()),
|
Err(e) => return Err(e.into()),
|
||||||
}
|
}
|
||||||
self.retire_trash_op(op, &uid)?;
|
self.retire_trash_op(op, &uid)?;
|
||||||
|
// A file trashed while open kept the bytes its handle reads, and a
|
||||||
|
// final close while the trash was queued left them to this
|
||||||
|
// (`docs/BUGS.md` B186).
|
||||||
|
if !self.is_open_anywhere(&uid) {
|
||||||
|
if let Some(blob) = self.pending_blob(&uid) {
|
||||||
|
self.release_pending(&uid, &blob);
|
||||||
|
self.cache.discard_staged(&blob);
|
||||||
|
}
|
||||||
|
self.cache.evict(&uid);
|
||||||
|
self.evict_reader(&uid);
|
||||||
|
}
|
||||||
if let Err(e) = self.db.clear_own_sealed_rev(&uid.to_string()) {
|
if let Err(e) = self.db.clear_own_sealed_rev(&uid.to_string()) {
|
||||||
debug!(%uid, error = %e, "clearing the sealed-revision record failed");
|
debug!(%uid, error = %e, "clearing the sealed-revision record failed");
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -3902,6 +3902,11 @@ impl Core {
|
|||||||
/// staged bytes that never landed — which is precisely what deleting an
|
/// staged bytes that never landed — which is precisely what deleting an
|
||||||
/// un-uploaded file means, and the alternative (upload it, then trash it) is
|
/// un-uploaded file means, and the alternative (upload it, then trash it) is
|
||||||
/// worse in every way.
|
/// worse in every way.
|
||||||
|
///
|
||||||
|
/// A file still open keeps the bytes its handle reads, the queued ones and
|
||||||
|
/// the cached ones, as when the trash is sent at once: a partial write
|
||||||
|
/// fills its gaps from them, offline too (`docs/BUGS.md` B186). The final
|
||||||
|
/// close or the trash landing, whichever comes last, lets go of them.
|
||||||
fn queue_trash(&self, uid: &NodeUid, name: &str) -> Result<(), Errno> {
|
fn queue_trash(&self, uid: &NodeUid, name: &str) -> Result<(), Errno> {
|
||||||
self.require_uid_writable(uid)?;
|
self.require_uid_writable(uid)?;
|
||||||
// The bytes below are about to be discarded, and a drain worker may be
|
// The bytes below are about to be discarded, and a drain worker may be
|
||||||
@@ -3917,7 +3922,18 @@ impl Core {
|
|||||||
error!(%uid, error = %e, "queueing trash failed");
|
error!(%uid, error = %e, "queueing trash failed");
|
||||||
Errno::EIO
|
Errno::EIO
|
||||||
})?;
|
})?;
|
||||||
self.release_dropped_ops(uid, blobs);
|
let open_now = self.is_open_anywhere(uid);
|
||||||
|
if open_now {
|
||||||
|
let read_from = self.pending_blob(uid);
|
||||||
|
self.release_dropped_below(
|
||||||
|
blobs
|
||||||
|
.into_iter()
|
||||||
|
.filter(|blob| read_from.as_deref() != Some(Path::new(blob)))
|
||||||
|
.collect(),
|
||||||
|
);
|
||||||
|
} else {
|
||||||
|
self.release_dropped_ops(uid, blobs);
|
||||||
|
}
|
||||||
self.hidden.lock().insert(uid.clone());
|
self.hidden.lock().insert(uid.clone());
|
||||||
// Withdrawn from the tree, which every mount serves from, so no other
|
// Withdrawn from the tree, which every mount serves from, so no other
|
||||||
// mount goes on serving a file the user just trashed (`docs/BUGS.md`
|
// mount goes on serving a file the user just trashed (`docs/BUGS.md`
|
||||||
@@ -3925,8 +3941,10 @@ impl Core {
|
|||||||
self.for_each_state(|st| {
|
self.for_each_state(|st| {
|
||||||
st.unlink_mem(uid);
|
st.unlink_mem(uid);
|
||||||
});
|
});
|
||||||
self.cache.evict(uid);
|
if !open_now {
|
||||||
self.evict_reader(uid);
|
self.cache.evict(uid);
|
||||||
|
self.evict_reader(uid);
|
||||||
|
}
|
||||||
self.wake_drain();
|
self.wake_drain();
|
||||||
debug!(%uid, name, "trashed offline; queued");
|
debug!(%uid, name, "trashed offline; queued");
|
||||||
Ok(())
|
Ok(())
|
||||||
|
|||||||
@@ -423,6 +423,59 @@ mod tests {
|
|||||||
let _ = std::fs::remove_dir_all(&dir);
|
let _ = std::fs::remove_dir_all(&dir);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
#[ignore = "mounts FUSE: run with `cargo test -p pdfs-fuse sim:: -- --ignored`"]
|
||||||
|
fn a_file_deleted_offline_while_open_still_reads_and_takes_writes() {
|
||||||
|
// Queuing the trash dropped the cached bytes the open handle reads,
|
||||||
|
// and a partial write offline failed with EIO (B186).
|
||||||
|
use std::io::{Read, Seek, SeekFrom, Write};
|
||||||
|
let drive = FakeDrive::new();
|
||||||
|
let dir = scratch("unlink-open-offline");
|
||||||
|
let daemon = Daemon::start(&dir, drive.client(1, Faults::lan())).unwrap();
|
||||||
|
let path = daemon.mountpoint.join("f.txt");
|
||||||
|
|
||||||
|
std::fs::write(&path, b"0123456789").unwrap();
|
||||||
|
assert!(wait_until(Duration::from_secs(30), || {
|
||||||
|
daemon.pending().is_ok_and(|items| items.is_empty())
|
||||||
|
&& drive.tree().get("f.txt") == Some(&Entry::File(Arc::new(b"0123456789".to_vec())))
|
||||||
|
}));
|
||||||
|
daemon.client.set_online(false);
|
||||||
|
let mut file = std::fs::OpenOptions::new()
|
||||||
|
.read(true)
|
||||||
|
.write(true)
|
||||||
|
.open(&path)
|
||||||
|
.unwrap();
|
||||||
|
std::fs::remove_file(&path).unwrap();
|
||||||
|
file.seek(SeekFrom::End(0)).unwrap();
|
||||||
|
file.write_all(b"ab").unwrap();
|
||||||
|
let mut read = Vec::new();
|
||||||
|
file.seek(SeekFrom::Start(0)).unwrap();
|
||||||
|
file.read_to_end(&mut read).unwrap();
|
||||||
|
assert_eq!(read, b"0123456789ab");
|
||||||
|
drop(file);
|
||||||
|
|
||||||
|
daemon.client.set_online(true);
|
||||||
|
assert!(
|
||||||
|
wait_until(Duration::from_secs(30), || {
|
||||||
|
daemon.pending().is_ok_and(|items| items.is_empty()) && drive.tree().is_empty()
|
||||||
|
}),
|
||||||
|
"{:?} {:?}",
|
||||||
|
daemon.pending(),
|
||||||
|
drive.tree()
|
||||||
|
);
|
||||||
|
// The bytes kept for the handle went once the trash landed with it closed.
|
||||||
|
let cached: Vec<_> = std::fs::read_dir(dir.join("cache"))
|
||||||
|
.unwrap()
|
||||||
|
.filter_map(|e| e.ok())
|
||||||
|
.filter(|e| e.file_type().is_ok_and(|t| t.is_file()))
|
||||||
|
.map(|e| e.file_name())
|
||||||
|
.collect();
|
||||||
|
assert!(cached.is_empty(), "{cached:?}");
|
||||||
|
|
||||||
|
assert!(matches!(daemon.stop(), Ok(MountOutcome::Shutdown)));
|
||||||
|
let _ = std::fs::remove_dir_all(&dir);
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
#[ignore = "mounts FUSE: run with `cargo test -p pdfs-fuse sim:: -- --ignored`"]
|
#[ignore = "mounts FUSE: run with `cargo test -p pdfs-fuse sim:: -- --ignored`"]
|
||||||
fn a_write_opened_as_an_upload_lands_takes_the_landed_revision_as_its_base() {
|
fn a_write_opened_as_an_upload_lands_takes_the_landed_revision_as_its_base() {
|
||||||
|
|||||||
@@ -12,6 +12,25 @@ Conventions:
|
|||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
|
## B186 — A file deleted offline while open fails a partial write with "Input/output error"
|
||||||
|
|
||||||
|
**Status:** Fixed (unverified).
|
||||||
|
**Found:** 2026-10-05, in the confirmation run on 3073052 (`three_clients_one_writer_each`
|
||||||
|
seed 4; it fails on da013c2 too). With its link down, a client opened a file it had uploaded,
|
||||||
|
deleted it, and appended to it. The append took 30 s and failed with `EIO`.
|
||||||
|
|
||||||
|
**Where:** `Core::queue_trash` in `crates/pdfs-fuse/src/lib.rs` and `Core::drain_trash` in
|
||||||
|
`crates/pdfs-fuse/src/drain.rs`.
|
||||||
|
|
||||||
|
**Cause.** A delete sent to Drive at once keeps what an open handle reads, its queued and cached
|
||||||
|
bytes, until the final close (B106). A delete queued, as local-first and offline do, dropped
|
||||||
|
them at once. A partial write fills its gaps from those bytes, so it had to download the file,
|
||||||
|
and offline it could not.
|
||||||
|
|
||||||
|
**Fix.** A queued delete of an open file keeps them as well. The final close lets go of them
|
||||||
|
once the delete has landed, and the delete landing lets go of them once the file is closed.
|
||||||
|
Sim test `a_file_deleted_offline_while_open_still_reads_and_takes_writes`.
|
||||||
|
|
||||||
## B185 — A new file written again just as it starts uploading waits out a retry
|
## B185 — A new file written again just as it starts uploading waits out a retry
|
||||||
|
|
||||||
**Status:** Fixed (unverified).
|
**Status:** Fixed (unverified).
|
||||||
|
|||||||
@@ -107,6 +107,8 @@ by its local id, in the queue, the stored tree and pins alike.
|
|||||||
(B184).
|
(B184).
|
||||||
- **A new file written again just as it starts uploading no longer waits out a retry** before
|
- **A new file written again just as it starts uploading no longer waits out a retry** before
|
||||||
it reaches Drive (B185).
|
it reaches Drive (B185).
|
||||||
|
- **A file deleted while open can still be read and written until it is closed**, offline too.
|
||||||
|
A write into it no longer failed with "Input/output error" (B186).
|
||||||
- **A file renamed just as its upload finished keeps its new name.** The mount no longer showed
|
- **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,
|
the old name, and a second rename no longer failed with "No such file or directory" (B152,
|
||||||
B172).
|
B172).
|
||||||
|
|||||||
@@ -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: B125, B127, B130 and B141 to B185.
|
- Fixes from the simulation and account runs since then: B125, B127, B130 and B141 to B186.
|
||||||
- **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`; Discard button and
|
tray and CLI; Export button and `pdfs sync export`; `pdfs sync issues`; Discard button and
|
||||||
`pdfs sync discard`; a warning badge on the node in the file browser.
|
`pdfs sync discard`; a warning badge on the node in the file browser.
|
||||||
@@ -111,7 +111,8 @@ Tracks `docs/MILESTONE-3.0.0.md`. Update it with every commit that moves a phase
|
|||||||
what its create sent (B183). Replays with those fixed found a third: a create renamed
|
what its create sent (B183). Replays with those fixed found a third: a create renamed
|
||||||
after it lost its answer (B184). All three fixed; no known-bug matcher fired. It was
|
after it lost its answer (B184). All three fixed; no known-bug matcher fired. It was
|
||||||
stopped before `three_clients`. The rerun on 3073052 found a create that waited out
|
stopped before `three_clients`. The rerun on 3073052 found a create that waited out
|
||||||
a retry after a write replaced its blob (B185, fixed).
|
a retry after a write replaced its blob (B185, fixed), and a file deleted offline
|
||||||
|
while open that lost the bytes its handle read (B186, fixed; older than 3.0.0).
|
||||||
- [ ] 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)
|
||||||
failed B113 and B121 on every on-demand mount, two mirror-to-mirror move cases, and
|
failed B113 and B121 on every on-demand mount, two mirror-to-mirror move cases, and
|
||||||
the journal check. B121 was a bug (B149, fixed); B113's case still expected creates
|
the journal check. B121 was a bug (B149, fixed); B113's case still expected creates
|
||||||
|
|||||||
Reference in new issue
Block a user