mirror of
https://github.com/narrrl/proton-drive-linux.git
synced 2026-11-03 16:41:26 +01:00
fix(fuse): wait for a create on the wire that holds the name (B154); a write whose answer was lost is not a conflict (B155)
B154 is a regression from the B153 fix: after a failure on a held name the op woke the ops waiting for it, but the database cannot see a create on the wire, so the op was due again at once and spent its waits in milliseconds. No wake while another create on the wire holds the name. B155: a revision upload that reached Drive but lost its answer left the file at a revision this daemon never heard of, and the retry kept the write as a conflict copy of itself. A complete write whose bytes the remote holds, by size and SHA-1, is sent again instead. The fake Drive gains hold_reply_to_next_create and lose_reply_to_next_revision for the two new sim tests.
This commit is contained in:
1 parent
7269cb5118
commit
3f74b95b34
6 files changed
+204
-12
No files matched your search
@@ -402,8 +402,12 @@ impl Core {
|
||||
// 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.
|
||||
// A create on the wire holds it where the database cannot see:
|
||||
// woken here, the op spent its waits in milliseconds and landed
|
||||
// under a conflict name (`docs/BUGS.md` B154).
|
||||
if held
|
||||
&& let Some(name) = op.name.as_deref()
|
||||
&& !self.creating_named(&op, name)
|
||||
&& let Err(e) = self.db.wake_ops_waiting_for(name)
|
||||
{
|
||||
debug!(uid = %op.uid, error = %e, "waking the ops waiting for a name failed");
|
||||
@@ -1238,6 +1242,15 @@ impl Core {
|
||||
Ok(creating || self.db.has_pending_move_from(name)?)
|
||||
}
|
||||
|
||||
/// Whether a create other than `op` is on the wire under `name`, in any
|
||||
/// folder.
|
||||
fn creating_named(&self, op: &PendingOp, name: &str) -> bool {
|
||||
self.creating
|
||||
.lock()
|
||||
.iter()
|
||||
.any(|(id, (_, n))| *id != op.id && n == name)
|
||||
}
|
||||
|
||||
/// Trash the node a create made after the file was deleted, or replaced by
|
||||
/// a rename, while its upload was on the wire (`docs/BUGS.md` B129).
|
||||
///
|
||||
@@ -1657,6 +1670,7 @@ impl Core {
|
||||
&self,
|
||||
uid: &NodeUid,
|
||||
meta: &StagedWrite,
|
||||
blob: &Path,
|
||||
) -> Result<Option<String>, Box<dyn std::error::Error>> {
|
||||
let Some(ref base) = meta.based_on else {
|
||||
return Ok(None);
|
||||
@@ -1687,6 +1701,21 @@ impl Core {
|
||||
"queued write chains onto our own sealed revision; not a conflict");
|
||||
return Ok(None);
|
||||
}
|
||||
// An earlier attempt at this write may have landed with its answer
|
||||
// lost: the remote then holds these very bytes in a revision we never
|
||||
// heard of. Forked, the write was a conflict copy of itself
|
||||
// (docs/BUGS.md B155); sent again, it is a revision of the same bytes.
|
||||
if meta.complete
|
||||
&& node_size(&node) == meta.len
|
||||
&& let NodeKind::File {
|
||||
content_sha1: Some(remote),
|
||||
..
|
||||
} = &node.kind
|
||||
&& remote.eq_ignore_ascii_case(&staged_sha1(blob)?)
|
||||
{
|
||||
debug!(%uid, ?remote_rev, "the remote already holds the queued write; not a conflict");
|
||||
return Ok(None);
|
||||
}
|
||||
Ok(Some(reason))
|
||||
}
|
||||
|
||||
@@ -2014,7 +2043,7 @@ impl Core {
|
||||
let meta: StagedWrite = serde_json::from_str(op.meta_json.as_deref().unwrap_or(""))?;
|
||||
let uid = parse_node_uid(&meta.uid).ok_or("staged write has an unparseable uid")?;
|
||||
|
||||
if let Some(reason) = self.revision_conflict(&uid, &meta)? {
|
||||
if let Some(reason) = self.revision_conflict(&uid, &meta, &blob)? {
|
||||
// Trashing the file from the mount drops its queued write, but not
|
||||
// from under a worker already holding it. That worker then finds the
|
||||
// node trashed — by us — and would upload the withdrawn bytes as a
|
||||
|
||||
@@ -535,6 +535,84 @@ 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_made_under_the_name_of_a_create_on_the_wire_waits_for_it() {
|
||||
// A file renamed while its create was on the wire, and a new one made
|
||||
// under its old name. The new file's create found the name held by the
|
||||
// first and woke itself again after every try: it spent its waits in
|
||||
// milliseconds and landed under a conflict name, which the mount then
|
||||
// showed instead of the user's (B154).
|
||||
let drive = FakeDrive::new();
|
||||
let dir = scratch("create-waits-for-create");
|
||||
let daemon = Daemon::start(&dir, drive.client(1, Faults::lan())).unwrap();
|
||||
let mount = &daemon.mountpoint;
|
||||
let file = |bytes: &[u8]| Some(Entry::File(Arc::new(bytes.to_vec())));
|
||||
|
||||
let held = daemon.client.hold_reply_to_next_create();
|
||||
std::fs::write(mount.join("c.bin"), b"first").unwrap();
|
||||
assert!(wait_until(Duration::from_secs(30), || held.reached()));
|
||||
std::fs::rename(mount.join("c.bin"), mount.join("d")).unwrap();
|
||||
std::fs::write(mount.join("c.bin"), b"second").unwrap();
|
||||
assert!(wait_until(Duration::from_secs(30), || {
|
||||
daemon
|
||||
.pending()
|
||||
.is_ok_and(|items| items.iter().any(|op| op.attempts > 0))
|
||||
}));
|
||||
// Long enough for the waits to run out, were they not waits.
|
||||
std::thread::sleep(Duration::from_millis(300));
|
||||
drop(held);
|
||||
assert!(
|
||||
wait_until(Duration::from_secs(30), || {
|
||||
daemon.pending().is_ok_and(|items| items.is_empty())
|
||||
&& drive.tree().get("d").cloned() == file(b"first")
|
||||
&& drive.tree().get("c.bin").cloned() == file(b"second")
|
||||
}),
|
||||
"{:?} {:?}",
|
||||
daemon.pending(),
|
||||
drive.tree()
|
||||
);
|
||||
assert_eq!(drive.tree().len(), 2, "{:?}", drive.tree());
|
||||
assert_eq!(std::fs::read(mount.join("c.bin")).unwrap(), b"second");
|
||||
|
||||
assert!(matches!(daemon.stop(), Ok(MountOutcome::Shutdown)));
|
||||
let _ = std::fs::remove_dir_all(&dir);
|
||||
}
|
||||
|
||||
#[test]
|
||||
#[ignore = "mounts FUSE: run with `cargo test -p pdfs-fuse sim:: -- --ignored`"]
|
||||
fn a_write_whose_answer_was_lost_lands_once() {
|
||||
// Drive made the revision, but its answer was lost. The retry found
|
||||
// the file at a revision it did not know and kept the write as a
|
||||
// conflict copy of itself (B155).
|
||||
let drive = FakeDrive::new();
|
||||
let dir = scratch("lost-revision-reply");
|
||||
let daemon = Daemon::start(&dir, drive.client(1, Faults::lan())).unwrap();
|
||||
let path = daemon.mountpoint.join("f.txt");
|
||||
let file = |bytes: &[u8]| Some(Entry::File(Arc::new(bytes.to_vec())));
|
||||
|
||||
std::fs::write(&path, b"first").unwrap();
|
||||
assert!(wait_until(Duration::from_secs(30), || {
|
||||
daemon.pending().is_ok_and(|items| items.is_empty())
|
||||
&& drive.tree().get("f.txt").cloned() == file(b"first")
|
||||
}));
|
||||
daemon.client.lose_reply_to_next_revision();
|
||||
std::fs::write(&path, b"second").unwrap();
|
||||
assert!(
|
||||
wait_until(Duration::from_secs(30), || {
|
||||
daemon.pending().is_ok_and(|items| items.is_empty())
|
||||
&& drive.tree().get("f.txt").cloned() == file(b"second")
|
||||
}),
|
||||
"{:?} {:?}",
|
||||
daemon.pending(),
|
||||
drive.tree()
|
||||
);
|
||||
assert_eq!(drive.tree().len(), 1, "{:?}", drive.tree());
|
||||
|
||||
assert!(matches!(daemon.stop(), Ok(MountOutcome::Shutdown)));
|
||||
let _ = std::fs::remove_dir_all(&dir);
|
||||
}
|
||||
|
||||
#[test]
|
||||
#[ignore = "mounts FUSE: run with `cargo test -p pdfs-fuse sim:: -- --ignored`"]
|
||||
fn a_pdfs_rename_after_a_queued_rename_is_not_undone() {
|
||||
|
||||
@@ -332,6 +332,8 @@ impl FakeDrive {
|
||||
hold: Mutex::new(None),
|
||||
drop_after_create: AtomicBool::new(false),
|
||||
lose_create_reply: AtomicBool::new(false),
|
||||
hold_create_reply: Mutex::new(None),
|
||||
lose_revision_reply: AtomicBool::new(false),
|
||||
}))
|
||||
}
|
||||
|
||||
@@ -583,6 +585,10 @@ struct ClientInner {
|
||||
drop_after_create: AtomicBool,
|
||||
/// Set by [`FakeClient::lose_reply_to_next_create`].
|
||||
lose_create_reply: AtomicBool,
|
||||
/// The create [`FakeClient::hold_reply_to_next_create`] is waiting for.
|
||||
hold_create_reply: Mutex<Option<Arc<HoldState>>>,
|
||||
/// Set by [`FakeClient::lose_reply_to_next_revision`].
|
||||
lose_revision_reply: AtomicBool,
|
||||
}
|
||||
|
||||
struct HoldState {
|
||||
@@ -591,8 +597,17 @@ struct HoldState {
|
||||
released: AtomicBool,
|
||||
}
|
||||
|
||||
/// A read held on its way to Drive, from [`FakeClient::hold_next_read`]. It
|
||||
/// goes on when this is dropped.
|
||||
/// Wait while `state` holds a call.
|
||||
async fn wait_until_released(state: &HoldState) {
|
||||
state.reached.store(true, Ordering::SeqCst);
|
||||
while !state.released.load(Ordering::SeqCst) {
|
||||
tokio::time::sleep(Duration::from_millis(5)).await;
|
||||
}
|
||||
}
|
||||
|
||||
/// A call held on its way to or from Drive, from [`FakeClient::hold_next_read`]
|
||||
/// or [`FakeClient::hold_reply_to_next_create`]. It goes on when this is
|
||||
/// dropped.
|
||||
pub(crate) struct Held(Arc<HoldState>);
|
||||
|
||||
impl Held {
|
||||
@@ -646,6 +661,23 @@ impl FakeClient {
|
||||
Held(state)
|
||||
}
|
||||
|
||||
/// Make the next file create on Drive, then hold its answer until the
|
||||
/// answer is dropped, as a slow link does.
|
||||
pub(crate) fn hold_reply_to_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_reply.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);
|
||||
}
|
||||
|
||||
/// Wait here if `uids` is the read a [`Held`] is for.
|
||||
async fn wait_if_held(&self, uids: &[NodeUid]) {
|
||||
let state = {
|
||||
@@ -656,10 +688,7 @@ impl FakeClient {
|
||||
}
|
||||
};
|
||||
let Some(state) = state else { return };
|
||||
state.reached.store(true, Ordering::SeqCst);
|
||||
while !state.released.load(Ordering::SeqCst) {
|
||||
tokio::time::sleep(Duration::from_millis(5)).await;
|
||||
}
|
||||
wait_until_released(&state).await;
|
||||
}
|
||||
|
||||
/// Requests made through this client so far.
|
||||
@@ -1073,6 +1102,12 @@ impl DriveApi for FakeClient {
|
||||
if made.is_ok() && self.0.lose_create_reply.swap(false, Ordering::SeqCst) {
|
||||
return Err(no_answer());
|
||||
}
|
||||
let held = self.0.hold_create_reply.lock().take();
|
||||
if made.is_ok()
|
||||
&& let Some(state) = held
|
||||
{
|
||||
wait_until_released(&state).await;
|
||||
}
|
||||
made
|
||||
}
|
||||
|
||||
@@ -1129,7 +1164,11 @@ impl DriveApi for FakeClient {
|
||||
server.put(file_uid, Some(remote));
|
||||
Ok(())
|
||||
})
|
||||
.await
|
||||
.await?;
|
||||
if self.0.lose_revision_reply.swap(false, Ordering::SeqCst) {
|
||||
return Err(no_answer());
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
async fn open_revision(&self, uid: &NodeUid) -> Result<Arc<dyn RevisionRead>> {
|
||||
|
||||
@@ -12,6 +12,46 @@ Conventions:
|
||||
|
||||
---
|
||||
|
||||
## B155 — A write whose upload lost its answer lands as a conflict copy of itself
|
||||
|
||||
**Status:** Fixed (unverified).
|
||||
**Found:** 2026-10-04, by the simulation's Wi-Fi profile (`one_client_on_a_slow_link`, seed 2,
|
||||
second replay on 7269cb5). `x/z/y/a.txt` was written after its create landed. The revision upload
|
||||
reached Drive, but the answer was lost and the call timed out. On the retry, Drive held
|
||||
`a (sync-conflict …).txt` beside the file, with the same bytes.
|
||||
|
||||
**Where:** `Core::revision_conflict` in `crates/pdfs-fuse/src/drain.rs`.
|
||||
|
||||
**Cause.** A queued write remembers the revision it was made against. The retry found the file one
|
||||
revision further, sealed by the lost upload, which this daemon never heard of, so it was not
|
||||
recorded as our own (`own_sealed_rev`). The write was taken for a change from elsewhere and kept as
|
||||
a conflict copy.
|
||||
|
||||
**Fix.** A complete write whose bytes the remote already holds, by size and SHA-1, is not a
|
||||
conflict: it is sent again as a revision of the same bytes. Sim test
|
||||
`a_write_whose_answer_was_lost_lands_once`, with the fake Drive's new
|
||||
`lose_reply_to_next_revision`.
|
||||
|
||||
## B154 — A file made under the name of a create on the wire lands as a conflict copy
|
||||
|
||||
**Status:** Fixed (unverified).
|
||||
**Found:** 2026-10-04, by CI on 7269cb5 (`one_client_on_a_good_link`, seed 9, every run). `c.bin`
|
||||
was renamed to `d` while its create was on the wire, and a new `c.bin` was made. The new file landed
|
||||
as `c (sync-conflict …).bin`, the mount showed that name, and `unlink c.bin` answered `ENOENT`.
|
||||
|
||||
**Where:** the failure path of `Core::run_pending_drain` in `crates/pdfs-fuse/src/drain.rs`.
|
||||
|
||||
**Cause.** A regression from the B153 fix. The new file's create found `c.bin` held by the first
|
||||
create, which was still on the wire, and failed as a wait. The B153 fix then woke the ops waiting
|
||||
for the name. The database cannot see a create on the wire, only queued trashes and renames, so
|
||||
it found the name free and the op was due again at once. Four waits went in 70 ms, and the create
|
||||
took a conflict name.
|
||||
|
||||
**Fix.** No wake while another create on the wire holds the name. It frees the name when it
|
||||
lands, or hands it to the rename queued behind it, which wakes the waiters when it lands. Sim
|
||||
test `a_file_made_under_the_name_of_a_create_on_the_wire_waits_for_it`, with the fake Drive's
|
||||
new `hold_reply_to_next_create`.
|
||||
|
||||
## B153 — An op waiting for a name sat out its backoff after the name was free
|
||||
|
||||
**Status:** Fixed (unverified).
|
||||
|
||||
@@ -33,6 +33,10 @@ folder by local id too.
|
||||
for this release.
|
||||
|
||||
### Fixed
|
||||
- **A file made under the name of a file whose upload is still going out keeps its name.** It
|
||||
no longer lands as a conflict copy, and the mount no longer shows that copy's name (B154).
|
||||
- **A change to a file no longer lands as a conflict copy of itself when the link loses Drive's
|
||||
answer to its upload** (B155).
|
||||
- **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 B153.
|
||||
- Fixes from the simulation and account runs since then: B127 and B141 to B155.
|
||||
- **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,9 +49,11 @@ 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.
|
||||
Replays then found two races on the LAN profile too (B152, B153), both fixed. CI on
|
||||
7269cb5 failed LAN seed 9, a regression from the B153 fix (B154), and Wi-Fi seed 2,
|
||||
a write whose answer was lost (B155). Both fixed. One slow-link run of seed 2 failed
|
||||
once with a rename answering `ENOENT` as a folder move landed. Not reproduced 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)
|
||||
|
||||
Reference in new issue
Block a user