fix(decom): move a tiered version whose restored-copy removal is refused

A decommission walk removes a restored copy that is past its restore
deadline before it moves the version, as the scanner would. Since
0a546c7a7 a refused removal made filterLifecycle return the
DeleteRestored action: the walk counted the version as decommissioned
without moving it and without queuing an expiry, removeMovedVersions
then found a source copy with no retained copy and failed the key with
PreConditionFailed, the moved versions of the key stayed duplicated in
both pools, the bucket stayed queued and the decommission ended Failed;
the retry after a restart failed the same way while the refusal held.

The walk now moves the version like any other when the removal is
refused, as the release before 0a546c7a7 did: a copy past its deadline
reads as remote, so the version moves with its tier metadata and its
restore deadline through DecomTieredObject, the remote object stays,
the source copy retires with the key once the destination holds it, and
the scanner of the destination pool removes the restored copy by that
deadline. The rebalance walk never reaches this action for such a
version, which it skips as remote; its filterLifecycle now names the
same decision so neither walk counts a restored-copy expiry as a
deletion it performed.

TestDecommissionMovesRestoredCopyPastDeadline refuses the removal on
every drive of the set; before this change it failed with complete=false
failed=true, pool 0 holding the version and pool 1 nothing.
TestRebalanceLeavesRestoredCopyPastDeadline is the control for the
rebalance walk.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Signed-off-by: Feng Ruohang <rh@vonng.com>

cmd/erasure-server-pool-decom.go

@@ -813,10 +813,14 @@ func (z *erasureServerPools) decommissionPool(ctx context.Context, idx int, pool
 
 			evt := evalActionFromLifecycle(ctx, *lc, lr, rcfg, objInfo)
 			switch {
-			case evt.Action.DeleteRestored(): // if restored copy has expired, delete it synchronously
-				if !applyExpiryOnTransitionedObject(ctx, z, objInfo, evt, lcEventSrc_Decom) {
-					return evt.Action
-				}
+			case evt.Action.DeleteRestored():
+				// A restored copy past its deadline is removed now, before the
+				// version moves, as the scanner would. When that removal is
+				// refused, the version moves like any other, with its tier
+				// metadata and its deadline, and the scanner of the pool that
+				// holds it then removes the copy. A version the walk neither
+				// moves nor expires leaves the key behind, and fails it.
+				applyExpiryOnTransitionedObject(ctx, z, objInfo, evt, lcEventSrc_Decom)
 				return lifecycle.NoneAction
 			case evt.Action == lifecycle.DeleteAction && versioned:
 				// Expiring the latest version of a versioned bucket only puts

cmd/erasure-server-pool-rebalance.go

@@ -600,6 +600,12 @@ func (z *erasureServerPools) rebalanceBucket(ctx context.Context, bucket string,
 
 			evt := evalActionFromLifecycle(ctx, *lc, lr, rcfg, objInfo)
 			switch {
+			case evt.Action.DeleteRestored():
+				// A restored copy expires by its own deadline, in the pool
+				// that holds the version; a rebalance does not remove it.
+				// Such a version reads as remote and the walk leaves it, as
+				// the decommission walk moves it: neither counts it as deleted.
+				return lifecycle.NoneAction
 			case evt.Action == lifecycle.DeleteAction && versioned:
 				// Expiring the latest version of a versioned bucket only puts
 				// a delete marker on top of it. The version itself survives as

cmd/erasure-server-pool-tier-ownership_test.go

@@ -4,11 +4,14 @@
 package cmd
 
 import (
+	"context"
 	"errors"
 	"slices"
 	"testing"
 
 	"github.com/minio/minio/internal/bucket/lifecycle"
+	objectlock "github.com/minio/minio/internal/bucket/object/lock"
+	xhttp "github.com/minio/minio/internal/http"
 )
 
 func rewriteTierTestMeta(t *testing.T, disk StorageAPI, bucket, key string, change func(*xlMetaV2)) {
@@ -216,3 +219,160 @@ func TestDecommissionMovesTieredDeleteAll(t *testing.T) {
 		})
 	}
 }
+
+// expireRestoredFaultDisk fails the removal of the restored copy of one key,
+// as a drive that refuses that write does. Every other operation passes.
+type expireRestoredFaultDisk struct {
+	StorageAPI
+	bucket, object string
+}
+
+func (d expireRestoredFaultDisk) DeleteVersion(ctx context.Context, volume, path string, fi FileInfo, forceDelMarker bool, opts DeleteOptions) error {
+	if volume == d.bucket && path == d.object && fi.ExpireRestored {
+		return errDiskFull
+	}
+	return d.StorageAPI.DeleteVersion(ctx, volume, path, fi, forceDelMarker, opts)
+}
+
+// restoredPastDeadline turns the version oi of key in pool 0 into a tiered
+// version with a restored copy whose restore deadline has passed: its data
+// stays on the drives, its metadata records the completed transition and the
+// deadline. Past the deadline the version reads as remote. It returns the
+// name of the remote object.
+func (f *freeVersionTest) restoredPastDeadline(key string, oi ObjectInfo) string {
+	f.t.Helper()
+	ctx := f.t.Context()
+	label := f.labels[oi.VersionID]
+	remote := f.remote(key, label)
+	infos, errs := readAllFileInfo(ctx, f.z.serverPools[0].getHashedSet(key).getDisks(), "", f.bucket, key, oi.VersionID, false, false)
+	if errs[0] != nil {
+		f.t.Fatal(errs[0])
+	}
+	fi := infos[0]
+	fi.TransitionStatus, fi.TransitionTier, fi.TransitionedObjName = lifecycle.TransitionComplete, "WARM", remote
+	fi.Metadata[xhttp.AmzRestore] = completedRestoreObj(daysAgo(1)).String()
+	f.tier.objects[remote] = []byte(key + " " + label)
+	if err := f.z.serverPools[0].DecomTieredObject(ctx, f.bucket, key, fi, ObjectOptions{}); err != nil {
+		f.t.Fatal(err)
+	}
+	cur, err := f.z.GetObjectInfo(ctx, f.bucket, key, ObjectOptions{VersionID: oi.VersionID})
+	if err != nil || cur.RestoreExpires.IsZero() || !UTCNow().After(cur.RestoreExpires) {
+		f.t.Fatalf("fixture: restore expires %v, %v", cur.RestoreExpires, err)
+	}
+	lc, err := globalLifecycleSys.Get(f.bucket)
+	if err != nil {
+		f.t.Fatal(err)
+	}
+	if evt := evalActionFromLifecycle(ctx, *lc, objectlock.Retention{}, nil, cur); !evt.Action.DeleteRestored() {
+		f.t.Fatalf("fixture: lifecycle action %v, want the restored copy to expire", evt.Action)
+	}
+	return remote
+}
+
+// tieredVersion fails the test unless pool holds the version oi of key with
+// its tier metadata of remote and its restore deadline, and returns it.
+func (f *freeVersionTest) tieredVersion(pool int, key, remote string, oi ObjectInfo) ObjectInfo {
+	f.t.Helper()
+	got, err := f.z.serverPools[pool].GetObjectInfo(f.t.Context(), f.bucket, key, ObjectOptions{VersionID: oi.VersionID})
+	if err != nil {
+		f.t.Fatalf("pool %d: %v", pool, err)
+	}
+	if tr := got.TransitionedObject; tr.Status != lifecycle.TransitionComplete || tr.Tier != "WARM" || tr.Name != remote {
+		f.t.Errorf("pool %d: the version lost its tier metadata: %+v", pool, tr)
+	}
+	if got.RestoreExpires.IsZero() || !UTCNow().After(got.RestoreExpires) {
+		f.t.Errorf("pool %d: the version lost its restore deadline: %v", pool, got.RestoreExpires)
+	}
+	if slices.Contains(f.tier.removed, remote) {
+		f.t.Errorf("remote object removed: %v", f.tier.removed)
+	}
+	return got
+}
+
+// expireRestoredCopy removes the restored copy of oi, as the scanner of the
+// pool that holds the version does once its deadline has passed, and fails
+// the test unless the version then reads as remote without a deadline and
+// the remote object stays.
+func (f *freeVersionTest) expireRestoredCopy(pool int, key, remote string, oi ObjectInfo) {
+	f.t.Helper()
+	ctx := f.t.Context()
+	lc, err := globalLifecycleSys.Get(f.bucket)
+	if err != nil {
+		f.t.Fatal(err)
+	}
+	evt := evalActionFromLifecycle(ctx, *lc, objectlock.Retention{}, nil, oi)
+	if !evt.Action.DeleteRestored() {
+		f.t.Fatalf("lifecycle action in pool %d: %v", pool, evt.Action)
+	}
+	if !applyExpiryOnTransitionedObject(ctx, f.z, oi, evt, lcEventSrc_Scanner) {
+		f.t.Fatalf("the restored copy did not expire in pool %d", pool)
+	}
+	after, err := f.z.serverPools[pool].GetObjectInfo(ctx, f.bucket, key, ObjectOptions{VersionID: oi.VersionID})
+	if err != nil || !after.IsRemote() || !after.RestoreExpires.IsZero() || after.TransitionedObject.Name != remote {
+		f.t.Fatalf("version after the restored copy expired: remote=%v expires=%v %+v, %v", after.IsRemote(), after.RestoreExpires, after.TransitionedObject, err)
+	}
+	if slices.Contains(f.tier.removed, remote) {
+		f.t.Errorf("remote object removed with the restored copy: %v", f.tier.removed)
+	}
+}
+
+// A decommission removes a restored copy that is past its deadline before it
+// moves the version. When the drives refuse that removal, the version moves
+// like any other tiered version, with its tier metadata and its deadline: the
+// remote object stays, the source copy retires with the key, and the scanner
+// of the pool that holds the version removes the copy by that deadline.
+// Skipping the version instead leaves the key behind with a
+// PreConditionFailed and fails the decommission on every retry.
+func TestDecommissionMovesRestoredCopyPastDeadline(t *testing.T) {
+	f := newFreeVersionTest(t, freeVersionNoncurrent)
+	const key = "obj-restored"
+	oi := f.put(key, "v1", daysAgo(4))
+	remote := f.restoredPastDeadline(key, oi)
+
+	set := f.z.serverPools[0].getHashedSet(key)
+	getDisks := set.getDisks
+	faulty := slices.Clone(getDisks())
+	for i := range faulty {
+		faulty[i] = expireRestoredFaultDisk{StorageAPI: faulty[i], bucket: f.bucket, object: key}
+	}
+	set.getDisks = func() []StorageAPI { return faulty }
+	t.Cleanup(func() { set.getDisks = getDisks })
+
+	info := f.decommission()
+	if !info.Complete || info.Failed || info.ItemsDecommissionFailed != 0 {
+		t.Fatalf("decommission with a refused restored-copy removal: complete=%v failed=%v items failed=%d, pool 0 holds %v, pool 1 holds %v",
+			info.Complete, info.Failed, info.ItemsDecommissionFailed, f.stored(0, key), f.stored(1, key))
+	}
+	if got := f.stored(0, key); len(got) != 0 {
+		t.Errorf("pool 0 still holds %v", got)
+	}
+	if got := f.stored(1, key); !slices.Equal(got, []string{"v1"}) {
+		t.Fatalf("pool 1 holds %v, want [v1]", got)
+	}
+	moved := f.tieredVersion(1, key, remote, oi)
+	t.Logf("the moved version reads as remote: %v", moved.IsRemote())
+	f.expireRestoredCopy(1, key, remote, moved)
+}
+
+// Control: a rebalance moves no tiered version, and a restored copy past its
+// deadline reads as remote. The walk leaves the version in the source pool
+// without counting it as deleted, so the key is not retired, and the scanner
+// of that pool removes the copy by its deadline.
+func TestRebalanceLeavesRestoredCopyPastDeadline(t *testing.T) {
+	f := newFreeVersionTest(t, freeVersionNoncurrent)
+	const key = "obj-restored"
+	oi := f.put(key, "v1", daysAgo(4))
+	remote := f.restoredPastDeadline(key, oi)
+
+	if failed := migrationTestWalk(t, f.z, f.bucket, 0, "rebalance"); failed != 0 {
+		t.Fatalf("%d versions failed to move", failed)
+	}
+	if got := f.stored(0, key); !slices.Equal(got, []string{"v1"}) {
+		t.Fatalf("pool 0 holds %v, want [v1]", got)
+	}
+	if got := f.stored(1, key); len(got) != 0 {
+		t.Errorf("pool 1 holds %v", got)
+	}
+	left := f.tieredVersion(0, key, remote, oi)
+	f.expireRestoredCopy(0, key, remote, left)
+}