fix(storage): retry metadata writes against a late RenameData cleanup

A RenameData that fails after its caller gave up removes its empty
object directory and the empty parents (ed05d982a, 73025824a). Only
the two renames of RenameData itself survived that cleanup, through
renameDataAll. Any other write of the same key that creates the object
directory failed on the drive when the cleanup removed the directory
between the directory creation and the open: WriteMetadata for a
delete marker on an absent key or for a first version, through
openFile, returned the raw open error, and writeAllDirect returned
errFileNotFound. The version was then missing on that drive, and
DeleteObject queues only offline drives for MRF. Before ed05d982a the
failure path never removed the directory, so this window is new.

Move the retry policy of renameDataAll into the shared helpers. A write
below the volume directory retries once per path level when the
directory creation or the use of the directory reports "not found"
(retriesAgainstCleanup): a cleanup removes each directory at most once
and each failed attempt needs one such removal. reliableRename applies
it in place of its blind single rename retry, which never recreated the
directory, and only while the source still exists, so renameDataAll is
replaced by renameAll. openFileAll applies it to the directory creation
and open shared by openFile and writeAllDirect. mkdirAllErr is split
out of mkdirAll, like renameAllErr, so that the retry decision uses the
OS error: on unix, mkdirAll maps a vanished parent to
errFileAccessDenied through isSysErrPathNotFound.

Invariants kept: the cleanup still removes only empty directories and
never a committed xl.meta or data directory; the error mapping of
mkdirAll, renameAll, openFile and writeAllDirect is unchanged, including
the Windows isSysErrPathNotFound branch; retries are bounded by the
path depth; a rename whose source is gone still fails at once.
UpdateMetadata renames over an existing xl.meta, so its directory is
never empty and the cleanup cannot remove it; it needs no change.

TestXLStorageMetadataWriteSurvivesLateCleanup injects, right after the
writer's os.Mkdir of the object directory, either a late RenameData
whose staged data is gone or a cleanup that walks up the whole path one
directory per step, for a delete marker, a first version and a large
xl.meta. Before the fix all six cases failed with "no such file or
directory" or "file not found".

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Signed-off-by: Feng Ruohang <rh@vonng.com>

cmd/os-reliable.go

@@ -21,6 +21,7 @@ import (
 	"fmt"
 	"os"
 	"path"
+	"strings"
 )
 
 // Wrapper functions to os.RemoveAll, which calls reliableRemoveAll
@@ -82,20 +83,63 @@ func mkdirAll(dirPath string, mode os.FileMode, baseDir string) (err error) {
 		return err
 	}
 
-	if err = reliableMkdirAll(dirPath, mode, baseDir); err != nil {
+	return mkdirAllErr(reliableMkdirAll(dirPath, mode, baseDir))
+}
+
+// mkdirAllErr converts an error of reliableMkdirAll into the error that
+// mkdirAll returns.
+func mkdirAllErr(err error) error {
+	switch {
+	case err == nil:
+		return nil
+	case isSysErrNotDir(err):
 		// File path cannot be verified since one of the parents is a file.
-		if isSysErrNotDir(err) {
-			return errFileAccessDenied
-		} else if isSysErrPathNotFound(err) {
-			// This is a special case should be handled only for
-			// windows, because windows API does not return "not a
-			// directory" error message. Handle this specifically here.
-			return errFileAccessDenied
-		}
+		return errFileAccessDenied
+	case isSysErrPathNotFound(err):
+		// This is a special case should be handled only for
+		// windows, because windows API does not return "not a
+		// directory" error message. Handle this specifically here.
+		return errFileAccessDenied
+	default:
 		return osErrToFileErr(err)
 	}
+}
 
-	return nil
+// openFileAll is mkdirAll for the parent of filePath followed by the open
+// of filePath with flags, with direct I/O when direct is set. A cleanup of a
+// failed write of the same key can remove the directories in between, see
+// retriesAgainstCleanup; then it creates them again. An error of the
+// directory creation is the error of mkdirAll, an error of the open is the
+// error of the OS call.
+func openFileAll(filePath string, flags int, direct bool, baseDir string) (*os.File, error) {
+	parentDir := path.Dir(filePath)
+	if err := checkPathLength(parentDir); err != nil {
+		return nil, err
+	}
+	retries := retriesAgainstCleanup(filePath, baseDir)
+	for {
+		// Create top level directories if they don't exist.
+		// with mode 0777 mkdir honors system umask.
+		if err := reliableMkdirAll(parentDir, 0o777, baseDir); err != nil {
+			if retries > 0 && osIsNotExist(err) {
+				retries--
+				continue
+			}
+			return nil, mkdirAllErr(err)
+		}
+		var f *os.File
+		var err error
+		if direct {
+			f, err = OpenFileDirectIO(filePath, flags, 0o666)
+		} else {
+			f, err = OpenFile(filePath, flags, 0o666)
+		}
+		if err != nil && retries > 0 && osIsNotExist(err) && flags&os.O_CREATE != 0 {
+			retries--
+			continue
+		}
+		return f, err
+	}
 }
 
 // Reliably retries os.MkdirAll if for some reason os.MkdirAll returns
@@ -175,24 +219,41 @@ func renameAllErr(err error, srcFilePath, dstFilePath string) error {
 	}
 }
 
-// Reliably retries os.RenameAll if for some reason os.RenameAll returns
-// syscall.ENOENT (parent does not exist).
-func reliableRename(srcFilePath, dstFilePath, baseDir string) (err error) {
-	if err = reliableMkdirAll(path.Dir(dstFilePath), 0o777, baseDir); err != nil {
-		return err
-	}
-
-	i := 0
-	for {
-		// After a successful parent directory create attempt a renameAll.
-		if err = Rename(srcFilePath, dstFilePath); err != nil {
-			// Retry only for the first retryable error.
-			if osIsNotExist(err) && i == 0 {
-				i++
-				continue
-			}
-		}
-		break
-	}
-	return err
+// retriesAgainstCleanup returns how often a write to filePath is retried
+// when a directory of its path disappears between the directory creation
+// and the use of the directory: once per path level below baseDir.
+//
+// A failed RenameData, typically one that runs after its caller gave up,
+// removes its object directory and the parents that are empty, so it can
+// remove the directories that another write of the same key has just
+// created. The directory creation or the write then reports "not found"
+// although nothing is wrong with the write: retry, which creates the
+// directories again. A cleanup removes each directory of the path at most
+// once and each failed attempt needs at least one such removal, so one retry
+// per path level is enough against any single cleanup.
+func retriesAgainstCleanup(filePath, baseDir string) int {
+	return strings.Count(strings.TrimPrefix(path.Clean(filePath), path.Clean(baseDir)), SlashSeparator)
+}
+
+// Reliably retries os.Rename if for some reason the directory creation or
+// os.Rename returns syscall.ENOENT (parent does not exist) while the source
+// is still in place, see retriesAgainstCleanup. The decision uses the error
+// of the attempt itself, since another write can recreate the directories
+// in between.
+func reliableRename(srcFilePath, dstFilePath, baseDir string) (err error) {
+	retries := retriesAgainstCleanup(dstFilePath, baseDir)
+	for {
+		if err = reliableMkdirAll(path.Dir(dstFilePath), 0o777, baseDir); err == nil {
+			// After a successful parent directory create attempt a rename.
+			err = Rename(srcFilePath, dstFilePath)
+		}
+		if err == nil || retries == 0 || !osIsNotExist(err) {
+			return err
+		}
+		if _, serr := Lstat(path.Clean(srcFilePath)); serr != nil {
+			// The source is gone: a late call, which the caller cleans up.
+			return err
+		}
+		retries--
+	}
 }

cmd/xl-storage-errors_test.go

@@ -21,10 +21,12 @@ import (
 	"bytes"
 	"context"
 	"errors"
+	"fmt"
 	"net/http"
 	"os"
 	"path/filepath"
 	"runtime"
+	"strings"
 	"sync"
 	"sync/atomic"
 	"syscall"
@@ -32,6 +34,7 @@ import (
 	"time"
 
 	"github.com/minio/madmin-go/v3"
+	xioutil "github.com/minio/minio/internal/ioutil"
 )
 
 func TestSysErrors(t *testing.T) {
@@ -526,3 +529,153 @@ func TestXLStorageRenameDataRetriesWhileCleanupWalksUp(t *testing.T) {
 		})
 	}
 }
+
+// A failed RenameData that runs late, after its caller gave up, removes its
+// empty object directory and the empty parents. When it does so right after
+// another write of the same key created the object directory, that write
+// must create the directory again and succeed, whichever way it writes its
+// metadata file: a fresh xl.meta opened in place (a delete marker on an
+// absent key), a first version of a key without xl.meta, or a large xl.meta
+// written directly. A cleanup that walks up the whole path, one directory
+// per step, is survived as well. UpdateMetadata renames over an existing
+// xl.meta, so its directory is never empty and the cleanup cannot remove it.
+func TestXLStorageMetadataWriteSurvivesLateCleanup(t *testing.T) {
+	if runtime.GOOS == globalWindowsOSName {
+		t.Skip("directory creation is not traced on Windows")
+	}
+	const object = "a/b/object"
+	writers := []struct {
+		name  string
+		write func(ctx context.Context, s *xlStorage, fi FileInfo) (FileInfo, error)
+	}{
+		{"delete_marker", func(ctx context.Context, s *xlStorage, fi FileInfo) (FileInfo, error) {
+			// DeleteVersion with forceDelMarker on a key without xl.meta.
+			fi.Deleted, fi.Fresh = true, true
+			return fi, s.WriteMetadata(ctx, "", "bucket", object, fi)
+		}},
+		{"first_version", func(ctx context.Context, s *xlStorage, fi FileInfo) (FileInfo, error) {
+			// Not fresh: the missing xl.meta is read first.
+			fi.Size, fi.Data = 1, []byte("x")
+			fi.AddObjectPart(1, "", 1, 1, fi.ModTime, nil, nil)
+			fi.SetInlineData()
+			return fi, s.WriteMetadata(ctx, "", "bucket", object, fi)
+		}},
+		{"direct_write", func(ctx context.Context, s *xlStorage, fi FileInfo) (FileInfo, error) {
+			// Larger than the direct I/O alignment: written through
+			// writeAllDirect, with O_DIRECT where supported.
+			fi.Metadata = map[string]string{"x-amz-meta-pad": strings.Repeat("p", 2*xioutil.DirectioAlignSize)}
+			var xlMeta xlMetaV2
+			if err := xlMeta.AddVersion(fi); err != nil {
+				return fi, err
+			}
+			buf, err := xlMeta.AppendTo(nil)
+			if err != nil {
+				return fi, err
+			}
+			return fi, s.WriteAll(ctx, "bucket", pathJoin(object, xlStorageFormatFile), buf)
+		}},
+	}
+	for _, wr := range writers {
+		for _, walkUp := range []bool{false, true} {
+			name := wr.name + "/late_rename_data"
+			if walkUp {
+				name = wr.name + "/walk_up"
+			}
+			t.Run(name, func(t *testing.T) {
+				disk, root, err := newXLStorageTestSetup(t)
+				if err != nil {
+					t.Fatal(err)
+				}
+				defer disk.Close()
+				s := disk.storage
+				ctx := t.Context()
+				if err := s.MakeVol(ctx, "bucket"); err != nil {
+					t.Fatal(err)
+				}
+				objDir := filepath.Join(root, "bucket", filepath.FromSlash(object))
+				bDir := filepath.Dir(objDir)
+				aDir := filepath.Dir(bDir)
+				fi := newFileInfo(object, 2, 2)
+				fi.DataDir, fi.VersionID = mustGetUUID(), mustGetUUID()
+				fi.Erasure.Index = 1
+				fi.ModTime = time.Now().UTC()
+
+				// The walk removes object/, then b/, then a/, each right after
+				// the write's next directory creation reached it, whether it
+				// created the directory or found it in place.
+				walk := []string{objDir, bDir, aDir}
+				var mu sync.Mutex
+				steps := 0
+				var hookErrs []error
+				var fired atomic.Bool
+				traceOSCalls(t, func(tr madmin.TraceInfo) {
+					if tr.FuncName != "os.Mkdir" {
+						return
+					}
+					if walkUp {
+						mu.Lock()
+						defer mu.Unlock()
+						if steps == len(walk) || filepath.Clean(tr.Path) != walk[steps] {
+							return
+						}
+						if err := os.Remove(walk[steps]); err != nil {
+							hookErrs = append(hookErrs, err)
+						}
+						steps++
+						return
+					}
+					if tr.Error != "" || filepath.Clean(tr.Path) != objDir || !fired.CompareAndSwap(false, true) {
+						return
+					}
+					// The write has just created the object directory. A late
+					// RenameData of the same key, whose staged data is gone,
+					// fails now and removes the directory and its empty parents.
+					late := newFileInfo(object, 2, 2)
+					late.DataDir, late.VersionID = mustGetUUID(), mustGetUUID()
+					late.Erasure.Index = 1
+					late.ModTime = time.Now().UTC()
+					late.Size = 1
+					late.AddObjectPart(1, "", 1, 1, late.ModTime, nil, nil)
+					_, err := s.RenameData(context.Background(), minioMetaTmpBucket, mustGetUUID(), late, "bucket", object, RenameOptions{})
+					var errs []error
+					if !errors.Is(err, errFileNotFound) {
+						errs = append(errs, fmt.Errorf("late RenameData returned %v, want %v", err, errFileNotFound))
+					}
+					if _, serr := os.Stat(objDir); !os.IsNotExist(serr) {
+						errs = append(errs, fmt.Errorf("late RenameData left %s (%v)", objDir, serr))
+					}
+					mu.Lock()
+					hookErrs = append(hookErrs, errs...)
+					steps++
+					mu.Unlock()
+				})
+
+				fi, err = wr.write(ctx, s, fi)
+				mu.Lock()
+				done, errs := steps, hookErrs
+				mu.Unlock()
+				for _, herr := range errs {
+					t.Error(herr)
+				}
+				if err != nil {
+					t.Fatalf("%s failed after a concurrent cleanup: %v", wr.name, err)
+				}
+				want := 1
+				if walkUp {
+					want = len(walk)
+				}
+				if done != want {
+					t.Fatalf("the cleanup made %d of %d steps", done, want)
+				}
+				got, err := s.ReadVersion(ctx, "", "bucket", object, fi.VersionID, ReadOptions{ReadData: true})
+				if err != nil {
+					t.Fatalf("written version lost: %v", err)
+				}
+				if got.VersionID != fi.VersionID || got.Deleted != fi.Deleted || !bytes.Equal(got.Data, fi.Data) {
+					t.Fatalf("read back version %s deleted=%v data=%q, want %s deleted=%v data=%q",
+						got.VersionID, got.Deleted, got.Data, fi.VersionID, fi.Deleted, fi.Data)
+				}
+			})
+		}
+	}
+}

cmd/xl-storage.go

@@ -2001,13 +2001,8 @@ func (s *xlStorage) openFile(filePath string, mode int, skipParent string) (f *o
 	if skipParent == "" {
 		skipParent = s.drivePath
 	}
-	// Create top level directories if they don't exist.
-	// with mode 0777 mkdir honors system umask.
-	if err = mkdirAll(pathutil.Dir(filePath), 0o777, skipParent); err != nil {
-		return nil, osErrToFileErr(err)
-	}
-
-	w, err := OpenFile(filePath, mode, 0o666)
+	// Create top level directories if they don't exist, then open the file.
+	w, err := openFileAll(filePath, mode, false, skipParent)
 	if err != nil {
 		// File path cannot be verified since one of the parents is a file.
 		switch {
@@ -2158,21 +2153,10 @@ func (s *xlStorage) writeAllDirect(ctx context.Context, filePath string, fileSiz
 		skipParent = s.drivePath
 	}
 
-	// Create top level directories if they don't exist.
-	// with mode 0777 mkdir honors system umask.
-	parentFilePath := pathutil.Dir(filePath)
-	if err = mkdirAll(parentFilePath, 0o777, skipParent); err != nil {
-		return osErrToFileErr(err)
-	}
-
 	odirectEnabled := globalAPIConfig.odirectEnabled() && s.oDirect && fileSize > 0
 
-	var w *os.File
-	if odirectEnabled {
-		w, err = OpenFileDirectIO(filePath, flags, 0o666)
-	} else {
-		w, err = OpenFile(filePath, flags, 0o666)
-	}
+	// Create top level directories if they don't exist, then open the file.
+	w, err := openFileAll(filePath, flags, odirectEnabled, skipParent)
 	if err != nil {
 		return osErrToFileErr(err)
 	}
@@ -2583,40 +2567,6 @@ func skipAccessChecks(volume string) (ok bool) {
 	return strings.HasPrefix(volume, minioMetaBucket)
 }
 
-// renameDataAll is renameAll for the two renames of RenameData. A failed
-// RenameData, typically one that runs after its caller gave up, removes its
-// object directory and the parents that are empty, so it can remove the
-// directories that renameAll is creating for this rename. The directory
-// creation or the rename then reports "not found" although the source is
-// still in place: retry, which creates the directories again. A cleanup
-// removes each directory of the path at most once and each failed attempt
-// needs at least one such removal, so one retry per path level is enough
-// against any single cleanup. The decision uses the error of the attempt
-// itself, since another write can recreate the directories in between.
-func renameDataAll(srcPath, dstPath, skipParent string) error {
-	if srcPath == "" || dstPath == "" {
-		return errInvalidArgument
-	}
-	if err := checkPathLength(srcPath); err != nil {
-		return err
-	}
-	if err := checkPathLength(dstPath); err != nil {
-		return err
-	}
-	retries := strings.Count(strings.TrimPrefix(pathutil.Clean(dstPath), pathutil.Clean(skipParent)), SlashSeparator)
-	for {
-		err := reliableRename(srcPath, dstPath, skipParent)
-		if err == nil || retries == 0 || !osIsNotExist(err) {
-			return renameAllErr(err, srcPath, dstPath)
-		}
-		if _, serr := Lstat(pathutil.Clean(srcPath)); serr != nil {
-			// The source is gone: a late call, which the caller cleans up.
-			return renameAllErr(err, srcPath, dstPath)
-		}
-		retries--
-	}
-}
-
 // RenameData - rename source path to destination path atomically, metadata and data directory.
 func (s *xlStorage) RenameData(ctx context.Context, srcVolume, srcPath string, fi FileInfo, dstVolume, dstPath string, opts RenameOptions) (res RenameDataResp, err error) {
 	defer func() {
@@ -2923,7 +2873,7 @@ func (s *xlStorage) RenameData(ctx context.Context, srcVolume, srcPath string, f
 		if contextCanceled(ctx) {
 			return res, ctx.Err()
 		}
-		if err = renameDataAll(srcDataPath, dstDataPath, skipParent); err != nil {
+		if err = renameAll(srcDataPath, dstDataPath, skipParent); err != nil {
 			if legacyPreserved {
 				// Any failed rename calls un-roll previous transaction.
 				s.deleteFile(dstVolumeDir, legacyDataPath, true, false)
@@ -2962,7 +2912,7 @@ func (s *xlStorage) RenameData(ctx context.Context, srcVolume, srcPath string, f
 	}
 
 	// Commit meta-file
-	if err = renameDataAll(srcFilePath, dstFilePath, skipParent); err != nil {
+	if err = renameAll(srcFilePath, dstFilePath, skipParent); err != nil {
 		if legacyPreserved {
 			// Any failed rename calls un-roll previous transaction.
 			s.deleteFile(dstVolumeDir, legacyDataPath, true, false)