diff --git a/crates/ecstore/src/disk/local.rs b/crates/ecstore/src/disk/local.rs index bcd662cf8..386c99bea 100644 --- a/crates/ecstore/src/disk/local.rs +++ b/crates/ecstore/src/disk/local.rs @@ -119,7 +119,7 @@ fn read_all_data_std(path: &Path) -> core::result::Result<(Vec, Option Uuid { +pub(crate) fn inline_metadata_rollback_dir(version_id: Uuid, meta: &FileMeta) -> Uuid { let used_data_dirs: HashSet = meta.get_data_dirs().unwrap_or_default().into_iter().flatten().collect(); let base = version_id.as_u128() ^ INLINE_METADATA_ROLLBACK_DIR_XOR; let mut salt = 0u128; @@ -240,8 +240,15 @@ async fn write_metadata_rollback_backup(object_dir: &Path, rollback_dir: Uuid, d } async fn restore_metadata_backup(object_dir: &Path, xl_path: &Path, rollback_dir: Uuid) -> Result<()> { - let backup_path = object_dir.join(rollback_dir.to_string()).join(STORAGE_FORMAT_FILE_BACKUP); - rename_all(&backup_path, xl_path, object_dir).await + let rollback_path = object_dir.join(rollback_dir.to_string()); + let backup_path = rollback_path.join(STORAGE_FORMAT_FILE_BACKUP); + rename_all(&backup_path, xl_path, object_dir).await?; + // A synthetic inline rollback dir held only the backup the rename above + // just consumed; reclaim it so the object dir can empty out. A real data + // dir still holds its parts, so the non-recursive remove is a benign + // no-op there (mirrors restore_delete_rollback). + let _ = fs::remove_dir(&rollback_path).await; + Ok(()) } async fn restore_delete_rollback(object_dir: &Path, xl_path: &Path, rollback_dir: Uuid) -> Result<()> { @@ -12228,6 +12235,54 @@ mod test { ); } + // The undo_write restore consumes `/xl.meta.bkp` by rename; a + // synthetic rollback dir is then empty and must be reclaimed so the object + // dir can empty out (BucketNotEmpty leak). A real data dir still holds its + // parts and must survive the non-recursive remove. + #[tokio::test] + async fn restore_metadata_backup_reclaims_empty_rollback_dir_only() { + use tempfile::tempdir; + + let dir = tempdir().expect("temp dir should be created"); + let object_dir = dir.path().join("bucket").join("obj"); + let xl_path = object_dir.join(STORAGE_FORMAT_FILE); + let rollback_dir = Uuid::new_v4(); + let rollback_path = object_dir.join(rollback_dir.to_string()); + fs::create_dir_all(&rollback_path) + .await + .expect("rollback dir should be created"); + fs::write(rollback_path.join(STORAGE_FORMAT_FILE_BACKUP), b"old-meta") + .await + .expect("backup should be written"); + + restore_metadata_backup(&object_dir, &xl_path, rollback_dir) + .await + .expect("restore should succeed"); + assert_eq!( + fs::read(&xl_path).await.expect("xl.meta should be restored"), + b"old-meta", + "restore must move the backup back onto xl.meta" + ); + assert!(!rollback_path.exists(), "an emptied synthetic rollback dir must be reclaimed"); + + // Real data dir: parts remain, the dir must survive. + let real_dir = Uuid::new_v4(); + let real_path = object_dir.join(real_dir.to_string()); + fs::create_dir_all(&real_path).await.expect("real data dir should be created"); + fs::write(real_path.join(STORAGE_FORMAT_FILE_BACKUP), b"older-meta") + .await + .expect("backup should be written"); + fs::write(real_path.join("part.1"), b"data") + .await + .expect("part should be written"); + + restore_metadata_backup(&object_dir, &xl_path, real_dir) + .await + .expect("restore should succeed"); + assert!(real_path.join("part.1").exists(), "a real data dir must keep its parts"); + assert!(real_path.exists(), "a non-empty data dir must not be removed"); + } + #[tokio::test] async fn rename_commit_failure_cleans_local_rollback_backup() { use tempfile::tempdir; diff --git a/crates/ecstore/src/set_disk/core/io_primitives.rs b/crates/ecstore/src/set_disk/core/io_primitives.rs index de753de41..416f7c65d 100644 --- a/crates/ecstore/src/set_disk/core/io_primitives.rs +++ b/crates/ecstore/src/set_disk/core/io_primitives.rs @@ -49,7 +49,7 @@ use crate::diagnostics::get::{ use crate::disk::local::DELETE_DATA_DIR_MARKER_PREFIX; use crate::disk::{ DataDirDeleteStatus, OldCurrentSize, PART_TRANSACTION_NEW_META, PART_TRANSACTION_OLD_META, PART_TRANSACTION_ROLLBACK, - PartTransactionAction, part_transaction_path, + PartTransactionAction, STORAGE_FORMAT_FILE_BACKUP, part_transaction_path, }; use crate::erasure::coding::BitrotReader; use crate::io_support::bitrot::ShardReader; @@ -2950,63 +2950,63 @@ impl SetDisks { return Err(ret_err); } - // A synthetic inline-rollback dir (rollback_data_dir set, cleanup_data_dir - // unset) holds only the xl.meta.bkp consumed by the quorum-failure undo - // above. Once the commit holds quorum that window is closed, and the - // commit_rename_data_dir pass reclaims real old data dirs only, so the - // synthetic dir must be reclaimed here — otherwise every overwrite of an - // inline version by a non-inline one leaves // - // behind, and DeleteBucket keeps failing with BucketNotEmpty after the - // object is deleted. Best-effort: residue must not fail the durable write. - let rollback_only_futures: Vec<_> = disks - .iter() - .enumerate() - .filter_map(|(idx, disk)| { - let disk = disk.clone()?; - if errs[idx].is_some() || cleanup_data_dirs[idx].is_some() { - return None; - } - let rollback_dir = data_dirs[idx]?; - // Anti-misdelete guard, same posture as commit_rename_data_dir: - // never touch the data dir the commit just published. - if file_infos[idx].data_dir == Some(rollback_dir) { - return None; - } - let dst_bucket = dst_bucket.clone(); - let path = format!("{dst_object}/{rollback_dir}"); - Some(tokio::spawn(async move { - disk.delete_data_dir( - &dst_bucket, - &path, - DeleteOptions { - recursive: true, - ..Default::default() - }, - ) - .await - })) - }) - .collect(); - for result in join_all(rollback_only_futures).await { + // The write is authoritatively committed, so the per-disk rollback + // backup (`object//xl.meta.bkp`) is dead weight now. + // When the rollback dir doubles as the real dereferenced data dir it + // is reclaimed wholesale by `commit_rename_data_dir`; a rollback dir + // reported separately (an overwrite of an inline version, whose dir is + // synthetic) is excluded from that recursive reclamation for safety + // (#5703) and must be reclaimed here instead — otherwise every inline + // overwrite strands a backup file that keeps the object dir non-empty + // and makes a later DeleteBucket fail with BucketNotEmpty forever. + // Delete exactly the backup file, never the directory tree: the + // synthetic UUID is a fixed, publicly-known constant for unversioned + // objects, so `object/` can simultaneously be a + // legitimate child key's directory — recursively deleting it would + // reopen the authorization bypass #5703 closed. The non-recursive + // delete removes the directory only when the backup was its sole + // content. Best-effort space reclamation — like + // `commit_rename_data_dir`, this must never negate the already-durable + // ACK. + let mut backup_reclaims = Vec::new(); + for (idx, disk) in disks.iter().enumerate() { + if errs[idx].is_some() { + continue; + } + let Some(rollback_dir) = data_dirs[idx] else { + continue; + }; + if cleanup_data_dirs[idx] == Some(rollback_dir) { + continue; + } + let Some(disk) = disk.clone() else { + continue; + }; + let dst_bucket = dst_bucket.clone(); + let dst_object = dst_object.clone(); + backup_reclaims.push(tokio::spawn(async move { + let backup_path = format!("{dst_object}/{rollback_dir}/{STORAGE_FORMAT_FILE_BACKUP}"); + disk.delete(&dst_bucket, &backup_path, DeleteOptions::default()).await + })); + } + for result in join_all(backup_reclaims).await { match result { - Ok(Ok(_)) => {} - Ok(Err(err)) if err == DiskError::FileNotFound || err == DiskError::VolumeNotFound => {} + Ok(Ok(())) => {} + Ok(Err(DiskError::FileNotFound | DiskError::VolumeNotFound)) => {} Ok(Err(err)) => { warn!( - target: "rustfs_ecstore::set_disk", dst_bucket = %dst_bucket, dst_object = %dst_object, error = %err, - "failed to reclaim synthetic inline-rollback dir after commit" + "rollback backup reclamation failed after committed rename" ); } - Err(err) => { + Err(join_err) => { warn!( - target: "rustfs_ecstore::set_disk", dst_bucket = %dst_bucket, dst_object = %dst_object, - error = %err, - "synthetic inline-rollback reclaim task failed" + error = %join_err, + "rollback backup reclamation task failed after committed rename" ); } } diff --git a/crates/ecstore/src/set_disk/mod.rs b/crates/ecstore/src/set_disk/mod.rs index 585952a58..af8d33442 100644 --- a/crates/ecstore/src/set_disk/mod.rs +++ b/crates/ecstore/src/set_disk/mod.rs @@ -6309,6 +6309,136 @@ mod tests { assert_eq!(read_back.size, 9, "HEAD must observe the new version, not stale metadata"); } + // Regression for the inline-overwrite rollback backup leak: #5703 stopped + // reporting the synthetic rollback dir for recursive post-commit cleanup, + // which stranded `object//xl.meta.bkp` after every inline + // overwrite. The object dir then never emptied, so the s3-tests teardown + // sequence (delete object, delete bucket) failed with BucketNotEmpty + // forever. After a committed overwrite the backup must be reclaimed and a + // subsequent delete must leave nothing behind. + #[tokio::test] + async fn inline_overwrite_reclaims_synthetic_rollback_backup() { + let set_disks = make_local_bucket_test_set_disks().await; + let bucket = "bucket-inline-rollback-leak"; + let object = "obj"; + + set_disks + .make_bucket(bucket, &MakeBucketOptions::default()) + .await + .expect("bucket should be created"); + + for body in [b"hello".to_vec(), b"goodbye".to_vec()] { + let mut reader = PutObjReader::from_vec(body); + set_disks + .put_object( + bucket, + object, + &mut reader, + &ObjectOptions { + no_lock: true, + ..ObjectOptions::default() + }, + ) + .await + .expect("inline write should succeed"); + } + + // The committed overwrite must leave only xl.meta in the object dir on + // every disk; a stranded rollback dir keeps the bucket undeletable. + for endpoint in &set_disks.set_endpoints { + let object_dir = std::path::PathBuf::from(endpoint.get_file_path()).join(bucket).join(object); + let mut entries: Vec = std::fs::read_dir(&object_dir) + .expect("object dir should exist") + .map(|entry| entry.expect("entry should read").file_name().to_string_lossy().into_owned()) + .collect(); + entries.sort(); + assert_eq!( + entries, + vec![STORAGE_FORMAT_FILE.to_string()], + "only xl.meta may remain after an inline overwrite in {object_dir:?}" + ); + } + + // With only xl.meta left, the s3-tests teardown (delete object, delete + // bucket) empties the dir; the delete paths themselves are covered by + // their own tests. This harness has no bucket metadata sys, so the + // full delete_object flow cannot run here. + } + + // #5703's security property must survive the backup reclamation: the + // synthetic rollback dir of key K maps to the directory `K/`, which + // can simultaneously be a legitimate child key. Reclaiming the backup must + // remove exactly the backup file — never the child key's metadata. + #[tokio::test] + async fn inline_overwrite_backup_reclaim_spares_child_key_dir() { + let set_disks = make_local_bucket_test_set_disks().await; + let bucket = "bucket-inline-rollback-child"; + let object = "obj"; + let synthetic = crate::disk::local::inline_metadata_rollback_dir(Uuid::nil(), &FileMeta::new()); + let child_object = format!("{object}/{synthetic}"); + + set_disks + .make_bucket(bucket, &MakeBucketOptions::default()) + .await + .expect("bucket should be created"); + + let mut reader = PutObjReader::from_vec(b"child".to_vec()); + set_disks + .put_object( + bucket, + &child_object, + &mut reader, + &ObjectOptions { + no_lock: true, + ..ObjectOptions::default() + }, + ) + .await + .expect("child write should succeed"); + + // Create then overwrite the parent key: the overwrite writes its + // rollback backup into the child's directory and must afterwards + // reclaim only that file. + for body in [b"first".to_vec(), b"second".to_vec()] { + let mut reader = PutObjReader::from_vec(body); + set_disks + .put_object( + bucket, + object, + &mut reader, + &ObjectOptions { + no_lock: true, + ..ObjectOptions::default() + }, + ) + .await + .expect("parent write should succeed"); + } + + let child_info = set_disks + .get_object_info(bucket, &child_object, &ObjectOptions::default()) + .await + .expect("child key must survive the parent's rollback backup reclamation"); + assert_eq!(child_info.size, 5, "child key content must be untouched"); + + for endpoint in &set_disks.set_endpoints { + let child_dir = std::path::PathBuf::from(endpoint.get_file_path()) + .join(bucket) + .join(object) + .join(synthetic.to_string()); + let mut entries: Vec = std::fs::read_dir(&child_dir) + .expect("child object dir should exist") + .map(|entry| entry.expect("entry should read").file_name().to_string_lossy().into_owned()) + .collect(); + entries.sort(); + assert_eq!( + entries, + vec![STORAGE_FORMAT_FILE.to_string()], + "the child dir must keep its xl.meta and lose only the stray backup in {child_dir:?}" + ); + } + } + #[tokio::test(flavor = "multi_thread")] #[serial] async fn test_acquire_dist_delete_object_locks_batch_succeeds_with_two_healthy_lockers() {