fix: ListObjectVersions interleave Version and DeleteMarker in sort order (#8567)

* fix: ListObjectVersions interleave Version and DeleteMarker in sort order

Go's default xml.Marshal serializes struct fields in definition order,
causing all <Version> elements to appear before all <DeleteMarker>
elements. The S3 API contract requires these elements to be interleaved
in the correct global sort order (by key ascending, then newest version
first within each key).

This broke clients that validate version list ordering within a single
key — an older Version would appear before a newer DeleteMarker for the
same object.

Fix: Replace the separate Versions/DeleteMarkers/CommonPrefixes arrays
with a single Entries []VersionListEntry slice. Each VersionListEntry
uses a per-element MarshalXML that outputs the correct XML tag name
(<Version>, <DeleteMarker>, or <CommonPrefixes>) based on which field
is populated. Since the entries are already in their correct sorted
order from buildSortedCombinedList, the XML output is automatically
interleaved correctly.

Also removes the unused ListObjectVersionsResult struct.

Note: The reporter also mentioned a cross-key timestamp ordering issue
when paginating with max-keys=1, but that is correct S3 behavior —
ListObjectVersions sorts by key name (ascending), not by timestamp.
Different keys having non-monotonic timestamps is expected.

* test: add CommonPrefixes XML marshaling coverage for ListObjectVersions

* fix: validate VersionListEntry has exactly one field set in MarshalXML

Return an error instead of silently emitting an empty <Version> element
when no field (or multiple fields) are populated. Also clean up the
misleading xml:"Version" struct tag on the Entries field.

weed/s3api/s3api_object_handlers_list_versioned_test.go

@@ -3,6 +3,7 @@ package s3api
 import (
 	"context"
 	"encoding/hex"
+	"encoding/xml"
 	"fmt"
 	"strconv"
 	"strings"
@@ -432,6 +433,120 @@ func (c *customTestFilerClient) ListEntries(ctx context.Context, in *filer_pb.Li
 	return c.testFilerClient.ListEntries(ctx, in, opts...)
 }
 
+// TestListObjectVersionsResult_XMLInterleaving validates that Version and DeleteMarker elements
+// are interleaved in the correct sort order in the XML output, matching the S3 API contract.
+func TestListObjectVersionsResult_XMLInterleaving(t *testing.T) {
+	now := time.Now()
+	earlier := now.Add(-24 * time.Hour)
+
+	dm := &DeleteMarkerEntry{
+		Key:          "object1.txt",
+		VersionId:    "newer-dm",
+		IsLatest:     true,
+		LastModified: now,
+	}
+	ver := &VersionEntry{
+		Key:          "object1.txt",
+		VersionId:    "older-version",
+		IsLatest:     false,
+		LastModified: earlier,
+		ETag:         "\"abc\"",
+		Size:         100,
+		StorageClass: "STANDARD",
+	}
+
+	// Entries are in correct sort order: newest first for same key
+	result := &S3ListObjectVersionsResult{
+		Name:    "test-bucket",
+		MaxKeys: 1000,
+		Entries: []VersionListEntry{
+			{DeleteMarker: dm},
+			{Version: ver},
+		},
+	}
+
+	xmlBytes, err := xml.Marshal(result)
+	assert.NoError(t, err)
+	xmlStr := string(xmlBytes)
+
+	// Verify DeleteMarker appears BEFORE Version in the XML output
+	dmIdx := strings.Index(xmlStr, "<DeleteMarker>")
+	vIdx := strings.Index(xmlStr, "<Version>")
+	assert.True(t, dmIdx >= 0, "DeleteMarker element should be present")
+	assert.True(t, vIdx >= 0, "Version element should be present")
+	assert.True(t, dmIdx < vIdx, "DeleteMarker should appear before Version for same key (newest first), got DM at %d, V at %d", dmIdx, vIdx)
+}
+
+// TestListObjectVersionsResult_XMLInterleavingMultipleKeys validates interleaving across multiple keys
+func TestListObjectVersionsResult_XMLInterleavingMultipleKeys(t *testing.T) {
+	now := time.Now()
+	earlier := now.Add(-24 * time.Hour)
+
+	dm := &DeleteMarkerEntry{Key: "aaa", VersionId: "dm1", IsLatest: true, LastModified: now}
+	v1 := &VersionEntry{Key: "aaa", VersionId: "v1", IsLatest: false, LastModified: earlier, StorageClass: "STANDARD"}
+	v2 := &VersionEntry{Key: "bbb", VersionId: "v2", IsLatest: true, LastModified: now, StorageClass: "STANDARD"}
+
+	// Expected XML order: aaa/DM, aaa/V, bbb/V
+	result := &S3ListObjectVersionsResult{
+		Name:    "test-bucket",
+		MaxKeys: 1000,
+		Entries: []VersionListEntry{
+			{DeleteMarker: dm},
+			{Version: v1},
+			{Version: v2},
+		},
+	}
+
+	xmlBytes, err := xml.Marshal(result)
+	assert.NoError(t, err)
+	xmlStr := string(xmlBytes)
+
+	// Find positions of each element
+	dmIdx := strings.Index(xmlStr, "<DeleteMarker>")
+	v1Idx := strings.Index(xmlStr, "<Version><Key>aaa</Key>")
+	v2Idx := strings.Index(xmlStr, "<Version><Key>bbb</Key>")
+
+	assert.True(t, dmIdx >= 0, "DeleteMarker should be present")
+	assert.True(t, v1Idx >= 0, "Version for aaa should be present")
+	assert.True(t, v2Idx >= 0, "Version for bbb should be present")
+	assert.True(t, dmIdx < v1Idx, "aaa DeleteMarker should appear before aaa Version")
+	assert.True(t, v1Idx < v2Idx, "aaa Version should appear before bbb Version")
+}
+
+// TestListObjectVersionsResult_XMLCommonPrefixes validates that CommonPrefixes entries
+// are correctly marshaled when interleaved with Version and DeleteMarker entries.
+func TestListObjectVersionsResult_XMLCommonPrefixes(t *testing.T) {
+	now := time.Now()
+
+	ver := &VersionEntry{Key: "docs/readme.txt", VersionId: "v1", IsLatest: true, LastModified: now, StorageClass: "STANDARD"}
+
+	result := &S3ListObjectVersionsResult{
+		Name:      "test-bucket",
+		MaxKeys:   1000,
+		Delimiter: "/",
+		Entries: []VersionListEntry{
+			{Version: ver},
+			{Prefix: &PrefixEntry{Prefix: "images/"}},
+			{Prefix: &PrefixEntry{Prefix: "logs/"}},
+		},
+	}
+
+	xmlBytes, err := xml.Marshal(result)
+	assert.NoError(t, err)
+	xmlStr := string(xmlBytes)
+
+	// Verify CommonPrefixes elements are present with correct content
+	assert.Contains(t, xmlStr, "<CommonPrefixes><Prefix>images/</Prefix></CommonPrefixes>")
+	assert.Contains(t, xmlStr, "<CommonPrefixes><Prefix>logs/</Prefix></CommonPrefixes>")
+
+	// Verify ordering: Version before CommonPrefixes (as placed in Entries)
+	vIdx := strings.Index(xmlStr, "<Version>")
+	cpIdx := strings.Index(xmlStr, "<CommonPrefixes>")
+	assert.True(t, vIdx >= 0, "Version element should be present")
+	assert.True(t, cpIdx >= 0, "CommonPrefixes element should be present")
+	assert.True(t, vIdx < cpIdx, "Version should appear before CommonPrefixes as ordered in Entries")
+}
+
 // TestListObjectVersions_PrefixWithLeadingSlash tests that prefixes with leading slashes work correctly
 // This validates the fix for the bug where "/Veeam/Archive/" would fail to match relative paths
 func TestListObjectVersions_PrefixWithLeadingSlash(t *testing.T) {

weed/s3api/s3api_object_versioning.go

@@ -82,9 +82,14 @@ func clearCachedListMetadata(extended map[string][]byte) {
 	clearCachedVersionMetadata(extended)
 }
 
-// S3ListObjectVersionsResult - Custom struct for S3 list-object-versions response
-// This avoids conflicts with the XSD generated ListVersionsResult struct
-// and ensures proper separation of versions and delete markers into arrays
+// S3ListObjectVersionsResult - Custom struct for S3 list-object-versions response.
+// This avoids conflicts with the XSD generated ListVersionsResult struct.
+//
+// The Entries slice holds Version, DeleteMarker, and CommonPrefix items in their
+// correct interleaved sort order (by key ascending, then newest version first).
+// Each entry uses a per-element MarshalXML to output the correct XML element name.
+// This ensures the XML output matches the S3 API contract where these elements
+// are interleaved in sort order, not grouped by type.
 type S3ListObjectVersionsResult struct {
 	XMLName xml.Name `xml:"http://s3.amazonaws.com/doc/2006-03-01/ ListVersionsResult"`
 
@@ -98,29 +103,53 @@ type S3ListObjectVersionsResult struct {
 	Delimiter           string `xml:"Delimiter,omitempty"`
 	IsTruncated         bool   `xml:"IsTruncated"`
 
-	// These are the critical fields - arrays instead of single elements
-	Versions      []VersionEntry      `xml:"Version,omitempty"`      // Array for versions
-	DeleteMarkers []DeleteMarkerEntry `xml:"DeleteMarker,omitempty"` // Array for delete markers
+	// Entries holds all versions, delete markers, and common prefixes in their
+	// correct interleaved sort order. Each entry's MarshalXML outputs the correct
+	// XML element name (<Version>, <DeleteMarker>, or <CommonPrefixes>).
+	// MarshalXML on each entry overrides the element name to <Version>,
+	// <DeleteMarker>, or <CommonPrefixes> as appropriate.
+	Entries []VersionListEntry `xml:",omitempty"`
+
+	EncodingType string `xml:"EncodingType,omitempty"`
+}
 
-	CommonPrefixes []PrefixEntry `xml:"CommonPrefixes,omitempty"`
-	EncodingType   string        `xml:"EncodingType,omitempty"`
+// VersionListEntry represents a single item in the ListObjectVersions response.
+// It wraps either a VersionEntry, DeleteMarkerEntry, or PrefixEntry and outputs
+// the correct XML element name via custom MarshalXML.
+type VersionListEntry struct {
+	Version      *VersionEntry
+	DeleteMarker *DeleteMarkerEntry
+	Prefix       *PrefixEntry
 }
 
-// Original struct - keeping for compatibility but will use S3ListObjectVersionsResult for XML response
-type ListObjectVersionsResult struct {
-	XMLName             xml.Name            `xml:"http://s3.amazonaws.com/doc/2006-03-01/ ListVersionsResult"`
-	Name                string              `xml:"Name"`
-	Prefix              string              `xml:"Prefix"`
-	KeyMarker           string              `xml:"KeyMarker,omitempty"`
-	VersionIdMarker     string              `xml:"VersionIdMarker,omitempty"`
-	NextKeyMarker       string              `xml:"NextKeyMarker,omitempty"`
-	NextVersionIdMarker string              `xml:"NextVersionIdMarker,omitempty"`
-	MaxKeys             int                 `xml:"MaxKeys"`
-	Delimiter           string              `xml:"Delimiter,omitempty"`
-	IsTruncated         bool                `xml:"IsTruncated"`
-	Versions            []VersionEntry      `xml:"Version,omitempty"`
-	DeleteMarkers       []DeleteMarkerEntry `xml:"DeleteMarker,omitempty"`
-	CommonPrefixes      []PrefixEntry       `xml:"CommonPrefixes,omitempty"`
+// MarshalXML outputs the entry as <Version>, <DeleteMarker>, or <CommonPrefixes>
+// depending on which field is populated. Exactly one field must be set.
+func (e VersionListEntry) MarshalXML(enc *xml.Encoder, start xml.StartElement) error {
+	var (
+		value any
+		name  string
+		count int
+	)
+	if e.DeleteMarker != nil {
+		value = e.DeleteMarker
+		name = "DeleteMarker"
+		count++
+	}
+	if e.Prefix != nil {
+		value = e.Prefix
+		name = "CommonPrefixes"
+		count++
+	}
+	if e.Version != nil {
+		value = e.Version
+		name = "Version"
+		count++
+	}
+	if count != 1 {
+		return fmt.Errorf("VersionListEntry must have exactly one of DeleteMarker, Prefix, or Version set (got %d)", count)
+	}
+	start.Name.Local = name
+	return enc.EncodeElement(value, start)
 }
 
 // ObjectVersion represents a version of an S3 object
@@ -260,7 +289,7 @@ func (s3a *S3ApiServer) listObjectVersions(bucket, prefix, keyMarker, versionIdM
 
 	// Build the final response by splitting items back into their respective fields
 	result := s3a.splitIntoResult(truncatedList, bucket, prefix, keyMarker, versionIdMarker, delimiter, maxKeys, isTruncated, nextKeyMarker, nextVersionIdMarker)
-	glog.V(1).Infof("listObjectVersions: final result - %d versions, %d delete markers, %d common prefixes", len(result.Versions), len(result.DeleteMarkers), len(result.CommonPrefixes))
+	glog.V(1).Infof("listObjectVersions: final result - %d entries", len(result.Entries))
 
 	return result, nil
 }
@@ -329,7 +358,8 @@ func (s3a *S3ApiServer) truncateAndSetMarkers(combinedList []versionListItem, ma
 	return combinedList, nextKeyMarker, nextVersionIdMarker, isTruncated
 }
 
-// splitIntoResult builds the final S3ListObjectVersionsResult from the combined list
+// splitIntoResult builds the final S3ListObjectVersionsResult from the combined list.
+// It populates a single Entries slice that preserves the interleaved sort order.
 func (s3a *S3ApiServer) splitIntoResult(combinedList []versionListItem, bucket, prefix, keyMarker, versionIdMarker, delimiter string, maxKeys int, isTruncated bool, nextKeyMarker, nextVersionIdMarker string) *S3ListObjectVersionsResult {
 	result := &S3ListObjectVersionsResult{
 		Name:                bucket,
@@ -341,20 +371,20 @@ func (s3a *S3ApiServer) splitIntoResult(combinedList []versionListItem, bucket,
 		IsTruncated:         isTruncated,
 		NextKeyMarker:       nextKeyMarker,
 		NextVersionIdMarker: nextVersionIdMarker,
-		Versions:            make([]VersionEntry, 0),
-		DeleteMarkers:       make([]DeleteMarkerEntry, 0),
-		CommonPrefixes:      make([]PrefixEntry, 0),
+		Entries:             make([]VersionListEntry, 0, len(combinedList)),
 	}
 
 	for _, item := range combinedList {
 		if item.isPrefix {
-			result.CommonPrefixes = append(result.CommonPrefixes, PrefixEntry{Prefix: item.key})
+			result.Entries = append(result.Entries, VersionListEntry{
+				Prefix: &PrefixEntry{Prefix: item.key},
+			})
 		} else {
 			switch v := item.versionData.(type) {
 			case *VersionEntry:
-				result.Versions = append(result.Versions, *v)
+				result.Entries = append(result.Entries, VersionListEntry{Version: v})
 			case *DeleteMarkerEntry:
-				result.DeleteMarkers = append(result.DeleteMarkers, *v)
+				result.Entries = append(result.Entries, VersionListEntry{DeleteMarker: v})
 			}
 		}
 	}