fix(s3/versioning): repair dangling latest-version pointer after partial delete (#9460)

* fix(s3/versioning): repair dangling latest-version pointer after partial delete

deleteSpecificObjectVersion did two non-atomic filer ops: rm the version
blob, then update the .versions/ pointer. Step 2 failures were silently
logged and the client got 204 OK, so any transient blip (filer timeout,
process restart between RPCs, lock contention) left the .versions/
directory naming a missing file. Subsequent GETs paid the 10-retry
self-heal cost and returned NoSuchKey — surfacing as "Storage not found"
to Veeam, which is what triggered this investigation.

Three changes:

1. Pre-roll the pointer for the singleton / multi-version-deleting-latest
   cases. The pointer is repointed (multi) or cleared (singleton) before
   the blob rm. A failure between leaves a recoverable orphan blob —
   pointer is consistent, GETs succeed or correctly miss without
   entering the stale-pointer self-heal path.

2. Wrap the load-bearing filer ops in updateLatestVersionAfterDeletion
   with bounded retries (~6.3s worst case). When retries are exhausted
   the function now returns a non-nil error instead of swallowing it.
   The caller logs at Error level and queues the path for the
   reconciler.

3. Background reconciler drains stranded .versions/ pointer-to-missing
   states off the hot path. Bounded in-memory queue with capped retries;
   read-path heal remains as a last-resort safety net.

* fix(s3/versioning): address review on #9460

Four fixes addressing review on PR #9460. All four are correctness;
no behavioural change for the happy path.

1. repointLatestBeforeDeletion: discriminate NotFound from transient
   errors when re-fetching the .versions/ entry. Previously any error
   returned rolled=true,nil — a transient filer hiccup at that point
   would cause the caller to skip the post-delete reconciliation AND
   proceed with the blob rm, producing exactly the dangling pointer
   state the PR aims to prevent. NotFound stays "vacuously consistent"
   (directory already gone); other errors surface so the caller aborts
   before removing the blob.

2. Move the singleton .versions/ teardown out of
   repointLatestBeforeDeletion (where it ran BEFORE the blob rm and
   always failed with "non-empty folder") into deleteSpecificObjectVersion
   AFTER the blob rm. Adds a wasSingleton return value so the caller
   knows when to run the teardown. Without this, every singleton-version
   delete in a versioned bucket leaked an empty .versions/ directory.

3. Wrap the list, getEntry, and mkFile calls inside
   repointLatestBeforeDeletion with retryFilerOp so the pre-roll has
   the same transient-failure resilience as the post-roll path. Without
   retries, a single transient blip causes the caller to fall back to
   the legacy non-atomic flow even when the filer recovers immediately.

4. healVersionsPointer in the reconciler: same NotFound-vs-transient
   discrimination on both the .versions/ getEntry and the latest-file
   presence probe. Previously a transient filer error would silently
   evict the candidate from the queue as "healed", leaving the real
   stranded state until a client read happened to surface it.

Also fixes the gemini-flagged consistency nit: the queued-for-reconciler
error log now uses normalizedObject instead of object so it matches the
queue entry's key.

* fix(s3/versioning): short-circuit terminal errors in retryFilerOp

Add isRetryableFilerErr that returns false for filer_pb.ErrNotFound,
gRPC NotFound, context.Canceled, and context.DeadlineExceeded.
retryFilerOp now bails immediately on a terminal error and returns it
unwrapped, so callers like repointLatestBeforeDeletion.getEntry and
updateLatestVersionAfterDeletion.rm see the raw NotFound instead of
paying the ~6.3 s retry-budget delay AND parsing it out of an
"exhausted N retries" wrapper.

errors.Is and status.Code already walk the %w chain so today's call
sites still work, but the delay was real on the hot DELETE path
whenever a key was genuinely absent. Test added covering all five
terminal-error shapes — each must run the wrapped fn exactly once and
return in under 50 ms.

weed/s3api/s3api_object_versioning.go

@@ -5,6 +5,7 @@ package s3api
 
 import (
 	"bytes"
+	"context"
 	"encoding/hex"
 	"encoding/xml"
 	"errors"
@@ -1044,6 +1045,28 @@ func (s3a *S3ApiServer) deleteSpecificObjectVersion(bucket, object, versionId st
 		}
 	}
 
+	// Option 1: when deleting the current latest, repoint the .versions/
+	// pointer BEFORE removing the blob. A failure between the two steps
+	// then leaves a recoverable orphan blob (the pointer is already
+	// consistent, GETs serve the prior version or NoSuchKey correctly)
+	// instead of a dangling pointer (which forces every subsequent GET
+	// through the 10-retry self-heal path and returns NoSuchKey for
+	// objects whose latest version was singleton).
+	var (
+		prePointerRolled bool
+		preWasSingleton  bool
+	)
+	if isLatestVersion {
+		rolled, singleton, prepErr := s3a.repointLatestBeforeDeletion(bucket, normalizedObject, versionId)
+		if prepErr != nil {
+			// Surface to the client so they can retry the DELETE. The
+			// blob has NOT been removed yet, so retrying is safe.
+			return fmt.Errorf("failed to repoint latest version before deleting %s: %w", versionId, prepErr)
+		}
+		prePointerRolled = rolled
+		preWasSingleton = singleton
+	}
+
 	// Attempt to delete the version file
 	// Note: We don't check if the file exists first to avoid race conditions
 	// The deletion operation should be idempotent
@@ -1059,24 +1082,254 @@ func (s3a *S3ApiServer) deleteSpecificObjectVersion(bucket, object, versionId st
 		return fmt.Errorf("failed to delete version %s: %v", versionId, deleteErr)
 	}
 
-	// If we deleted the latest version, update the .versions directory metadata to point to the new latest
-	if isLatestVersion {
-		err := s3a.updateLatestVersionAfterDeletion(bucket, normalizedObject)
-		if err != nil {
-			glog.Warningf("deleteSpecificObjectVersion: failed to update latest version after deletion: %v", err)
-			// Don't return error since the deletion was successful
+	switch {
+	case isLatestVersion && prePointerRolled && preWasSingleton:
+		// Pre-roll cleared a singleton pointer. The blob is now gone,
+		// so the .versions/ directory should be empty — try to tear it
+		// down. Non-recursive: any orphan from older code paths leaves
+		// the directory in place for the empty-folder cleaner or our
+		// reconciler to handle.
+		if rmErr := s3a.rm(s3a.bucketDir(bucket), normalizedObject+s3_constants.VersionsFolder, true, false); rmErr != nil {
+			glog.V(2).Infof("deleteSpecificObjectVersion: deferring .versions/ teardown for %s/%s: %v", bucket, normalizedObject, rmErr)
+		}
+	case isLatestVersion && !prePointerRolled:
+		// Multi-version case where another version still exists, or the
+		// pre-step decided the pointer was no longer ours to roll. Run
+		// the post-deletion reconciliation: it updates the pointer to
+		// the new latest and tears down .versions/ when nothing remains.
+		if err := s3a.updateLatestVersionAfterDeletion(bucket, normalizedObject); err != nil {
+			// Option 2: surface this so the operator sees the load-bearing
+			// failure, and queue the path for the reconciler to retry off
+			// the hot path. The blob delete already succeeded, so we don't
+			// want to fail the client request (Veeam et al. treat 5xx on
+			// DELETE as a hard storage error) — but we MUST drive the
+			// pointer to consistency, otherwise the next read pays the
+			// self-heal cost.
+			glog.Errorf("deleteSpecificObjectVersion: failed to update latest version after deletion for %s/%s (queued for reconciler): %v", bucket, normalizedObject, err)
+			if s3a.versionsHealQueue != nil {
+				s3a.versionsHealQueue.Enqueue(bucket, normalizedObject)
+			}
 		}
 	}
 
 	return nil
 }
 
+// repointLatestBeforeDeletion is the "option 1" pre-step for deleting a
+// version that the .versions/ pointer currently names. It scans the
+// directory to find the next-newest version excluding the one about to
+// be deleted, and either repoints to it (multi-version case) or clears
+// the pointer entirely (single-version case). When this returns rolled
+// = true the caller should NOT run updateLatestVersionAfterDeletion
+// after the blob rm — the pointer is already consistent.
+//
+// wasSingleton = true indicates the caller cleared a singleton pointer.
+// After the caller's blob rm completes, deleteSpecificObjectVersion is
+// expected to run the post-deletion .versions/ teardown so the now-empty
+// directory entry is removed; doing that teardown inside this function
+// (i.e. before the blob rm) always fails with "non-empty folder" because
+// the blob is still present.
+//
+// When this returns rolled = false the .versions/ pointer was not in
+// sync with the caller's view (some concurrent writer changed it) and
+// the current deletion is no longer touching the latest; the caller
+// proceeds with the historical multi-step path which will re-snapshot
+// the state inside updateLatestVersionAfterDeletion.
+//
+// All load-bearing filer ops here are wrapped in retryFilerOp so the
+// pre-roll matches the resilience of the post-roll path; without this,
+// a transient filer hiccup during the pre-step would cause the caller
+// to fall back to the legacy non-atomic flow.
+func (s3a *S3ApiServer) repointLatestBeforeDeletion(bucket, normalizedObject, versionIdToDelete string) (rolled bool, wasSingleton bool, err error) {
+	bucketDir := s3a.bucketDir(bucket)
+	versionsObjectPath := normalizedObject + s3_constants.VersionsFolder
+	versionsDir := bucketDir + "/" + versionsObjectPath
+
+	// Find the chronologically newest *other* version (excluding the
+	// one we're about to delete). For singleton objects this returns
+	// nil; for multi-version it returns the previous version.
+	var (
+		newLatestEntry        *filer_pb.Entry
+		newLatestVersionId    string
+		newLatestVersionFile  string
+		newLatestIsDeleteMark bool
+		startFrom             string
+	)
+	for {
+		var (
+			entries []*filer_pb.Entry
+			isLast  bool
+		)
+		if listErr := retryFilerOp("repointLatestBeforeDeletion.list", func() error {
+			var lerr error
+			entries, isLast, lerr = s3a.list(versionsDir, "", startFrom, false, filer.PaginationSize)
+			return lerr
+		}); listErr != nil {
+			return false, false, fmt.Errorf("list %s: %w", versionsDir, listErr)
+		}
+		for _, entry := range entries {
+			if entry == nil || entry.Extended == nil {
+				continue
+			}
+			vidBytes, ok := entry.Extended[s3_constants.ExtVersionIdKey]
+			if !ok {
+				continue
+			}
+			vid := string(vidBytes)
+			if vid == versionIdToDelete {
+				continue
+			}
+			if newLatestVersionId == "" || compareVersionIds(vid, newLatestVersionId) < 0 {
+				newLatestEntry = entry
+				newLatestVersionId = vid
+				newLatestVersionFile = entry.Name
+				newLatestIsDeleteMark = string(entry.Extended[s3_constants.ExtDeleteMarkerKey]) == "true"
+			}
+		}
+		if isLast || len(entries) == 0 {
+			break
+		}
+		startFrom = entries[len(entries)-1].Name
+	}
+
+	// Re-fetch the .versions/ entry so the CAS persist below sees the
+	// most recent Extended state. Distinguish NotFound (directory
+	// already gone — vacuous consistency, skip the post-step) from
+	// transient errors (must surface so the caller aborts before the
+	// blob rm — otherwise we'd remove the blob without having updated
+	// the pointer, reproducing the very dangling-state this PR fixes).
+	var versionsEntry *filer_pb.Entry
+	if getErr := retryFilerOp("repointLatestBeforeDeletion.getEntry", func() error {
+		var gerr error
+		versionsEntry, gerr = s3a.getEntry(bucketDir, versionsObjectPath)
+		return gerr
+	}); getErr != nil {
+		if errors.Is(getErr, filer_pb.ErrNotFound) || status.Code(getErr) == codes.NotFound {
+			// Directory already gone. Pointer is vacuously consistent.
+			return true, false, nil
+		}
+		return false, false, fmt.Errorf("read .versions entry: %w", getErr)
+	}
+	if versionsEntry.Extended == nil {
+		return false, false, nil
+	}
+	currentLatestIdBytes, hasCurrent := versionsEntry.Extended[s3_constants.ExtLatestVersionIdKey]
+	if !hasCurrent || string(currentLatestIdBytes) != versionIdToDelete {
+		// A concurrent writer already moved the pointer; our delete is
+		// no longer the latest. Caller falls back to the existing path.
+		return false, false, nil
+	}
+
+	if newLatestEntry != nil {
+		// Multi-version: repoint to the prior latest.
+		versionsEntry.Extended[s3_constants.ExtLatestVersionIdKey] = []byte(newLatestVersionId)
+		versionsEntry.Extended[s3_constants.ExtLatestVersionFileNameKey] = []byte(newLatestVersionFile)
+		setCachedListMetadata(versionsEntry, newLatestEntry)
+		glog.V(2).Infof("repointLatestBeforeDeletion: %s/%s pre-roll to %s (deleteMarker=%v) before deleting %s", bucket, normalizedObject, newLatestVersionId, newLatestIsDeleteMark, versionIdToDelete)
+	} else {
+		// Singleton: clear the pointer fields. The blob rm and the
+		// post-rm .versions/ teardown follow in the caller; if those
+		// fail halfway through, the pointer is already absent so
+		// reads fall through to a clean NoSuchKey without entering the
+		// 10-retry stale-pointer self-heal path.
+		delete(versionsEntry.Extended, s3_constants.ExtLatestVersionIdKey)
+		delete(versionsEntry.Extended, s3_constants.ExtLatestVersionFileNameKey)
+		clearCachedVersionMetadata(versionsEntry.Extended)
+		glog.V(2).Infof("repointLatestBeforeDeletion: %s/%s clearing singleton pointer before deleting %s", bucket, normalizedObject, versionIdToDelete)
+	}
+
+	if mkErr := retryFilerOp("repointLatestBeforeDeletion.mkFile", func() error {
+		return s3a.mkFile(bucketDir, versionsObjectPath, versionsEntry.Chunks, func(updatedEntry *filer_pb.Entry) {
+			updatedEntry.Extended = versionsEntry.Extended
+			updatedEntry.Attributes = versionsEntry.Attributes
+			updatedEntry.Chunks = versionsEntry.Chunks
+		})
+	}); mkErr != nil {
+		return false, false, fmt.Errorf("persist repointed pointer: %w", mkErr)
+	}
+
+	return true, newLatestEntry == nil, nil
+}
+
+// retryAttempts and retryStep tune the bounded retries used when the
+// load-bearing filer ops in updateLatestVersionAfterDeletion fail with
+// transient errors. Doubled per attempt, capped at retryCap. Total
+// worst-case wall time ≈ 6.3s before propagating.
+const (
+	updateLatestRetryAttempts = 6
+	updateLatestRetryStep     = 100 * time.Millisecond
+	updateLatestRetryCap      = 2 * time.Second
+)
+
+// isRetryableFilerErr reports whether err is worth retrying through
+// retryFilerOp. Terminal conditions return false so the caller surfaces
+// them immediately without the backoff delay or the retry-budget
+// wrapper:
+//
+//   - NotFound: the entry genuinely doesn't exist. Retrying won't make
+//     it appear, and callers (e.g. repointLatestBeforeDeletion) want
+//     to act on this directly.
+//   - context.Canceled / DeadlineExceeded: the request was aborted by
+//     the client or hit a deadline. Continuing to retry just delays
+//     the failure return.
+//
+// Everything else (gRPC Unavailable, transient network errors, filer
+// overload signals, etc.) is treated as retryable.
+func isRetryableFilerErr(err error) bool {
+	if err == nil {
+		return false
+	}
+	if errors.Is(err, filer_pb.ErrNotFound) || status.Code(err) == codes.NotFound {
+		return false
+	}
+	if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) {
+		return false
+	}
+	return true
+}
+
+func retryFilerOp(name string, fn func() error) error {
+	var lastErr error
+	backoff := updateLatestRetryStep
+	for attempt := 1; attempt <= updateLatestRetryAttempts; attempt++ {
+		err := fn()
+		if err == nil {
+			if attempt > 1 {
+				glog.V(1).Infof("retryFilerOp: %s succeeded on attempt %d", name, attempt)
+			}
+			return nil
+		}
+		if !isRetryableFilerErr(err) {
+			// Terminal — return raw so callers can errors.Is /
+			// status.Code on the unwrapped error and avoid the
+			// retry-budget delay.
+			return err
+		}
+		lastErr = err
+		if attempt == updateLatestRetryAttempts {
+			break
+		}
+		time.Sleep(backoff)
+		backoff *= 2
+		if backoff > updateLatestRetryCap {
+			backoff = updateLatestRetryCap
+		}
+	}
+	return fmt.Errorf("%s exhausted %d retries: %w", name, updateLatestRetryAttempts, lastErr)
+}
+
 // updateLatestVersionAfterDeletion finds the new latest version after deleting
 // the current latest. The pointer may refer to a delete marker: if a delete
 // marker is chronologically newer than the most recent remaining content
 // version, S3 semantics treat the object as deleted and the pointer must
 // reflect that. Restricting the scan to content versions here would resurrect
 // the object by promoting an older content version over a newer delete marker.
+//
+// All load-bearing filer interactions (list, getEntry, mkFile, rm) are
+// retried with bounded backoff. The function now returns a non-nil error
+// when those retries are exhausted; the caller is expected to surface the
+// failure (log + enqueue for the reconciler) instead of swallowing it,
+// which was the historic behaviour that left dangling pointers in place.
 func (s3a *S3ApiServer) updateLatestVersionAfterDeletion(bucket, object string) error {
 	bucketDir := s3a.bucketDir(bucket)
 	versionsObjectPath := object + s3_constants.VersionsFolder
@@ -1097,10 +1350,17 @@ func (s3a *S3ApiServer) updateLatestVersionAfterDeletion(bucket, object string)
 		startFrom             string
 	)
 	for {
-		entries, isLast, err := s3a.list(versionsDir, "", startFrom, false, filer.PaginationSize)
-		if err != nil {
+		var (
+			entries []*filer_pb.Entry
+			isLast  bool
+		)
+		if err := retryFilerOp("updateLatestVersionAfterDeletion.list", func() error {
+			var listErr error
+			entries, isLast, listErr = s3a.list(versionsDir, "", startFrom, false, filer.PaginationSize)
+			return listErr
+		}); err != nil {
 			glog.Errorf("updateLatestVersionAfterDeletion: failed to list versions in %s: %v", versionsDir, err)
-			return fmt.Errorf("failed to list versions: %v", err)
+			return fmt.Errorf("failed to list versions: %w", err)
 		}
 		totalEntries += len(entries)
 		if pageEntry, pageId, pageFile, pageDM := selectLatestVersion(entries); pageEntry != nil {
@@ -1120,9 +1380,13 @@ func (s3a *S3ApiServer) updateLatestVersionAfterDeletion(bucket, object string)
 	glog.V(1).Infof("updateLatestVersionAfterDeletion: scanned %d entries in %s", totalEntries, versionsDir)
 
 	// Update the .versions directory metadata
-	versionsEntry, err := s3a.getEntry(bucketDir, versionsObjectPath)
-	if err != nil {
-		return fmt.Errorf("failed to get .versions directory: %v", err)
+	var versionsEntry *filer_pb.Entry
+	if err := retryFilerOp("updateLatestVersionAfterDeletion.getEntry", func() error {
+		var getErr error
+		versionsEntry, getErr = s3a.getEntry(bucketDir, versionsObjectPath)
+		return getErr
+	}); err != nil {
+		return fmt.Errorf("failed to get .versions directory: %w", err)
 	}
 
 	if versionsEntry.Extended == nil {
@@ -1137,13 +1401,14 @@ func (s3a *S3ApiServer) updateLatestVersionAfterDeletion(bucket, object string)
 		setCachedListMetadata(versionsEntry, latestVersionEntry)
 
 		glog.V(2).Infof("updateLatestVersionAfterDeletion: new latest version for %s/%s is %s (deleteMarker=%v)", bucket, object, latestVersionId, latestIsDeleteMarker)
-		err = s3a.mkFile(bucketDir, versionsObjectPath, versionsEntry.Chunks, func(updatedEntry *filer_pb.Entry) {
-			updatedEntry.Extended = versionsEntry.Extended
-			updatedEntry.Attributes = versionsEntry.Attributes
-			updatedEntry.Chunks = versionsEntry.Chunks
-		})
-		if err != nil {
-			return fmt.Errorf("failed to update .versions directory metadata: %v", err)
+		if err := retryFilerOp("updateLatestVersionAfterDeletion.mkFile", func() error {
+			return s3a.mkFile(bucketDir, versionsObjectPath, versionsEntry.Chunks, func(updatedEntry *filer_pb.Entry) {
+				updatedEntry.Extended = versionsEntry.Extended
+				updatedEntry.Attributes = versionsEntry.Attributes
+				updatedEntry.Chunks = versionsEntry.Chunks
+			})
+		}); err != nil {
+			return fmt.Errorf("failed to update .versions directory metadata: %w", err)
 		}
 	} else {
 		// No version-tagged entries remain - try to delete the .versions
@@ -1161,12 +1426,36 @@ func (s3a *S3ApiServer) updateLatestVersionAfterDeletion(bucket, object string)
 		// object is correctly absent.
 		glog.V(2).Infof("updateLatestVersionAfterDeletion: no versions left for %s/%s, deleting .versions directory", bucket, object)
 
-		err = s3a.rm(bucketDir, versionsObjectPath, true, false)
-		if err == nil {
+		rmErr := s3a.rm(bucketDir, versionsObjectPath, true, false)
+		if rmErr == nil {
 			return nil
 		}
-		glog.Warningf("updateLatestVersionAfterDeletion: failed to delete .versions directory for %s/%s: %v", bucket, object, err)
+		// Two ways rm can fail here: "non-empty folder" (orphan entries
+		// blocking the teardown — fall through to pointer clear) and a
+		// transient filer error (worth retrying). Distinguish by the
+		// canonical error substring; if we can't tell, treat as transient.
+		if strings.Contains(rmErr.Error(), filer.MsgFailDelNonEmptyFolder) {
+			glog.V(2).Infof("updateLatestVersionAfterDeletion: .versions/ for %s/%s still has orphan entries: %v", bucket, object, rmErr)
+			s3a.clearStaleLatestVersionPointer(bucket, object, bucketDir, versionsObjectPath, versionsEntry, "updateLatestVersionAfterDeletion")
+			return nil
+		}
+		// Transient — retry the rm a few times before giving up. Even
+		// if it ultimately fails, we still clear the stale pointer so
+		// readers get a clean miss; the directory can be tidied by the
+		// reconciler later.
+		retryErr := retryFilerOp("updateLatestVersionAfterDeletion.rm", func() error {
+			return s3a.rm(bucketDir, versionsObjectPath, true, false)
+		})
+		if retryErr == nil {
+			return nil
+		}
+		if strings.Contains(retryErr.Error(), filer.MsgFailDelNonEmptyFolder) {
+			s3a.clearStaleLatestVersionPointer(bucket, object, bucketDir, versionsObjectPath, versionsEntry, "updateLatestVersionAfterDeletion")
+			return nil
+		}
+		glog.Warningf("updateLatestVersionAfterDeletion: failed to delete .versions directory for %s/%s after retries: %v", bucket, object, retryErr)
 		s3a.clearStaleLatestVersionPointer(bucket, object, bucketDir, versionsObjectPath, versionsEntry, "updateLatestVersionAfterDeletion")
+		return fmt.Errorf("delete .versions directory: %w", retryErr)
 	}
 
 	return nil