mirror of
https://github.com/narrrl/proton-drive-linux.git
synced 2026-11-03 16:41:26 +01:00
fix(core): drop a rename replaced while its failure was being recorded (B158)
B141 drops a rename that fails with a newer one queued behind it. The worker records the failure and releases its claim in two steps, and a rename queued between them waited behind the old one, which was kept with a backoff. Released, the old one was not due and the new one was, so the new one landed first and the old one then put the first name back. Releasing the claim now drops the old rename too when a newer one is queued, and both checks match the node by local id as well as by uid.
This commit is contained in:
1 parent
4676a4e32b
commit
c2f93449ad
5 files changed
+67
-13
No files matched your search
@@ -222,6 +222,20 @@ fn on_node(n: usize) -> String {
|
||||
ON_NODE.replace("?L", &format!("?{n}"))
|
||||
}
|
||||
|
||||
/// Delete op `id` if it is a rename with a newer rename of the same node queued
|
||||
/// behind it, which says where the node goes now. Returns whether it did.
|
||||
fn drop_replaced_rename(conn: &rusqlite::Connection, id: i64) -> Result<bool> {
|
||||
let dropped = conn.execute(
|
||||
"DELETE FROM pending_op
|
||||
WHERE id = ?1 AND kind = ?2
|
||||
AND EXISTS (SELECT 1 FROM pending_op AS newer
|
||||
WHERE (newer.uid = pending_op.uid OR newer.lid = pending_op.lid)
|
||||
AND newer.kind = ?2 AND newer.id > ?1)",
|
||||
params![id, OP_RENAME],
|
||||
)?;
|
||||
Ok(dropped > 0)
|
||||
}
|
||||
|
||||
/// The uid a queued op's node has now: the one its row holds, which a landing
|
||||
/// create has already moved to its real uid, or the one the op was queued with
|
||||
/// when the node has no row.
|
||||
@@ -501,7 +515,7 @@ impl Db {
|
||||
/// A rename a drain worker holds is not replaced but queued behind: the
|
||||
/// claim keeps the newer one from being sent until it retires, so the two
|
||||
/// land in the order they were made (`docs/BUGS.md` B141). Should it fail,
|
||||
/// [`Db::record_op_failure`] drops it.
|
||||
/// [`Db::record_op_failure`] or [`Db::release_op_claim`] drops it.
|
||||
pub fn enqueue_op(&self, op: &PendingOp) -> Result<(i64, Option<String>)> {
|
||||
let mut conn = self.conn.lock();
|
||||
let tx = conn.transaction()?;
|
||||
@@ -1034,8 +1048,17 @@ impl Db {
|
||||
/// `UPDATE` matches nothing), and a handler that returns without retiring
|
||||
/// *and* without failing would otherwise leave the row claimed by a worker
|
||||
/// that has moved on — invisible to every worker, forever.
|
||||
///
|
||||
/// A rename still here with a newer one queued behind it goes instead, as
|
||||
/// in [`Db::record_op_failure`]. The newer one may have been queued after
|
||||
/// the failure was recorded: kept, the older one sat out its backoff while
|
||||
/// the newer one landed, and then put the old name back (`docs/BUGS.md`
|
||||
/// B158).
|
||||
pub fn release_op_claim(&self, id: i64) -> Result<()> {
|
||||
let conn = self.conn.lock();
|
||||
if drop_replaced_rename(&conn, id)? {
|
||||
return Ok(());
|
||||
}
|
||||
conn.execute(
|
||||
"UPDATE pending_op SET claimed_at = 0 WHERE id = ?1",
|
||||
params![id],
|
||||
@@ -1254,15 +1277,7 @@ impl Db {
|
||||
/// dropped instead: retried, it could land after the newer one and undo it.
|
||||
pub fn record_op_failure(&self, id: i64, error: &str, next_attempt_at: i64) -> Result<()> {
|
||||
let conn = self.conn.lock();
|
||||
let dropped = conn.execute(
|
||||
"DELETE FROM pending_op
|
||||
WHERE id = ?1 AND kind = ?2
|
||||
AND EXISTS (SELECT 1 FROM pending_op AS newer
|
||||
WHERE newer.uid = pending_op.uid AND newer.kind = ?2
|
||||
AND newer.id > ?1)",
|
||||
params![id, OP_RENAME],
|
||||
)?;
|
||||
if dropped > 0 {
|
||||
if drop_replaced_rename(&conn, id)? {
|
||||
return Ok(());
|
||||
}
|
||||
conn.execute(
|
||||
|
||||
@@ -2977,7 +2977,8 @@ fn renaming_a_backing_off_create_makes_it_due() {
|
||||
}
|
||||
|
||||
/// A rename queued while an older one is on the wire waits for it. The older
|
||||
/// one, if it fails, is dropped rather than retried after the newer one.
|
||||
/// one, if it fails, is dropped rather than retried after the newer one, also
|
||||
/// when the newer one came after the failure was recorded.
|
||||
#[test]
|
||||
fn a_rename_on_the_wire_lands_before_the_one_that_replaces_it() {
|
||||
let db = Db::open_in_memory().unwrap();
|
||||
@@ -3013,6 +3014,16 @@ fn a_rename_on_the_wire_lands_before_the_one_that_replaces_it() {
|
||||
let (third, _) = db.enqueue_op(&rename("x")).unwrap();
|
||||
let ops = db.pending_ops().unwrap();
|
||||
assert_eq!(ops.iter().map(|o| o.id).collect::<Vec<_>>(), [third]);
|
||||
|
||||
// Queued after the failure was recorded but before the claim went back:
|
||||
// the older one goes then, rather than landing after the newer (B158).
|
||||
assert_eq!(db.claim_next_due_op(1).unwrap().unwrap().id, third);
|
||||
db.record_op_failure(third, "e.md is still held", 10_000)
|
||||
.unwrap();
|
||||
let (fourth, _) = db.enqueue_op(&rename("c0")).unwrap();
|
||||
db.release_op_claim(third).unwrap();
|
||||
let ops = db.pending_ops().unwrap();
|
||||
assert_eq!(ops.iter().map(|o| o.id).collect::<Vec<_>>(), [fourth]);
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
||||
@@ -12,6 +12,31 @@ Conventions:
|
||||
|
||||
---
|
||||
|
||||
## B158 — A file renamed twice in a row ends up under the first name, again
|
||||
|
||||
**Status:** Fixed (unverified).
|
||||
**Found:** 2026-10-04, by the simulation's Wi-Fi profile (`one_client_on_a_slow_link`, seed 3,
|
||||
four of ten runs on 4676a4e). A file was renamed to `b.txt`, whose name a create on the wire
|
||||
still held, and then to `e.md`. Drive renamed it to `e.md` and then back to `b.txt`. The
|
||||
liveness check found `b.txt` on Drive and `e.md` in the mount.
|
||||
|
||||
**Where:** `Db::record_op_failure` and `Db::release_op_claim` in
|
||||
`crates/pdfs-core/src/db/ops.rs`.
|
||||
|
||||
**Cause.** B141's fix drops a rename that fails while a newer one is queued behind it. The worker
|
||||
records the failure first and releases its claim after that. The second rename came in between:
|
||||
the first was still claimed, so the second queued behind it, but the failure had already been
|
||||
recorded with nothing behind it, so the first was kept with a backoff. Once released, it was not
|
||||
due and the second was, so the second landed first. More workers made the gap easier to hit.
|
||||
|
||||
**Fix.** Releasing the claim of a rename drops it as well when a newer rename of the same node is
|
||||
queued behind it. Both checks match the node by local id as well as by uid.
|
||||
|
||||
**Test:** unit test `a_rename_on_the_wire_lands_before_the_one_that_replaces_it`, extended.
|
||||
Seeds 2 and 3 of `one_client_on_a_slow_link` pass eight replays each.
|
||||
|
||||
---
|
||||
|
||||
## B157 — A second write to a file whose upload was not read back fails with `EIO`
|
||||
|
||||
**Status:** Fixed (unverified).
|
||||
|
||||
@@ -44,6 +44,8 @@ by its local id, in the queue, the stored tree and pins alike.
|
||||
of itself when the link lost Drive's answer to the first try** (B156).
|
||||
- **Writing to a file twice in a row no longer fails with "Input/output error" when the link
|
||||
lost Drive's answer to reading back the file's last upload** (B157).
|
||||
- **A file renamed twice in a row, while the first name was still taken, keeps the second name**
|
||||
(B158).
|
||||
- **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 B157.
|
||||
- Fixes from the simulation and account runs since then: B127 and B141 to B158.
|
||||
- **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`.
|
||||
|
||||
@@ -81,7 +81,8 @@ Tracks `docs/MILESTONE-3.0.0.md`. Update it with every commit that moves a phase
|
||||
it may be B152. Replays of slow-link seed 4 failed two of six times on a file written
|
||||
again after its create lost its answer (B156, fixed). With sixteen drain workers
|
||||
(16fca10), seed 4 failed every replay on a write after an upload that was not read
|
||||
back (B157, fixed).
|
||||
back (B157, fixed). Seed 3 then failed four of ten runs on two renames of one file
|
||||
landing out of order (B158, fixed).
|
||||
- [ ] 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