From 63b4568f850a6e581ac667fd85707c3112ae92cf Mon Sep 17 00:00:00 2001 From: houseme Date: Sun, 12 Jul 2026 22:19:47 +0800 Subject: [PATCH] fix(ecstore): reclaim orphan data dirs on the healthy heal path (#4781) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #4356 wired `reclaim_orphan_data_dirs` only into `heal_object`'s post-heal tail, which runs after the `disks_to_heal_count == 0` early return. That early return is exactly the state of the objects the sweep targets: a valid `xl.meta` with all shards present plus a leaked pre-#3510 data dir needs no shard healing, so a healthy heal returned before reclaim and swept nothing. On a healthy deployment (single node, no degraded disks) the reclaim was therefore dead code — an admin heal walked the objects, "healed" them, and reclaimed no leaked space. Run the best-effort reclaim on the `disks_to_heal_count == 0` path as well, gated on `!opts.dry_run`. The shared match+log block is factored into `reclaim_orphan_data_dirs_best_effort` so both exits behave identically. A reclaim failure still never fails the heal. Adds an end-to-end regression: put a healthy non-inline object, plant an unreferenced UUID data dir under it on every disk that holds the object, then drive `heal_object`. A dry-run heal must leave the stray in place; a real heal must reclaim it while preserving the live data dirs, `xl.meta`, and object contents. The test fails against the pre-fix control flow. Refs #3231, #3191, #4356. Co-authored-by: heihutu --- crates/ecstore/src/set_disk/ops/heal.rs | 37 +++++-- crates/heal/tests/heal_integration_test.rs | 121 +++++++++++++++++++++ 2 files changed, 149 insertions(+), 9 deletions(-) diff --git a/crates/ecstore/src/set_disk/ops/heal.rs b/crates/ecstore/src/set_disk/ops/heal.rs index 0fcea5b9a..639f7d08b 100644 --- a/crates/ecstore/src/set_disk/ops/heal.rs +++ b/crates/ecstore/src/set_disk/ops/heal.rs @@ -37,6 +37,23 @@ impl SetDisks { Box::pin(self.heal_object_with_regen(bucket, object, version_id, opts, true)).await } + /// Best-effort orphan-data-dir reclaim for an object that is healthy on this + /// set. Wraps [`Self::reclaim_orphan_data_dirs`] with the shared logging so + /// both `heal_object` exits — the already-healthy early return and the + /// post-heal tail — reclaim identically. Never fails the heal: delete errors + /// are logged and swallowed. Callers must gate this on `!opts.dry_run`. + async fn reclaim_orphan_data_dirs_best_effort(&self, bucket: &str, object: &str) { + match self.reclaim_orphan_data_dirs(bucket, object).await { + Ok(removed) if removed > 0 => { + info!(bucket, object, removed, "heal_object: reclaimed orphaned data directories"); + } + Ok(_) => {} + Err(e) => { + warn!(bucket, object, error = %e, "heal_object: orphan data-dir reclaim failed"); + } + } + } + #[allow(clippy::too_many_lines)] async fn heal_object_with_regen( &self, @@ -209,6 +226,16 @@ impl SetDisks { } if disks_to_heal_count == 0 { + // The object is already healthy: no disk needs healing. + // This is the common case for the very objects PR #4356 + // targets — a valid `xl.meta` plus a leaked pre-#3510 + // data dir needs no shard healing, so it would otherwise + // return here and never reach the post-heal reclaim tail + // below. Sweep the strays on this path too (issues #3231, + // #3191). Skipped on dry-run, like every mutating step. + if !opts.dry_run { + self.reclaim_orphan_data_dirs_best_effort(bucket, object).await; + } return Ok((result, None)); } @@ -661,15 +688,7 @@ impl SetDisks { // by pre-#3510 unversioned overwrites, which the dangling paths // above never touch (issues #3231, #3191). Best effort — a // failure must not fail the heal. - match self.reclaim_orphan_data_dirs(bucket, object).await { - Ok(removed) if removed > 0 => { - info!(bucket, object, removed, "heal_object: reclaimed orphaned data directories"); - } - Ok(_) => {} - Err(e) => { - warn!(bucket, object, error = %e, "heal_object: orphan data-dir reclaim failed"); - } - } + self.reclaim_orphan_data_dirs_best_effort(bucket, object).await; Ok((result, None)) } diff --git a/crates/heal/tests/heal_integration_test.rs b/crates/heal/tests/heal_integration_test.rs index 31a97243d..6000ecdda 100644 --- a/crates/heal/tests/heal_integration_test.rs +++ b/crates/heal/tests/heal_integration_test.rs @@ -226,6 +226,127 @@ mod serial_tests { info!("Heal object basic test passed"); } + // Regression for PR #4356 review (issues #3231, #3191): healing an object that + // needs no shard repair must still reclaim leaked pre-#3510 data dirs. + // + // The reclaim was originally wired only into `heal_object`'s post-heal tail, + // which is unreachable when `disks_to_heal_count == 0` — exactly the state of + // the objects the sweep targets (valid `xl.meta`, all shards present, plus one + // orphaned UUID data dir). A healthy heal took the early return and reclaimed + // nothing. This drives the full heal path on an untouched object and asserts + // the stray dir is swept, while a dry-run heal leaves it in place. + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] + #[serial] + async fn test_heal_object_reclaims_orphan_data_dir_when_healthy() { + let (disk_paths, ecstore, heal_storage) = setup_test_env().await; + + let bucket_name = "test-heal-reclaim-orphan"; + let object_name = "healthy-object.bin"; + let test_data = non_inline_test_data(); + + create_test_bucket(&ecstore, bucket_name).await; + upload_test_object(&ecstore, bucket_name, object_name, &test_data).await; + + // Plant a leaked, unreferenced UUID data dir under the object on every disk + // that actually holds the object (i.e. has an `xl.meta`). Planting on a disk + // without `xl.meta` would (correctly) trip the fail-closed guard and abort + // the whole reclaim, so we only seed disks that carry the object. + let orphan_dir = uuid::Uuid::new_v4().to_string(); + let mut seeded_orphans: Vec = Vec::new(); + for disk_path in &disk_paths { + let obj_dir = disk_path.join(bucket_name).join(object_name); + if !obj_dir.join("xl.meta").exists() { + continue; + } + let stray = obj_dir.join(&orphan_dir); + fs::create_dir_all(&stray).await.expect("orphan data dir should be created"); + fs::write(stray.join("part.1"), b"leaked pre-#3510 data") + .await + .expect("orphan part should be written"); + seeded_orphans.push(stray); + } + assert!( + !seeded_orphans.is_empty(), + "test setup failed: no disk carried the object to seed an orphan under" + ); + + let dry_run_opts = HealOpts { + dry_run: true, + recreate: true, + remove: false, + update_parity: true, + ..Default::default() + }; + let (_res, err) = heal_storage + .heal_object(bucket_name, object_name, None, &dry_run_opts) + .await + .expect("dry-run heal should succeed"); + assert!(err.is_none(), "dry-run heal returned error: {err:?}"); + for stray in &seeded_orphans { + assert!(stray.exists(), "dry-run heal must NOT remove the orphan data dir: {stray:?}"); + } + + // Snapshot the live (referenced) data dirs so we can assert they survive. + let mut live_dirs: Vec = Vec::new(); + for disk_path in &disk_paths { + let obj_dir = disk_path.join(bucket_name).join(object_name); + if !obj_dir.join("xl.meta").exists() { + continue; + } + for entry in WalkDir::new(&obj_dir) + .min_depth(1) + .max_depth(1) + .into_iter() + .filter_map(Result::ok) + { + let name = entry.file_name().to_string_lossy().to_string(); + if entry.file_type().is_dir() && name != orphan_dir { + live_dirs.push(entry.into_path()); + } + } + } + assert!(!live_dirs.is_empty(), "expected at least one live data dir to remain"); + + let heal_opts = HealOpts { + dry_run: false, + recreate: true, + remove: false, + update_parity: true, + ..Default::default() + }; + let (_res, err) = heal_storage + .heal_object(bucket_name, object_name, None, &heal_opts) + .await + .expect("heal should succeed"); + assert!(err.is_none(), "heal returned error: {err:?}"); + + for stray in &seeded_orphans { + assert!(!stray.exists(), "healthy heal must reclaim the orphan data dir: {stray:?}"); + } + for live in &live_dirs { + assert!(live.exists(), "referenced data dir must be preserved: {live:?}"); + } + for disk_path in &disk_paths { + let obj_dir = disk_path.join(bucket_name).join(object_name); + if obj_dir.exists() { + assert!(obj_dir.join("xl.meta").exists(), "xl.meta must be preserved: {obj_dir:?}"); + } + } + + // The object must remain intact and readable after the reclaim. + let mut reader = ecstore + .get_object_reader(bucket_name, object_name, None, HeaderMap::new(), &ObjectOptions::default()) + .await + .expect("Failed to get object reader after reclaim"); + let mut downloaded_data = Vec::new(); + tokio::io::copy(&mut reader, &mut downloaded_data) + .await + .expect("Failed to read object after reclaim"); + assert_eq!(downloaded_data, test_data, "object contents must survive orphan reclaim"); + + info!("Heal object orphan-reclaim test passed"); + } + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] #[serial] async fn test_heal_bucket_basic() {