fix: make sure to delete dangling objects during heal (#13138)

heal with --remove was not removing dangling versions
on versioned buckets, this PR fixes this properly.

this is a regression introduced in PR #12617

cmd/erasure-healing_test.go

@@ -24,6 +24,7 @@ import (
 	"os"
 	"path"
 	"reflect"
+	"runtime"
 	"testing"
 	"time"
 
@@ -145,6 +146,150 @@ func TestHealing(t *testing.T) {
 	}
 }
 
+func TestHealingDanglingObject(t *testing.T) {
+	if runtime.GOOS == "windows" {
+		t.Skip()
+	}
+
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+
+	resetGlobalHealState()
+	defer resetGlobalHealState()
+
+	nDisks := 16
+	fsDirs, err := getRandomDisks(nDisks)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	//defer removeRoots(fsDirs)
+
+	// Everything is fine, should return nil
+	objLayer, disks, err := initObjectLayer(ctx, mustGetPoolEndpoints(fsDirs...))
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	bucket := getRandomBucketName()
+	object := getRandomObjectName()
+	data := bytes.Repeat([]byte("a"), 128*1024)
+
+	err = objLayer.MakeBucketWithLocation(ctx, bucket, BucketOptions{})
+	if err != nil {
+		t.Fatalf("Failed to make a bucket - %v", err)
+	}
+
+	// Enable versioning.
+	globalBucketMetadataSys.Update(bucket, bucketVersioningConfig, []byte(`<VersioningConfiguration><Status>Enabled</Status></VersioningConfiguration>`))
+
+	_, err = objLayer.PutObject(ctx, bucket, object, mustGetPutObjReader(t, bytes.NewReader(data), int64(len(data)), "", ""), ObjectOptions{
+		Versioned: true,
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	for _, fsDir := range fsDirs[:4] {
+		if err = os.Chmod(fsDir, 0400); err != nil {
+			t.Fatal(err)
+		}
+	}
+
+	// Create delete marker under quorum.
+	objInfo, err := objLayer.DeleteObject(ctx, bucket, object, ObjectOptions{Versioned: true})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	for _, fsDir := range fsDirs[:4] {
+		if err = os.Chmod(fsDir, 0755); err != nil {
+			t.Fatal(err)
+		}
+	}
+
+	fileInfoPreHeal, err := disks[0].ReadVersion(context.Background(), bucket, object, "", false)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if fileInfoPreHeal.NumVersions != 1 {
+		t.Fatalf("Expected versions 1, got %d", fileInfoPreHeal.NumVersions)
+	}
+
+	if err = objLayer.HealObjects(ctx, bucket, "", madmin.HealOpts{Remove: true},
+		func(bucket, object, vid string) error {
+			_, err := objLayer.HealObject(ctx, bucket, object, vid, madmin.HealOpts{Remove: true})
+			return err
+		}); err != nil {
+		t.Fatal(err)
+	}
+
+	fileInfoPostHeal, err := disks[0].ReadVersion(context.Background(), bucket, object, "", false)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if fileInfoPostHeal.NumVersions != 2 {
+		t.Fatalf("Expected versions 2, got %d", fileInfoPreHeal.NumVersions)
+	}
+
+	if objInfo.DeleteMarker {
+		if _, err = objLayer.DeleteObject(ctx, bucket, object, ObjectOptions{
+			Versioned: true,
+			VersionID: objInfo.VersionID,
+		}); err != nil {
+			t.Fatal(err)
+		}
+	}
+
+	for _, fsDir := range fsDirs[:4] {
+		if err = os.Chmod(fsDir, 0400); err != nil {
+			t.Fatal(err)
+		}
+	}
+
+	rd := mustGetPutObjReader(t, bytes.NewReader(data), int64(len(data)), "", "")
+	_, err = objLayer.PutObject(ctx, bucket, object, rd, ObjectOptions{
+		Versioned: true,
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	for _, fsDir := range fsDirs[:4] {
+		if err = os.Chmod(fsDir, 0755); err != nil {
+			t.Fatal(err)
+		}
+	}
+
+	fileInfoPreHeal, err = disks[0].ReadVersion(context.Background(), bucket, object, "", false)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if fileInfoPreHeal.NumVersions != 1 {
+		t.Fatalf("Expected versions 1, got %d", fileInfoPreHeal.NumVersions)
+	}
+
+	if err = objLayer.HealObjects(ctx, bucket, "", madmin.HealOpts{Remove: true},
+		func(bucket, object, vid string) error {
+			_, err := objLayer.HealObject(ctx, bucket, object, vid, madmin.HealOpts{Remove: true})
+			return err
+		}); err != nil {
+		t.Fatal(err)
+	}
+
+	fileInfoPostHeal, err = disks[0].ReadVersion(context.Background(), bucket, object, "", false)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if fileInfoPostHeal.NumVersions != 2 {
+		t.Fatalf("Expected versions 2, got %d", fileInfoPreHeal.NumVersions)
+	}
+}
+
 func TestHealObjectCorrupted(t *testing.T) {
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
@@ -166,8 +311,8 @@ func TestHealObjectCorrupted(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	bucket := "bucket"
+	bucket := getRandomBucketName()
-	object := "object"
+	object := getRandomObjectName()
 	data := bytes.Repeat([]byte("a"), 5*1024*1024)
 	var opts ObjectOptions
 

cmd/erasure-server-pool.go

@@ -1668,10 +1668,13 @@ func (z *erasureServerPools) HealObjects(ctx context.Context, bucket, prefix str
 						}
 						fivs, err := entry.fileInfoVersions(bucket)
 						if err != nil {
+							if err := healObject(bucket, entry.name, ""); err != nil {
 								errCh <- err
 								cancel()
 								return
 							}
+							return
+						}
 
 						for _, version := range fivs.Versions {
 							if err := healObject(bucket, version.Name, version.VersionID); err != nil {
@@ -1705,9 +1708,13 @@ func (z *erasureServerPools) HealObjects(ctx context.Context, bucket, prefix str
 						agreed:         healEntry,
 						partial: func(entries metaCacheEntries, nAgreed int, errs []error) {
 							entry, ok := entries.resolve(&resolver)
-							if ok {
+							if !ok {
-								healEntry(*entry)
+								// check if we can get one entry atleast
+								// proceed to heal nonetheless.
+								entry, _ = entries.firstFound()
 							}
+
+							healEntry(*entry)
 						},
 						finished: nil,
 					}

cmd/metacache-entries.go

@@ -24,6 +24,7 @@ import (
 	"sort"
 	"strings"
 
+	"github.com/minio/minio/internal/logger"
 	"github.com/minio/pkg/console"
 )
 
@@ -104,18 +105,21 @@ func resolveEntries(a, b *metaCacheEntry, bucket string) *metaCacheEntry {
 		return a
 	}
 
-	if !aFi.ModTime.Equal(bFi.ModTime) {
+	if aFi.NumVersions == bFi.NumVersions {
+		if aFi.ModTime.Equal(bFi.ModTime) {
+			return a
+		}
 		if aFi.ModTime.After(bFi.ModTime) {
 			return a
 		}
 		return b
 	}
 
-	if aFi.NumVersions > bFi.NumVersions {
+	if bFi.NumVersions > aFi.NumVersions {
-		return a
+		return b
 	}
 
-	return b
+	return a
 }
 
 // isInDir returns whether the entry is in the dir when considering the separator.
@@ -269,6 +273,7 @@ func (m metaCacheEntries) resolve(r *metadataResolutionParams) (selected *metaCa
 
 		// Get new entry metadata
 		if _, err := entry.fileInfo(r.bucket); err != nil {
+			logger.LogIf(context.Background(), err)
 			continue
 		}
 
@@ -318,7 +323,11 @@ func (m metaCacheEntries) resolve(r *metadataResolutionParams) (selected *metaCa
 			return nil, false
 		}
 
-		if r.candidates[0].n > r.candidates[1].n {
+		// if r.objQuorum == 1 then it is guaranteed that
+		// this resolver is for HealObjects(), so use resolveEntries()
+		// instead to resolve candidates, this check is only useful
+		// for regular cases of ListObjects()
+		if r.candidates[0].n > r.candidates[1].n && r.objQuorum > 1 {
 			ok := r.candidates[0].e != nil && r.candidates[0].e.name != ""
 			return r.candidates[0].e, ok
 		}

internal/config/api/api.go

@@ -84,7 +84,7 @@ var (
 		},
 		config.KV{
 			Key:   apiListQuorum,
-			Value: "optimal",
+			Value: "strict",
 		},
 		config.KV{
 			Key:   apiReplicationWorkers,