From 7a13780d81bf5929b10cf16aff9c6c1137982ace Mon Sep 17 00:00:00 2001 From: Feng Ruohang Date: Sat, 7 Nov 2026 16:20:09 +0800 Subject: [PATCH] fix(ilm): queue version heals when an undecodable history refuses a purge A purge of all versions reads the history of the key in every pool before it checks the drives. When half of the drives of a set could not be read or decoded, objectVersions refused the history with a read quorum error and the purge failed with InsufficientWriteQuorum before deleteAllDrives, which alone queued a heal of each version when a drive did not decode. With parity of half the drives, a key whose metadata did not decode on exactly half of them still read and healed, but no purge queued that heal: the refusal repeated until a scanner or drive heal happened to repair the drives. objectVersions now refuses such a history with undecodableHistory, which wraps the read quorum error and carries the IDs of the versions the decodable drives hold, the null version by its explicit ID. The purge queues a heal of each of them before it fails, from the history of the rule check as from the placement read, and still returns InsufficientWriteQuorum. The threshold of unknown drives, the refusal and every other caller of the history are unchanged; expiryApplies matches the read quorum error through the wrapper. TestPurgeAllVersionsHealsUndecodableHistory corrupts the metadata CRC on four of eight drives; before this change the purge refused the key without queuing any heal. Co-Authored-By: Claude Fable 5.1 Signed-off-by: Feng Ruohang --- ...ure-server-pool-delete-all-reasons_test.go | 97 +++++++++++++++++++ cmd/erasure-server-pool-delete-all.go | 20 ++++ cmd/erasure-server-pool-lifecycle.go | 59 +++++++++-- 3 files changed, 170 insertions(+), 6 deletions(-) diff --git a/cmd/erasure-server-pool-delete-all-reasons_test.go b/cmd/erasure-server-pool-delete-all-reasons_test.go index 815ef6868..546f81747 100644 --- a/cmd/erasure-server-pool-delete-all-reasons_test.go +++ b/cmd/erasure-server-pool-delete-all-reasons_test.go @@ -247,6 +247,103 @@ func TestPurgeAllVersionsRefusesCorruptDrive(t *testing.T) { } } +// Half of the drives of an eight-drive set hold metadata that does not +// decode. The key still reads and heals from the other half, but the drives +// cannot tell its history, so the purge refuses the key before it checks the +// drives. It still queues a heal of every version the decodable drives hold, +// the null version by its explicit ID, so that the next purge finds the key +// whole; the drives refuse nothing once healed. +func TestPurgeAllVersionsHealsUndecodableHistory(t *testing.T) { + z := purgeSinglePool(t, 8) + p := newPurgeTest(t, z, "purge-undecodable", MakeBucketOptions{VersioningEnabled: true}, purgeRules) + ctx := t.Context() + const key = "ea/undecodable" + p.put(0, key, "null", daysAgo(7), ObjectOptions{}) + p.version(0, key, "v1", 6) + p.version(0, key, "v2", 5) + ops := []expiryOp{p.deleteAllTask(0, key, lifecycle.DeleteAllVersionsAction)} + + disks := z.serverPools[0].getHashedSet(key).getDisks() + metaPath := pathJoin(key, xlStorageFormatFile) + good, err := disks[0].ReadAll(ctx, p.bucket, metaPath) + if err != nil { + t.Fatal(err) + } + corrupt := slices.Clone(good) + meta, _, err := isIndexedMetaV2(corrupt) + if err != nil { + t.Fatal(err) + } + meta[len(meta)-1] ^= 1 + bad := disks[len(disks)/2:] + for _, disk := range bad { + if err := disk.WriteAll(ctx, p.bucket, metaPath, corrupt); err != nil { + t.Fatal(err) + } + } + if got := p.get(key); got != "v2" { + t.Fatalf("fixture: with half of the drives undecodable the key reads %s, want v2", got) + } + + var healMu sync.Mutex + queued := make(map[string]bool) + hook := func(op PartialOperation) { + if op.Bucket == p.bucket && op.Object == key { + healMu.Lock() + queued[op.VersionID] = true + healMu.Unlock() + } + } + previousHook := mrfPartialOpHook.Swap(&hook) + t.Cleanup(func() { mrfPartialOpHook.Store(previousHook) }) + delta := incompleteDelta(func() { p.run(ops) }) + if delta[ilmIncompleteHistoryUnavailable] != 1 { + t.Errorf("counted %v, want one %s", delta, ilmIncompleteHistoryUnavailable) + } + _, err = z.DeleteObject(ctx, p.bucket, key, ObjectOptions{DeleteAllVersions: true, Expiration: ExpirationOptions{Expire: true}}) + if !isErrWriteQuorum(err) { + t.Errorf("purge on an undecodable history: %v, want a write quorum error", err) + } + if got := p.get(key); got != "v2" { + t.Fatalf("the refused purge left %s readable", got) + } + mrfPartialOpHook.Store(previousHook) + + versions, err := getFileInfoVersions(good, p.bucket, key, false) + if err != nil { + t.Fatal(err) + } + healMu.Lock() + for _, fi := range versions.Versions { + if !queued[deleteAllVersionID(fi.VersionID)] { + t.Errorf("no MRF heal for %q: %v", fi.VersionID, queued) + } + } + if queued[""] { + t.Error("MRF used latest instead of explicit null version") + } + healMu.Unlock() + + for _, fi := range versions.Versions { + id := deleteAllVersionID(fi.VersionID) + if _, err := z.HealObject(ctx, p.bucket, key, id, madmin.HealOpts{}); err != nil { + t.Fatalf("heal %s: %v", id, err) + } + for i, disk := range bad { + if _, err := disk.ReadVersion(ctx, "", p.bucket, key, id, ReadOptions{}); err != nil { + t.Fatalf("version %s on healed drive %d: %v", id, i, err) + } + } + } + if got := p.get(key); got != "v2" { + t.Fatalf("heal changed latest data: %s", got) + } + p.run([]expiryOp{p.deleteAllTask(0, key, lifecycle.DeleteAllVersionsAction)}) + if got := p.held(0, key); got != nil { + t.Errorf("the purge after the heal left %v", got) + } +} + // The scanner of a pool that holds a tiered version of a key whose latest // version is hot does not queue the expiry of all versions, which the purge // would refuse, and counts that it skipped it. It accounts every version of diff --git a/cmd/erasure-server-pool-delete-all.go b/cmd/erasure-server-pool-delete-all.go index f20f8f05c..205f7b816 100644 --- a/cmd/erasure-server-pool-delete-all.go +++ b/cmd/erasure-server-pool-delete-all.go @@ -250,6 +250,21 @@ func (z *erasureServerPools) deleteAllVersions(ctx context.Context, bucket, obje start := time.Now() name := decodeDirObject(object) fail := func(err error) (ObjectInfo, error) { + // A history refused for metadata that did not decode: queue a heal + // of each version the drives show, as deleteAllDrives does for a + // drive it checks, so that a later attempt finds the key whole. The + // purge remains refused. + var undecodable undecodableHistory + if errors.As(err, &undecodable) { + for _, id := range undecodable.ids { + globalMRFState.addPartialOp(PartialOperation{ + Bucket: bucket, + Object: object, + VersionID: id, + Queued: time.Now(), + }) + } + } if isErrReadQuorum(err) { err = InsufficientWriteQuorum{} } @@ -268,6 +283,11 @@ func (z *erasureServerPools) deleteAllVersions(ctx context.Context, bucket, obje all.versions, all.events, all.err = z.lifecycleEventsAllPools(ctx, bucket, object) if all.err != nil || len(all.versions) > 0 { if err := all.expiryApplies(bucket, object, ObjectOptions{DeletePrefix: true}); err != nil { + if all.err != nil { + // The history refused, not the rule: fail with it, which + // callers see as the same write quorum error. + err = all.err + } return fail(err) } } diff --git a/cmd/erasure-server-pool-lifecycle.go b/cmd/erasure-server-pool-lifecycle.go index d3b652b97..af12c7a39 100644 --- a/cmd/erasure-server-pool-lifecycle.go +++ b/cmd/erasure-server-pool-lifecycle.go @@ -29,6 +29,7 @@ import ( "sync" "time" + "github.com/google/uuid" "github.com/minio/minio/internal/bucket/lifecycle" objectlock "github.com/minio/minio/internal/bucket/object/lock" "github.com/minio/minio/internal/bucket/replication" @@ -76,10 +77,11 @@ func (er erasureObjects) objectVersions(ctx context.Context, bucket, object stri majority := len(disks)/2 + 1 var ( - metas [][]xlMetaV2ShallowVersion - metaVs []uint8 - metaV uint8 - unknown int + metas [][]xlMetaV2ShallowVersion + metaVs []uint8 + metaV uint8 + unknown int + undecodable bool ) for i, rf := range rawFileInfos { if errs[i] != nil { @@ -91,6 +93,7 @@ func (er erasureObjects) objectVersions(ctx context.Context, bucket, object stri var xl xlMetaV2 if err := xl.LoadOrConvert(rf.Buf); err != nil { unknown++ + undecodable = true continue } metas = append(metas, xl.versions) @@ -99,7 +102,11 @@ func (er erasureObjects) objectVersions(ctx context.Context, bucket, object stri } // Without an unknown drive the history is complete, also on one drive. if unknown > 0 && unknown >= len(disks)/2 { - return nil, toObjectErr(errErasureReadQuorum, bucket, object) + err := toObjectErr(errErasureReadQuorum, bucket, object) + if undecodable { + return nil, undecodableHistory{ids: shallowVersionIDs(metas), err: err} + } + return nil, err } quorumMetas, quorumErrs := make([]FileInfo, len(disks)), make([]error, len(disks)) @@ -125,6 +132,46 @@ func (er erasureObjects) objectVersions(ctx context.Context, bucket, object stri return versions, nil } +// undecodableHistory is the refusal of objectVersions when too many drives +// could not be read and the metadata of some of them did not decode. It +// wraps the read quorum error and carries the IDs of the versions that the +// drives that decoded hold, nullVersionID for the null version. A drive +// whose metadata does not decode does not come back by itself: a heal +// rewrites it from a quorum. A purge queues one of each version before it +// refuses the key, as deleteAllDrives does for a drive it checks. +type undecodableHistory struct { + ids []string + err error +} + +func (e undecodableHistory) Error() string { + return e.err.Error() + ": the metadata of a drive did not decode" +} + +func (e undecodableHistory) Unwrap() error { return e.err } + +// shallowVersionIDs returns the IDs of the versions in metas that are not +// free versions, each once, nullVersionID for the null version. +func shallowVersionIDs(metas [][]xlMetaV2ShallowVersion) []string { + var ids []string + seen := make(map[[16]byte]bool) + for _, versions := range metas { + for _, ver := range versions { + id := ver.header.VersionID + if ver.header.FreeVersion() || seen[id] { + continue + } + seen[id] = true + if id == [16]byte{} { + ids = append(ids, nullVersionID) + } else { + ids = append(ids, uuid.UUID(id).String()) + } + } + } + return ids +} + // revisionKey identifies a revision of a version: its header and a strong // digest of its metadata. The header Signature is a 32-bit fold of weak // hashes; two revisions can share it. @@ -675,7 +722,7 @@ func (all lifecycleAllPools) protection(versionID string) objectLockState { // or a delete marker without noncurrent versions. func (all lifecycleAllPools) expiryApplies(bucket, object string, opts ObjectOptions) error { if all.err != nil { - if _, ok := all.err.(InsufficientReadQuorum); ok { + if isErrReadQuorum(all.err) { return InsufficientWriteQuorum{} } return all.err