From 8ffe230a89040ce2e39abcb41c2a0e468f665104 Mon Sep 17 00:00:00 2001 From: Zhengchao An Date: Mon, 6 Jul 2026 22:08:50 +0800 Subject: [PATCH] fix(multipart): don't pre-delete the committed part before rename_part (backlog#853) (#4316) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `rename_part` unconditionally ran `cleanup_multipart_path([part.N, part.N.meta])` on every disk *before* the per-disk rename fan-out. The per-disk `rename_part` already replaces `part.N` atomically (std::fs::rename) and rewrites `part.N.meta`, so the pre-delete is redundant — and destructive: it removed an already-committed (ACKed) part on all disks before the new rename landed, so a re-upload of the same part that then failed write quorum destroyed the committed part outright. Remove the pre-rename cleanup and rely on the atomic rename to overwrite in place; the quorum-failure rollback below is unchanged. No behaviour change on the success paths (first upload / retry both end with part.N replaced), verified by the existing multipart suite. Refs backlog#799 (B4). The remaining half of B4 — serializing concurrent same-part uploads with an uploadId-scoped lock so two generations can't be mixed across disks — is a larger concurrency change tracked as a follow-up on the issue. --- crates/ecstore/src/set_disk/core/io_primitives.rs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/crates/ecstore/src/set_disk/core/io_primitives.rs b/crates/ecstore/src/set_disk/core/io_primitives.rs index 940462a6c..8940c26bc 100644 --- a/crates/ecstore/src/set_disk/core/io_primitives.rs +++ b/crates/ecstore/src/set_disk/core/io_primitives.rs @@ -2688,10 +2688,14 @@ impl SetDisks { let dst_bucket = Arc::new(dst_bucket.to_string()); let dst_object = Arc::new(dst_object.to_string()); - // Match MinIO's multipart overwrite semantics: clear any stale destination - // part payload and metadata before the new per-disk rename fan-out begins. - self.cleanup_multipart_path(&[dst_object.to_string(), format!("{dst_object}.meta")]) - .await; + // Do NOT pre-delete the destination part before renaming: the per-disk + // `rename_part` replaces `part.N` atomically (std::fs::rename) and rewrites + // `part.N.meta`, so the pre-delete is redundant — and destructive. It + // opened a window where an already-committed (ACKed) part was removed on + // every disk before the new rename landed, so a re-upload that then failed + // quorum destroyed the committed part outright (backlog#853 / #799 B4). + // The atomic rename overwrites in place; on quorum failure below we roll + // the destination back. let mut errs = Vec::with_capacity(disks.len());