fix: treat empty object lock values as absent when comparing

getReplicationAction builds its source map from oi1.UserDefined, where a removed retention is a present key with an empty value, and its target map from the destination's HEAD headers, which can never carry those keys because setObjectHeaders skips empty lock values and FilterObjectLockMetadata drops both keys when the mode is invalid. The comparison then always reports a difference, the replicateNone fast path is dead for such versions, and an otherwise matching version re-copies its metadata on every evaluation. Skip an entry whose value is empty and whose key is x-amz-object-lock-mode or x-amz-object-lock-retain-until-date, case-insensitively, in both comparison loops, using the joined value on the target side. Normalizing only the source would regress the case where both sides hold the empty pair. HEAD also omits a real retention from a credential without s3:GetObjectRetention, which the documented target policy does not grant, so that normalization alone would read a destination hiding a retention as in sync and drop the removal. replicationActionForTarget therefore confirms with the destination before skipping the resend: only an explicit answer, no retention on the version, clears it. Everything else keeps today's metadata resend, including a denied or unreachable destination, a mode the SDK does not recognize, and InvalidRequest, which names a bucket without Object Lock but is also what a destination answers when its own read of that configuration fails. The null version an existing object resync excludes is never reopened. Tests: TestGetReplicationActionEmptyObjectLockValues (eight cases, red on 2 and 3 before this change), TestRetentionRemovedAtSource, TestTargetRetentionConfirmedAbsent, TestReplicationActionForTargetRetentionRemoval, TestReplicationActionForTargetNullVersionResync and TestEmptyRetentionValuesAreOmittedFromObjectResponseHeaders. Compatibility: sender side only, no wire or storage change, so a fixed source converges against any destination version. Fixes pgsty/silo#117 Signed-off-by: Feng Ruohang <rh@vonng.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L7qJqWwy8oFA6aCXWRzXQe
2026-09-05 18:16:16 +03:00 · 2026-09-05 14:52:56 +08:00
parent f0bd164b92
commit 05ab4ca475
2 changed files with 407 additions and 5 deletions
@@ -931,11 +931,17 @@ func equals(k1 string, keys ...string) bool {
 	return false
 }

+// nullVersionExcludedFromResync reports the exclusion at the head of getReplicationAction, kept
+// verbatim from upstream: an existing object resync leaves a null version alone when the source
+// modification time is later than the one the target reports, without comparing anything else.
+func nullVersionExcludedFromResync(oi1 ObjectInfo, oi2 minio.ObjectInfo, opType replication.Type) bool {
+	return opType == replication.ExistingObjectReplicationType &&
+		oi1.ModTime.Unix() > oi2.LastModified.Unix() && oi1.VersionID == nullVersionID
+}
+
 // returns replicationAction by comparing metadata between source and target
 func getReplicationAction(oi1 ObjectInfo, oi2 minio.ObjectInfo, opType replication.Type) replicationAction {
-	// Avoid resyncing null versions created prior to enabling replication if target has a newer copy
-	if opType == replication.ExistingObjectReplicationType &&
-		oi1.ModTime.Unix() > oi2.LastModified.Unix() && oi1.VersionID == nullVersionID {
+	if nullVersionExcludedFromResync(oi1, oi2, opType) {
 		return replicateNone
 	}
 	sz, _ := oi1.GetActualSize()
@@ -986,9 +992,21 @@ func getReplicationAction(oi1 ObjectInfo, oi2 minio.ObjectInfo, opType replicati
 		"X-Amz-Meta-",
 	}

+	// An empty object lock mode or retain-until-date records a removed retention, but
+	// it is omitted from GET/HEAD response headers: setObjectHeaders() skips both keys
+	// when the value is empty, and FilterObjectLockMetadata() drops them when the mode
+	// is not valid. The target can therefore never report them, so treat empty and
+	// absent as equal rather than as a permanent difference.
+	emptyLockValue := func(k, v string) bool {
+		return v == "" && equals(k, xhttp.AmzObjectLockMode, xhttp.AmzObjectLockRetainUntilDate)
+	}
+
 	// compare metadata on both maps to see if meta is identical
 	compareMeta1 := make(map[string]string)
 	for k, v := range oi1.UserDefined {
+		if emptyLockValue(k, v) {
+			continue
+		}
 		var found bool
 		for _, prefix := range compareKeys {
 			if !stringsHasPrefixFold(k, prefix) {
@@ -1004,6 +1022,10 @@ func getReplicationAction(oi1 ObjectInfo, oi2 minio.ObjectInfo, opType replicati

 	compareMeta2 := make(map[string]string)
 	for k, v := range oi2.Metadata {
+		val := strings.Join(v, ",")
+		if emptyLockValue(k, val) {
+			continue
+		}
 		var found bool
 		for _, prefix := range compareKeys {
 			if !stringsHasPrefixFold(k, prefix) {
@@ -1013,7 +1035,7 @@ func getReplicationAction(oi1 ObjectInfo, oi2 minio.ObjectInfo, opType replicati
 			break
 		}
 		if found {
-			compareMeta2[strings.ToLower(k)] = strings.Join(v, ",")
+			compareMeta2[strings.ToLower(k)] = val
 		}
 	}

@@ -1024,6 +1046,65 @@ func getReplicationAction(oi1 ObjectInfo, oi2 minio.ObjectInfo, opType replicati
 	return replicateNone
 }

+// objectRetentionGetter is the part of the replication target client used to confirm whether a
+// destination version still holds Object Lock retention.
+type objectRetentionGetter interface {
+	GetObjectRetention(ctx context.Context, bucketName, objectName, versionID string) (*minio.RetentionMode, *time.Time, error)
+}
+
+// retentionRemovedAtSource reports whether oi carries the shape a removed retention leaves behind,
+// an object lock key present with an empty value (cmd/object-handlers.go:3211-3215).
+func retentionRemovedAtSource(oi ObjectInfo) bool {
+	lkMap := caseInsensitiveMap(oi.UserDefined)
+	for _, k := range []string{xhttp.AmzObjectLockMode, xhttp.AmzObjectLockRetainUntilDate} {
+		if v, ok := lkMap.Lookup(k); ok && v == "" {
+			return true
+		}
+	}
+	return false
+}
+
+// targetRetentionConfirmedAbsent reports whether the destination version is known to hold no
+// retention. A HEAD response omits retention both when the version has none and when the
+// replication credential lacks s3:GetObjectRetention (cmd/object-handlers.go:942-946), so the
+// comparison in getReplicationAction on its own cannot tell a removal that is already in sync from
+// one the destination still holds. Only NoSuchObjectLockConfiguration, the answer for a version
+// that carries no retention, and a response naming no retention mode count as absent. Everything
+// else is uncertainty and is treated as still present, so that the removal is resent exactly as it
+// is today: a denied or unreachable destination, a mode the SDK returned without recognizing since
+// it does not validate it, and InvalidRequest, which names a bucket with no Object Lock
+// configuration but is also what a destination answers when its own read of that configuration
+// fails (cmd/bucket-object-lock.go:39-50 returns an error with a zero Retention, discarded at
+// cmd/object-handlers.go:3275).
+func targetRetentionConfirmedAbsent(ctx context.Context, tgt objectRetentionGetter, bucket, object, versionID string) bool {
+	mode, _, err := tgt.GetObjectRetention(ctx, bucket, object, versionID)
+	if err != nil {
+		return minio.ToErrorResponse(err).Code == "NoSuchObjectLockConfiguration"
+	}
+	// An absent or empty mode is no retention. A non-empty mode is retention, whether or not this
+	// SDK recognizes it.
+	return mode == nil || *mode == ""
+}
+
+// replicationActionForTarget returns the action for a source version against a destination that
+// answered HEAD. It is getReplicationAction plus the confirmation that a removed retention which
+// compares as in sync really is: see targetRetentionConfirmedAbsent.
+func replicationActionForTarget(ctx context.Context, oi1 ObjectInfo, oi2 minio.ObjectInfo, opType replication.Type, tgt objectRetentionGetter, bucket, object string) replicationAction {
+	rAction := getReplicationAction(oi1, oi2, opType)
+	if rAction != replicateNone || !retentionRemovedAtSource(oi1) {
+		return rAction
+	}
+	// A null version the resync deliberately leaves alone is not a comparison result, so it is
+	// not the confirmation's to reopen.
+	if nullVersionExcludedFromResync(oi1, oi2, opType) {
+		return rAction
+	}
+	if targetRetentionConfirmedAbsent(ctx, tgt, bucket, object, oi1.VersionID) {
+		return rAction
+	}
+	return replicateMetadata
+}
+
 // replicateObject replicates the specified version of the object to destination bucket
 // The source object is then updated to reflect the replication status.
 func replicateObject(ctx context.Context, ri ReplicateObjectInfo, objectAPI ObjectLayer) {
@@ -1467,7 +1548,7 @@ func (ri ReplicateObjectInfo) replicateAll(ctx context.Context, objectAPI Object
 	sOpts.Set(xhttp.AmzTagDirective, "ACCESS")
 	oi, cerr := tgt.StatObject(ctx, tgt.Bucket, object, sOpts)
 	if cerr == nil {
-		rAction = getReplicationAction(objInfo, oi, ri.OpType)
+		rAction = replicationActionForTarget(ctx, objInfo, oi, ri.OpType, tgt, tgt.Bucket, object)
 		rinfo.ReplicationStatus = replication.Completed
 		if rAction == replicateNone {
 			if ri.OpType == replication.ExistingObjectReplicationType &&
@@ -18,13 +18,19 @@
 package cmd

 import (
+	"context"
+	"errors"
 	"fmt"
 	"net/http"
+	"net/http/httptest"
 	"path"
+	"strings"
 	"testing"
 	"time"

 	"github.com/minio/madmin-go/v3"
+	"github.com/minio/minio-go/v7"
+	objectlock "github.com/minio/minio/internal/bucket/object/lock"
 	"github.com/minio/minio/internal/bucket/replication"
 	xhttp "github.com/minio/minio/internal/http"
 )
@@ -307,3 +313,318 @@ func TestReplicationValidationObjectUsesRulePrefix(t *testing.T) {
 		})
 	}
 }
+
+// newMatchingReplicationPair returns a source/target pair that getReplicationAction must
+// classify as replicateNone: same ETag, version id, size, modification time and content
+// type. Any action other than replicateNone is therefore attributable to the object lock
+// entries a caller adds on top.
+func newMatchingReplicationPair() (ObjectInfo, minio.ObjectInfo) {
+	mtime := time.Date(2026, 9, 5, 10, 0, 0, 0, time.UTC)
+	size := int64(7)
+	src := ObjectInfo{
+		Bucket:      "bucket",
+		Name:        "object",
+		ETag:        "d41d8cd98f00b204e9800998ecf8427e",
+		VersionID:   "b0ff1d6e-0000-4000-8000-000000000001",
+		Size:        size,
+		ActualSize:  &size,
+		ModTime:     mtime,
+		ContentType: "application/octet-stream",
+		UserDefined: map[string]string{"content-type": "application/octet-stream"},
+	}
+	tgt := minio.ObjectInfo{
+		ETag:         src.ETag,
+		VersionID:    src.VersionID,
+		Size:         size,
+		LastModified: mtime,
+		ContentType:  src.ContentType,
+		Metadata:     http.Header{},
+	}
+	return src, tgt
+}
+
+// TestGetReplicationActionEmptyObjectLockValues covers the comparison of object lock entries
+// whose value is empty. Removing retention from a version stores the mode and retain-until-date
+// keys with empty values, while the target's HEAD response omits them entirely, so the two must
+// compare equal or the version can never be reported as in sync. Cases 3 and 4 are synthetic
+// comparison inputs, since a SILO target cannot return empty lock headers; cases 7 and 8 guard
+// against over-normalizing.
+func TestGetReplicationActionEmptyObjectLockValues(t *testing.T) {
+	var (
+		modeKey = strings.ToLower(xhttp.AmzObjectLockMode)
+		dateKey = strings.ToLower(xhttp.AmzObjectLockRetainUntilDate)
+		until   = "2026-10-05T10:00:00.000Z"
+	)
+	emptyRetention := map[string]string{modeKey: "", dateKey: ""}
+	realRetention := map[string]string{modeKey: "GOVERNANCE", dateKey: until}
+
+	tests := []struct {
+		name    string
+		srcMeta map[string]string
+		tgtHdr  map[string]string
+		want    replicationAction
+	}{
+		{"1-both-clean-never-had-retention", nil, nil, replicateNone},
+		{"2-source-present-empty-target-absent", emptyRetention, nil, replicateNone},
+		{"3-source-absent-target-present-empty", nil, emptyRetention, replicateNone},
+		{"4-both-present-empty", emptyRetention, emptyRetention, replicateNone},
+		{"5-both-governance-equal", realRetention, realRetention, replicateNone},
+		{"6-source-governance-target-absent", realRetention, nil, replicateMetadata},
+		{"7-source-empty-target-real-retention", emptyRetention, realRetention, replicateMetadata},
+		{"8-empty-user-metadata-is-not-normalized", map[string]string{"x-amz-meta-foo": ""}, nil, replicateMetadata},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			src, tgt := newMatchingReplicationPair()
+			for k, v := range test.srcMeta {
+				src.UserDefined[k] = v
+			}
+			for k, v := range test.tgtHdr {
+				tgt.Metadata.Set(k, v)
+			}
+			if got := getReplicationAction(src, tgt, replication.HealReplicationType); got != test.want {
+				t.Fatalf("getReplicationAction() = %q, want %q (source %v, target %v)", got, test.want, src.UserDefined, tgt.Metadata)
+			}
+		})
+	}
+}
+
+// TestEmptyRetentionValuesAreOmittedFromObjectResponseHeaders records why the target half of the
+// comparison in getReplicationAction can never report an empty object lock entry:
+// FilterObjectLockMetadata drops both keys because an empty mode is not a valid retention mode,
+// and setObjectHeaders skips them when writing response headers. Neither filter reaches the
+// replication wire: the empty entries are still carried by getCopyObjMetadata and sent by the
+// metadata CopyObject, which is why the sender's comparison is what has to tolerate them.
+// FilterObjectLockMetadata is also applied by CopyObject (cmd/object-handlers.go:1708), where it
+// strips the source's lock metadata before the destination re-derives it from the request.
+func TestEmptyRetentionValuesAreOmittedFromObjectResponseHeaders(t *testing.T) {
+	modeKey := strings.ToLower(xhttp.AmzObjectLockMode)
+	dateKey := strings.ToLower(xhttp.AmzObjectLockRetainUntilDate)
+	meta := map[string]string{
+		modeKey:        "",
+		dateKey:        "",
+		"content-type": "application/octet-stream",
+	}
+
+	filtered := objectlock.FilterObjectLockMetadata(meta, false, false)
+	if _, ok := filtered[modeKey]; ok {
+		t.Errorf("FilterObjectLockMetadata() kept the empty lock mode key: %v", filtered)
+	}
+	if _, ok := filtered[dateKey]; ok {
+		t.Errorf("FilterObjectLockMetadata() kept the empty retain-until-date key: %v", filtered)
+	}
+
+	rec := httptest.NewRecorder()
+	if err := setObjectHeaders(t.Context(), rec, ObjectInfo{UserDefined: meta, ModTime: time.Now(), Size: 7}, nil, ObjectOptions{}); err != nil {
+		t.Fatalf("setObjectHeaders() = %v", err)
+	}
+	if v, ok := rec.Header()[http.CanonicalHeaderKey(xhttp.AmzObjectLockMode)]; ok {
+		t.Errorf("setObjectHeaders() emitted an empty lock mode header: %v", v)
+	}
+	if v, ok := rec.Header()[http.CanonicalHeaderKey(xhttp.AmzObjectLockRetainUntilDate)]; ok {
+		t.Errorf("setObjectHeaders() emitted an empty retain-until-date header: %v", v)
+	}
+}
+
+// fakeRetentionGetter answers GetObjectRetention with a fixed result and counts its calls.
+type fakeRetentionGetter struct {
+	mode  *minio.RetentionMode
+	err   error
+	calls int
+}
+
+func (f *fakeRetentionGetter) GetObjectRetention(_ context.Context, _, _, _ string) (*minio.RetentionMode, *time.Time, error) {
+	f.calls++
+	return f.mode, nil, f.err
+}
+
+func TestRetentionRemovedAtSource(t *testing.T) {
+	modeKey := strings.ToLower(xhttp.AmzObjectLockMode)
+	dateKey := strings.ToLower(xhttp.AmzObjectLockRetainUntilDate)
+	tests := []struct {
+		name string
+		meta map[string]string
+		want bool
+	}{
+		{"no lock keys", map[string]string{"content-type": "text/plain"}, false},
+		{"empty pair", map[string]string{modeKey: "", dateKey: ""}, true},
+		{"empty mode only", map[string]string{modeKey: ""}, true},
+		{"empty date only", map[string]string{dateKey: ""}, true},
+		{"real retention", map[string]string{modeKey: "GOVERNANCE", dateKey: "2026-10-05T10:00:00.000Z"}, false},
+		{"canonical case", map[string]string{xhttp.AmzObjectLockMode: ""}, true},
+		{"empty user metadata", map[string]string{"x-amz-meta-foo": ""}, false},
+	}
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			if got := retentionRemovedAtSource(ObjectInfo{UserDefined: test.meta}); got != test.want {
+				t.Fatalf("retentionRemovedAtSource() = %v, want %v", got, test.want)
+			}
+		})
+	}
+}
+
+// TestTargetRetentionConfirmedAbsent pins the rule that only an explicit answer from the
+// destination clears a removed retention. A denied or unreachable destination must read as still
+// holding retention, because HEAD hides a real retention from a credential without
+// s3:GetObjectRetention exactly as it hides one that does not exist.
+func TestTargetRetentionConfirmedAbsent(t *testing.T) {
+	governance := minio.Governance
+	var emptyMode minio.RetentionMode
+	unknownMode := minio.RetentionMode("ARCHIVE")
+	tests := []struct {
+		name string
+		mode *minio.RetentionMode
+		err  error
+		want bool
+	}{
+		{"version holds governance retention", &governance, nil, false},
+		{"no retention on the version", nil, minio.ErrorResponse{Code: "NoSuchObjectLockConfiguration"}, true},
+		{
+			// The destination also answers this when its own read of the bucket's Object Lock
+			// configuration fails, so it does not establish that Object Lock is disabled.
+			"invalid request naming a missing object lock configuration",
+			nil,
+			minio.ErrorResponse{Code: "InvalidRequest", Message: "Bucket is missing ObjectLockConfiguration"},
+			false,
+		},
+		{
+			"unrelated invalid request",
+			nil,
+			minio.ErrorResponse{Code: "InvalidRequest", Message: "Object is WORM protected and cannot be overwritten"},
+			false,
+		},
+		{"retention read denied", nil, minio.ErrorResponse{Code: "AccessDenied"}, false},
+		{"destination unreachable", nil, errors.New("dial tcp: connection refused"), false},
+		{"empty mode returned", &emptyMode, nil, true},
+		{"unknown non-empty mode returned", &unknownMode, nil, false},
+		{"nil mode returned", nil, nil, true},
+	}
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			tgt := &fakeRetentionGetter{mode: test.mode, err: test.err}
+			if got := targetRetentionConfirmedAbsent(t.Context(), tgt, "bucket", "object", "v1"); got != test.want {
+				t.Fatalf("targetRetentionConfirmedAbsent() = %v, want %v", got, test.want)
+			}
+			if tgt.calls != 1 {
+				t.Fatalf("GetObjectRetention called %d times, want 1", tgt.calls)
+			}
+		})
+	}
+}
+
+// TestReplicationActionForTargetRetentionRemoval covers the decision the replication worker makes
+// for a version whose retention was removed. The destination's HEAD never reports the empty keys,
+// so the comparison alone reads every one of these as in sync; only the confirmation separates a
+// destination that really dropped the retention from one that is hiding it.
+func TestReplicationActionForTargetRetentionRemoval(t *testing.T) {
+	modeKey := strings.ToLower(xhttp.AmzObjectLockMode)
+	dateKey := strings.ToLower(xhttp.AmzObjectLockRetainUntilDate)
+	governance := minio.Governance
+
+	tests := []struct {
+		name      string
+		srcMeta   map[string]string
+		mode      *minio.RetentionMode
+		err       error
+		want      replicationAction
+		wantCalls int
+	}{
+		{
+			name:      "removal confirmed by destination",
+			srcMeta:   map[string]string{modeKey: "", dateKey: ""},
+			err:       minio.ErrorResponse{Code: "NoSuchObjectLockConfiguration"},
+			want:      replicateNone,
+			wantCalls: 1,
+		},
+		{
+			name:      "destination still holds the retention hidden from HEAD",
+			srcMeta:   map[string]string{modeKey: "", dateKey: ""},
+			mode:      &governance,
+			want:      replicateMetadata,
+			wantCalls: 1,
+		},
+		{
+			name:      "retention hidden from HEAD by permissions",
+			srcMeta:   map[string]string{modeKey: "", dateKey: ""},
+			err:       minio.ErrorResponse{Code: "AccessDenied"},
+			want:      replicateMetadata,
+			wantCalls: 1,
+		},
+		{
+			// A destination that names a missing Object Lock configuration answers the same way
+			// when its own read of that configuration failed, so it confirms nothing.
+			name:      "destination reports no object lock configuration",
+			srcMeta:   map[string]string{modeKey: "", dateKey: ""},
+			err:       minio.ErrorResponse{Code: "InvalidRequest", Message: "Bucket is missing ObjectLockConfiguration"},
+			want:      replicateMetadata,
+			wantCalls: 1,
+		},
+		{
+			name:      "version never had retention is not confirmed",
+			srcMeta:   nil,
+			want:      replicateNone,
+			wantCalls: 0,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			src, tgtInfo := newMatchingReplicationPair()
+			for k, v := range test.srcMeta {
+				src.UserDefined[k] = v
+			}
+			tgt := &fakeRetentionGetter{mode: test.mode, err: test.err}
+			got := replicationActionForTarget(t.Context(), src, tgtInfo, replication.HealReplicationType, tgt, "bucket", "object")
+			if got != test.want {
+				t.Fatalf("replicationActionForTarget() = %q, want %q", got, test.want)
+			}
+			if tgt.calls != test.wantCalls {
+				t.Fatalf("GetObjectRetention called %d times, want %d", tgt.calls, test.wantCalls)
+			}
+		})
+	}
+}
+
+// TestReplicationActionForTargetNullVersionResync pins that the confirmation does not reopen the
+// null-version exclusion at the head of getReplicationAction. An existing object resync returns
+// replicateNone for a null version whose source modification time is later than the target's,
+// before comparing anything, and that must stand even when the source carries a removed retention
+// and the destination would report retention or refuse to answer.
+func TestReplicationActionForTargetNullVersionResync(t *testing.T) {
+	modeKey := strings.ToLower(xhttp.AmzObjectLockMode)
+	dateKey := strings.ToLower(xhttp.AmzObjectLockRetainUntilDate)
+	governance := minio.Governance
+
+	tests := []struct {
+		name string
+		mode *minio.RetentionMode
+		err  error
+	}{
+		{"destination holds retention", &governance, nil},
+		{"retention read denied", nil, minio.ErrorResponse{Code: "AccessDenied"}},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			src, tgtInfo := newMatchingReplicationPair()
+			// A null version whose source modification time is later, and whose content differs,
+			// so only the exclusion can hold the action at replicateNone.
+			src.VersionID = nullVersionID
+			src.ModTime = tgtInfo.LastModified.Add(time.Hour)
+			src.ETag = "5d41402abc4b2a76b9719d911017c592"
+			src.UserDefined[modeKey] = ""
+			src.UserDefined[dateKey] = ""
+			tgtInfo.VersionID = nullVersionID
+
+			tgt := &fakeRetentionGetter{mode: test.mode, err: test.err}
+			got := replicationActionForTarget(t.Context(), src, tgtInfo, replication.ExistingObjectReplicationType, tgt, "bucket", "object")
+			if got != replicateNone {
+				t.Fatalf("replicationActionForTarget() = %q, want %q", got, replicateNone)
+			}
+			if tgt.calls != 0 {
+				t.Fatalf("GetObjectRetention called %d times, want 0", tgt.calls)
+			}
+		})
+	}
+}