Merge pull request #232 from julienlau/julienlau/fix/commit-metadata-missing-503

Classify missing final-commit metadata as a retryable write failure, preserving storage RPC and quorum behavior. Cover real staged-metadata loss, API responses, successful quorums, and rollback boundaries.

Signed-off-by: Feng Ruohang <rh@vonng.com>

cmd/erasure-metadata-utils_test.go

@@ -20,6 +20,7 @@ package cmd
 import (
 	"context"
 	"encoding/hex"
+	"errors"
 	"fmt"
 	"math/rand"
 	"reflect"
@@ -110,6 +111,96 @@ 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)
+	}
+}
+
+// renameDataFailingDisk is a StorageAPI stub returning a fixed error
+// from RenameData, enough to drive renameData() in unit tests.
+type renameDataFailingDisk struct {
+	StorageAPI
+	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, _ FileInfo, _ bool, opts DeleteOptions) error {
+	d.undoWrite = opts.UndoWrite
+	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) {
+	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)
+				}
+			}
+		})
+	}
+}
+
 // TestHashOrder - test order of ints in array
 func TestHashOrder(t *testing.T) {
 	testCases := []struct {

cmd/erasure-object.go

@@ -1083,6 +1083,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 {

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]

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")
 

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():

cmd/xl-storage-errors_test.go

@@ -18,10 +18,16 @@
 package cmd
 
 import (
+	"context"
+	"errors"
+	"net/http"
 	"os"
+	"path/filepath"
 	"runtime"
+	"sync"
 	"syscall"
 	"testing"
+	"time"
 )
 
 func TestSysErrors(t *testing.T) {
@@ -56,3 +62,120 @@ 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)
+	}
+	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)
+			}
+		})
+	}
+}

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 {