From c8dd5678bb60daceb4ccb252e4527d43a57864c5 Mon Sep 17 00:00:00 2001 From: Feng Ruohang Date: Thu, 5 Nov 2026 01:21:13 +0800 Subject: [PATCH 1/2] fix(heal): do not purge dangling data during a dry-run heal healObject handles a metadata read-quorum failure before its dry-run return, so an admin heal with DryRun set still called deleteIfDangling. When the drives without the version's xl.meta all answered "not found" and outnumbered its parity blocks, the dry run deleted the version, data directory included, from every drive that still held it. A site-wide dry run reaches .minio.sys/config the same way. When DryRun is set, return the quorum error with a set-level result built from the observed read errors, before the purge. Real heals are unchanged. The production change is the first cmd/erasure-healing.go hunk of PR #280, unchanged. The tests are the PR's dry-run tests that do not depend on its parallel deep verification or its dry-run reporting change; both stay out of this commit. The tests now assert the delete counter first and the converted error for each quorum case, and drop the CheckParts stub that only the excluded tests use. Co-authored-by: Jasper Bernhardt Co-Authored-By: Claude Opus 5.5 Signed-off-by: Feng Ruohang --- cmd/erasure-healing-native-audit_test.go | 244 +++++++++++++++++++++++ cmd/erasure-healing.go | 5 + 2 files changed, 249 insertions(+) create mode 100644 cmd/erasure-healing-native-audit_test.go diff --git a/cmd/erasure-healing-native-audit_test.go b/cmd/erasure-healing-native-audit_test.go new file mode 100644 index 000000000..bb9b02a13 --- /dev/null +++ b/cmd/erasure-healing-native-audit_test.go @@ -0,0 +1,244 @@ +// Copyright (c) 2026 Jasper Bernhardt +// +// This file is part of Silo Object Storage. +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . + +package cmd + +import ( + "bytes" + "context" + "crypto/sha256" + "errors" + "fmt" + "net/url" + "os" + "path/filepath" + "reflect" + "sync/atomic" + "testing" + "time" + + "github.com/minio/madmin-go/v3" +) + +// Unimplemented operations panic: the dry-run tests only allow reads and checks. +type nativeAuditDisk struct { + StorageAPI + info FileInfo + readErr error + verify func() (*CheckPartsResp, error) + deletes atomic.Int32 +} + +func (d *nativeAuditDisk) ReadVersion(context.Context, string, string, string, string, ReadOptions) (FileInfo, error) { + return d.info, d.readErr +} + +func (d *nativeAuditDisk) VerifyFile(context.Context, string, string, FileInfo) (*CheckPartsResp, error) { + return d.verify() +} + +func (d *nativeAuditDisk) DeleteVersion(context.Context, string, string, FileInfo, bool, DeleteOptions) error { + d.deletes.Add(1) + return nil +} + +func nativeAuditMetadata() []FileInfo { + meta := make([]FileInfo, 4) + for i := range meta { + meta[i] = newFileInfo("object", 2, 2) + meta[i].Volume, meta[i].Name = "bucket", "object" + meta[i].DataDir = "data" + meta[i].ModTime = time.Unix(123, 0).UTC() + meta[i].Size = 1024 + meta[i].Erasure.Distribution = []int{1, 2, 3, 4} + meta[i].Erasure.Index = i + 1 + meta[i].Parts = []ObjectPartInfo{{Number: 1, Size: 512}, {Number: 2, Size: 512}} + } + return meta +} + +func nativeAuditEndpoints() []Endpoint { + endpoints := make([]Endpoint, 4) + for i := range endpoints { + endpoints[i].URL = &url.URL{Path: fmt.Sprintf("/synthetic/disk%d", i)} + } + return endpoints +} + +func TestNativeAuditDryRunPreservesFaults(t *testing.T) { + for _, fault := range []string{"metadata-quorum", "corrupt-metadata", "missing-parts", "corrupt-parts", "inline-corrupt", "delete-marker-quorum"} { + t.Run(fault, func(t *testing.T) { + meta := nativeAuditMetadata() + disks := make([]StorageAPI, len(meta)) + for i := range disks { + disk := &nativeAuditDisk{info: meta[i], verify: func() (*CheckPartsResp, error) { + return &CheckPartsResp{Results: []int{checkPartSuccess, checkPartSuccess}}, nil + }} + if i < 3 { + switch fault { + case "metadata-quorum", "delete-marker-quorum": + disk.readErr = errFileVersionNotFound + case "corrupt-metadata": + disk.readErr = errFileCorrupt + case "missing-parts": + disk.verify = func() (*CheckPartsResp, error) { return nil, errFileNotFound } + case "corrupt-parts": + disk.verify = func() (*CheckPartsResp, error) { return nil, errFileCorrupt } + } + } + if fault == "delete-marker-quorum" { + disk.info.Deleted = true + disk.info.Size = 0 + } + if fault == "inline-corrupt" { + disk.info.Data = []byte("corrupt") + disk.info.Parts = disk.info.Parts[:1] + disk.info.SetInlineData() + disk.info.Erasure.Checksums = []ChecksumInfo{{PartNumber: 1, Algorithm: SHA256, Hash: make([]byte, sha256.Size)}} + } + disks[i] = disk + } + er := erasureObjects{ + setDriveCount: 4, defaultParityCount: 2, + getDisks: func() []StorageAPI { return disks }, + getEndpoints: nativeAuditEndpoints, + } + result, err := er.HealObject(t.Context(), "bucket", "object", "version", madmin.HealOpts{DryRun: true, NoLock: true, Remove: true, ScanMode: madmin.HealDeepScan}) + for _, disk := range disks { + if got := disk.(*nativeAuditDisk).deletes.Load(); got != 0 { + t.Fatalf("dry-run deleted data %d times", got) + } + } + quorumFault := fault == "metadata-quorum" || fault == "corrupt-metadata" || fault == "delete-marker-quorum" + if quorumFault && err == nil { + t.Fatal("metadata quorum failure reported success") + } + if !quorumFault && err != nil { + t.Fatal(err) + } + if len(result.Before.Drives) != len(disks) || !reflect.DeepEqual(result.Before, result.After) { + t.Fatalf("dry-run states must preserve observed damage: %+v", result) + } + switch fault { + case "metadata-quorum", "delete-marker-quorum": + if !isErrVersionNotFound(err) { + t.Fatalf("dry-run must return the reduced metadata error, got %v", err) + } + case "corrupt-metadata": + if !errors.Is(err, errFileCorrupt) { + t.Fatalf("dry-run must return the reduced metadata error, got %v", err) + } + } + unhealthy := 0 + for _, drive := range result.Before.Drives { + if drive.State != madmin.DriveStateOk { + unhealthy++ + } + } + if unhealthy < 3 { + t.Fatalf("faults were not reported: %+v", result.Before) + } + }) + } +} + +// This control proves the missing-metadata fixture reaches dangling deletion. +func TestNativeAuditNonDryRunStillDeletesDangling(t *testing.T) { + meta := nativeAuditMetadata() + disks := make([]StorageAPI, len(meta)) + for i := range disks { + disk := &nativeAuditDisk{info: meta[i]} + if i < 3 { + disk.readErr = errFileVersionNotFound + } + disks[i] = disk + } + er := erasureObjects{ + setDriveCount: 4, defaultParityCount: 2, + getDisks: func() []StorageAPI { return disks }, + getEndpoints: nativeAuditEndpoints, + } + _, err := er.HealObject(t.Context(), "bucket", "object", "version", madmin.HealOpts{NoLock: true, Remove: true, ScanMode: madmin.HealDeepScan}) + if err == nil { + t.Fatal("dangling deletion must report the object removed") + } + for _, disk := range disks { + if got := disk.(*nativeAuditDisk).deletes.Load(); got != 1 { + t.Fatalf("control did not reach dangling deletion: %d", got) + } + } +} + +func TestNativeAuditDryRunPreservesDanglingFiles(t *testing.T) { + obj, roots, err := prepareErasure(t.Context(), 4) + if err != nil { + t.Fatal(err) + } + setObjectLayer(obj) + defer removeRoots(roots) + defer obj.Shutdown(t.Context()) + const bucket, object = "native-audit", "dangling-object" + if err = obj.MakeBucket(t.Context(), bucket, MakeBucketOptions{}); err != nil { + t.Fatal(err) + } + data := bytes.Repeat([]byte("x"), 1024*1024) + if _, err = obj.PutObject(t.Context(), bucket, object, mustGetPutObjReader(t, bytes.NewReader(data), int64(len(data)), "", ""), ObjectOptions{}); err != nil { + t.Fatal(err) + } + for _, root := range roots[:3] { + if err = os.Remove(filepath.Join(root, bucket, object, xlStorageFormatFile)); err != nil { + t.Fatal(err) + } + } + snapshot := func() map[string]string { + files := make(map[string]string) + for _, root := range roots { + err := filepath.Walk(filepath.Join(root, bucket, object), func(path string, info os.FileInfo, err error) error { + if err != nil { + return err + } + if info.IsDir() { + return nil + } + data, err := os.ReadFile(path) + if err != nil { + return err + } + files[path] = fmt.Sprintf("%d/%d/%x", info.Size(), info.ModTime().UnixNano(), sha256.Sum256(data)) + return nil + }) + if err != nil { + t.Fatal(err) + } + } + return files + } + before := snapshot() + if len(before) < 2 { + t.Fatal("fixture has no surviving metadata and payload files") + } + result, err := obj.HealObject(t.Context(), bucket, object, "", madmin.HealOpts{DryRun: true, Remove: true, ScanMode: madmin.HealDeepScan}) + if err == nil { + t.Fatal("missing metadata quorum reported success") + } + if !reflect.DeepEqual(result.Before, result.After) { + t.Fatalf("dry-run reported changes: %+v", result) + } + if after := snapshot(); !reflect.DeepEqual(before, after) { + t.Fatalf("dry-run changed surviving object files: before=%v after=%v", before, after) + } +} diff --git a/cmd/erasure-healing.go b/cmd/erasure-healing.go index 8fb371270..17e1e8cd5 100644 --- a/cmd/erasure-healing.go +++ b/cmd/erasure-healing.go @@ -343,6 +343,11 @@ func (er *erasureObjects) healObject(ctx context.Context, bucket string, object readQuorum, _, err := objectQuorumFromMeta(ctx, partsMetadata, errs, er.defaultParityCount) if err != nil { + if dryRun { + // A metadata quorum failure must not delete dangling data in dry-run. + return er.defaultHealResult(FileInfo{}, storageDisks, storageEndpoints, + errs, bucket, object, versionID), err + } m, derr := er.deleteIfDangling(ctx, bucket, object, partsMetadata, errs, nil, ObjectOptions{ VersionID: versionID, }) From dd0a2da55e456bfe69212cd6bcb11c4af65f19bb Mon Sep 17 00:00:00 2001 From: Feng Ruohang Date: Thu, 5 Nov 2026 01:21:13 +0800 Subject: [PATCH 2/2] fix(heal): send the dry-run flag to remote bucket heal remotePeerS3Client.HealBucket sent only the bucket name and the remove flag, and HealBucketHandler built its heal options from those two values. During a dry-run bucket heal every remote node therefore ran a real one: it created the bucket directory on drives that lacked it, or removed empty bucket directories when the bucket was missing from every pool. Only the coordinating node honored DryRun. Add a "dry-run" key to the peer message and read it into HealOpts.DryRun on the receiving side, so healBucketLocal returns before MakeVol and DeleteVol on every node. The message is a string map: an older node ignores the key, and a missing key reads as false, so a mixed-version cluster behaves as before until every node is upgraded. The regression calls the real client over a test grid connection to the real handler. It fails on main, and it fails if either side of the change is removed. Co-Authored-By: Claude Opus 5.5 Signed-off-by: Feng Ruohang --- cmd/peer-rest-server.go | 2 + cmd/peer-s3-client.go | 15 +++-- cmd/peer-s3-heal-dryrun_test.go | 107 ++++++++++++++++++++++++++++++++ cmd/peer-s3-server.go | 1 + 4 files changed, 121 insertions(+), 4 deletions(-) create mode 100644 cmd/peer-s3-heal-dryrun_test.go diff --git a/cmd/peer-rest-server.go b/cmd/peer-rest-server.go index b28c8db44..25ca056b3 100644 --- a/cmd/peer-rest-server.go +++ b/cmd/peer-rest-server.go @@ -1291,8 +1291,10 @@ func (s *peerRESTServer) HealBucketHandler(mss *grid.MSS) (np *grid.JSON[madmin. } bucketDeleted := mss.Get(peerS3BucketDeleted) == "true" + dryRun := mss.Get(peerS3BucketDryRun) == "true" res, err := healBucketLocal(context.Background(), bucket, madmin.HealOpts{ Remove: bucketDeleted, + DryRun: dryRun, }) if err != nil { return np, grid.NewRemoteErr(err) diff --git a/cmd/peer-s3-client.go b/cmd/peer-s3-client.go index 5b30f76b2..a0262ef6b 100644 --- a/cmd/peer-s3-client.go +++ b/cmd/peer-s3-client.go @@ -354,10 +354,7 @@ func (client *remotePeerS3Client) HealBucket(ctx context.Context, bucket string, return madmin.HealResultItem{}, nil } - mss := grid.NewMSSWith(map[string]string{ - peerS3Bucket: bucket, - peerS3BucketDeleted: strconv.FormatBool(opts.Remove), - }) + mss := healBucketPeerMSS(bucket, opts) ctx, cancel := context.WithTimeout(ctx, globalDriveConfig.GetMaxTimeout()) defer cancel() @@ -366,6 +363,16 @@ func (client *remotePeerS3Client) HealBucket(ctx context.Context, bucket string, return resp.ValueOrZero(), toStorageErr(err) } +// healBucketPeerMSS encodes the bucket heal options a peer applies to its +// local drives. Peers that predate the dry-run key ignore it. +func healBucketPeerMSS(bucket string, opts madmin.HealOpts) *grid.MSS { + return grid.NewMSSWith(map[string]string{ + peerS3Bucket: bucket, + peerS3BucketDeleted: strconv.FormatBool(opts.Remove), + peerS3BucketDryRun: strconv.FormatBool(opts.DryRun), + }) +} + // GetBucketInfo returns bucket stat info from a peer func (client *remotePeerS3Client) GetBucketInfo(ctx context.Context, bucket string, opts BucketOptions) (BucketInfo, error) { conn := client.gridConn() diff --git a/cmd/peer-s3-heal-dryrun_test.go b/cmd/peer-s3-heal-dryrun_test.go new file mode 100644 index 000000000..fa75ed708 --- /dev/null +++ b/cmd/peer-s3-heal-dryrun_test.go @@ -0,0 +1,107 @@ +// Copyright (c) 2026 Feng Ruohang +// +// This file is part of Silo Object Storage stack +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . + +package cmd + +import ( + "os" + "path/filepath" + "testing" + + "github.com/minio/madmin-go/v3" + "github.com/minio/minio/internal/grid" +) + +// A remote node must apply the caller's dry run to its local bucket heal. +// The request goes from remotePeerS3Client.HealBucket over a grid connection +// to the peer handler, as it does between the nodes of a deployment. +func TestPeerHealBucketDryRun(t *testing.T) { + obj, roots, err := prepareErasure(t.Context(), 4) + if err != nil { + t.Fatal(err) + } + setObjectLayer(obj) + defer removeRoots(roots) + defer obj.Shutdown(t.Context()) + + tg, err := grid.SetupTestGrid(2) + if err != nil { + t.Fatal(err) + } + defer tg.Cleanup() + if err = healBucketRPC.Register(tg.Managers[1], (&peerRESTServer{}).HealBucketHandler); err != nil { + t.Fatal(err) + } + conn := func() *grid.Connection { return tg.Managers[0].Connection(tg.Hosts[1]) } + client := &remotePeerS3Client{gridConn: conn} + + const bucket = "peer-heal-dry-run" + if err = obj.MakeBucket(t.Context(), bucket, MakeBucketOptions{}); err != nil { + t.Fatal(err) + } + dirs := make([]string, len(roots)) + for i, root := range roots { + dirs[i] = filepath.Join(root, bucket) + } + if err = os.Remove(dirs[0]); err != nil { + t.Fatal(err) + } + exists := func(dir string) bool { + _, err := os.Stat(dir) + return err == nil + } + heal := func(opts madmin.HealOpts) { + t.Helper() + if _, err := client.HealBucket(t.Context(), bucket, opts); err != nil { + t.Fatal(err) + } + } + + // Dry run must not recreate the missing bucket directory. + heal(madmin.HealOpts{DryRun: true}) + if exists(dirs[0]) { + t.Fatal("dry-run peer bucket heal created a missing bucket directory") + } + + // Dry run must not remove the empty bucket directories. + heal(madmin.HealOpts{DryRun: true, Remove: true}) + for _, dir := range dirs[1:] { + if !exists(dir) { + t.Fatalf("dry-run peer bucket heal removed %s", dir) + } + } + + // Controls. A message without the dry-run key, as sent by an older node, + // still heals: the fixture reaches MakeVol. + if _, err = healBucketRPC.Call(t.Context(), conn(), grid.NewMSSWith(map[string]string{ + peerS3Bucket: bucket, + peerS3BucketDeleted: "false", + })); err != nil { + t.Fatal(err) + } + if !exists(dirs[0]) { + t.Fatal("control: peer bucket heal did not recreate the missing directory") + } + + // A real heal with Remove reaches DeleteVol for the empty directories. + heal(madmin.HealOpts{Remove: true}) + for _, dir := range dirs { + if exists(dir) { + t.Fatalf("control: peer bucket heal kept empty directory %s", dir) + } + } +} diff --git a/cmd/peer-s3-server.go b/cmd/peer-s3-server.go index a252ee85f..295866410 100644 --- a/cmd/peer-s3-server.go +++ b/cmd/peer-s3-server.go @@ -29,6 +29,7 @@ import ( const ( peerS3Bucket = "bucket" peerS3BucketDeleted = "bucket-deleted" + peerS3BucketDryRun = "dry-run" peerS3BucketForceCreate = "force-create" peerS3BucketForceDelete = "force-delete" )