fix(s3): recover versioned reads when the .versions latest pointer is absent (#9782)

GetObject on a versioned object returned NoSuchKey forever when the .versions directory existed but carried no latest-version pointer (empty Extended metadata) while real version files remained inside it. The self-heal path only fired for a dangling pointer (present but referencing a missing file), not an absent one, so doGetLatestObjectVersion fell straight through and errored on every read. - doGetLatestObjectVersion now calls recoverLatestVersionWithoutPointer when the pointer is missing or empty. An absent pointer is the legitimate signal that a pre-versioning or suspended-versioning "null" object is current, so that object wins; only when it is absent do we rescan .versions/ and rebuild the pointer from the version files present. Transient rescan failures propagate instead of being masked as NotFound. - selectLatestVersion derives the version id from the v_<versionId> file name when the Seaweed-X-Amz-Version-Id attribute is absent, so version files written outside the normal versioned-PUT path (replicated or restored entries) are still promotable. The orphan diagnostic uses the same detection so an entry can't be both promoted and counted an orphan.
2026-08-19 21:56:54 +00:00 · 2026-06-01 20:01:30 -07:00
parent ca81c0c525
commit bf37fba0e1
2 changed files with 155 additions and 40 deletions
@@ -1394,18 +1394,14 @@ func (s3a *S3ApiServer) updateLatestVersionAfterDeletion(ctx context.Context, bu
 				latestIsDeleteMarker = pageDM
 			}
 		}
-		// Sample orphan entries (those without ExtVersionIdKey) for
-		// post-scan diagnostics. selectLatestVersion already filters them
-		// out for the latest-pick; we collect names separately so the
-		// anomaly warning has something concrete to point at.
+		// Count entries with no derivable version id as orphans for diagnostics,
+		// using the same detection as selectLatestVersion.
 		for _, e := range entries {
 			if e == nil {
 				continue
 			}
-			if e.Extended != nil {
-				if _, ok := e.Extended[s3_constants.ExtVersionIdKey]; ok {
-					continue
-				}
+			if versionIdFromEntry(e) != "" {
+				continue
 			}
 			orphanCount++
 			if len(orphanSamples) < orphanSampleCap {
@@ -1719,35 +1715,22 @@ func (s3a *S3ApiServer) doGetLatestObjectVersion(bucket, object string, maxRetri
 			}
 		}

-		// If still no metadata after retries, fall back to pre-versioning object
+		// No pointer after retries — the null object (pre/suspended versioning)
+		// wins if present; otherwise rescan in case the pointer was lost.
 		if versionsEntry.Extended == nil {
-			glog.V(2).Infof("getLatestObjectVersion: no Extended metadata in .versions directory for %s/%s after retries, checking for pre-versioning object", bucket, object)
-
-			regularEntry, regularErr := s3a.getEntry(bucketDir, normalizedObject)
-			if regularErr != nil {
-				return nil, fmt.Errorf("no version metadata in .versions directory and no regular object found for %s/%s", bucket, normalizedObject)
-			}
-
-			glog.V(2).Infof("getLatestObjectVersion: found pre-versioning object for %s/%s (no Extended metadata case)", bucket, object)
-			return regularEntry, nil
+			glog.V(2).Infof("getLatestObjectVersion: no Extended metadata in .versions directory for %s/%s after retries, attempting rescan", bucket, object)
+			return s3a.recoverLatestVersionWithoutPointer(bucket, normalizedObject, versionsEntry)
 		}
 	}

 	latestVersionIdBytes, hasLatestVersionId := versionsEntry.Extended[s3_constants.ExtLatestVersionIdKey]
 	latestVersionFileBytes, hasLatestVersionFile := versionsEntry.Extended[s3_constants.ExtLatestVersionFileNameKey]

-	if !hasLatestVersionId || !hasLatestVersionFile {
-		// No version metadata means all versioned objects have been deleted.
-		// Fall back to checking for a pre-versioning object.
-		glog.V(2).Infof("getLatestObjectVersion: no version metadata in .versions directory for %s/%s, checking for pre-versioning object", bucket, object)
-
-		regularEntry, regularErr := s3a.getEntry(bucketDir, normalizedObject)
-		if regularErr != nil {
-			return nil, fmt.Errorf("no version metadata in .versions directory and no regular object found for %s/%s", bucket, normalizedObject)
-		}
-
-		glog.V(2).Infof("getLatestObjectVersion: found pre-versioning object for %s/%s after version deletion", bucket, object)
-		return regularEntry, nil
+	if !hasLatestVersionId || len(latestVersionIdBytes) == 0 || !hasLatestVersionFile || len(latestVersionFileBytes) == 0 {
+		// No usable pointer (suspended/all-deleted, or pointer lost/empty). The
+		// null object wins if present; otherwise rescan in case version files remain.
+		glog.V(2).Infof("getLatestObjectVersion: no usable latest-version pointer for %s/%s, recovering", bucket, object)
+		return s3a.recoverLatestVersionWithoutPointer(bucket, normalizedObject, versionsEntry)
 	}

 	latestVersionId := string(latestVersionIdBytes)
@@ -1775,10 +1758,54 @@ func (s3a *S3ApiServer) doGetLatestObjectVersion(bucket, object string, maxRetri
 	return latestVersionEntry, nil
 }

+// recoverLatestVersionWithoutPointer handles a .versions directory that exists
+// but has no usable latest-version pointer. An absent pointer is the legitimate
+// signal that a pre-versioning or suspended-versioning "null" object at the
+// regular path is current, so that object wins; only when it is absent do we
+// rescan .versions/ to rebuild a pointer lost while real version files remain.
+func (s3a *S3ApiServer) recoverLatestVersionWithoutPointer(bucket, normalizedObject string, versionsEntry *filer_pb.Entry) (*filer_pb.Entry, error) {
+	bucketDir := s3a.bucketDir(bucket)
+
+	if regularEntry, regularErr := s3a.getEntry(bucketDir, normalizedObject); regularErr == nil {
+		return regularEntry, nil
+	}
+
+	// No null object — the pointer may have been lost while version files
+	// remain. Rescan to rebuild it, propagating transient rescan failures
+	// instead of masking them as a not-found miss.
+	healed, healErr := s3a.healStaleLatestVersionPointer(bucket, normalizedObject, versionsEntry, "")
+	if healErr == nil {
+		return healed, nil
+	}
+	if !errors.Is(healErr, filer_pb.ErrNotFound) && status.Code(healErr) != codes.NotFound {
+		return nil, healErr
+	}
+
+	return nil, fmt.Errorf("no version metadata in .versions directory and no regular object found for %s/%s", bucket, normalizedObject)
+}
+
+// versionIdFromEntry returns a .versions child entry's version id, preferring
+// the Seaweed-X-Amz-Version-Id attribute and falling back to the v_<versionId>
+// file name so entries written outside the normal PUT path still self-heal.
+func versionIdFromEntry(entry *filer_pb.Entry) string {
+	if entry == nil || entry.IsDirectory {
+		return ""
+	}
+	if entry.Extended != nil {
+		if versionIdBytes, ok := entry.Extended[s3_constants.ExtVersionIdKey]; ok && len(versionIdBytes) > 0 {
+			return string(versionIdBytes)
+		}
+	}
+	if versionId, ok := strings.CutPrefix(entry.Name, "v_"); ok {
+		return versionId
+	}
+	return ""
+}
+
 // selectLatestVersion returns the chronologically newest entry with a version
 // id, including delete markers. isDeleteMarker reflects whether the selected
 // entry is a delete marker. Returns nil for latestEntry when the directory
-// contains no version-id-tagged entries.
+// contains no version entries (see versionIdFromEntry for what qualifies).
 //
 // This is the correct selector for the self-heal path: the .versions pointer
 // tracks the current-version-regardless-of-type (see createDeleteMarker), and
@@ -1786,22 +1813,17 @@ func (s3a *S3ApiServer) doGetLatestObjectVersion(bucket, object string, maxRetri
 // ExtDeleteMarkerKey on the returned entry and respond with NoSuchKey.
 func selectLatestVersion(entries []*filer_pb.Entry) (latestEntry *filer_pb.Entry, latestVersionId, latestVersionFileName string, isDeleteMarker bool) {
 	for _, entry := range entries {
-		if entry == nil || entry.Extended == nil {
+		versionId := versionIdFromEntry(entry)
+		if versionId == "" {
 			continue
 		}

-		versionIdBytes, hasVersionId := entry.Extended[s3_constants.ExtVersionIdKey]
-		if !hasVersionId {
-			continue
-		}
-
-		versionId := string(versionIdBytes)
 		// compareVersionIds returns negative when the first arg is newer
 		if latestVersionId == "" || compareVersionIds(versionId, latestVersionId) < 0 {
 			latestVersionId = versionId
 			latestVersionFileName = entry.Name
 			latestEntry = entry
-			isDeleteMarker = string(entry.Extended[s3_constants.ExtDeleteMarkerKey]) == "true"
+			isDeleteMarker = entry.Extended != nil && string(entry.Extended[s3_constants.ExtDeleteMarkerKey]) == "true"
 		}
 	}
 	return
@@ -109,7 +109,8 @@ func TestSelectLatestVersion_OnlyDeleteMarkers(t *testing.T) {
 }

 // TestSelectLatestVersion_EmptyOrUntagged verifies nil latestEntry when there
-// is no version-id-tagged entry at all.
+// is no version entry at all. Names that are neither tagged with a version id
+// nor shaped like a v_<versionId> file are ignored.
 func TestSelectLatestVersion_EmptyOrUntagged(t *testing.T) {
 	entries := []*filer_pb.Entry{
 		nil,
@@ -125,3 +126,95 @@ func TestSelectLatestVersion_EmptyOrUntagged(t *testing.T) {
 	assert.Empty(t, latestName)
 	assert.False(t, isDM)
 }
+
+// newUntaggedVersionFile builds a v_<versionId> entry missing the
+// Seaweed-X-Amz-Version-Id attribute, like ones the filename fallback recovers.
+func newUntaggedVersionFile(versionId string) *filer_pb.Entry {
+	return &filer_pb.Entry{
+		Name:       "v_" + versionId,
+		Attributes: &filer_pb.FuseAttributes{},
+		Extended:   map[string][]byte{"X-Amz-Storage-Class": []byte("STANDARD")},
+	}
+}
+
+// TestVersionIdFromEntry covers the attribute-first, filename-fallback contract
+// that lets selectLatestVersion recover version files written outside the
+// normal versioned-PUT path.
+func TestVersionIdFromEntry(t *testing.T) {
+	id := "6775adb0d7b0d2e303e0fced6989bb57"
+
+	// Attribute present: used verbatim.
+	assert.Equal(t, id, versionIdFromEntry(newVersionEntry("v_"+id, id, false)))
+
+	// Attribute absent but named v_<id>: derived from the filename.
+	assert.Equal(t, id, versionIdFromEntry(newUntaggedVersionFile(id)))
+
+	// Empty attribute value falls back to the filename.
+	emptyAttr := &filer_pb.Entry{Name: "v_" + id, Extended: map[string][]byte{s3_constants.ExtVersionIdKey: []byte("")}}
+	assert.Equal(t, id, versionIdFromEntry(emptyAttr))
+
+	// Not a version file and not tagged: no id.
+	assert.Empty(t, versionIdFromEntry(&filer_pb.Entry{Name: "not-a-version", Extended: map[string][]byte{"x": []byte("y")}}))
+
+	// Nil and directory entries are ignored even if named like a version file.
+	assert.Empty(t, versionIdFromEntry(nil))
+	assert.Empty(t, versionIdFromEntry(&filer_pb.Entry{Name: "v_" + id, IsDirectory: true}))
+}
+
+// TestSelectLatestVersion_FilenameFallback verifies untagged version files are
+// still selected by deriving the id from the v_<versionId> filename.
+func TestSelectLatestVersion_FilenameFallback(t *testing.T) {
+	baseTs := int64(1700000000000000000)
+	olderId := createNewFormatVersionId(baseTs)
+	newerId := createNewFormatVersionId(baseTs + int64(time.Minute))
+
+	entries := []*filer_pb.Entry{
+		newUntaggedVersionFile(olderId),
+		newUntaggedVersionFile(newerId),
+	}
+
+	latest, latestId, latestName, isDM := selectLatestVersion(entries)
+
+	assert.NotNil(t, latest, "untagged version files must still be selectable by filename")
+	assert.Equal(t, newerId, latestId, "newest version wins via filename-derived id")
+	assert.Equal(t, "v_"+newerId, latestName)
+	assert.False(t, isDM)
+}
+
+// TestSelectLatestVersion_FilenameFallbackMixedWithTagged ensures filename-only
+// entries compete correctly against properly tagged ones.
+func TestSelectLatestVersion_FilenameFallbackMixedWithTagged(t *testing.T) {
+	baseTs := int64(1700000000000000000)
+	taggedOlderId := createNewFormatVersionId(baseTs)
+	untaggedNewerId := createNewFormatVersionId(baseTs + int64(time.Minute))
+
+	entries := []*filer_pb.Entry{
+		newVersionEntry("v_"+taggedOlderId, taggedOlderId, false),
+		newUntaggedVersionFile(untaggedNewerId),
+	}
+
+	latest, latestId, latestName, isDM := selectLatestVersion(entries)
+
+	assert.NotNil(t, latest)
+	assert.Equal(t, untaggedNewerId, latestId, "newer untagged version must win over older tagged one")
+	assert.Equal(t, "v_"+untaggedNewerId, latestName)
+	assert.False(t, isDM)
+}
+
+// TestSelectLatestVersion_FilenameFallbackDeleteMarker verifies a delete marker
+// whose version-id attribute is missing is still recognized (id from filename)
+// and still reported as a delete marker so the caller renders NoSuchKey.
+func TestSelectLatestVersion_FilenameFallbackDeleteMarker(t *testing.T) {
+	id := createNewFormatVersionId(1700000000000000000)
+	entry := &filer_pb.Entry{
+		Name:       "v_" + id,
+		Attributes: &filer_pb.FuseAttributes{},
+		Extended:   map[string][]byte{s3_constants.ExtDeleteMarkerKey: []byte("true")},
+	}
+
+	latest, latestId, _, isDM := selectLatestVersion([]*filer_pb.Entry{entry})
+
+	assert.NotNil(t, latest)
+	assert.Equal(t, id, latestId)
+	assert.True(t, isDM, "delete marker recognized by filename must still report isDeleteMarker")
+}