diff --git a/crates/ecstore/src/disk/local.rs b/crates/ecstore/src/disk/local.rs index 9f5bfef42..37410cd40 100644 --- a/crates/ecstore/src/disk/local.rs +++ b/crates/ecstore/src/disk/local.rs @@ -9956,6 +9956,66 @@ mod test { assert!(!object_dir.join(rollback_dir.to_string()).exists()); } + // backlog#1158 safety premise: the set layer fans the undo out to every online + // disk on a quorum-failed delete, including disks that never staged a rollback + // (e.g. a disk that errored before staging). A delete-undo on such a disk must be + // a safe no-op that leaves the committed object untouched. + #[tokio::test] + async fn test_delete_version_undo_is_noop_when_nothing_staged() { + use tempfile::tempdir; + + let dir = tempdir().expect("temp dir should be created"); + let endpoint = Endpoint::try_from(dir.path().to_str().expect("temp dir should be utf8")).expect("endpoint should parse"); + let disk = LocalDisk::new(&endpoint, false).await.expect("local disk should be created"); + + let bucket = "bucket"; + let object = "dir/object"; + let version_id = Uuid::parse_str("aaaaaaaa-1111-2222-3333-444444444444").expect("version id should parse"); + let data_dir = Uuid::parse_str("bbbbbbbb-1111-2222-3333-444444444444").expect("data dir should parse"); + let rollback_dir = Uuid::parse_str("cccccccc-1111-2222-3333-444444444444").expect("rollback dir should parse"); + + ensure_test_volume(&disk, bucket).await; + + // A committed object exists on this disk, but no rollback state was staged. + let object_dir = dir.path().join(bucket).join("dir/object"); + let data_path = object_dir.join(data_dir.to_string()); + fs::create_dir_all(&data_path).await.expect("data dir should be created"); + fs::write(data_path.join("part.1"), b"live-data") + .await + .expect("part data should be written"); + let fi = test_file_info(object, version_id, Some(data_dir), None); + let meta = test_meta(fi.clone()); + fs::write(object_dir.join(STORAGE_FORMAT_FILE), meta.clone()) + .await + .expect("metadata should be written"); + + // Undo targeting a rollback dir that was never created must be an Ok no-op. + disk.delete_version( + bucket, + object, + fi.clone(), + false, + DeleteOptions { + undo_write: true, + undo_delete: true, + old_data_dir: Some(rollback_dir), + ..Default::default() + }, + ) + .await + .expect("undo with no staged rollback state must be a no-op"); + + // The committed object is untouched. + assert_eq!( + fs::read(object_dir.join(STORAGE_FORMAT_FILE)) + .await + .expect("metadata should remain"), + meta + ); + assert_eq!(fs::read(data_path.join("part.1")).await.expect("data should remain"), b"live-data"); + assert!(!object_dir.join(rollback_dir.to_string()).exists()); + } + #[tokio::test] async fn test_delete_marker_rollback_removes_new_metadata_without_backup() { use tempfile::tempdir; diff --git a/crates/ecstore/src/set_disk/ops/object.rs b/crates/ecstore/src/set_disk/ops/object.rs index e99b8373a..74dec731e 100644 --- a/crates/ecstore/src/set_disk/ops/object.rs +++ b/crates/ecstore/src/set_disk/ops/object.rs @@ -1409,7 +1409,14 @@ impl crate::storage_api_contracts::object::ObjectOperations for SetDisks { let should_rollback = quorum_result.is_err(); let mut rollback_futures = Vec::new(); for (index, err) in errs.iter().enumerate() { - if err.is_some() { + // backlog#1158: when rolling back, fan the idempotent undo out to every + // online disk (each self-decides from its staged backup: restore if the + // rollback dir is present, no-op otherwise). This covers a disk that + // staged + applied the delete and *then* errored, which the plain + // `err.is_some()` skip would leave deleted while its peers were restored. + // On success only the successful disks' backup dirs need cleaning; errored + // disks' residue is reclaimed by heal/scanner. + if !should_rollback && err.is_some() { continue; } @@ -1764,7 +1771,11 @@ impl crate::storage_api_contracts::object::ObjectOperations for SetDisks { // delete_versions commits one xl.meta per object group, so rollback must use the same boundary. let should_rollback = fi_vers.versions.iter().any(|fi| del_errs[fi.idx].is_some()); for (disk_idx, disk) in disks.iter().enumerate() { - if fi_vers.versions.iter().any(|fi| del_obj_errs[disk_idx][fi.idx].is_some()) { + // backlog#1158: on rollback, include every online disk so a disk that + // staged + applied the delete and then errored is still restored (the + // disk-side undo is idempotent, no-op when nothing was staged). On + // success, skip the errored disks and only clean up successful ones. + if !should_rollback && fi_vers.versions.iter().any(|fi| del_obj_errs[disk_idx][fi.idx].is_some()) { continue; }