From f480086b6bf8f78341324da7a165d18b10386674 Mon Sep 17 00:00:00 2001 From: Julien Laurenceau Date: Mon, 12 Oct 2026 16:11:06 +0200 Subject: [PATCH 1/3] fix: return 503 instead of 404 when commit metadata is missing When the staged xl.meta disappears between the data rename and the meta-file commit inside RenameData(), the affected drives return errFileNotFound. An in-quorum errFileNotFound is reduced by reduceWriteQuorumErrs() and previously surfaced as: - IncompleteBody (400) from PutObject(), misattributing the failure to a client disconnect, or - ObjectNotFound (404 NoSuchKey) from CompleteMultipartUpload(), telling clients the object is absent so they skip retries. Map errFileNotFound at the commit step to a dedicated internal errCommitMetaMissing, convert it to errErasureWriteQuorum in renameData(), and map it to InsufficientWriteQuorum (HTTP 503) in toObjectErr() so clients retry. Below-quorum outcomes already return InsufficientWriteQuorum and are unchanged, as is the IncompleteBody mapping for staging failures of client-disconnected uploads. Signed-off-by: Julien Laurenceau --- cmd/erasure-metadata-utils_test.go | 30 ++++++++++++++++++++++++++++++ cmd/erasure-object.go | 6 ++++++ cmd/object-api-errors.go | 2 +- cmd/storage-errors.go | 5 +++++ cmd/storage-rest-client.go | 2 ++ cmd/xl-storage-errors_test.go | 26 ++++++++++++++++++++++++++ cmd/xl-storage.go | 10 +++++++++- 7 files changed, 79 insertions(+), 2 deletions(-) diff --git a/cmd/erasure-metadata-utils_test.go b/cmd/erasure-metadata-utils_test.go index b14cc25df..8fc1b4be9 100644 --- a/cmd/erasure-metadata-utils_test.go +++ b/cmd/erasure-metadata-utils_test.go @@ -20,6 +20,7 @@ package cmd import ( "context" "encoding/hex" + "errors" "fmt" "math/rand" "reflect" @@ -110,6 +111,35 @@ func TestReduceErrs(t *testing.T) { } } +// TestReduceWriteQuorumErrsCommitMetaMissing verifies that an in-quorum +// errCommitMetaMissing reduces to the sentinel, which renameData() converts +// to errErasureWriteQuorum, while below-quorum reduces directly to +// errErasureWriteQuorum. +func TestReduceWriteQuorumErrsCommitMetaMissing(t *testing.T) { + errs := make([]error, 16) + for i := range 9 { + errs[i] = errCommitMetaMissing + } + if err := reduceWriteQuorumErrs(t.Context(), errs, objectOpIgnoredErrs, 9); !errors.Is(err, errCommitMetaMissing) { + t.Errorf("expected %s, got %s", errCommitMetaMissing, err) + } + // The sentinel must be distinguishable from errFileNotFound so that + // commit failures are never mapped to ObjectNotFound (404). + if errors.Is(errCommitMetaMissing, errFileNotFound) { + t.Error("errCommitMetaMissing must not match errFileNotFound") + } + + // Below quorum the reduction must yield a write-quorum failure (503); + // eight sentinels against eight committed drives is not a quorum. + belowQuorum := make([]error, 16) + for i := range 8 { + belowQuorum[i] = errCommitMetaMissing + } + if err := reduceWriteQuorumErrs(t.Context(), belowQuorum, objectOpIgnoredErrs, 9); !errors.Is(err, errErasureWriteQuorum) { + t.Errorf("expected %s, got %s", errErasureWriteQuorum, err) + } +} + // TestHashOrder - test order of ints in array func TestHashOrder(t *testing.T) { testCases := []struct { diff --git a/cmd/erasure-object.go b/cmd/erasure-object.go index 8ab3d17a1..4bf060440 100644 --- a/cmd/erasure-object.go +++ b/cmd/erasure-object.go @@ -1072,6 +1072,12 @@ func renameData(ctx context.Context, disks []StorageAPI, srcBucket, srcEntry str errs := g.Wait() err := reduceWriteQuorumErrs(ctx, errs, objectOpIgnoredErrs, writeQuorum) + if errors.Is(err, errCommitMetaMissing) { + // Commit metadata went missing on a quorum of drives; the write + // did not commit. Return a 503 class error so clients retry + // instead of treating the object as absent (404). + err = errErasureWriteQuorum + } if err != nil { dg := errgroup.WithNErrs(len(disks)) for index, nerr := range errs { diff --git a/cmd/object-api-errors.go b/cmd/object-api-errors.go index 7b2b5e404..457970fb0 100644 --- a/cmd/object-api-errors.go +++ b/cmd/object-api-errors.go @@ -162,7 +162,7 @@ func toObjectErr(oerr error, params ...string) error { apiErr.Type = v.Type } return apiErr - case errErasureWriteQuorum.Error(): + case errErasureWriteQuorum.Error(), errCommitMetaMissing.Error(): apiErr := InsufficientWriteQuorum{} if len(params) >= 1 { apiErr.Bucket = params[0] diff --git a/cmd/storage-errors.go b/cmd/storage-errors.go index 69ffcebb0..c0a908eac 100644 --- a/cmd/storage-errors.go +++ b/cmd/storage-errors.go @@ -70,6 +70,11 @@ var errDiskAccessDenied = StorageErr("drive access denied") // errFileNotFound - cannot find the file. var errFileNotFound = StorageErr("file not found") +// errCommitMetaMissing - the staged xl.meta disappeared between the +// data rename and the meta-file commit; a write-quorum failure, not +// a missing object. +var errCommitMetaMissing = StorageErr("commit metadata is missing") + // errFileNotFound - cannot find requested file version. var errFileVersionNotFound = StorageErr("file version not found") diff --git a/cmd/storage-rest-client.go b/cmd/storage-rest-client.go index 76993bc06..0ba573847 100644 --- a/cmd/storage-rest-client.go +++ b/cmd/storage-rest-client.go @@ -106,6 +106,8 @@ func toStorageErr(err error) error { return errVolumeExists case errFileNotFound.Error(): return errFileNotFound + case errCommitMetaMissing.Error(): + return errCommitMetaMissing case errFileVersionNotFound.Error(): return errFileVersionNotFound case errFileNameTooLong.Error(): diff --git a/cmd/xl-storage-errors_test.go b/cmd/xl-storage-errors_test.go index 0a4757765..0551fed6a 100644 --- a/cmd/xl-storage-errors_test.go +++ b/cmd/xl-storage-errors_test.go @@ -18,6 +18,7 @@ package cmd import ( + "errors" "os" "runtime" "syscall" @@ -56,3 +57,28 @@ func TestSysErrors(t *testing.T) { } } } + +// TestCommitMetaMissingMapping verifies the RPC and object-layer mapping +// of errCommitMetaMissing, returned when the staged xl.meta disappears +// between the data rename and the meta-file commit in RenameData(). +func TestCommitMetaMissingMapping(t *testing.T) { + // The sentinel must survive a storage REST/grid round-trip; errors + // cross the wire as strings and are remapped by toStorageErr(). + if got := toStorageErr(errors.New(errCommitMetaMissing.Error())); !errors.Is(got, errCommitMetaMissing) { + t.Fatalf("expected %s, got %s", errCommitMetaMissing, got) + } + if got := toStorageErr(errCommitMetaMissing); got != errCommitMetaMissing { + t.Fatalf("expected %s, got %s", errCommitMetaMissing, got) + } + + // toObjectErr() must map the sentinel to InsufficientWriteQuorum + // (503), never to ObjectNotFound (404). + err := toObjectErr(errCommitMetaMissing, "bucket", "object") + qerr, ok := err.(InsufficientWriteQuorum) + if !ok { + t.Fatalf("expected InsufficientWriteQuorum, got %s (%T)", err, err) + } + if qerr.Bucket != "bucket" || qerr.Object != "object" { + t.Fatalf("unexpected bucket/object %s/%s", qerr.Bucket, qerr.Object) + } +} diff --git a/cmd/xl-storage.go b/cmd/xl-storage.go index 92d8c8dec..81e415553 100644 --- a/cmd/xl-storage.go +++ b/cmd/xl-storage.go @@ -2925,7 +2925,15 @@ func (s *xlStorage) RenameData(ctx context.Context, srcVolume, srcPath string, f // if its a partial rename() do not attempt to delete recursively. // this can be healed since all parts are available. s.deleteFile(dstVolumeDir, dstDataPath, false, false) - return res, osErrToFileErr(err) + err = osErrToFileErr(err) + if errors.Is(err, errFileNotFound) { + // The staged xl.meta disappeared between staging and commit. + // This is a write-quorum failure, not a missing object; use a + // dedicated error so the object layer returns 503 instead of + // mapping it to ObjectNotFound (404). + err = errCommitMetaMissing + } + return res, err } if srcVolume != minioMetaMultipartBucket { From cb85461c420d87107a63c469a7fcc1400cedd774 Mon Sep 17 00:00:00 2001 From: Julien Laurenceau Date: Mon, 12 Oct 2026 17:24:40 +0200 Subject: [PATCH 2/3] test: cover renameData commit-metadata-missing conversion Drive renameData() with stub disks to assert that an in-quorum errCommitMetaMissing is converted to errErasureWriteQuorum (HTTP 503) and that an in-quorum errFileNotFound still surfaces unchanged so PutObject() keeps returning IncompleteBody for client disconnects. Signed-off-by: Julien Laurenceau --- cmd/erasure-metadata-utils_test.go | 43 ++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/cmd/erasure-metadata-utils_test.go b/cmd/erasure-metadata-utils_test.go index 8fc1b4be9..917d8249b 100644 --- a/cmd/erasure-metadata-utils_test.go +++ b/cmd/erasure-metadata-utils_test.go @@ -140,6 +140,49 @@ func TestReduceWriteQuorumErrsCommitMetaMissing(t *testing.T) { } } +// renameDataFailingDisk is a StorageAPI stub returning a fixed error +// from RenameData, enough to drive renameData() in unit tests. +type renameDataFailingDisk struct { + StorageAPI + err error +} + +func (d *renameDataFailingDisk) RenameData(context.Context, string, string, FileInfo, string, string, RenameOptions) (RenameDataResp, error) { + return RenameDataResp{}, d.err +} + +func (d *renameDataFailingDisk) DeleteVersion(context.Context, string, string, FileInfo, bool, DeleteOptions) error { + return nil +} + +// TestRenameDataCommitMetaMissing verifies that renameData() converts an +// in-quorum errCommitMetaMissing into errErasureWriteQuorum (HTTP 503), +// while an in-quorum errFileNotFound keeps its meaning so that +// PutObject() still reports IncompleteBody for client disconnects. +func TestRenameDataCommitMetaMissing(t *testing.T) { + metadata := make([]FileInfo, 16) + for i := range metadata { + metadata[i] = FileInfo{Deleted: true} // valid without erasure info + } + + disks := make([]StorageAPI, 16) + for i := range disks { + disks[i] = &renameDataFailingDisk{err: errCommitMetaMissing} + } + _, _, _, err := renameData(t.Context(), disks, minioMetaTmpBucket, "tmp", metadata, "bucket", "object", 9) + if !errors.Is(err, errErasureWriteQuorum) { + t.Fatalf("expected %s for in-quorum commit failure, got %v", errErasureWriteQuorum, err) + } + + for i := range disks { + disks[i] = &renameDataFailingDisk{err: errFileNotFound} + } + _, _, _, err = renameData(t.Context(), disks, minioMetaTmpBucket, "tmp", metadata, "bucket", "object", 9) + if !errors.Is(err, errFileNotFound) { + t.Fatalf("expected %s for in-quorum staging failure, got %v", errFileNotFound, err) + } +} + // TestHashOrder - test order of ints in array func TestHashOrder(t *testing.T) { testCases := []struct { From eeff112491f0cd2454af91833b4475c3106fe079 Mon Sep 17 00:00:00 2001 From: Feng Ruohang Date: Thu, 15 Oct 2026 18:18:45 +0800 Subject: [PATCH 3/3] test: verify commit metadata loss and write quorum boundaries Signed-off-by: Feng Ruohang --- cmd/erasure-metadata-utils_test.go | 62 ++++++++++++------- cmd/xl-storage-errors_test.go | 97 ++++++++++++++++++++++++++++++ 2 files changed, 137 insertions(+), 22 deletions(-) diff --git a/cmd/erasure-metadata-utils_test.go b/cmd/erasure-metadata-utils_test.go index 917d8249b..559b892bf 100644 --- a/cmd/erasure-metadata-utils_test.go +++ b/cmd/erasure-metadata-utils_test.go @@ -144,14 +144,16 @@ func TestReduceWriteQuorumErrsCommitMetaMissing(t *testing.T) { // from RenameData, enough to drive renameData() in unit tests. type renameDataFailingDisk struct { StorageAPI - err error + err error + undoWrite bool } func (d *renameDataFailingDisk) RenameData(context.Context, string, string, FileInfo, string, string, RenameOptions) (RenameDataResp, error) { return RenameDataResp{}, d.err } -func (d *renameDataFailingDisk) DeleteVersion(context.Context, string, string, FileInfo, bool, DeleteOptions) error { +func (d *renameDataFailingDisk) DeleteVersion(_ context.Context, _, _ string, _ FileInfo, _ bool, opts DeleteOptions) error { + d.undoWrite = opts.UndoWrite return nil } @@ -160,26 +162,42 @@ func (d *renameDataFailingDisk) DeleteVersion(context.Context, string, string, F // while an in-quorum errFileNotFound keeps its meaning so that // PutObject() still reports IncompleteBody for client disconnects. func TestRenameDataCommitMetaMissing(t *testing.T) { - metadata := make([]FileInfo, 16) - for i := range metadata { - metadata[i] = FileInfo{Deleted: true} // valid without erasure info - } - - disks := make([]StorageAPI, 16) - for i := range disks { - disks[i] = &renameDataFailingDisk{err: errCommitMetaMissing} - } - _, _, _, err := renameData(t.Context(), disks, minioMetaTmpBucket, "tmp", metadata, "bucket", "object", 9) - if !errors.Is(err, errErasureWriteQuorum) { - t.Fatalf("expected %s for in-quorum commit failure, got %v", errErasureWriteQuorum, err) - } - - for i := range disks { - disks[i] = &renameDataFailingDisk{err: errFileNotFound} - } - _, _, _, err = renameData(t.Context(), disks, minioMetaTmpBucket, "tmp", metadata, "bucket", "object", 9) - if !errors.Is(err, errFileNotFound) { - t.Fatalf("expected %s for in-quorum staging failure, got %v", errFileNotFound, err) + for _, tc := range []struct { + name string + failures int + diskErr error + wantErr error + }{ + {"all_commit_metadata_missing", 16, errCommitMetaMissing, errErasureWriteQuorum}, + {"quorum_commit_metadata_missing", 9, errCommitMetaMissing, errErasureWriteQuorum}, + {"neither_side_has_quorum", 8, errCommitMetaMissing, errErasureWriteQuorum}, + {"successful_write_quorum", 7, errCommitMetaMissing, nil}, + {"staging_failure", 16, errFileNotFound, errFileNotFound}, + } { + t.Run(tc.name, func(t *testing.T) { + metadata := make([]FileInfo, 16) + disks := make([]StorageAPI, 16) + for i := range disks { + metadata[i] = FileInfo{Deleted: true} // valid without erasure info + disk := &renameDataFailingDisk{} + if i < tc.failures { + disk.err = tc.diskErr + } + disks[i] = disk + } + _, _, _, err := renameData(t.Context(), disks, minioMetaTmpBucket, "tmp", metadata, "bucket", "object", 9) + if !errors.Is(err, tc.wantErr) { + t.Fatalf("expected %v, got %v", tc.wantErr, err) + } + for i, disk := range disks { + // Failed writes undo only the drives that committed. A + // successful quorum must never be rolled back. + wantUndo := tc.wantErr != nil && i >= tc.failures + if got := disk.(*renameDataFailingDisk).undoWrite; got != wantUndo { + t.Errorf("disk %d: UndoWrite = %v, want %v", i, got, wantUndo) + } + } + }) } } diff --git a/cmd/xl-storage-errors_test.go b/cmd/xl-storage-errors_test.go index 0551fed6a..b9d12fc4d 100644 --- a/cmd/xl-storage-errors_test.go +++ b/cmd/xl-storage-errors_test.go @@ -18,11 +18,16 @@ package cmd import ( + "context" "errors" + "net/http" "os" + "path/filepath" "runtime" + "sync" "syscall" "testing" + "time" ) func TestSysErrors(t *testing.T) { @@ -81,4 +86,96 @@ func TestCommitMetaMissingMapping(t *testing.T) { if qerr.Bucket != "bucket" || qerr.Object != "object" { t.Fatalf("unexpected bucket/object %s/%s", qerr.Bucket, qerr.Object) } + apiErr := toAPIError(t.Context(), err) + if apiErr.Code != "SlowDownWrite" || apiErr.HTTPStatusCode != http.StatusServiceUnavailable { + t.Fatalf("expected SlowDownWrite/503, got %+v", apiErr) + } +} + +// missingCommitMetaContext removes staged metadata at an existing cancellation +// check, after WriteAll and (for non-inline objects) the data rename succeeded. +// This deterministically models concurrent cleanup without changing production +// code or relying on a filesystem race. The context itself remains live. +type missingCommitMetaContext struct { + context.Context + mu sync.Mutex + meta, dataDest string + removed bool + removeErr error +} + +func (c *missingCommitMetaContext) Done() <-chan struct{} { + c.mu.Lock() + defer c.mu.Unlock() + if !c.removed { + if _, err := os.Stat(c.meta); err == nil { + dataMoved := c.dataDest == "" + if !dataMoved { + _, err = os.Stat(c.dataDest) + dataMoved = err == nil + } + if dataMoved { + c.removeErr = os.Remove(c.meta) + c.removed = c.removeErr == nil + } + } + } + return c.Context.Done() +} + +func TestXLStorageRenameDataCommitMetaMissing(t *testing.T) { + for _, tc := range []struct { + name string + dataFile bool + }{ + {"empty_object", false}, + {"data_directory", true}, + } { + t.Run(tc.name, func(t *testing.T) { + disk, root, err := newXLStorageTestSetup(t) + if err != nil { + t.Fatal(err) + } + defer disk.Close() + s := disk.storage + if err := s.MakeVol(t.Context(), "bucket"); err != nil { + t.Fatal(err) + } + fi := newFileInfo("object", 2, 2) + fi.DataDir, fi.VersionID = mustGetUUID(), mustGetUUID() + fi.Erasure.Index = 1 + fi.ModTime = time.Now().UTC() + fi.Fresh = true + ctx := &missingCommitMetaContext{ + Context: t.Context(), + meta: filepath.Join(root, minioMetaTmpBucket, "staged", xlStorageFormatFile), + } + if tc.dataFile { + fi.Size = 1 + fi.AddObjectPart(1, "", 1, 1, fi.ModTime, nil, nil) + part := filepath.Join(root, minioMetaTmpBucket, "staged", fi.DataDir, "part.1") + if err := os.MkdirAll(filepath.Dir(part), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(part, []byte("a"), 0o644); err != nil { + t.Fatal(err) + } + ctx.dataDest = filepath.Join(root, "bucket", "object", fi.DataDir) + } + _, err = s.RenameData(ctx, minioMetaTmpBucket, "staged", fi, "bucket", "object", RenameOptions{}) + if !ctx.removed { + t.Fatalf("metadata removal did not execute: removal error = %v, RenameData error = %v", ctx.removeErr, err) + } + if !errors.Is(err, errCommitMetaMissing) { + t.Fatalf("expected commit metadata missing, got %v", err) + } + apiErr := toAPIError(t.Context(), toObjectErr(err, "bucket", "object")) + if apiErr.Code != "SlowDownWrite" || apiErr.HTTPStatusCode != http.StatusServiceUnavailable { + t.Fatalf("expected SlowDownWrite/503, got %+v", apiErr) + } + if _, err := os.Stat(filepath.Join(root, "bucket", "object", xlStorageFormatFile)); !os.IsNotExist(err) { + t.Fatalf("failed commit must not produce destination metadata: %v", err) + } + }) + } }