diff --git a/cmd/bucket-replication.go b/cmd/bucket-replication.go index d9c97cd2b..bfba4741a 100644 --- a/cmd/bucket-replication.go +++ b/cmd/bucket-replication.go @@ -781,6 +781,18 @@ func (m caseInsensitiveMap) Lookup(key string) (string, bool) { return "", false } +// replicationTaggingTimestamp carries a recorded removal even when tags are +// empty. Only legacy nonempty tags use ModTime; absence is not a tombstone. +func replicationTaggingTimestamp(objInfo ObjectInfo) (time.Time, error) { + if stamp, ok := caseInsensitiveMap(objInfo.UserDefined).Lookup(ReservedMetadataPrefixLower + TaggingTimestamp); ok { + return time.Parse(time.RFC3339Nano, stamp) + } + if objInfo.UserTags != "" { + return objInfo.ModTime, nil + } + return time.Time{}, nil +} + func putReplicationOpts(ctx context.Context, sc string, objInfo ObjectInfo) (putOpts minio.PutObjectOptions, isMP bool, err error) { meta := make(map[string]string) isSSEC := crypto.SSEC.IsEncrypted(objInfo.UserDefined) @@ -850,17 +862,12 @@ func putReplicationOpts(ctx context.Context, sc string, objInfo ObjectInfo) (put tag, _ := tags.ParseObjectTags(objInfo.UserTags) if tag != nil { putOpts.UserTags = tag.ToMap() - // set tag timestamp in opts - tagTimestamp := objInfo.ModTime - if tagTmstampStr, ok := objInfo.UserDefined[ReservedMetadataPrefixLower+TaggingTimestamp]; ok { - tagTimestamp, err = time.Parse(time.RFC3339Nano, tagTmstampStr) - if err != nil { - return putOpts, false, err - } - } - putOpts.Internal.TaggingTimestamp = tagTimestamp } } + putOpts.Internal.TaggingTimestamp, err = replicationTaggingTimestamp(objInfo) + if err != nil { + return putOpts, false, err + } lkMap := caseInsensitiveMap(objInfo.UserDefined) if lang, ok := lkMap.Lookup(xhttp.ContentLanguage); ok { @@ -1003,6 +1010,12 @@ func getReplicationAction(oi1 ObjectInfo, oi2 minio.ObjectInfo, opType replicati if (oi2.UserTagCount > 0 && !reflect.DeepEqual(oi2Map, t.ToMap())) || (oi2.UserTagCount != len(t.ToMap())) { return replicateMetadata } + // HEAD does not report the tag revision. Equal values can hide a newer + // deletion or re-addition, so scheduled metadata/heal work must deliver it. + // Completed objects are still excluded by the existing scanner gates. + if _, ok := caseInsensitiveMap(oi1.UserDefined).Lookup(ReservedMetadataPrefixLower + TaggingTimestamp); ok { + return replicateMetadata + } // Compare only necessary headers compareKeys := []string{ @@ -1269,9 +1282,6 @@ func replicateObject(ctx context.Context, ri ReplicateObjectInfo, objectAPI Obje oi.UserDefined[targetResetHeader(rinfo.Arn)] = rinfo.ResyncTimestamp } } - if ri.UserTags != "" { - oi.UserDefined[xhttp.AmzObjectTagging] = ri.UserTags - } return dsc, nil }, } @@ -1689,14 +1699,11 @@ applyAction: if _, ok := lkMap.Lookup(xhttp.AmzObjectLockRetainUntilDate); ok { dstOpts.Internal.RetentionTimestamp = objInfo.ModTime } - if objInfo.UserTags != "" { - dstOpts.Internal.TaggingTimestamp = objInfo.ModTime - } - if tagTmStr, ok := lkMap.Lookup(ReservedMetadataPrefixLower + TaggingTimestamp); ok { - ondiskTimestamp, err := time.Parse(time.RFC3339, tagTmStr) - if err == nil { - dstOpts.Internal.TaggingTimestamp = ondiskTimestamp - } + dstOpts.Internal.TaggingTimestamp, rinfo.Err = replicationTaggingTimestamp(objInfo) + if rinfo.Err != nil { + rinfo.ReplicationStatus = replication.Failed + replLogIf(ctx, fmt.Errorf("invalid tagging timestamp for object %s/%s(%s): %w", bucket, object, objInfo.VersionID, rinfo.Err)) + return rinfo } if retTmStr, ok := lkMap.Lookup(ReservedMetadataPrefixLower + ObjectLockRetentionTimestamp); ok { ondiskTimestamp, err := time.Parse(time.RFC3339, retTmStr) diff --git a/cmd/erasure-object.go b/cmd/erasure-object.go index da8b6fcc4..29c6011cd 100644 --- a/cmd/erasure-object.go +++ b/cmd/erasure-object.go @@ -2327,7 +2327,11 @@ func (er erasureObjects) PutObjectTags(ctx context.Context, bucket, object strin fi.Metadata[xhttp.AmzObjectTagging] = tags fi.ReplicationState = opts.PutReplicationState() + stamp := monotonicTaggingTimestamp(opts.UserDefined[ReservedMetadataPrefixLower+TaggingTimestamp], fi.Metadata[ReservedMetadataPrefixLower+TaggingTimestamp]) maps.Copy(fi.Metadata, opts.UserDefined) + if stamp != "" { + fi.Metadata[ReservedMetadataPrefixLower+TaggingTimestamp] = stamp + } if err = er.updateObjectMeta(ctx, bucket, object, fi, onlineDisks); err != nil { return ObjectInfo{}, toObjectErr(err, bucket, object) diff --git a/cmd/erasure-server-pool-consistency.go b/cmd/erasure-server-pool-consistency.go index f668db88e..b4e587516 100644 --- a/cmd/erasure-server-pool-consistency.go +++ b/cmd/erasure-server-pool-consistency.go @@ -242,6 +242,21 @@ func reconcileStoredObjectTags(metadata map[string]string, storedTags, storedTim } } +// Local tagging mutations must advance the revision they overwrite, even when +// a request's clock or lock acquisition order is behind the stored revision. +// Replica writes use reconcileStoredObjectTags instead of minting a revision. +func monotonicTaggingTimestamp(incoming, stored string) string { + requested, err := time.Parse(time.RFC3339Nano, incoming) + if err != nil { + return incoming + } + current, err := time.Parse(time.RFC3339Nano, stored) + if err != nil || requested.After(current) { + return incoming + } + return current.Add(time.Nanosecond).UTC().Format(time.RFC3339Nano) +} + // A restored version still owns its tier reference even while IsRemote is // false. Only the last copy of a reference may schedule its contents for GC. func sharesTierObject(oi ObjectInfo, copies []PoolObjInfo) bool { diff --git a/cmd/erasure-server-pool.go b/cmd/erasure-server-pool.go index f5f612c2a..555986bc0 100644 --- a/cmd/erasure-server-pool.go +++ b/cmd/erasure-server-pool.go @@ -3051,6 +3051,15 @@ func (z *erasureServerPools) PutObjectTags(ctx context.Context, bucket, object s if err != nil { return ObjectInfo{}, err } + // Ordinary reads and replication can return any owning pool. Persist one + // revision beyond all copies, so the returned value and every pool agree. + if stamp := opts.UserDefined[ReservedMetadataPrefixLower+TaggingTimestamp]; stamp != "" { + for _, copy := range copies { + stamp = monotonicTaggingTimestamp(stamp, copy.ObjInfo.UserDefined[ReservedMetadataPrefixLower+TaggingTimestamp]) + } + opts.UserDefined = cloneMSS(opts.UserDefined) + opts.UserDefined[ReservedMetadataPrefixLower+TaggingTimestamp] = stamp + } opts.NoLock = true opts.VersionID = copies[0].ObjInfo.VersionID if opts.VersionID == "" { diff --git a/cmd/object-handlers-common.go b/cmd/object-handlers-common.go index 438ba8d3a..c7b4e9b94 100644 --- a/cmd/object-handlers-common.go +++ b/cmd/object-handlers-common.go @@ -240,7 +240,10 @@ func checkPreconditionsPUT(ctx context.Context, w http.ResponseWriter, r *http.R // updated. The predicate is the incoming request's restored SSE-C metadata, // not what the destination happens to hold. ssecReplica := isReplicaTrusted(r.Context()) && crypto.SSEC.IsEncrypted(opts.UserDefined) - if etagMatch && vidMatch && !ssecReplica { + // Matching content does not imply that its tag revision was delivered. + // Keep client preconditions above; relax only the internal duplicate check. + newerTags := isReplicaTrusted(r.Context()) && olderThan(objInfo.UserDefined[ReservedMetadataPrefixLower+TaggingTimestamp], opts.ReplicationSourceTaggingTimestamp) + if etagMatch && vidMatch && !ssecReplica && !newerTags { writeHeaders() writeErrorResponse(ctx, w, errorCodes.ToAPIErr(ErrPreconditionFailed), r.URL) return true diff --git a/cmd/object-handlers.go b/cmd/object-handlers.go index a06910684..fa44129af 100644 --- a/cmd/object-handlers.go +++ b/cmd/object-handlers.go @@ -1797,6 +1797,7 @@ func (api objectAPIHandlers) CopyObjectHandler(w http.ResponseWriter, r *http.Re // source timestamp is newer than the stored one, and a stale update must // leave the stored state in place instead of erasing it. storedLock := storedObjectLockState(srcInfo.UserDefined) + storedTagTimestamp := srcInfo.UserDefined[ReservedMetadataPrefixLower+TaggingTimestamp] srcInfo.UserDefined, err = getCpObjMetadataFromHeader(ctx, r, srcInfo.UserDefined, allowReplicationMetadata) if err != nil { @@ -1814,23 +1815,29 @@ func (api objectAPIHandlers) CopyObjectHandler(w http.ResponseWriter, r *http.Re } } - if objTags != "" { - lastTaggingTimestamp := srcInfo.UserDefined[ReservedMetadataPrefixLower+TaggingTimestamp] - if dstOpts.ReplicationRequest { - srcTimestamp := dstOpts.ReplicationSourceTaggingTimestamp - if !srcTimestamp.IsZero() { - ondiskTimestamp, err := time.Parse(time.RFC3339Nano, lastTaggingTimestamp) - // update tagging metadata only if replica timestamp is newer than what's on disk - if err != nil || (err == nil && !ondiskTimestamp.After(srcTimestamp)) { - srcInfo.UserDefined[ReservedMetadataPrefixLower+TaggingTimestamp] = srcTimestamp.UTC().Format(time.RFC3339Nano) - srcInfo.UserDefined[xhttp.AmzObjectTagging] = objTags - } - } - } else { + if dstOpts.ReplicationRequest { + srcTimestamp := dstOpts.ReplicationSourceTaggingTimestamp + if !srcTimestamp.IsZero() { + // An empty value with a timestamp is an ordered deletion. Recheck + // the captured state even if metadata REPLACE rebuilt the map. + srcInfo.UserDefined[ReservedMetadataPrefixLower+TaggingTimestamp] = srcTimestamp.UTC().Format(time.RFC3339Nano) srcInfo.UserDefined[xhttp.AmzObjectTagging] = objTags - srcInfo.UserDefined[ReservedMetadataPrefixLower+TaggingTimestamp] = UTCNow().Format(time.RFC3339Nano) + reconcileStoredObjectTags(srcInfo.UserDefined, srcInfo.UserTags, storedTagTimestamp) + } else { + srcInfo.UserDefined[xhttp.AmzObjectTagging] = srcInfo.UserTags + if storedTagTimestamp != "" { + srcInfo.UserDefined[ReservedMetadataPrefixLower+TaggingTimestamp] = storedTagTimestamp + } else { + delete(srcInfo.UserDefined, ReservedMetadataPrefixLower+TaggingTimestamp) + } } + } else { + srcInfo.UserDefined[xhttp.AmzObjectTagging] = objTags + srcInfo.UserDefined[ReservedMetadataPrefixLower+TaggingTimestamp] = UTCNow().Format(time.RFC3339Nano) } + // SSE-C rotation snapshots reserved metadata before the tag decision. Its + // later merge must not put the old timestamp back over the accepted state. + delete(encMetadata, ReservedMetadataPrefixLower+TaggingTimestamp) srcInfo.UserDefined = filterReplicationStatusMetadata(srcInfo.UserDefined) srcInfo.UserDefined = objectlock.FilterObjectLockMetadata(srcInfo.UserDefined, true, true) @@ -2313,6 +2320,9 @@ func (api objectAPIHandlers) PutObjectHandler(w http.ResponseWriter, r *http.Req writeErrorResponse(ctx, w, toAPIError(ctx, err), r.URL) return } + if opts.ReplicationRequest && !opts.ReplicationSourceTaggingTimestamp.IsZero() { + metadata[ReservedMetadataPrefixLower+TaggingTimestamp] = opts.ReplicationSourceTaggingTimestamp.UTC().Format(time.RFC3339Nano) + } actualSize := size var idxCb func() []byte @@ -3760,11 +3770,11 @@ func (api objectAPIHandlers) PutObjectTaggingHandler(w http.ResponseWriter, r *h } dsc := mustReplicate(ctx, bucket, object, getMustReplicateOptions(objInfo.UserDefined, tagsStr, objInfo.ReplicationStatus, replication.MetadataReplicationType, opts)) + stamp := UTCNow().Format(time.RFC3339Nano) + opts.UserDefined = map[string]string{ReservedMetadataPrefixLower + TaggingTimestamp: stamp} if dsc.ReplicateAny() { - opts.UserDefined = make(map[string]string) - opts.UserDefined[ReservedMetadataPrefixLower+ReplicationTimestamp] = UTCNow().Format(time.RFC3339Nano) + opts.UserDefined[ReservedMetadataPrefixLower+ReplicationTimestamp] = stamp opts.UserDefined[ReservedMetadataPrefixLower+ReplicationStatus] = dsc.PendingStatus() - opts.UserDefined[ReservedMetadataPrefixLower+TaggingTimestamp] = UTCNow().Format(time.RFC3339Nano) } // Put object tags @@ -3863,9 +3873,10 @@ func (api objectAPIHandlers) DeleteObjectTaggingHandler(w http.ResponseWriter, r } dsc := mustReplicate(ctx, bucket, object, oi.getMustReplicateOptions(replication.MetadataReplicationType, opts)) + stamp := UTCNow().Format(time.RFC3339Nano) + opts.UserDefined = map[string]string{ReservedMetadataPrefixLower + TaggingTimestamp: stamp} if dsc.ReplicateAny() { - opts.UserDefined = make(map[string]string) - opts.UserDefined[ReservedMetadataPrefixLower+ReplicationTimestamp] = UTCNow().Format(time.RFC3339Nano) + opts.UserDefined[ReservedMetadataPrefixLower+ReplicationTimestamp] = stamp opts.UserDefined[ReservedMetadataPrefixLower+ReplicationStatus] = dsc.PendingStatus() } diff --git a/cmd/object-multipart-handlers.go b/cmd/object-multipart-handlers.go index 44f61f353..e56630005 100644 --- a/cmd/object-multipart-handlers.go +++ b/cmd/object-multipart-handlers.go @@ -312,6 +312,10 @@ func (api objectAPIHandlers) NewMultipartUploadHandler(w http.ResponseWriter, r writeErrorResponse(ctx, w, toAPIError(ctx, err), r.URL) return } + // Completion orders the upload's persisted tag state under the object lock. + if opts.ReplicationRequest && !opts.ReplicationSourceTaggingTimestamp.IsZero() { + metadata[ReservedMetadataPrefixLower+TaggingTimestamp] = opts.ReplicationSourceTaggingTimestamp.UTC().Format(time.RFC3339Nano) + } if r.Header.Get(xhttp.IfMatch) != "" { opts.HasIfMatch = true diff --git a/cmd/replication-tagging-order_test.go b/cmd/replication-tagging-order_test.go new file mode 100644 index 000000000..7baf3b7bf --- /dev/null +++ b/cmd/replication-tagging-order_test.go @@ -0,0 +1,599 @@ +// Copyright (c) 2026 Feng Ruohang +// +// This file is part of Silo Object Storage stack +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . + +package cmd + +import ( + "context" + "encoding/xml" + "fmt" + "maps" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" + "time" + + "github.com/minio/minio-go/v7" + "github.com/minio/minio/internal/auth" + "github.com/minio/minio/internal/bucket/replication" + xhttp "github.com/minio/minio/internal/http" + "github.com/minio/minio/internal/kms" +) + +const r5TagStamp = ReservedMetadataPrefixLower + TaggingTimestamp + +func r5Capacity(z *erasureServerPools) func() { + var restores []func() + for _, pool := range z.serverPools { + for _, set := range pool.sets { + old := set.getDisks + disks := append([]StorageAPI(nil), old()...) + for i := range disks { + disks[i] = tagTestCapacityDisk{StorageAPI: disks[i]} + } + set.getDisks = func() []StorageAPI { return disks } + restores = append(restores, func() { set.getDisks = old }) + } + } + return func() { + for _, restore := range restores { + restore() + } + } +} + +func r5Request(t *testing.T, router http.Handler, cred auth.Credentials, method, path, body string, headers map[string]string) *httptest.ResponseRecorder { + t.Helper() + r, err := newTestSignedRequestV4(method, path, int64(len(body)), strings.NewReader(body), cred.AccessKey, cred.SecretKey, headers) + if err != nil { + t.Fatal(err) + } + w := httptest.NewRecorder() + router.ServeHTTP(w, r) + return w +} + +func r5Stored(t *testing.T, obj interface { + GetObjectInfo(context.Context, string, string, ObjectOptions) (ObjectInfo, error) +}, bucket, name, vid, wantTags, wantStamp string, +) ObjectInfo { + t.Helper() + oi, err := obj.GetObjectInfo(t.Context(), bucket, name, ObjectOptions{VersionID: vid}) + if err != nil { + t.Fatal(err) + } + if oi.UserTags != wantTags || oi.UserDefined[r5TagStamp] != wantStamp { + t.Fatalf("%s(%s): tags=%q stamp=%q, want %q %q", name, vid, oi.UserTags, oi.UserDefined[r5TagStamp], wantTags, wantStamp) + } + return oi +} + +// The request bytes are signed and enter the real API and storage implementation. +// Equal/stale retransmits may retain the existing 412 duplicate response. +func r5Receive(t *testing.T, obj ObjectLayer, router http.Handler, cred auth.Credentials, bucket, operation string, source ObjectInfo, stamp string, afterInit func()) { + t.Helper() + opts, _, err := putReplicationOpts(t.Context(), "", source) + if err != nil { + t.Fatal(err) + } + if operation == "multipart" { + opts.Internal.SourceMTime = time.Time{} + } + headers := make(map[string]string) + for k, vs := range opts.Header() { + if len(vs) > 0 { + headers[k] = vs[0] + } + } + if stamp == "" { + delete(headers, http.CanonicalHeaderKey(xhttp.MinIOSourceTaggingTimestamp)) + delete(headers, xhttp.MinIOSourceTaggingTimestamp) + } else { + headers[http.CanonicalHeaderKey(xhttp.MinIOSourceTaggingTimestamp)] = stamp + } + path := "/" + bucket + "/" + source.Name + "?versionId=" + source.VersionID + var w *httptest.ResponseRecorder + switch operation { + case "copy", "copy-default": + maps.Copy(headers, getCopyObjMetadata(source, "")) + headers[xhttp.AmzCopySource] = "/" + bucket + "/" + source.Name + "?versionId=" + source.VersionID + if operation == "copy" { + headers[xhttp.AmzMetadataDirective] = "REPLACE" + } + w = r5Request(t, router, cred, http.MethodPut, path, "", headers) + case "put": + w = r5Request(t, router, cred, http.MethodPut, path, "data", headers) + case "multipart": + w = r5Request(t, router, cred, http.MethodPost, path+"&uploads", "", headers) + if w.Code == http.StatusPreconditionFailed { + return + } + if w.Code != http.StatusOK { + t.Fatalf("init: %d %s", w.Code, w.Body.String()) + } + var init struct { + UploadID string `xml:"UploadId"` + } + if err := xml.Unmarshal(w.Body.Bytes(), &init); err != nil || init.UploadID == "" { + t.Fatalf("init XML: %v %s", err, w.Body.String()) + } + mi, err := obj.GetMultipartInfo(t.Context(), bucket, source.Name, init.UploadID, ObjectOptions{}) + if err != nil { + t.Fatal(err) + } + if stamp != "" && mi.UserDefined[r5TagStamp] != stamp { + t.Fatalf("upload persisted stamp=%q, want %q", mi.UserDefined[r5TagStamp], stamp) + } + partPath := "/" + bucket + "/" + source.Name + "?uploadId=" + url.QueryEscape(init.UploadID) + ph := map[string]string{xhttp.MinIOSourceReplicationRequest: "true"} + part := r5Request(t, router, cred, http.MethodPut, partPath+"&partNumber=1", "data", ph) + if part.Code != http.StatusOK { + t.Fatalf("part: %d %s", part.Code, part.Body.String()) + } + if afterInit != nil { + afterInit() + } + body := "1" + canonicalizeETag(part.Header()[xhttp.ETag][0]) + "" + ph[xhttp.MinIOSourceMTime] = source.ModTime.Format(time.RFC3339Nano) + ph[xhttp.MinIOSourceETag] = source.ETag + w = r5Request(t, router, cred, http.MethodPost, partPath, body, ph) + } + if w.Code != http.StatusOK && w.Code != http.StatusPreconditionFailed { + t.Fatalf("%s: %d %s", operation, w.Code, w.Body.String()) + } +} + +func TestAPITaggingReplicationOrdering(t *testing.T) { r5APIOrdering(t, false) } +func TestAPITaggingReplicationOrderingKMS(t *testing.T) { r5APIOrdering(t, true) } +func r5APIOrdering(t *testing.T, encrypted bool) { + ExecObjectLayerAPITest(ExecObjectLayerAPITestArgs{t: t, objAPITest: func(obj ObjectLayer, instance, bucket string, router http.Handler, cred auth.Credentials, t *testing.T) { + defer r5Capacity(obj.(*erasureServerPools))() + if _, err := globalBucketMetadataSys.Update(t.Context(), bucket, bucketVersioningConfig, enabledBucketVersioningConfig); err != nil { + t.Fatal(err) + } + if encrypted { + prev := GlobalKMS + GlobalKMS = kms.NewStub("r5-tag-order") + defer func() { GlobalKMS = prev }() + sse := []byte(`aws:kmsr5-tag-order`) + if _, err := globalBucketMetadataSys.Update(t.Context(), bucket, bucketSSEConfig, sse); err != nil { + t.Fatal(err) + } + } + base := time.Now().UTC().Add(-5 * time.Hour) + for _, op := range []string{"copy", "copy-default", "put", "multipart"} { + for _, version := range []string{"uuid", "null"} { + t.Run(instance+"/"+op+"/"+version, func(t *testing.T) { + name := op + "-" + version + vid := mustGetUUID() + if version == "null" { + vid = nullVersionID + } + original, err := obj.PutObject(t.Context(), bucket, name, mustGetPutObjReader(t, strings.NewReader("data"), 4, "", ""), ObjectOptions{Versioned: true, VersionID: vid, UserDefined: map[string]string{xhttp.AmzObjectTagging: "key=original", r5TagStamp: base.Format(time.RFC3339Nano)}}) + if err != nil { + t.Fatal(err) + } + if op == "multipart" { + // The production sender uses multipart only for multipart + // sources. Preserve a real multipart ETag and part layout. + metadata := maps.Clone(original.UserDefined) + metadata[xhttp.AmzObjectTagging] = original.UserTags + mp, err := obj.NewMultipartUpload(t.Context(), bucket, name, ObjectOptions{Versioned: true, VersionID: vid, UserDefined: metadata}) + if err != nil { + t.Fatal(err) + } + part, err := obj.PutObjectPart(t.Context(), bucket, name, mp.UploadID, 1, mustGetPutObjReader(t, strings.NewReader("data"), 4, "", ""), ObjectOptions{}) + if err != nil { + t.Fatal(err) + } + original, err = obj.CompleteMultipartUpload(t.Context(), bucket, name, mp.UploadID, []CompletePart{{PartNumber: 1, ETag: part.ETag}}, ObjectOptions{Versioned: true}) + if err != nil { + t.Fatal(err) + } + } + // A later unrelated version must not contribute tags to an explicitly addressed UUID/null version. + latest, err := obj.PutObject(t.Context(), bucket, name, mustGetPutObjReader(t, strings.NewReader("other"), 5, "", ""), ObjectOptions{Versioned: true, UserDefined: map[string]string{xhttp.AmzObjectTagging: "key=latest", r5TagStamp: base.Add(10 * time.Hour).Format(time.RFC3339Nano)}}) + if err != nil { + t.Fatal(err) + } + for _, event := range []struct { + name, tags string + hours int + wantTags string + wantHours int + }{ + {"delete", "", 3, "", 3}, {"stale", "key=stale", 2, "", 3}, {"equal-conflict", "key=conflict", 3, "", 3}, {"newer", "key=new", 4, "key=new", 4}, + } { + t.Run(event.name, func(t *testing.T) { + source := original + source.VersionID = vid + source.UserDefined = maps.Clone(original.UserDefined) + source.UserTags = event.tags + stamp := base.Add(time.Duration(event.hours) * time.Hour).Format(time.RFC3339Nano) + source.UserDefined[r5TagStamp] = stamp + r5Receive(t, obj, router, cred, bucket, op, source, stamp, nil) + r5Stored(t, obj, bucket, name, vid, event.wantTags, base.Add(time.Duration(event.wantHours)*time.Hour).Format(time.RFC3339Nano)) + r5Stored(t, obj, bucket, name, latest.VersionID, "key=latest", base.Add(10*time.Hour).Format(time.RFC3339Nano)) + }) + } + source := original + source.VersionID = vid + source.UserTags = "key=unversioned-event" + r5Receive(t, obj, router, cred, bucket, op, source, "", nil) + r5Stored(t, obj, bucket, name, vid, "key=new", base.Add(4*time.Hour).Format(time.RFC3339Nano)) + get := r5Request(t, router, cred, http.MethodGet, "/"+bucket+"/"+name+"?versionId="+vid, "", nil) + if get.Code != http.StatusOK || get.Body.String() != "data" { + t.Fatalf("plaintext GET: %d %q", get.Code, get.Body.String()) + } + }) + } + } + }}) +} + +func TestAPITaggingMultipartCommitRechecksRevision(t *testing.T) { + ExecObjectLayerAPITest(ExecObjectLayerAPITestArgs{t: t, objAPITest: func(obj ObjectLayer, instance, bucket string, router http.Handler, cred auth.Credentials, t *testing.T) { + defer r5Capacity(obj.(*erasureServerPools))() + if _, err := globalBucketMetadataSys.Update(t.Context(), bucket, bucketVersioningConfig, enabledBucketVersioningConfig); err != nil { + t.Fatal(err) + } + base := time.Now().UTC().Add(-time.Hour) + source := ObjectInfo{Name: "commit-recheck", VersionID: mustGetUUID(), ModTime: base, UserTags: "key=incoming", UserDefined: map[string]string{r5TagStamp: base.Format(time.RFC3339Nano)}} + later := base.Add(time.Minute).Format(time.RFC3339Nano) + r5Receive(t, obj, router, cred, bucket, "multipart", source, base.Format(time.RFC3339Nano), func() { + _, err := obj.PutObject(t.Context(), bucket, source.Name, mustGetPutObjReader(t, strings.NewReader("data"), 4, "", ""), ObjectOptions{Versioned: true, VersionID: source.VersionID, MTime: base, UserDefined: map[string]string{r5TagStamp: later}}) + if err != nil { + t.Fatal(err) + } + }) + r5Stored(t, obj, bucket, source.Name, source.VersionID, "", later) + t.Logf("%s: deletion committed between initiation and completion survived", instance) + }}) +} + +func TestAPILocalTaggingAlwaysAdvancesRevision(t *testing.T) { + ExecObjectLayerAPITest(ExecObjectLayerAPITestArgs{t: t, objAPITest: func(obj ObjectLayer, instance, bucket string, router http.Handler, cred auth.Credentials, t *testing.T) { + defer r5Capacity(obj.(*erasureServerPools))() + if _, err := globalBucketMetadataSys.Update(t.Context(), bucket, bucketVersioningConfig, enabledBucketVersioningConfig); err != nil { + t.Fatal(err) + } + oi, err := obj.PutObject(t.Context(), bucket, "local-tags", mustGetPutObjReader(t, strings.NewReader("data"), 4, "", ""), ObjectOptions{Versioned: true}) + if err != nil { + t.Fatal(err) + } + path := "/" + bucket + "/local-tags?tagging&versionId=" + oi.VersionID + for n, method := range []string{http.MethodPut, http.MethodDelete, http.MethodDelete, http.MethodPut} { + body := "" + want := "" + status := http.StatusNoContent + if method == http.MethodPut { + status = http.StatusOK + body = "" + if n == 0 { + want = "key=local" + body = "keylocal" + } + } + before := time.Now().UTC() + w := r5Request(t, router, cred, method, path, body, nil) + if w.Code != status { + t.Fatalf("%s: %d %s", method, w.Code, w.Body.String()) + } + now, err := obj.GetObjectInfo(t.Context(), bucket, "local-tags", ObjectOptions{VersionID: oi.VersionID}) + if err != nil { + t.Fatal(err) + } + stamp, err := time.Parse(time.RFC3339Nano, now.UserDefined[r5TagStamp]) + if err != nil || stamp.Before(before) || now.UserTags != want { + t.Fatalf("local %s: tags=%q timestamp=%q err=%v", method, now.UserTags, now.UserDefined[r5TagStamp], err) + } + if !now.ModTime.Equal(oi.ModTime) { + t.Fatal("tagging changed the object's data modification time") + } + t.Logf("%s mutation %d persisted tags=%q timestamp=%s", instance, n, now.UserTags, stamp) + } + // Ordinary COPY with empty REPLACE has the same local-deletion semantics. + before := time.Now().UTC() + headers := map[string]string{xhttp.AmzCopySource: "/" + bucket + "/local-tags?versionId=" + oi.VersionID, xhttp.AmzMetadataDirective: "REPLACE", xhttp.AmzTagDirective: "REPLACE"} + w := r5Request(t, router, cred, http.MethodPut, "/"+bucket+"/copied-empty", "", headers) + if w.Code != http.StatusOK { + t.Fatalf("copy: %d %s", w.Code, w.Body.String()) + } + copied, err := obj.GetObjectInfo(t.Context(), bucket, "copied-empty", ObjectOptions{}) + if err != nil { + t.Fatal(err) + } + stamp, err := time.Parse(time.RFC3339Nano, copied.UserDefined[r5TagStamp]) + if err != nil || stamp.Before(before) || copied.UserTags != "" { + t.Fatalf("local COPY: %v %+v", err, copied) + } + }}) +} + +func TestTaggingTimestampWire(t *testing.T) { + now := time.Now().UTC() + for _, tag := range []string{"", "key=value"} { + for _, stamp := range []string{"", now.Format(time.RFC3339Nano), "invalid"} { + t.Run(fmt.Sprintf("%s/%s", tag, stamp), func(t *testing.T) { + source := ObjectInfo{ModTime: now.Add(-time.Hour), UserTags: tag, UserDefined: map[string]string{}} + if stamp != "" { + source.UserDefined[r5TagStamp] = stamp + } + opts, _, err := putReplicationOpts(t.Context(), "", source) + if stamp == "invalid" { + if err == nil { + t.Fatal("invalid timestamp accepted") + } + return + } + if err != nil { + t.Fatal(err) + } + want := stamp + if want == "" && tag != "" { + want = source.ModTime.Format(time.RFC3339Nano) + } + if got := opts.Header().Get(xhttp.MinIOSourceTaggingTimestamp); got != want { + t.Fatalf("wire timestamp=%q want %q", got, want) + } + }) + } + } +} + +func TestAPIPoolsTaggingReplicaDeletion(t *testing.T) { + z, bucket := consistencyPools(t) + defer r5Capacity(z)() + if err := newTestConfig(globalMinioDefaultRegion, z); err != nil { + t.Fatal(err) + } + if _, err := globalBucketMetadataSys.Update(t.Context(), bucket, bucketVersioningConfig, enabledBucketVersioningConfig); err != nil { + t.Fatal(err) + } + router := initTestAPIEndPoints(z, nil) + base := time.Now().UTC().Add(-5 * time.Hour) + for _, op := range []string{"copy", "copy-default", "put", "multipart"} { + for _, kind := range []string{"uuid", "null"} { + t.Run(op+"/"+kind, func(t *testing.T) { + vid := mustGetUUID() + if kind == "null" { + vid = nullVersionID + } + name := "pool-tags-" + op + "-" + kind + var source ObjectInfo + for pool := range 2 { + tag := "key=stale-pool" + ts := base + if pool == 1 { + tag = "" + ts = base.Add(3 * time.Hour) + } + source = putConsistencyObject(t, z, bucket, name, pool, "data", ObjectOptions{Versioned: true, VersionID: vid, MTime: base, UserDefined: map[string]string{xhttp.AmzObjectTagging: tag, r5TagStamp: ts.Format(time.RFC3339Nano)}}) + } + // Clear all copies through the signed local handler, then replay a stale incoming state. + req := r5Request(t, router, globalActiveCred, http.MethodDelete, "/"+bucket+"/"+name+"?tagging&versionId="+vid, "", nil) + if req.Code != http.StatusNoContent { + t.Fatalf("DELETE: %d %s", req.Code, req.Body.String()) + } + current, err := z.GetObjectInfo(t.Context(), bucket, name, ObjectOptions{VersionID: vid}) + if err != nil { + t.Fatal(err) + } + deletedAt := current.UserDefined[r5TagStamp] + for pool := range 2 { + r5Stored(t, z.serverPools[pool], bucket, name, vid, "", deletedAt) + } + source.VersionID = vid + source.UserTags = "key=delayed" + source.UserDefined = map[string]string{r5TagStamp: base.Add(time.Hour).Format(time.RFC3339Nano)} + r5Receive(t, z, router, globalActiveCred, bucket, op, source, source.UserDefined[r5TagStamp], nil) + // The addressed version must remain readable through normal routing. + r5Stored(t, z, bucket, name, vid, "", deletedAt) + // Existing duplicate suppression may leave both identical tombstones; any retained copy must be correct. + for pool := range 2 { + got, err := z.serverPools[pool].GetObjectInfo(t.Context(), bucket, name, ObjectOptions{VersionID: vid}) + if isErrVersionNotFound(err) { + continue + } + if err != nil { + t.Fatal(err) + } + if got.UserTags != "" || got.UserDefined[r5TagStamp] != deletedAt { + t.Fatalf("pool %d: tags=%q stamp=%q want deletion %q", pool, got.UserTags, got.UserDefined[r5TagStamp], deletedAt) + } + } + }) + } + } +} + +func TestAPITaggingSSECRotationPreservesDeletionRevision(t *testing.T) { + ExecObjectLayerAPITest(ExecObjectLayerAPITestArgs{t: t, objAPITest: func(obj ObjectLayer, instance, bucket string, router http.Handler, cred auth.Credentials, t *testing.T) { + defer r5Capacity(obj.(*erasureServerPools))() + oldTLS := globalIsTLS + globalIsTLS = true + defer func() { globalIsTLS = oldTLS }() + if _, err := globalBucketMetadataSys.Update(t.Context(), bucket, bucketVersioningConfig, enabledBucketVersioningConfig); err != nil { + t.Fatal(err) + } + keys := [][]byte{[]byte(strings.Repeat("a", 32)), []byte(strings.Repeat("b", 32)), []byte(strings.Repeat("c", 32)), []byte(strings.Repeat("d", 32))} + const name = "tag-rotation" + putCopyChecksumSource(t, router, cred, bucket, name, []byte("data"), ssecKeyHeaders(keys[0], false)) + oi, err := obj.GetObjectInfo(t.Context(), bucket, name, ObjectOptions{}) + if err != nil { + t.Fatal(err) + } + base := time.Now().UTC().Add(-time.Hour) + for i, event := range []struct { + tags string + delta int + wantTags string + wantDelta int + }{ + {"key=live", 1, "key=live", 1}, {"", 3, "", 3}, {"key=stale", 2, "", 3}, + } { + h := ssecKeyHeaders(keys[i], true) + maps.Copy(h, ssecKeyHeaders(keys[i+1], false)) + h[xhttp.AmzObjectTagging] = event.tags + h[xhttp.AmzTagDirective] = "REPLACE" + h[xhttp.MinIOSourceTaggingTimestamp] = base.Add(time.Duration(event.delta) * time.Minute).Format(time.RFC3339Nano) + sendReplicaLockCopy(t, router, cred, bucket, name, oi.VersionID, h) + r5Stored(t, obj, bucket, name, oi.VersionID, event.wantTags, base.Add(time.Duration(event.wantDelta)*time.Minute).Format(time.RFC3339Nano)) + } + get := r5Request(t, router, cred, http.MethodGet, "/"+bucket+"/"+name+"?versionId="+oi.VersionID, "", ssecKeyHeaders(keys[3], false)) + if get.Code != http.StatusOK || get.Body.String() != "data" { + t.Fatalf("%s GET after rotations: %d %q", instance, get.Code, get.Body.String()) + } + }}) +} + +func TestTaggingRepeatedValueNeedsRevisionDelivery(t *testing.T) { + for _, tag := range []string{"", "key=same"} { + now := time.Now().UTC() + source := ObjectInfo{ModTime: now, UserTags: tag, UserDefined: map[string]string{r5TagStamp: now.Add(time.Hour).Format(time.RFC3339Nano)}} + target := minio.ObjectInfo{LastModified: now} + if tag != "" { + target.UserTags = map[string]string{"key": "same"} + target.UserTagCount = 1 + } + if got := getReplicationAction(source, target, replication.MetadataReplicationType); got != replicateMetadata { + t.Errorf("equal tags %q suppress a newer revision: got %s; an intervening delayed deletion can win", tag, got) + } + } +} + +func TestTaggingProductionCopyWireShape(t *testing.T) { + got := make(chan http.Header, 1) + peer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + got <- r.Header.Clone() + w.Header().Set(xhttp.ContentType, "application/xml") + w.Write([]byte(`2026-09-15T01:00:00Z"abc"`)) + })) + defer peer.Close() + c, err := minio.New(strings.TrimPrefix(peer.URL, "http://"), &minio.Options{Region: "us-east-1", MaxRetries: 1}) + if err != nil { + t.Fatal(err) + } + now := time.Now().UTC() + oi := ObjectInfo{Name: "object", ModTime: now, ETag: "abc", VersionID: mustGetUUID()} + core := minio.Core{Client: c} + _, err = core.CopyObject(t.Context(), "bucket", "object", "bucket", "object", getCopyObjMetadata(oi, ""), minio.CopySrcOptions{VersionID: oi.VersionID}, minio.PutObjectOptions{Internal: minio.AdvancedPutOptions{SourceVersionID: oi.VersionID, ReplicationRequest: true, TaggingTimestamp: now}}) + if err != nil { + t.Fatal(err) + } + h := <-got + t.Logf("SDK wire: metadata-directive=%q tagging-directive=%q tagging=%q time=%q", h.Get(xhttp.AmzMetadataDirective), h.Get(xhttp.AmzTagDirective), h.Get(xhttp.AmzObjectTagging), h.Get(xhttp.MinIOSourceTaggingTimestamp)) + if h.Get(xhttp.AmzMetadataDirective) != "" || h.Get(xhttp.AmzTagDirective) != "REPLACE" || h.Get(xhttp.MinIOSourceTaggingTimestamp) != now.Format(time.RFC3339Nano) { + t.Fatalf("unexpected SDK wire: %v", h) + } +} + +func TestLocalTaggingCommitCannotRegressRevision(t *testing.T) { + z, bucket := consistencyPools(t) + incoming := "2026-09-15T01:00:00Z" + newer := "2026-09-15T02:00:00Z" + newest := "2026-09-15T03:00:00Z" + vid := mustGetUUID() + name := "inverted-local-tags" + for pool := range 2 { + putConsistencyObject(t, z, bucket, name, pool, "data", ObjectOptions{Versioned: true, VersionID: vid, UserDefined: map[string]string{r5TagStamp: []string{newer, newest}[pool]}}) + } + oi, err := z.PutObjectTags(t.Context(), bucket, name, "key=after-delete", ObjectOptions{VersionID: vid, UserDefined: map[string]string{r5TagStamp: incoming}}) + if err != nil { + t.Fatal(err) + } + want := "2026-09-15T03:00:00.000000001Z" + for pool := range 2 { + r5Stored(t, z.serverPools[pool], bucket, name, vid, "key=after-delete", want) + } + if oi.UserDefined[r5TagStamp] != want { + t.Fatalf("response stamp=%q want %q", oi.UserDefined[r5TagStamp], want) + } + // The single-set guard also applies when the pools dispatcher is bypassed. + _, err = z.serverPools[0].PutObjectTags(t.Context(), bucket, name, "", ObjectOptions{VersionID: vid, UserDefined: map[string]string{r5TagStamp: incoming}}) + if err != nil { + t.Fatal(err) + } + r5Stored(t, z.serverPools[0], bucket, name, vid, "", "2026-09-15T03:00:00.000000002Z") +} + +func TestTaggingReplicaContentDuplicateGuard(t *testing.T) { + stamp := time.Date(2026, 9, 15, 1, 0, 0, 0, time.UTC) + for _, tc := range []struct { + name string + delta int + stored string + trusted, replica bool + ifMatch, ifNone string + wantSkip bool + }{ + {name: "newer", delta: 1, trusted: true, replica: true}, + {name: "equal", trusted: true, replica: true, wantSkip: true}, + {name: "older", delta: -1, trusted: true, replica: true, wantSkip: true}, + {name: "invalid-stored", stored: "invalid", delta: 1, trusted: true, replica: true}, + {name: "untrusted", delta: 1, wantSkip: true}, + {name: "marker-only", delta: 1, trusted: true, wantSkip: true}, + {name: "if-match-fails", delta: 1, trusted: true, replica: true, ifMatch: "other", wantSkip: true}, + {name: "if-none-match-fails", delta: 1, trusted: true, replica: true, ifNone: "etag", wantSkip: true}, + {name: "if-match-passes", delta: 1, trusted: true, replica: true, ifMatch: "etag"}, + } { + t.Run(tc.name, func(t *testing.T) { + ctx := withReplicationTrust(t.Context(), tc.trusted, tc.replica) + r := httptest.NewRequest(http.MethodPut, "/bucket/object", nil).WithContext(ctx) + if tc.ifMatch != "" { + r.Header.Set(xhttp.IfMatch, tc.ifMatch) + } + if tc.ifNone != "" { + r.Header.Set(xhttp.IfNoneMatch, tc.ifNone) + } + stored := tc.stored + if stored == "" { + stored = stamp.Format(time.RFC3339Nano) + } + oi := ObjectInfo{ModTime: stamp, VersionID: mustGetUUID(), ETag: "etag", UserDefined: map[string]string{r5TagStamp: stored}} + opts := ObjectOptions{VersionID: oi.VersionID, PreserveETag: oi.ETag, ReplicationRequest: tc.trusted, ReplicationSourceTaggingTimestamp: stamp.Add(time.Duration(tc.delta) * time.Second)} + if skip := checkPreconditionsPUT(ctx, httptest.NewRecorder(), r, oi, opts); skip != tc.wantSkip { + t.Fatalf("skip=%v, want %v", skip, tc.wantSkip) + } + }) + } +} + +func TestAPITaggingUnqualifiedCopyOrdering(t *testing.T) { + ExecObjectLayerAPITest(ExecObjectLayerAPITestArgs{t: t, objAPITest: func(obj ObjectLayer, instance, bucket string, router http.Handler, cred auth.Credentials, t *testing.T) { + defer r5Capacity(obj.(*erasureServerPools))() + stamp := time.Now().UTC().Add(-time.Hour) + oi, err := obj.PutObject(t.Context(), bucket, "unqualified-tags", mustGetPutObjReader(t, strings.NewReader("data"), 4, "", ""), ObjectOptions{UserDefined: map[string]string{xhttp.AmzObjectTagging: "key=stored", r5TagStamp: stamp.Format(time.RFC3339Nano)}}) + if err != nil { + t.Fatal(err) + } + source := oi + source.UserTags = "" + source.UserDefined = maps.Clone(oi.UserDefined) + r5Receive(t, obj, router, cred, bucket, "copy", source, stamp.Format(time.RFC3339Nano), nil) + r5Stored(t, obj, bucket, oi.Name, "", "key=stored", stamp.Format(time.RFC3339Nano)) + later := stamp.Add(time.Minute).Format(time.RFC3339Nano) + r5Receive(t, obj, router, cred, bucket, "copy-default", source, later, nil) + r5Stored(t, obj, bucket, oi.Name, "", "", later) + source.UserTags = "key=delayed" + r5Receive(t, obj, router, cred, bucket, "copy-default", source, stamp.Format(time.RFC3339Nano), nil) + r5Stored(t, obj, bucket, oi.Name, "", "", later) + t.Logf("%s: unqualified COPY keeps stored ties and ordered deletion", instance) + }}) +} diff --git a/cmd/replication-tagging-sender_test.go b/cmd/replication-tagging-sender_test.go new file mode 100644 index 000000000..e6ced526f --- /dev/null +++ b/cmd/replication-tagging-sender_test.go @@ -0,0 +1,154 @@ +// Copyright (c) 2026 Feng Ruohang +// +// This file is part of Silo Object Storage stack +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . + +package cmd + +import ( + "net/http" + "net/http/httptest" + "strings" + "sync/atomic" + "testing" + "time" + + "github.com/minio/madmin-go/v3" + "github.com/minio/minio-go/v7" + "github.com/minio/minio/internal/auth" + "github.com/minio/minio/internal/bucket/replication" + xhttp "github.com/minio/minio/internal/http" + "github.com/minio/minio/internal/once" +) + +func r5ReplicationFixture(t *testing.T, obj ObjectLayer, bucket string, client *minio.Client) (chan ReplicationWorkerOperation, func()) { + t.Helper() + const arn = "arn:minio:replication::af470089-d354-4473-934c-9e1f52f6da89:bucket" + target := &TargetClient{Client: client, ARN: arn, Bucket: bucket} + globalBucketTargetSys.arnRemotesMap[arn] = arnTarget{Client: target, lastRefresh: UTCNow()} + globalBucketTargetSys.targetsMap[bucket] = []madmin.BucketTarget{{Arn: arn, TargetBucket: bucket}} + meta, err := globalBucketMetadataSys.Get(bucket) + if err != nil { + t.Fatal(err) + } + cfg := configs[0] + cfg.RoleArn = arn + meta.replicationConfig = &cfg + globalBucketMetadataSys.Set(bucket, meta) + worker := make(chan ReplicationWorkerOperation, 10) + previous := globalReplicationPool + globalReplicationPool = once.NewSingleton[ReplicationPool]() + globalReplicationPool.Set(&ReplicationPool{ctx: t.Context(), objLayer: obj, workers: []chan ReplicationWorkerOperation{worker}, stats: globalReplicationStats.Load(), mrfSaveCh: make(chan MRFReplicateEntry, 10)}) + return worker, func() { globalReplicationPool = previous } +} + +func TestTaggingReplicationSenderRetryAndAcknowledgment(t *testing.T) { + ExecObjectLayerAPITest(ExecObjectLayerAPITestArgs{t: t, objAPITest: func(obj ObjectLayer, instance, bucket string, router http.Handler, cred auth.Credentials, t *testing.T) { + defer r5Capacity(obj.(*erasureServerPools))() + if _, err := globalBucketMetadataSys.Update(t.Context(), bucket, bucketVersioningConfig, enabledBucketVersioningConfig); err != nil { + t.Fatal(err) + } + const name = "tagging-old-queue" + oi, err := obj.PutObject(t.Context(), bucket, name, mustGetPutObjReader(t, strings.NewReader("data"), 4, "", ""), ObjectOptions{Versioned: true}) + if err != nil { + t.Fatal(err) + } + requests := make(chan http.Header, 4) + var attempts atomic.Int32 + peer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set(xhttp.AmzVersionID, oi.VersionID) + w.Header().Set(xhttp.ETag, "\""+oi.ETag+"\"") + w.Header().Set(xhttp.LastModified, oi.ModTime.Format(http.TimeFormat)) + w.Header().Set(xhttp.ContentType, oi.ContentType) + if r.Method == http.MethodHead { + w.Header().Set(xhttp.ContentLength, "4") + w.WriteHeader(http.StatusOK) + return + } + requests <- r.Header.Clone() + w.Header().Set(xhttp.ContentType, "application/xml") + if attempts.Add(1) == 1 { + w.WriteHeader(http.StatusServiceUnavailable) + w.Write([]byte(`SlowDownretry fixture`)) + return + } + w.Write([]byte("" + oi.ModTime.Format(time.RFC3339Nano) + "\"" + oi.ETag + "\"")) + })) + defer peer.Close() + client, err := minio.New(strings.TrimPrefix(peer.URL, "http://"), &minio.Options{Region: "us-east-1", MaxRetries: 1}) + if err != nil { + t.Fatal(err) + } + worker, cleanup := r5ReplicationFixture(t, obj, bucket, client) + defer cleanup() + body := `keyqueued` + w := r5Request(t, router, cred, http.MethodPut, "/"+bucket+"/"+name+"?tagging&versionId="+oi.VersionID, body, nil) + if w.Code != http.StatusOK || len(worker) != 1 { + t.Fatalf("tagging PUT: %d %s queued=%d", w.Code, w.Body.String(), len(worker)) + } + old := (<-worker).(ReplicateObjectInfo) + // Delete through the actual handler before processing the old task. + w = r5Request(t, router, cred, http.MethodDelete, "/"+bucket+"/"+name+"?tagging&versionId="+oi.VersionID, "", nil) + if w.Code != http.StatusNoContent || len(worker) != 1 { + t.Fatalf("tagging DELETE: %d %s queued=%d", w.Code, w.Body.String(), len(worker)) + } + deleted, err := obj.GetObjectInfo(t.Context(), bucket, name, ObjectOptions{VersionID: oi.VersionID}) + if err != nil { + t.Fatal(err) + } + stamp := deleted.UserDefined[r5TagStamp] + for attempt := 0; attempt < 2; attempt++ { + result := replicateObject(t.Context(), old, obj) + want := replication.Failed + if attempt == 1 { + want = replication.Completed + } + if result.ReplicationStatus() != want { + t.Fatalf("attempt %d result=%+v want %s", attempt, result, want) + } + if len(result.Targets) != 1 || result.Targets[0].ReplicationAction != replicateMetadata || (result.Targets[0].Err != nil) != (attempt == 0) { + t.Fatalf("attempt %d reported wrong action/error: %+v", attempt, result) + } + r5Stored(t, obj, bucket, name, oi.VersionID, "", stamp) + select { + case h := <-requests: + if h.Get(xhttp.MinIOSourceTaggingTimestamp) != stamp || h.Get(xhttp.AmzObjectTagging) != "" || h.Get(xhttp.AmzTagDirective) != "REPLACE" || h.Get(xhttp.AmzMetadataDirective) != "" { + t.Fatalf("sender did not carry current deletion: %v", h) + } + t.Logf("%s attempt %d sent tags=%q timestamp=%s status=%s", instance, attempt, h.Get(xhttp.AmzObjectTagging), stamp, want) + default: + t.Fatal("no metadata COPY sent for same-empty target") + } + } + // With a real outgoing rule enabled, a signed incoming replica COPY + // must not queue another outgoing event and create a feedback loop. + before := len(worker) + r5Receive(t, obj, router, cred, bucket, "copy", deleted, stamp, nil) + if len(worker) != before { + t.Fatal("incoming replica COPY scheduled another outgoing event") + } + // The metadata sender must fail malformed stored revisions before COPY, + // just as the full retransmission option builder does. + _, err = obj.PutObjectTags(t.Context(), bucket, name, "", ObjectOptions{VersionID: oi.VersionID, UserDefined: map[string]string{r5TagStamp: "invalid"}}) + if err != nil { + t.Fatal(err) + } + target := globalBucketTargetSys.GetRemoteTargetClient(bucket, globalBucketTargetSys.targetsMap[bucket][0].Arn) + invalid := old.replicateAll(t.Context(), obj, target) + if invalid.ReplicationStatus != replication.Failed || invalid.Err == nil || len(requests) != 0 { + t.Fatalf("invalid timestamp was not rejected before send: %+v", invalid) + } + }}) +} diff --git a/docs/investigations/r5/baseline.md b/docs/investigations/r5/baseline.md new file mode 100644 index 000000000..7211fd88c --- /dev/null +++ b/docs/investigations/r5/baseline.md @@ -0,0 +1,17 @@ +# R5 investigation baseline + +- Worktree: `/Users/vonng/.codex/worktrees/77ad/silo` +- HEAD: `9ebe81c1b3611f9cc73e676b5b741c2be62c467a`; GitHub main checked live on 2026-09-15. +- Branch: `codex/r5-tag-deletion-ordering`. +- WORKFLOW: `/Users/vonng/tmp/silo-r4-r8-20260915-01a0a5ab/WORKFLOW.md` read completely. +- This isolated worktree has no AGENTS.md. Read `/Users/vonng/pgsty/silo/AGENTS.md`: PGSTY supported stack, minimal compatible changes, separate local/merge/release gates. +- Existing tag storage reconciliation is already in HEAD; inspect and reuse it. +- No open R5 PR in live `gh pr list`; unrelated open PRs #184 and #187 belong to R6/R7. +- Parent reproduction: `/Users/vonng/tmp/silo-r4-r8-20260915-01a0a5ab/baseline-evidence/r5-handler.log`. +- Current reproduction overlay and raw output: `/Users/vonng/tmp/silo-r5-20260915-77ad/`. +- Claude Code actual version: 2.1.270 at `/opt/homebrew/bin/claude`. Required model `claude-opus-5`, effort `max`; model identity must be checked in assistant messages. +- Toolchain: go1.27.1 darwin/arm64. Targeted tests use GOMAXPROCS=2 and -p 1 to share the host. + +## Ownership + +R4 owns `cmd/object-api-options.go` KMS common-field preservation and option tests. R5 does not edit that file. R5 owns tag state generation, wire propagation, COPY/PUT/multipart persistence and ordered replay tests. Coordination requested through parent while R4 actual task ID is pending. diff --git a/docs/investigations/r5/consensus.md b/docs/investigations/r5/consensus.md new file mode 100644 index 000000000..913ef144e --- /dev/null +++ b/docs/investigations/r5/consensus.md @@ -0,0 +1,32 @@ +# R5 plan consensus + +Date: 2026-09-15. Research base: `9ebe81c1b3611f9cc73e676b5b741c2be62c467a`. + +## Accepted plan + +- Version: **v2**, `plan-v2.md`. +- SHA256: `5a782acf3f285b23d1ae43a73481c4eb772a9a6d917fc5a550ecfc7cbf7446ca`. +- Actual reviewer: **claude-opus-5**, explicitly invoked **--effort max** through `/opt/homebrew/bin/claude` 2.1.270. All assistant messages in both reviews identify this model. The auxiliary Haiku usage in CLI bookkeeping is separately retained in modelUsage and is not the reviewer. +- Actual result: **APPROVE_WITH_NONBLOCKING_NOTES; 0 blocking items** in opus-v2-review.md. +- Codex accepts this exact v2 and its bounded per-hop scope. Plan hash was checked locally immediately before implementation. Opus's read-only tools did not run hashing; the original caveat is retained in raw review. +- Workflow permission: after this written consensus, local implementation and verification proceed without another user approval. No main merge, remote push, release, deployment or production state rewrite. + +## Discussion and resolved differences + +V1 was REQUEST_CHANGES with five blockers. See opus-v1-response.md for individual treatment and source evidence. V2 resolves all five. Opus explicitly withdrew its empty-only transfer proposal after the same-value re-addition counterexample, corrected its KMS COPY statement after inspecting bucket-default/auto encryption, and accepted that per-pool-only local clock guards are insufficient for ordinary source reads. + +## Nonblocking notes accepted during implementation + +- Extra metadata I/O occurs on scheduled metadata/heal/existing-object work with a recorded revision; ordinary object replication dispatches straight to full transfer. Completed scanner gates and incoming replication suppression avoid a feedback loop. Test the incoming no-reschedule decision. +- Pin unchanged object ModTime for local tagging changes. +- Keep a single-set monotonic guard and one uniform multi-pool candidate; direct-to-set writes outside the pool lock can transiently differ and re-converge on the next pooled mutation. +- Check actual failed COPY status/action and subsequent retry. Malformed timestamps fail both PUT and metadata COPY construction. +- Preserve scope limitations: tag-filter target selection, historical missing revisions, arbitrary unversioned content overwrites, and real multi-site/host-clock skew are not solved or production-accepted here. + +## R4 dependency + +Reuse reviewed local R4 commit `dbcf8dec589deb5d91e17d295cb70997635f5b55` on this isolated branch before implementation. Its only production change is the KMS options field, already examined against the provided patch SHA256 `2d4806d986bbd94ba4bc3951f3aeee48401ee1921c28ded0988fa09ca76ca26f`. R5 does not reimplement or modify that field. This makes R4+R5 tests run on actual combined source, with R5's eventual commit measured against the R4 dependency. + +## Raw records + +`/Users/vonng/tmp/silo-r5-20260915-77ad/opus-v1.jsonl`, `opus-v1.stderr.log`, `opus-v1-request.json`, and matching `opus-v2.*`. In-repository review texts, prompts and metadata preserve plan hashes, model identity, usage and verdicts. V1 failure is not treated as approval. diff --git a/docs/investigations/r5/dependency-handoff.json b/docs/investigations/r5/dependency-handoff.json new file mode 100644 index 000000000..8c97d6ee2 --- /dev/null +++ b/docs/investigations/r5/dependency-handoff.json @@ -0,0 +1,35 @@ +{ + "tested_dependency": "dbcf8dec589deb5d91e17d295cb70997635f5b55", + "merged_dependency": "af2b1794d38d9e70e1d2c3ee692426e4b6cab4bd", + "pr": "https://github.com/pgsty/silo/pull/193", + "files": { + "cmd/object-api-options.go": { + "tested_sha256": "25e2e9484fafd94d1b2c857b94373758e481893ad93fb1a063edf7746277accc", + "merged_sha256": "25e2e9484fafd94d1b2c857b94373758e481893ad93fb1a063edf7746277accc", + "package_body_sha256": "23fa2a25307bf1e41b665217a3f39aa7a8b860686a54209092fa9c0f1f13243a", + "package_body_identical": true + }, + "cmd/object-api-options-replication_test.go": { + "tested_sha256": "1ea2a060987e32a4c76fce96ee974df475944c2d6ab482a4893e33daf7bca849", + "merged_sha256": "c21fc8889a079085d9a882499a1cbe868278a3517580651f3bed1102e2a6aef8", + "package_body_sha256": "1a57a47bdd370042fa0f0d2d90efe447abedee9b9ef48a938d4bed631d83ec0b", + "package_body_identical": true + }, + "cmd/object-copy-replication-tagging_test.go": { + "tested_sha256": "5437a77e68736b4ce69de9c777675251fef24b0352dfe30bd8a836fc7ee810e3", + "merged_sha256": "73f066ed7258d430f078ecc90e551ece878bd3d4672bc762094d434ff8fec23d", + "package_body_sha256": "ef77de91cd91d1bd1c3cb10e4fee171c724c4f548749a23c7e48dcfcfb6a1585", + "package_body_identical": true + } + }, + "other_changes": [ + "cmd/object-api-options-replication_test.go", + "cmd/object-copy-replication-tagging_test.go", + "docs/investigations/r4/implementation-review.md", + "docs/investigations/r4/implementation-review.metadata.json", + "docs/investigations/r4/merge-verification.json", + "docs/investigations/r4/merge-verification.md", + "docs/investigations/r4/verification.md" + ], + "scope": "R5 local branch will rebase onto this exact R4 merge; no R5 push or merge." +} diff --git a/docs/investigations/r5/evidence-manifest.json b/docs/investigations/r5/evidence-manifest.json new file mode 100644 index 000000000..8d9e0558a --- /dev/null +++ b/docs/investigations/r5/evidence-manifest.json @@ -0,0 +1,340 @@ +{ + "raw_files": [ + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/baseline-ack.log", + "bytes": 607, + "sha256": "ef698b8f46c850928057a31bf4e92f64f9a0dcebb8753bab00e9dcef2b44ffb6" + }, + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/baseline-extended.log", + "bytes": 2764, + "sha256": "a52bb6644b82a1986c233deeb9fb7b6cd3f4975337aa11446a1b27c459355db9" + }, + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/baseline-matrix.log", + "bytes": 20974, + "sha256": "5348c2ae4a20238ae50f70bcaea3aa55169b3479f60eab522692bdabe3420ab0" + }, + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/baseline-pools-rotation.log", + "bytes": 2249, + "sha256": "1d47acbe4d759b0f413f90589ff51b1f844f1885885d45d11c72a6295f5a4653" + }, + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/baseline-pools.log", + "bytes": 358, + "sha256": "56dd5efc0c833070576c4c7e2cb2abca8a82380060596be58871067526557c32" + }, + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/baseline-production/cmd/bucket-replication.go", + "bytes": 137081, + "sha256": "1e4d27c9eb2bff51eb28460d167faa3279b541d43c77ce35ad010dcab58bf7c5" + }, + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/baseline-production/cmd/erasure-object.go", + "bytes": 84089, + "sha256": "1012ae265e2453db125c6f2d16f866c72760b58d61c18f79dff4a551c208e3ce" + }, + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/baseline-production/cmd/erasure-server-pool-consistency.go", + "bytes": 14240, + "sha256": "78e63ca1117ea2d3e3e93864a4365dfa9bb303b4707de5087bdc144d0502a0db" + }, + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/baseline-production/cmd/erasure-server-pool.go", + "bytes": 99706, + "sha256": "2bfe0899fe3e42840fa4f078887184e4d7d2332d780ce63c31c9495af7056406" + }, + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/baseline-production/cmd/object-handlers-common.go", + "bytes": 19143, + "sha256": "00bf8409d25f9cf6a098a90f9e0bd7d2b237be7adc12622f0b9a66146af0a828" + }, + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/baseline-production/cmd/object-handlers.go", + "bytes": 146644, + "sha256": "27d47a17e12088f41b35de51da875f28a89a7e821bc27d0f88e6064ec386c993" + }, + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/baseline-production/cmd/object-multipart-handlers.go", + "bytes": 48935, + "sha256": "c818ce72d9115ed4f9cbe51571e7737957010a3934a4d000000895e45100edc7" + }, + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/baseline-production/empty_test.go", + "bytes": 12, + "sha256": "9c78355c4da37df8f708f143fe19173dc146adcd99d1636594d265c5407755bf" + }, + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/baseline-production-overlay.json", + "bytes": 1508, + "sha256": "874d728abc9cb67c5db17ef4c3ce875d789ae4b5000dcbb593fe8dcd47094533" + }, + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/baseline-related-suite.log", + "bytes": 3521, + "sha256": "68b23b21c4de8b8252689f841376dec990257d929f0277d3087f467a246728e8" + }, + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/baseline-resync.log", + "bytes": 116, + "sha256": "a620c0ecd112aceac9fd17b989b6283604a3866928ed51e9ed0b16079284fcbf" + }, + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/baseline.log", + "bytes": 1629, + "sha256": "29d80ad52b0302d4eb4993db7a63c88bbc920923afa9775634d3c2fe000c066f" + }, + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/baseline_test.go", + "bytes": 18743, + "sha256": "ce609764fa53f7a86e77dc8f2c00c6d8b878c4c9b6f885fb2618bf8f932cad04" + }, + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/build-result.json", + "bytes": 729, + "sha256": "2df4873188d94c3745631b6e774e76a7646b3366fab7a3badf7ac1be136f7bd9" + }, + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/cache-reclaim.json", + "bytes": 2482128, + "sha256": "8dc7866b30bfd7fed339a3cd4a2dd40c0ec8f3b471cb004fc14f303a41642ad8" + }, + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/capacity-fixture/cmd/erasure-server-pool-consistency_test.go", + "bytes": 54157, + "sha256": "c3c1bf441e5f97fd2648c5fc9b89cb11679018e349daa4d0eef4ebbfada322db" + }, + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/capacity-fixture/cmd/post-policy_test.go", + "bytes": 33420, + "sha256": "697f8a08dceae481fd1aae7b5e7f3906b55b34fe9928688456eaa943eba79020" + }, + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/capacity-fixture/cmd/test-utils_test.go", + "bytes": 79596, + "sha256": "fb4847b10d3d59c7d62ee79a61d54e8c0bc93d6eb32cb520f5662bb7b52750f5" + }, + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/capacity-fixture-manifest.json", + "bytes": 426, + "sha256": "8940a56a6f46b7e9c236c3a8d39d927735954ae42601ca52c4a190339fb1f21f" + }, + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/capacity-fixture-overlay.json", + "bytes": 368, + "sha256": "3b8586973d426f78145aa25f0c3c9d48faf93678ffe9410fc9aa089cb6167af0" + }, + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/capacity-fixture.patch", + "bytes": 842, + "sha256": "3e3732c7fab2b95b9f2e80a6ee973a588600f84eeb2b74c2f15a09373228247f" + }, + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/capacity-post-manifest.json", + "bytes": 205, + "sha256": "34ae2c860a5cc1a9615d7b410eff781f5f01d54dba65f66edae0f724d3d232c7" + }, + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/capacity-post-overlay.json", + "bytes": 522, + "sha256": "8d0b7594481fa9028fbc4284e1e94cb853828925423d2a7b3cd6f5cbabb82e3c" + }, + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/capacity-post.patch", + "bytes": 358, + "sha256": "f4aa03d4a0a9f0bb220fa3b3b988a8dda1ad7d6764daaeb4e60eb4ee4e996674" + }, + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/discussion-baseline.log", + "bytes": 1029, + "sha256": "9581de36ec9a403d304c32192d17265b1e9c9414c60e9f2cb57321faebbe23dc" + }, + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/final-check-results.json", + "bytes": 939, + "sha256": "6652d2db1ac98d65232e37eda7738972a74f9569ac63b534f1af798ebf19f642" + }, + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/final-post-and-pools.log", + "bytes": 1280, + "sha256": "e7e5fec0761976470eafbf31bcefe4abc241525514f6c3b9a2baa035354144fa" + }, + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/fixed-resync-isolated.log", + "bytes": 116, + "sha256": "bed15b381379998bbe2a0aa1be19c2cfec1b0063886143dbe82918f9652c28ff" + }, + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/fixed-targeted-final.log", + "bytes": 25221, + "sha256": "32283f5d7eea5ce4974fefa0724a1c4de565bb0ef9a0f4fbcad68140bccc32b1" + }, + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/fixed-targeted-latest.log", + "bytes": 32772, + "sha256": "45f362258e21b631cb5ebcd15dec98bd2fa3186f509f18fe216d7cd0ebdb5a9c" + }, + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/fixed-targeted.log", + "bytes": 27709, + "sha256": "a30f7754f90cfade50ed80a066c5e2bd53faf225bd615cd07b9cf1350b2e3139" + }, + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/golangci-lint-serial", + "bytes": 103, + "sha256": "b2a00c2702468165a7851ee3a6addbef9e581833a33492d44ac66d531cfc4fff" + }, + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/implementation-files.json", + "bytes": 936, + "sha256": "7bd1279e1c99da9da4562cda6e0265d36d53a9bb546f10d42c4174c1a34b5c70" + }, + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/implementation-v1.patch", + "bytes": 48399, + "sha256": "8f6f76ee874c43b0827fde272e8a947f118efb1c1af92bf4a02ef88f93554c1b" + }, + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/make-build.log", + "bytes": 55, + "sha256": "6ba9b545236be964861749c72e7609edf12b8f470df30d1ede8fd62f497e629b" + }, + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/make-verifiers-final.log", + "bytes": 217, + "sha256": "d973482061716daa245e7d7162765dec0e6ecd5fee41249d2702a2d8bfce1c32" + }, + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/make-verifiers-serial.log", + "bytes": 410, + "sha256": "e42a5bb55f5c1ebfcf02cebebf6d82cf1ec5a2d74590cdf838deba16dd80bfdf" + }, + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/make-verifiers-success.log", + "bytes": 162, + "sha256": "b4982b6a7302e733c7bec4a5fb36b8ee8865fe95f1595e7079ce7f8455406210" + }, + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/make-verifiers.log", + "bytes": 285, + "sha256": "42f4b147ef4aac45ba91457e7932db30f9b57f285466ee3f10bbaa9a911e5bc7" + }, + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/matrix-overlay.json", + "bytes": 153, + "sha256": "a0fb5eb71ebb2bdb3374813752de5867848454794bcbf302ba0f6d7d4f6c0519" + }, + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/matrix_test.go", + "bytes": 22996, + "sha256": "c9cf527c63800fa045bdf0b8e95d740b81a3d5be3a08c74811fa5c06bed56d4f" + }, + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/opus-implementation-request.json", + "bytes": 909, + "sha256": "9fab15ce1cfb5bad102b1880968e4731a7b5cb02d6d01e6cb2caf8bc9029a150" + }, + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/opus-implementation.jsonl", + "bytes": 1184150, + "sha256": "fef155a7382c8f66f69b7afd5fd94559edcc6f72fc13aeb7ef01319c22c09861" + }, + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/opus-implementation.stderr.log", + "bytes": 0, + "sha256": "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855" + }, + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/opus-v1-request.json", + "bytes": 216, + "sha256": "175e013154c241805e00368f7841d41faf48ee847ff5251aad96ae57831e244a" + }, + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/opus-v1.jsonl", + "bytes": 1017882, + "sha256": "63c00d8362f236a18293e1a637eff3b7c7e38b0bbd11805f75d91e8774efcdb2" + }, + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/opus-v1.stderr.log", + "bytes": 0, + "sha256": "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855" + }, + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/opus-v2-request.json", + "bytes": 216, + "sha256": "ed710eff03d3ebabab277c9e453048097a8649582df4b01f99d0ee0ad3a7c831" + }, + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/opus-v2.jsonl", + "bytes": 414265, + "sha256": "e888fbf38ba7fd49891e0757c18006875d02a6bbb325906a31fc8d98e54d0e39" + }, + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/opus-v2.stderr.log", + "bytes": 0, + "sha256": "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855" + }, + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/overlay.json", + "bytes": 142, + "sha256": "d51934eb99e2b19d149478e090ec327ed2753a5ad2a026c8745b8e2554962a00" + }, + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/related-filter.txt", + "bytes": 5771, + "sha256": "f022bc24ae0fe391ae51a5095db1d2e415a994934327c7508e6c65edc313cc78" + }, + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/related-names.txt", + "bytes": 5767, + "sha256": "369ed4b6742d15e8ab4d790615842304a7178fbdc598e231e9205fc096c0785a" + }, + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/related-suite-capacity-final.log", + "bytes": 137077, + "sha256": "322d4854ba909bd99d5c7740abeeac05eb76cb2d39d2c7541642335ae6a1ffe9" + }, + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/related-suite-capacity.log", + "bytes": 3362, + "sha256": "1e4f1bc6be2b4339a0d9b7a2e951774fc24ec5521be9fcf53b2ce02031f5cdbe" + }, + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/related-suite-rest.log", + "bytes": 172665, + "sha256": "846d10084299a77253153c0eed8546e275c789a0289a4198052773e49a73423f" + }, + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/related-suite.log", + "bytes": 3521, + "sha256": "38e3e8e4b7ae815fce40931009a0d4755601a4f3f7f9f44a569edff024c9239b" + }, + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/sender_test.go", + "bytes": 5140, + "sha256": "a1a58fd6968b41cf6c565d9f63a1d0fa1c907f008f70acfd13c7a6c525376357" + }, + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/silo-version.log", + "bytes": 317, + "sha256": "8ce9c5d15082a78e696aa79f8ec007f72ce969ce6ebd7dab2f7db69b20b51f8b" + }, + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/targeted-race-final.log", + "bytes": 39753, + "sha256": "127cfb73f415bad43e2fd79c05150ab765322dcadb29e687b752b6174d4ad850" + }, + { + "path": "/Users/vonng/tmp/silo-r5-20260915-77ad/verifiers-result.json", + "bytes": 417, + "sha256": "6faea89c420685ccae0642be88ddf86938bb25e24f066fd9e57138d16c9e9856" + } + ], + "binary": { + "path": "/Users/vonng/.codex/worktrees/77ad/silo/silo", + "bytes": 93070802, + "sha256": "dd789126966d4a42bc0a9bcd8b8eab9714e3eadc7a524505a6224ea6d76c750f" + }, + "scope": "Local development build and exact raw verification/review records; no publication or production acceptance." +} diff --git a/docs/investigations/r5/final-implementation-manifest.json b/docs/investigations/r5/final-implementation-manifest.json new file mode 100644 index 000000000..d1d167aeb --- /dev/null +++ b/docs/investigations/r5/final-implementation-manifest.json @@ -0,0 +1,20 @@ +{ + "research_base": "9ebe81c1b3611f9cc73e676b5b741c2be62c467a", + "tested_dependency": "dbcf8dec589deb5d91e17d295cb70997635f5b55", + "reviewed_patch_sha256": "8f6f76ee874c43b0827fde272e8a947f118efb1c1af92bf4a02ef88f93554c1b", + "plan_version": "v2", + "plan_sha256": "5a782acf3f285b23d1ae43a73481c4eb772a9a6d917fc5a550ecfc7cbf7446ca", + "files": { + "cmd/bucket-replication.go": "cbab22ffe316fabc076e7f4a1fa5b1417d07b265ae9dc1b27454689355926d35", + "cmd/erasure-object.go": "4bc848685ea714d88cabbd5d1b8585fbcc06f7b19c775e1a811030e783d0e1a4", + "cmd/erasure-server-pool-consistency.go": "d2736ef6bffbb5c5758eba8df38f8d4ecb888a838ab0de8ad3cf015c051f8ad7", + "cmd/erasure-server-pool.go": "87ad0b25dfa3081d0e63d0073b788614a9c88e2498a2ce0956b93f8a0a03ef53", + "cmd/object-handlers-common.go": "101bd7d7447072d13fed50983b69b562e4725632645e623d7fdd490f388ecdec", + "cmd/object-handlers.go": "61897a260f3f5f660f41edcb50956c60e914ef98f9a987da824f16d78171fde2", + "cmd/object-multipart-handlers.go": "d9622c69c540ab32dd23916e3f534b6886473a98370c9dd17673e69a423b2a7e", + "cmd/replication-tagging-order_test.go": "c8260b4ccf82fa615e1e24b35a07f2d1aacbcf776e5c6f9dadffea4a09ad6ea8", + "cmd/replication-tagging-sender_test.go": "3770a1a48a6efe58fe8127e1e4fdf6bd7cf171e17db20f15222ea2f7b85db1af" + }, + "production_unchanged_after_review": true, + "delivery_dependency": "af2b1794d38d9e70e1d2c3ee692426e4b6cab4bd" +} diff --git a/docs/investigations/r5/implementation-manifest.json b/docs/investigations/r5/implementation-manifest.json new file mode 100644 index 000000000..af39a0b5e --- /dev/null +++ b/docs/investigations/r5/implementation-manifest.json @@ -0,0 +1,17 @@ +{ + "base_commit": "dbcf8dec589deb5d91e17d295cb70997635f5b55", + "plan_version": "v2", + "plan_sha256": "5a782acf3f285b23d1ae43a73481c4eb772a9a6d917fc5a550ecfc7cbf7446ca", + "patch_sha256": "8f6f76ee874c43b0827fde272e8a947f118efb1c1af92bf4a02ef88f93554c1b", + "files": { + "cmd/bucket-replication.go": "cbab22ffe316fabc076e7f4a1fa5b1417d07b265ae9dc1b27454689355926d35", + "cmd/erasure-object.go": "4bc848685ea714d88cabbd5d1b8585fbcc06f7b19c775e1a811030e783d0e1a4", + "cmd/erasure-server-pool-consistency.go": "d2736ef6bffbb5c5758eba8df38f8d4ecb888a838ab0de8ad3cf015c051f8ad7", + "cmd/erasure-server-pool.go": "87ad0b25dfa3081d0e63d0073b788614a9c88e2498a2ce0956b93f8a0a03ef53", + "cmd/object-handlers-common.go": "101bd7d7447072d13fed50983b69b562e4725632645e623d7fdd490f388ecdec", + "cmd/object-handlers.go": "61897a260f3f5f660f41edcb50956c60e914ef98f9a987da824f16d78171fde2", + "cmd/object-multipart-handlers.go": "d9622c69c540ab32dd23916e3f534b6886473a98370c9dd17673e69a423b2a7e", + "cmd/replication-tagging-order_test.go": "64d6dfe3436970caeafcb914157bdedac5982a2105fe72c1753a8d68cf7ed6ef", + "cmd/replication-tagging-sender_test.go": "3770a1a48a6efe58fe8127e1e4fdf6bd7cf171e17db20f15222ea2f7b85db1af" + } +} diff --git a/docs/investigations/r5/implementation-review-response.md b/docs/investigations/r5/implementation-review-response.md new file mode 100644 index 000000000..40f47d5af --- /dev/null +++ b/docs/investigations/r5/implementation-review-response.md @@ -0,0 +1,26 @@ +# R5 implementation review disposition + +Real reviewer: `claude-opus-5`, explicit `--effort max`, session `599b4759-add2-4a41-b5b2-865af7a2c096`. +Verdict: **GO_WITH_NONBLOCKING_NOTES; 0 blockers**. Raw review is preserved verbatim in `opus-implementation-review.md`; model usage, original plan/patch hashes and raw log location are in `opus-implementation-metadata.json`. + +The accepted v2 plan remains immutable. The following implementation notes supplement it; they do not retroactively change the hash on which plan consensus was reached. + +## Nonblocking notes + +- **N1 accepted:** a scheduled metadata COPY can rewrite object data when the receiver applies bucket-default/automatic KMS encryption. Its cost can therefore exceed metadata I/O. The existing completed-object/scanner and incoming-replica scheduling gates still prevent a feedback loop. No new transfer optimization or HEAD protocol is introduced. +- **N2 accepted:** a malformed recorded source tag timestamp fails sender construction and remains a retry failure until an explicit correct tag mutation/repair supplies a valid revision. A missing revision is different from a present invalid/empty value. No historical time is fabricated, and no automatic production rewrite is performed. +- **N3 retained scope:** existing marker/trust/REPLICA/version predicates are preserved. Production sender requests satisfy the relevant predicates; R5 does not broaden replication trust. +- **N4 accepted compatibility change:** a trusted metadata COPY without a source tag revision preserves stored tags, including the metadata-REPLACE shape. This is the deliberate missing-revision rule in plan C, and is tested under UUID/null versions and unqualified COPY. +- **N5 accepted:** ordinary COPY records its chosen tag state, including an empty REPLACE and unchanged tags during key rotation, as a fresh local event. This is consistent with the accepted last-writer-wins scheme. +- **N6 no change:** all production writers use the lowercase reserved timestamp key. Case-insensitive sender lookup is compatible with those writers and existing lock timestamp handling. + +## Coverage notes + +- **L1:** the review was supplied a passing run with **13**, not 12, top-level R5 tests. Its verdict explicitly did not claim execution of the wider tests. The wider selection reproduced the same `TestReplicationResync` order-dependent initialization panic on the unmodified production baseline; that test passes in isolation on both baseline and R5. Host-capacity and actual ENOSPC failures are retained, not reported as passes. Final related, race and static/build results are recorded separately in `verification.md`. An unfiltered full `cmd` package run remains an integration check before any later merge; this task delivers a local patch and does not claim that full-package or multi-site production gate passed. +- **L2 addressed:** after every incoming multi-pool replay, the R5 test now rereads the addressed version through normal pool routing and checks its empty value and deletion revision. The per-pool checks still inspect every retained copy. This prevents a vacuous pass if all copies disappear. The test deliberately allows existing duplicate suppression to retain both identical copies; existing pool cleanup/retry tests separately exercise retirement. +- **L3 accepted boundary:** the combined KMS cases exercise destination encryption and plaintext readback; source fixtures are populated through storage APIs. They do not establish encrypted-source-to-encrypted-destination replication across two running sites. SSE-C key rotation has its own signed HTTP and decrypted GET test. +- **L4 confirmed:** both the actual SDK default metadata directive and peer metadata-REPLACE shapes are exercised. + +## Changes after review + +Production code is unchanged from reviewed patch SHA256 `8f6f76ee874c43b0827fde272e8a947f118efb1c1af92bf4a02ef88f93554c1b`. Test-only follow-up adds the L2 normal-routing read and applies the repository's gofumpt formatting. `implementation-manifest.json` records the exact reviewed files; the final verification manifest records the final files, so the two versions are distinguishable. diff --git a/docs/investigations/r5/opus-implementation-metadata.json b/docs/investigations/r5/opus-implementation-metadata.json new file mode 100644 index 000000000..760cb1128 --- /dev/null +++ b/docs/investigations/r5/opus-implementation-metadata.json @@ -0,0 +1,46 @@ +{ + "model": "claude-opus-5", + "effort": "max", + "baseline": "dbcf8dec589deb5d91e17d295cb70997635f5b55", + "plan_version": "v2", + "plan_sha256": "5a782acf3f285b23d1ae43a73481c4eb772a9a6d917fc5a550ecfc7cbf7446ca", + "patch_sha256": "8f6f76ee874c43b0827fde272e8a947f118efb1c1af92bf4a02ef88f93554c1b", + "actual_assistant_models": [ + "claude-opus-5" + ], + "session_id": "599b4759-add2-4a41-b5b2-865af7a2c096", + "is_error": false, + "modelUsage": { + "claude-haiku-4-5-20251001": { + "inputTokens": 2125, + "outputTokens": 15, + "cacheReadInputTokens": 0, + "cacheCreationInputTokens": 0, + "webSearchRequests": 0, + "costUSD": 0.0022, + "contextWindow": 200000, + "maxOutputTokens": 32000, + "thinkingTokens": 0, + "canonicalModel": "claude-haiku-4-5", + "provider": "firstParty", + "costBasis": "list" + }, + "claude-opus-5": { + "inputTokens": 106, + "outputTokens": 64414, + "cacheReadInputTokens": 7275193, + "cacheCreationInputTokens": 236959, + "webSearchRequests": 0, + "costUSD": 7.618066499999999, + "contextWindow": 1000000, + "maxOutputTokens": 64000, + "thinkingTokens": 45128, + "canonicalModel": "claude-opus-5", + "provider": "firstParty", + "costBasis": "list" + } + }, + "result": "GO_WITH_NONBLOCKING_NOTES", + "blocking_items": 0, + "raw_output": "/Users/vonng/tmp/silo-r5-20260915-77ad/opus-implementation.jsonl" +} diff --git a/docs/investigations/r5/opus-implementation-prompt.md b/docs/investigations/r5/opus-implementation-prompt.md new file mode 100644 index 000000000..140acfa7d --- /dev/null +++ b/docs/investigations/r5/opus-implementation-prompt.md @@ -0,0 +1,27 @@ +Review the actual R5 implementation independently for correctness and regressions, using Claude Opus 5 at max effort. This is a read-only final code review after an already recorded two-round plan consensus. Do not edit files. Do not simulate tests or claim you executed them. Read the relevant source and evidence yourself; focus on material blockers and minimal compatible fixes. + +Working tree: /Users/vonng/.codex/worktrees/77ad/silo +Base dependency commit: dbcf8dec589deb5d91e17d295cb70997635f5b55 (R4 KMS timestamp field, one production addition) +R5 plan v2 SHA256: 5a782acf3f285b23d1ae43a73481c4eb772a9a6d917fc5a550ecfc7cbf7446ca +R5 implementation patch SHA256: 8f6f76ee874c43b0827fde272e8a947f118efb1c1af92bf4a02ef88f93554c1b +Manifest: /Users/vonng/.codex/worktrees/77ad/silo/docs/investigations/r5/implementation-manifest.json +Frozen review patch (7 production files + 2 new tests): /Users/vonng/tmp/silo-r5-20260915-77ad/implementation-v1.patch +Plan: /Users/vonng/.codex/worktrees/77ad/silo/docs/investigations/r5/plan-v2.md +Prior actual review: /Users/vonng/.codex/worktrees/77ad/silo/docs/investigations/r5/opus-v2-review.md +Consensus and disagreements: /Users/vonng/.codex/worktrees/77ad/silo/docs/investigations/r5/consensus.md, /Users/vonng/.codex/worktrees/77ad/silo/docs/investigations/r5/opus-v1-response.md +Baseline reproduction: /Users/vonng/.codex/worktrees/77ad/silo/docs/investigations/r5/reproduction.md +Latest new regression run: /Users/vonng/tmp/silo-r5-20260915-77ad/fixed-targeted-latest.log (PASS, 9.161s; signed HTTP and actual storage on single/16 disks, null/UUID, COPY default and REPLACE, PUT/multipart, KMS plaintext GET, SSE-C rotation, multi-pool, retry and stale source ACK; test names and details in source.) +Additional related suite and race/static validation are ongoing and are not yet accepted. An expanded suite hit existing TestReplicationResync initialization panic before R5 tests; baseline isolation is ongoing. Do not treat that as a proven R5 regression or a passing test. + +Research and implementation points to scrutinize: +1. Empty tag values are ordered states only with recorded timestamp; no fabricated tombstone for empty legacy object. Nonempty legacy sender falls back to ModTime. Malformed stored timestamp fails PUT and metadata COPY sender construction. +2. Local PUT/DELETE tagging timestamps are unconditional, advance under existing storage locks; pooled mutation computes a single revision > every copy; do not mutate caller map. Replicas keep source ordering and equal timestamp stored-wins. +3. Actual sender getCopyObjMetadata + minio Core.CopyObject sends tagging REPLACE with no metadata directive; peers may also send metadata REPLACE. Capture stored timestamp before reconstruction, accept incoming empty value, prevent stale SSE-C encMetadata snapshot overwrite, keep existing storage rechecks. +4. PUT/multipart init persist parsed trusted timestamp. Matching ETag/version no-op is relaxed only for trusted newer tag revision, retaining explicit client preconditions and SSE-C behavior. Multipart completion rechecks revision under lock. +5. Equal visible tag values can hide a newer deletion/re-addition revision, so scheduled metadata/heal work with recorded timestamp sends metadata. Preserve existing Completed/scanner and incoming no-reschedule gates. +6. Replication status ACK no longer writes old queued ri.UserTags over the current tags. Preserve current value/timestamp under metadata lock including multi-pool. +7. Scope explicitly excludes tag-filter target eligibility changes, historical tombstone recovery, arbitrary independent unversioned content replacements, clock skew consensus, and production multi-site acceptance. R4 owns object-api-options.go; it is not in this R5 diff. + +There was an earlier KMS test-only fixture error: it assigned a single-PUT ETag to a multipart-layout retransmission; corrected by seeding a real multipart source and using its actual ETag/layout (full latest run passes plaintext read). Inspect the fixture rather than inferring production encryption correctness solely from prior failed test output. + +Return a clear GO / GO_WITH_NONBLOCKING_NOTES / REQUEST_CHANGES verdict; blocking count; concrete source locations with cause, trigger and effect for any blockers; tests/coverage limitations if material. State which baseline, plan hash and patch hash you reviewed (you may quote supplied hash, but cannot independently compute with Read/Grep/Glob). Do not demand unrelated broad redesign. Distinguish actual source defect from nonblocking improvement or scope limitation. diff --git a/docs/investigations/r5/opus-implementation-review.md b/docs/investigations/r5/opus-implementation-review.md new file mode 100644 index 000000000..a15e68b61 --- /dev/null +++ b/docs/investigations/r5/opus-implementation-review.md @@ -0,0 +1,58 @@ +Write is disabled in this session, so I'll deliver the review directly rather than to the plan file. + +# R5 implementation review — tag deletion and ordered replication + +## Verdict: **GO_WITH_NONBLOCKING_NOTES** — blocking count: **0** + +Read-only review. No files edited, no tests run, no results simulated. Every pass I cite is quoted from the supplied log. + +## What I reviewed + +| Item | Value | +|---|---| +| Working tree | `/Users/vonng/.codex/worktrees/77ad/silo`, branch `codex/r5-tag-deletion-ordering` | +| Baseline dependency | `dbcf8dec589deb5d91e17d295cb70997635f5b55` (R4 SSE-KMS field) | +| Plan | `plan-v2.md`, SHA256 `5a782acf…7fc7446ca` *(quoted)* | +| Patch | `implementation-v1.patch`, SHA256 `8f6f76ee…f88f93554c1b` *(quoted)* | + +**Hash caveat:** with Read/Grep/Glob I **cannot compute** either SHA256 — both are quoted from the manifest/task. What I did verify by reading is that every hunk in the frozen patch is present verbatim in the working tree across all seven production files. That is source identity by inspection, not by digest. I also confirmed R5 does not touch `cmd/object-api-options.go`; the R4 branch carrying `ReplicationSourceTaggingTimestamp` (`object-api-options.go:449-460`) is unmodified. + +## Per-claim findings + +**1. Empty values are ordered states; no fabricated legacy tombstone.** Confirmed. `replicationTaggingTimestamp` (`bucket-replication.go:786-794`) returns the recorded stamp even with empty tags, falls back to `ModTime` only for non-empty tags, zero otherwise. Used by both `putReplicationOpts` (`:861-870`) and the metadata sender (`:1702-1707`). The SDK omits the header for a zero time (`minio-go@…60bd07042d49/api-put-object.go:236-238`, `api-compose-object.go:286-288`), so "no revision" really travels as absence. Malformed stamps fail both constructions. + +**2. Local revisions unconditional and monotonic.** Confirmed. Both handlers mint one `UTCNow()` outside the `dsc.ReplicateAny()` branch (`object-handlers.go:3773-3778`, `:3876-3881`); `getOpts` leaves `opts.UserDefined` nil (`object-api-options.go:110`,`:39`), so the unconditional map replacement drops nothing. `er.PutObjectTags` applies the guard under the existing NS lock (`erasure-object.go:2273-2282`, `:2330-2334`); an absent stamp yields `""` and preserves legacy direct-storage semantics. `z.PutObjectTags` folds one candidate strictly beyond every copy and **clones** first (`erasure-server-pool.go:3054-3062`) — `opts` is a value parameter and `er.PutObjectTags` never writes `opts.UserDefined`, so no caller map is mutated. No replica path reaches `PutObjectTags` (the only two production callers are the tagging handlers), so replicas keep strict source ordering via `reconcileStoredObjectTags`, stored-wins on ties (`erasure-server-pool-consistency.go:238-242`). + +**3. COPY receiver.** Confirmed. Stored pair captured before reconstruction (`object-handlers.go:1800`); `srcInfo.UserTags` is never reassigned between the source read and the decision, so it genuinely is stored state. The decision block (`:1818-1837`) accepts an incoming empty value with a stamp and rechecks the captured state; all existing in-lock rechecks still run (`erasure-object.go:136-138`, `:1312-1315`; `erasure-multipart.go:1161-1190`; `erasure-server-pool.go:1443-1450`). The `encMetadata` fix (`:1840`) is safe and correctly placed — `encMetadata` receives reserved keys only on the SSE-C rotation path (`:1655-1659`), and the delete lands after `rotateKey`/`newEncryptReader` and before the merge at `:1910`. + +**4. PUT/multipart persistence and the duplicate exception.** Confirmed. `putOptsFromHeaders` aliases `opts.UserDefined = metadata` in both branches (`object-api-options.go:451`,`:464`), so post-build writes reach storage (`object-handlers.go:2323-2325`; `object-multipart-handlers.go:315-318`, correctly *after* `maps.Copy(metadata, encMetadata)` at `:300`). The relaxation (`object-handlers-common.go:243-246`) sits below the explicit `If-Match`/`If-None-Match` checks, is gated on `isReplicaTrusted` + `olderThan` (zero source never wins, `bucket-object-lock.go:370-376`), and leaves the SSE-C exemption intact. It cannot loop: once the write lands the stamps are equal and the next attempt 412s. `completeMultipartOpts` sets neither `PreserveETag` nor a tagging timestamp (`object-api-options.go:501-550`), so completion needs no new exception and reconciles under the lock (`object-multipart-handlers.go:1201`). + +**5. Equal values can hide a newer revision.** Confirmed and correctly scoped. The gate (`bucket-replication.go:1013-1018`) sits after the null-version resync exclusion and after **every** branch that can return `replicateAll`; from there only `replicateMetadata`/`replicateNone` are reachable, so it can never downgrade a needed full transfer. It is reached only from `replicationActionForTarget` → `replicateAll` (`:1608`), not from the object-replication fast path (`:1328-1343`). The Completed gate (`:3775`) and failures-only requeue (`:1316`) bound the work, and an incoming replica COPY schedules no outgoing event. Existing fixtures carry no tagging stamp (`bucket-replication_test.go:716-739`), so they are unaffected. + +**6. ACK no longer overwrites current tags.** Confirmed removed (`bucket-replication.go:1276-1286`). Preservation holds on both write-backs: `er.PutObjectMetadata` copies from `ObjectInfo.UserDefined`, which `cleanMetadata` strips of `x-amz-tagging` (`object-api-utils.go:403-407`; `erasure-object.go:2254-2260`); `updatePoolMetadata` falls back to merged `UserTags` and rewrites the merged newest stamp (`erasure-server-pool-consistency.go:194-214`). Both under the object lock (`erasure-object.go:2196-2205`; `erasure-server-pool.go:3020-3029`). The sender also re-reads current state first (`bucket-replication.go:1527-1550`). + +**7. Scope.** Respected — no tag-filter eligibility change, no historical tombstone invention, no clock-skew consensus, no `object-api-options.go` change. + +**Trust boundary re-checked:** the reserved key cannot be injected from the wire — `containsReservedMetadata` rejects the whole `X-Minio-Internal-` class outside the SSE allowlist (`generic-handlers.go:75-85`), and `extractMetadataFromMimeWithReplication` maps only `replicationToInternalHeaders` (`handler-utils.go:258-298`). + +**KMS fixture inspected directly**, not inferred from prior output: the multipart case now seeds a real multipart source and reuses its actual ETag/part layout (`replication-tagging-order_test.go:472-489`). The earlier single-PUT-ETag mismatch is gone. See L3 for what it still does not cover. + +## Non-blocking notes (no change required) + +- **N1 — on encrypted destinations the forced metadata COPY is not metadata-only.** The gate at `bucket-replication.go:1013-1018` yields a replica COPY; with bucket-default/auto KMS the destination applies SSE before `copyDstOpts` (`object-handlers.go:1428-1433`) and then clears `srcInfo.metadataOnly` (`:1669-1677`) — so it **rewrites object data**. Bounded to one COPY per object entering heal and one per object per explicit resync (not a loop), but the plan's "extra metadata I/O" understates this case. Worth a sentence in the cost note. +- **N2 — fail-closed on a malformed stored revision is terminal for that object** (`:786-794` → `:1702-1707`/`:867-870`, requeued by MRF at `:1316-1322`). No production writer can produce such a value, so this is a defensive tail. Note the asymmetry: storage self-heals the same corruption (invalid *stored* ⇒ incoming wins, `erasure-server-pool-consistency.go:238-242`) while the sender refuses to proceed. The minimal hardening, if ever wanted, is to treat a present-but-**empty** value as absent — I traced no reachable path producing one, and the current behavior is what plan and consensus chose, so I am not asking for it. +- **N3 — trusted-marker vs REPLICA asymmetry (pre-existing).** `object-handlers.go:2323` / `object-multipart-handlers.go:316` persist on `opts.ReplicationRequest`, while the in-lock recheck needs `isReplicaTrusted` **and** a version ID (`:2442`). Production sets both; the new precondition exception uses the stricter predicate. Accepted in plan §D. +- **N4 — a trusted metadata COPY with no source revision now ignores the request's tag value** (`object-handlers.go:1826-1833`). For a peer sending `x-amz-metadata-directive: REPLACE` without a revision, the value used to land (`X-Amz-Tagging` is in `supportedHeaders`, `handler-utils.go:271-283`). No MinIO sender produces that shape, and the R4 case `object-copy-replication-tagging_test.go:89` already expects stored-wins there, reaching it via the storage reconcile. Deliberate per plan §C. +- **N5 — ordinary COPY always writes an explicit tag value plus a fresh revision** (`:1834-1837`). (a) `x-amz-tagging-directive: REPLACE` with no tags now genuinely clears the destination, where the default-metadata path used to carry source tags forward — an S3 conformance improvement, covered by `TestAPILocalTaggingAlwaysAdvancesRevision`. (b) An in-place key-rotation COPY advances the revision without changing any value, re-asserting current tags against an older in-flight remote deletion. Both follow from last-writer-wins as specified. +- **N6 — cosmetic.** The key is read case-insensitively at `bucket-replication.go:787`/`:1016`, exactly elsewhere. `TaggingTimestamp` is lowercase (`:74`) and storage writes only lowercase, so they agree; the same mix already exists for lock timestamps in that file (`:896` vs `:1708`). + +## Tests and coverage limitations (material) + +- **L1 — the only established green result is the 12 R5 tests** (`fixed-targeted-latest.log`, `ok … 9.161s`): signed HTTP through real single-disk and 16-disk storage, null/UUID, COPY default and REPLACE, PUT/multipart, KMS, SSE-C rotation, multi-pool, sender retry and stale ACK. The wider `cmd` package, `-race`, `gofmt` and `git diff --check` are ongoing, and the `TestReplicationResync` panic is unattributed. I treat that as an **open verification item**, not a regression and not a pass. My static read found no existing test whose expectations R5 flips — I checked the `getReplicationAction` fixtures, the R4 KMS COPY table including its `missing-timestamp` case (R5 satisfies it via the handler instead of the storage reconcile), and confirmed no pre-existing test calls `PutObjectTags` with a supplied revision. +- **L2 — multi-pool convergence after an *incoming* replica write is asserted weakly.** `replication-tagging-order_test.go:688-698` skips any pool whose version is absent and only asserts a retained copy is correct; it pins neither which pool is retained nor that retirement ran. The case that matters for §A — one revision strictly greater than every copy, in the response *and* every stored copy — is pinned by `TestLocalTaggingCommitCannotRegressRevision`. +- **L3 — the KMS variant's source objects are plaintext.** Seeds use `obj.PutObject` (`:468`, `:477-488`), bypassing handler encryption, so encryption enters only via the incoming request and the destination bucket default. Real coverage of the R4 field on the receive side, but not encrypted-source-to-encrypted-destination end to end. The trailing plaintext GET (`:520-523`) does establish the final object is readable. +- **L4 — wire shape is well pinned.** `TestTaggingProductionCopyWireShape` asserts the real SDK request (`metadata-directive=""`, `tagging-directive=REPLACE`) against a live peer, and `r5Receive`'s `"copy"` operation covers peer metadata-REPLACE independently. Both required shapes are present. + +## Recommendation + +The production diff is merge-eligible as written; I found no actual source defect. The single gating action before merge is closing **L1** — a clean full-package run (plus `-race`, `gofmt`, `git diff --check`) with the `TestReplicationResync` panic isolated against the unpatched baseline. N1 and N2 deserve a sentence each in the plan's cost/limitations section; N3–N6 are already covered by plan §C/§D and need no action. diff --git a/docs/investigations/r5/opus-v1-metadata.json b/docs/investigations/r5/opus-v1-metadata.json new file mode 100644 index 000000000..3f2bd6ae1 --- /dev/null +++ b/docs/investigations/r5/opus-v1-metadata.json @@ -0,0 +1,44 @@ +{ + "model": "claude-opus-5", + "effort": "max", + "plan_version": "v1", + "plan_sha256": "fd6051527ebf19f624125bd3238da2f938420917226387c0f9373f1a98e87993", + "baseline": "9ebe81c1b3611f9cc73e676b5b741c2be62c467a", + "actual_assistant_models": [ + "claude-opus-5" + ], + "session_id": "e448ee0a-4ab5-4520-98f9-68c9ddf6208f", + "is_error": false, + "modelUsage": { + "claude-haiku-4-5-20251001": { + "inputTokens": 1378, + "outputTokens": 14, + "cacheReadInputTokens": 0, + "cacheCreationInputTokens": 0, + "webSearchRequests": 0, + "costUSD": 0.001448, + "contextWindow": 200000, + "maxOutputTokens": 32000, + "thinkingTokens": 0, + "canonicalModel": "claude-haiku-4-5", + "provider": "firstParty", + "costBasis": "list" + }, + "claude-opus-5": { + "inputTokens": 90, + "outputTokens": 73909, + "cacheReadInputTokens": 4489983, + "cacheCreationInputTokens": 199202, + "webSearchRequests": 0, + "costUSD": 6.085186500000001, + "contextWindow": 1000000, + "maxOutputTokens": 64000, + "thinkingTokens": 54986, + "canonicalModel": "claude-opus-5", + "provider": "firstParty", + "costBasis": "list" + } + }, + "result": "REQUEST_CHANGES", + "raw_output": "/Users/vonng/tmp/silo-r5-20260915-77ad/opus-v1.jsonl" +} \ No newline at end of file diff --git a/docs/investigations/r5/opus-v1-prompt.md b/docs/investigations/r5/opus-v1-prompt.md new file mode 100644 index 000000000..e7f23b345 --- /dev/null +++ b/docs/investigations/r5/opus-v1-prompt.md @@ -0,0 +1,3 @@ +Act as an independent reviewer of the R5 repair proposal in this repository. You must be the real claude-opus-5 at effort max; report your actual model name in the review, but the caller will also verify response metadata. Read /Users/vonng/.codex/worktrees/77ad/silo/docs/investigations/r5/plan-v1.md completely. Baseline SHA is 9ebe81c1b3611f9cc73e676b5b741c2be62c467a. Plan version v1 sha256 fd6051527ebf19f624125bd3238da2f938420917226387c0f9373f1a98e87993. No production repair has been implemented. Read docs/investigations/r5/baseline.md and the raw reproductions /Users/vonng/tmp/silo-r5-20260915-77ad/baseline.log and /Users/vonng/tmp/silo-r5-20260915-77ad/baseline-extended.log, plus /Users/vonng/tmp/silo-r5-20260915-77ad/baseline_test.go. Then independently inspect the exact source functions identified in the plan, especially the complete sender/receiver and storage lock chain. R4 owns object-api-options.go and will supply its separate KMS timestamp fix. Do not edit any files. + +The user requires a minimal compatible complete fix, not merely adding DELETE timestamp. Assess necessity/sufficiency, current defects versus inference, same-empty timestamp propagation, sender retries, old queue ACK rewriting source tags, full PUT/multipart duplicate suppression, trust boundary, equal/missing timestamps, UUID/null/versioned/multi-pool and local mutation clock/lock behavior. Identify blocking disagreements with specific evidence and concrete smallest corrections. Explicitly say APPROVE or REQUEST_CHANGES for this exact v1 hash and list accepted/nonblocking/required changes. Do not claim consensus if any blocking issue remains. Be exact about limitations and whether proposed test coverage can establish the scope. Use the permitted Read/Grep/Glob tools for source verification. Return a substantive review, not just a summary. diff --git a/docs/investigations/r5/opus-v1-response.md b/docs/investigations/r5/opus-v1-response.md new file mode 100644 index 000000000..0282af778 --- /dev/null +++ b/docs/investigations/r5/opus-v1-response.md @@ -0,0 +1,30 @@ +# Response to actual Opus v1 review + +The original review is retained verbatim in opus-v1-review.md, with actual model identity/usage in opus-v1-metadata.json and the full stream in the raw evidence directory. Result was REQUEST_CHANGES, five blockers. No implementation was started and no consensus is implied by the response below. + +## Blocking items + +| Item | Disposition for v2 | +|---|---| +| R1 empty/no-revision ModTime fallback | Accepted. Never synthesize a revision for empty tags without one. Keep only existing nonempty ModTime fallback. Add no-revision wire regression. | +| R2 force metadata only for empty values | Disagree with the proposed restriction; accept the I/O cost warning. Same nonempty values can carry different revisions: X@T1, delete@T2, re-add X@T3. Skipping T3 lets delayed delete T2 incorrectly win. A recorded revision requires delivery for either value. v2 explicitly accepts extra COPY per scheduled/resync invocation. queueReplicationHeal already skips Completed unless resync requested; replicateObject only requeues Failed. Thus the predicate is permanently conservative, but it does not create perpetual background work. Avoiding a new HEAD protocol is the smaller implementation. Re-review required. | +| R3 actual metadata COPY request shape | Accepted after inspecting the pinned minio-go Core.CopyObject/copyObjectDo. getCopyObjMetadata supplies only tagging REPLACE; the SDK adds no metadata directive. Update provenance and test both actual SDK shape and peers using metadata REPLACE. | +| R4 local revision inversion | Accepted and strengthened for uniform multi-pool persistence. er.PutObjectTags advances a valid supplied revision beyond stored time under lock. z.PutObjectTags computes one value beyond every addressed copy before writing, so the response, ordinary source read and all copies agree. Per-pool-only guards can produce different times; mergedPoolObjectInfo is not every ordinary read path, so relying on later merge is insufficient for a precise source revision. Direct calls without valid supplied revisions keep old semantics. | +| R5 duplicate suppression wording | Accepted. Explicitly use strictly-newer-than-stored, with existing olderThan semantics. Preserve client preconditions; only trusted REPLICA source timestamps can relax version/ETag duplicate suppression. Document possible data re-upload cost. | + +## Nonblocking items + +- Equal times: document stored-wins consistency across COPY and storage, including null/unqualified requests. +- Invalid COPY sender timestamp: fail the metadata send with Failed, as PUT option construction does; no silent fallback. +- Tag trust versus replica trust: preserve existing predicates, document production supplies both. No permission relaxation. +- KMS nuance: agree PUT/multipart depend on R4, but disagree that metadata COPY never depends on R4. Destination bucket defaults and globalAutoEncryption inject KMS before copyDstOpts at object-handlers.go 1425–1435. R4 independently reproduced all three explicit/default/auto entrypoints. Do not adopt the inaccurate broader exclusion. Combined tests required. +- Ordinary whole-object replacement/no-version clocks: document unchanged semantics. This plan addresses local tagging mutation and selected per-hop replicated version updates; it does not create a new conflict model for independent unversioned content overwrites. +- Tag-filtered target eligibility: document the pre-existing scope limitation; no selection/rule protocol redesign in R5. Final result must not claim arbitrary configuration convergence. +- ACK: additionally reproduced on both real storage backends in baseline-ack.log. The source revives `key=queued` with the deletion's timestamp after old queue event completion. Remove the stale assignment; storage preserves current state. +- Storage lock recheck: reuse and keep existing error behavior. The tests establish per-hop behavior, not a production multi-site deployment or physical clock-skew experiment. + +## Added evidence + +`/Users/vonng/tmp/silo-r5-20260915-77ad/matrix_test.go` contains temporary signed HTTP UUID/null tests, exact SDK COPY wire capture, local timestamp inversion, multi-pool deletes and SSE-C rotation. The original matrix fails baseline as expected. `discussion-baseline.log` isolates R2's equal nonempty case, R3's real SDK shape, and R4's commit inversion. A short first compile missed a test import and was corrected; only the subsequent compile/run is behavioral evidence. + +The v2 plan, not this commentary, is the next consensus target. Production diff remains empty. diff --git a/docs/investigations/r5/opus-v1-review.md b/docs/investigations/r5/opus-v1-review.md new file mode 100644 index 000000000..f2157c242 --- /dev/null +++ b/docs/investigations/r5/opus-v1-review.md @@ -0,0 +1,111 @@ +Write is disabled in this session, so the review is delivered here in full. + +# Independent review — R5 repair plan v1 + +**Reviewer model: `claude-opus-5` (Opus 5), effort max.** I read `docs/investigations/r5/plan-v1.md` completely, `docs/investigations/r5/baseline.md`, both raw logs, `baseline_test.go`, and then independently inspected the sender/receiver/storage chain at `9ebe81c1b`. + +**VERDICT: REQUEST_CHANGES** for plan v1 sha256 `fd6051527ebf19f624125bd3238da2f938420917226387c0f9373f1a98e87993`. Five blocking corrections (R1–R5). **No consensus is claimed.** + +Binding caveat, stated exactly: this session has read-only tools, so I did not execute `sha256sum`. I reviewed the file content at that path at the baseline SHA and cite the plan's own line numbers so you can bind findings to the hash. + +--- + +## 1. Claims I confirmed from source (necessity, not inference) + +| Plan | Claim | Evidence | Verdict | +|---|---|---|---| +| L14 | PUT tagging stamps only when replication is selected; DELETE never stamps | `cmd/object-handlers.go:3762-3768`, `:3865-3870` | **correct** (and PUT uses two separate `UTCNow()` calls, 3765/3767) | +| L14 | both write under the existing object lock; multi-pool updates the addressed version in each copy | `cmd/erasure-object.go:2272-2337`; `cmd/erasure-server-pool.go:3033-3075` | **correct** | +| L15 | `putReplicationOpts` stamps only inside nonempty `UserTags`; multipart clears SourceMTime | `cmd/bucket-replication.go:849-863`, `:1776` | **correct** | +| L16 | metadata COPY carries an explicit empty tag; ModTime default only for nonempty | `:747`, `:1692-1700`; SDK `copyObjectDo:262-264` writes the map verbatim | **correct** | +| L18 | PUT / multipart initiation parse but never persist the source stamp | `cmd/object-api-options.go:473`; no writer anywhere in `cmd/` | **correct** (matches `baseline-extended.log:19-26`) | +| L18 | multipart completion already rechecks the persisted upload under the object lock | `cmd/erasure-multipart.go:1161-1190` | **correct** | +| L19 | `getReplicationAction` compares values/counts, not ordering time | `cmd/bucket-replication.go:1000-1005` | **correct** | +| L20 | `checkPreconditionsPUT` skips matching version/ETag for non-SSE-C replicas | `cmd/object-handlers-common.go:233-247` | **correct**; and senders treat 412 as delivered (`:1466`; multipart `:1786-1788` returns `nil`) | +| L21 | ACK callback copies stale `ri.UserTags` | `cmd/bucket-replication.go:1272-1274` | **correct, and worse than stated** | +| L23 | `reconcileStoredObjectTags` gates as described | `cmd/erasure-server-pool-consistency.go:232-243` | **correct** | + +Two amplifiers the plan does not name, both strengthening it: + +- The ACK callback writes stale tags **without** a timestamp. The revived tag set therefore inherits the *deletion's newer* revision and propagates downstream as authoritative. Removal is the right fix and is sufficient: `er.PutObjectMetadata` preserves `fi.Metadata`'s tag key (`cmd/erasure-object.go:2260`) and `updatePoolMetadata` falls back to merged `UserTags` (`cmd/erasure-server-pool-consistency.go:194-214`). +- The `encMetadata` merge at `cmd/object-handlers.go:1903` restores every reserved key snapshotted at `:1655-1659`; the guard at `:1855-1864` covers only the two Object Lock stamps. Tag revision is genuinely exposed, so L47 is justified. + +--- + +## 2. Blocking disagreements + +### R1 — Do not synthesize a ModTime revision for objects with no tags and no revision +**Where:** L35 ("otherwise object ModTime (also for empty legacy objects)") composed with L49 ("persist a nonzero parsed trusted source timestamp"). + +**Evidence:** `PutObjectOptions.Header()` emits the header whenever `TaggingTimestamp` is non-zero (SDK `api-put-object.go:236-238`). If L35 moves selection outside the nonempty branch *and* defaults to ModTime, every replicated object — including every object that has never carried a tag — ships a non-zero stamp, and L49 persists it. Every object on the destination then owns a tag revision. Composed with L39 (recorded revision ⇒ force metadata replication), **every object at the next hop always selects metadata replication.** It also contradicts L10 ("not a reason to change the storage format") and L57 ("we do not invent historical deletion times"). + +**Smallest correction:** send a stamp only when `objInfo.UserTags != ""` **or** a recorded revision exists. That keeps the tombstone case (empty + revision — the entire point), keeps the existing nonempty ModTime fallback, and drops only empty + no-revision, which L57 already declares unrecoverable. This makes §B consistent with §D. + +### R2 — Bound the forced metadata replication in `getReplicationAction` +**Where:** L39. + +**Evidence:** the destination's revision is invisible to HEAD, so the condition never becomes false. Any object carrying a revision never returns `replicateNone` again: every heal, MRF retry and `ExistingObjectReplicationType` resync re-COPIES its metadata, rewriting `xl.meta` on the destination (and, multi-pool, running `retireReplicaCopies`) each pass. L39's "extra COPY only for already-scheduled work" understates a permanent non-convergence. Existing tests won't catch it — `newMatchingReplicationPair` (`cmd/bucket-replication_test.go:716-739`) carries no revision. + +**Smallest correction:** fire only when `oi1.UserTags == ""` and a revision is recorded — exactly the empty-to-empty tombstone L19 names and `TestReviewR5SameEmptyTagsMustTransferTimestamp` asserts. Nonempty states are already caught by the existing value/count comparison at `:1003`. Then state the residual: tag-deleted objects still never converge to `replicateNone`. + +### R3 — The production metadata COPY does not send `x-amz-metadata-directive: REPLACE` +**Where:** L17. + +**Evidence:** `getCopyObjMetadata` sets `x-amz-tagging-directive: REPLACE` (`:748`) but never the metadata directive, so `getCpObjMetadataFromHeader` takes the `defaultMeta` branch (`cmd/object-handlers.go:1143,1165-1170`). Therefore: +1. "its REPLACE metadata map also loses the previous timestamp before comparison" is **false on the production path** — `defaultMeta` preserves the stored revision. It is true only for a peer that does send REPLACE. +2. The empty tombstone is dropped for a *different* reason than the plan gives: `defaultMeta` carries the stored `X-Amz-Tagging` forward and the `objTags != ""` gate at `:1817` skips the overwrite. (Note `X-Amz-Tagging` is in `supportedHeaders`, `cmd/handler-utils.go:84`, so on the REPLACE path the empty value *does* arrive in the map — only the stamp is missing.) +3. The reproduction sends `x-amz-metadata-directive: REPLACE` (`baseline_test.go:134`), so it **does not pin the production request shape.** The conclusion still holds (with a stale stored stamp the delayed COPY wins either way), but the evidence chain as written is not the one production executes. + +**Smallest correction:** fix L17, and add a sender-shaped COPY case asserting against `getCopyObjMetadata` output rather than a hand-built header map. + +### R4 — Missing monotonic guard on the local revision at commit +**Where:** L31 explicitly asks the reviewer to decide. My answer: commit-time *generation* is not required; a commit-time monotonic *guard* is. + +**Evidence:** `er.PutObjectTags` writes `fi.Metadata[x-amz-tagging]` and copies `opts.UserDefined` with **no ordering check** (`cmd/erasure-object.go:2328-2330`), and the handler mints the stamp *before* the namespace lock. R5 newly makes DELETE mint a revision, so a DELETE→PUT pair can invert — via lock queueing (`globalOperationTimeout` waits) or clock skew between the two nodes serving the two requests. Result: source holds `tags=X @ t_old`, replica holds the tombstone `@ t_new`. Every retransmit is then rejected by `reconcileStoredObjectTags` (`stamp.Before(incoming)` false), the sender still records **Completed**, and — with R2's rule — re-sends forever. Permanent, silent divergence: precisely the failure class R5 exists to remove, newly broadened by change A. + +**Smallest correction:** in `er.PutObjectTags`, under the lock, if the incoming revision is not strictly after the stored one, advance it to stored + 1ns. Multi-pool is safe without a second site of change: `z.PutObjectTags` writes identical tags to all copies, so any per-pool stamp differences still merge to a consistent `(tags, newest stamp)` pair through `mergedPoolObjectInfo` (`cmd/erasure-server-pool-consistency.go:124-131`). + +### R5 — Under-specified duplicate-suppression comparison +**Where:** L51, "a valid newer source tag timestamp makes matching version/ETag insufficient". + +**Evidence:** read naively as "non-zero source stamp ⇒ bypass", this disables the duplicate guard for *every* tagged replica write; for multipart it re-uploads all parts, since 412 at initiation is currently the cheap exit (`:1786-1788`). `TestReviewR5NewerTagsMustBypassContentDuplicate` already encodes the correct comparison (source stamp vs. `oi`'s stored stamp), but the prose does not. + +**Smallest correction:** one sentence — "strictly newer than the destination's stored tag revision" — plus the cost note that even correctly scoped, this re-PUTs object data to deliver a tag-only change. + +--- + +## 3. Accepted / non-blocking (state them; do not necessarily fix) + +- **§A local generation semantics** (L29): accepted as sufficient, subject to R4. Use one `UTCNow()` for both stamps as proposed. +- **§B ACK removal** (L41): necessary and sufficient; preservation verified on both the single-pool and pooled write-back paths. +- **§C `encMetadata` reconciliation** (L47): accepted. Today's observable effect is fail-closed (update dropped) when `ReplicaLockReconcile` is on, and a mismatched `(new tags, old stamp)` pair when `VersionID == ""` — worth one sentence. +- **Equal timestamps:** adopting `reconcileStoredObjectTags` in the handler silently flips the non-versioned COPY path from "incoming wins on equal" (`cmd/object-handlers.go:1824`, `!ondiskTimestamp.After(srcTimestamp)`) to "stored wins on equal". This is the right direction and removes a real handler/storage inconsistency, but it is a compat-visible change and belongs in §D. +- **Missing/invalid timestamps:** the gate asymmetry is correct as the plan describes — invalid *stored* ⇒ incoming wins; invalid *incoming* with valid stored ⇒ stored wins. Note the metadata COPY sender swallows parse errors (`:1696-1699`, `if err == nil`); L35's fail-loud rule should cover that call site too. +- **Trust boundary: sound.** Stamp honored only under `trustedReplication` (`cmd/object-api-options.go:390-396`); headers stripped otherwise (`cmd/replication-trust.go:96-112,139-143`); client-supplied reserved headers rejected wholesale (`cmd/generic-handlers.go:75-85`). One asymmetry to resolve deliberately: the tag decision keys on `dstOpts.ReplicationRequest` (trusted marker) while Object Lock keys on `replicaTrusted` (marker + REPLICA). Production sets both. +- **SSE-KMS / R4 boundary — more precise than the plan:** the KMS early return (`cmd/object-api-options.go:431-461`) drops `ReplicationSourceTaggingTimestamp`, so R5's PUT/multipart persistence is a **silent no-op for KMS-header requests** until R4 lands. The metadata-COPY leg is **not** affected: the public SSE header is synthesized only in responses (`cmd/api-response.go:525-533`), so `getCopyObjMetadata` never forwards it and tag deletions still order correctly for SSE-KMS objects via COPY. Say this instead of only "KMS combined tests after R4". +- **Ordinary PUT/multipart with tags writes no revision,** so §A's invariant has a hole. Mostly masked by versioning; exposed on null/suspended versions, where `ReplicaLockReconcile` is off anyway (`:1847`, `:2432`). Either stamp there too (~3 lines) or document. +- **Tag-filtered replication rules never see a deletion:** `FilterTargetArns` runs on the *post*-deletion `UserTags` (`cmd/bucket-replication.go:1189-1193`; `scheduleReplication` is called with the post-write `oi` at `cmd/object-handlers.go:3879`), so a rule with a tag filter yields no target. Pre-existing, but it bounds any completeness claim. +- **Sender retries are otherwise clean:** `replicateAll` re-reads current state (`:1517`), so retries carry the current revision; staleness was confined to the ACK callback. No new HEAD protocol — agreed. +- **UUID / null / versioned / multi-pool:** `metadataPoolInfos`, `mergedPoolObjectInfo`, `retireReplicaCopies` and the `nullVersionID` normalization (`:136-138`, `:1162-1168`) do carry tag state correctly; I found no additional loss path there beyond R4's clock inversion. + +--- + +## 4. Can the proposed coverage establish the scope? + +**Can:** per-hop handler + storage behavior on real erasure disks (ErasureSD and 16-drive), multi-pool merge/retire through `consistencyPools` and the existing tag suite (`cmd/erasure-server-pool-tags_test.go`), sender option construction, and the ACK race with a deterministic interleave. + +**Cannot:** cross-site convergence, real clock skew, real lock contention. Both `ExecObjectLayerAPITest` instances run in one process against one clock. L73's "Not production multi-site acceptance" should be sharpened to say the suite establishes **per-hop** correctness only. + +**Must add:** (a) an untagged-object regression asserting **no** revision is synthesized (R1); (b) the sender-shaped metadata COPY (R3); (c) a DELETE→PUT inversion, by passing an older stamp directly at the storage call (R4); (d) equal-timestamp parity across the versioned and null-version COPY paths; (e) an SSE-KMS metadata COPY case marked blocked-on-R4. Also keep the three assertions inside `TestReviewR5DeleteThenDelayedTagUpdate` separable — once change A lands, the first passes and the later two silently depend on it. + +--- + +## 5. Summary + +- **Accepted:** §12.1/.2/.3/.5/.6/.7/.8 defect claims; §A generation model; §B ACK removal; §C `encMetadata` reconciliation, storage-recheck reuse, multipart persistence; §D R4 ownership and the no-migration position. +- **Non-blocking:** equal-timestamp behavior change, trust asymmetry, ordinary-PUT revision hole, tag-filtered rules, swallowed parse error in the COPY sender. +- **Required (blocking):** R1 ModTime fallback scope; R2 forced-replication scope; R3 metadata-directive correction + sender-shaped test; R4 monotonic guard at commit; R5 duplicate-suppression comparison wording. + +Answering L75 directly: §A's existing local timestamp semantics are **not** sufficient without R4. §B's forced metadata synchronization is necessary but **wrongly scoped** (R1, R2); the stale ACK removal is necessary and correctly scoped. §C's duplicate exception is defensible but **under-specified** (R5). And yes — a path can still lose or revive the deletion revision: the DELETE→PUT local inversion (R4), plus the documented SSE-KMS PUT leg and tag-filtered-rule gaps. + +I recommend a v2 addressing R1–R5, then re-review against the new hash. I made no edits. \ No newline at end of file diff --git a/docs/investigations/r5/opus-v2-metadata.json b/docs/investigations/r5/opus-v2-metadata.json new file mode 100644 index 000000000..4597e0717 --- /dev/null +++ b/docs/investigations/r5/opus-v2-metadata.json @@ -0,0 +1,45 @@ +{ + "model": "claude-opus-5", + "effort": "max", + "plan_version": "v2", + "plan_sha256": "5a782acf3f285b23d1ae43a73481c4eb772a9a6d917fc5a550ecfc7cbf7446ca", + "baseline": "9ebe81c1b3611f9cc73e676b5b741c2be62c467a", + "actual_assistant_models": [ + "claude-opus-5" + ], + "session_id": "1723246d-35c6-48de-9693-69f061427fcb", + "is_error": false, + "modelUsage": { + "claude-haiku-4-5-20251001": { + "inputTokens": 1578, + "outputTokens": 19, + "cacheReadInputTokens": 0, + "cacheCreationInputTokens": 0, + "webSearchRequests": 0, + "costUSD": 0.001673, + "contextWindow": 200000, + "maxOutputTokens": 32000, + "thinkingTokens": 0, + "canonicalModel": "claude-haiku-4-5", + "provider": "firstParty", + "costBasis": "list" + }, + "claude-opus-5": { + "inputTokens": 28, + "outputTokens": 30031, + "cacheReadInputTokens": 709817, + "cacheCreationInputTokens": 79637, + "webSearchRequests": 0, + "costUSD": 1.9021934999999999, + "contextWindow": 1000000, + "maxOutputTokens": 64000, + "thinkingTokens": 23251, + "canonicalModel": "claude-opus-5", + "provider": "firstParty", + "costBasis": "list" + } + }, + "result": "APPROVE_WITH_NONBLOCKING_NOTES", + "blocking_items": 0, + "raw_output": "/Users/vonng/tmp/silo-r5-20260915-77ad/opus-v2.jsonl" +} \ No newline at end of file diff --git a/docs/investigations/r5/opus-v2-prompt.md b/docs/investigations/r5/opus-v2-prompt.md new file mode 100644 index 000000000..14ad77d01 --- /dev/null +++ b/docs/investigations/r5/opus-v2-prompt.md @@ -0,0 +1,7 @@ +Independent real Opus 5 at effort max follow-up review. Production sources remain exactly 9ebe81c1b3611f9cc73e676b5b741c2be62c467a; no fix implemented. Consensus target: docs/investigations/r5/plan-v2.md sha256 5a782acf3f285b23d1ae43a73481c4eb772a9a6d917fc5a550ecfc7cbf7446ca. Read that complete v2 plan, docs/investigations/r5/opus-v1-response.md, and your preserved docs/investigations/r5/opus-v1-review.md. Focus on the five prior blocking items and disposition; do not repeat a broad repository audit. Read precise source as needed to decide. + +R1 accepted (no empty/no-revision fallback). R3 confirmed against pinned SDK; actual SDK wire test now in /Users/vonng/tmp/silo-r5-20260915-77ad/matrix_test.go and /Users/vonng/tmp/silo-r5-20260915-77ad/discussion-baseline.log. R4 accepted plus a uniform multi-pool guard before per-set write, since normal GetObjectInfo/GetObjectNInfo can return primary pool and not merged latest revision. R5 explicit strictly-newer-than-stored. R2 challenged: same nonempty value at T1 and T3 can hide an intervening delete at T2; empty-only forcing would drop re-addition T3 and allow delayed delete. We accept one metadata COPY per explicitly scheduled retry/heal/resync for explicit revisions. Completed queueReplicationHeal returns early unless resync (cmd/bucket-replication.go ~3758); replicateObject requeues only Failed (~1305). No endless background loop is introduced. A new HEAD revision protocol seems more complexity than justified; evaluate the concrete correctness/cost tradeoff. Both equal-empty and equal-nonempty baseline action tests in discussion-baseline.log expose loss. + +Correct one nonblocking claim too: although COPY sender does not forward a public KMS header, destination bucket defaults/globalAutoEncryption apply BEFORE copyDstOpts (object-handlers.go 1425-1435), so COPY can still depend on R4. R4 has independently reproduced this. Plan documents tag-filter eligibility and unversioned content overwrite as pre-existing scope limits; per-hop guarantees only. + +Return APPROVE / APPROVE_WITH_NONBLOCKING_NOTES / REQUEST_CHANGES for this EXACT v2 and hash, with explicit blocker count and per-item resolution. If R2 is still blocking, provide the smallest concrete alternative that passes the X@T1/delete@T2/X@T3/reordered-delete case, not an empty-only suggestion. Do not claim consensus with remaining blockers. Do not edit or attempt Write; only return text. Model identity will be verified from raw assistant metadata. diff --git a/docs/investigations/r5/opus-v2-review.md b/docs/investigations/r5/opus-v2-review.md new file mode 100644 index 000000000..b32433042 --- /dev/null +++ b/docs/investigations/r5/opus-v2-review.md @@ -0,0 +1,43 @@ +Write is disabled for this task by your instruction, so the review is returned in full as text. No files were created or edited. + +# Independent follow-up review — R5 repair plan v2 + +**Reviewer model: `claude-opus-5` (Opus 5), effort max.** Read: `docs/investigations/r5/plan-v2.md` (complete), `opus-v1-response.md`, my preserved `opus-v1-review.md`, plus `/Users/vonng/tmp/silo-r5-20260915-77ad/matrix_test.go` (discussion cases) and `discussion-baseline.log`. Source re-read at `9ebe81c1b` only where an item's disposition depended on it. + +**VERDICT: APPROVE_WITH_NONBLOCKING_NOTES** for `docs/investigations/r5/plan-v2.md`, consensus target sha256 `5a782acf3f285b23d1ae43a73481c4eb772a9a6d917fc5a550ecfc7cbf7446ca`. **Blocking items remaining: 0 (zero).** All five v1 blockers are resolved; R2 I withdraw as a blocker on the evidence below. + +Same binding caveat as v1, unchanged: this session has Glob/Grep/Read only, so I did not execute `sha256sum`. I reviewed the file at that path in this worktree at the stated base and cite its line numbers so findings bind to the hash. + +## Per-item resolution + +| Item | Status | Basis | +|---|---|---| +| **R1** empty/no-revision ModTime fallback | **Resolved** | Plan L35 is exactly my v1 correction: revision-when-recorded, existing nonempty ModTime fallback retained, empty + no revision sends nothing. §B and §D (L61) are now consistent. | +| **R2** forced metadata replication scope | **Resolved — my correction was wrong; I withdraw the blocker** | See below. | +| **R3** production COPY request shape | **Resolved, with wire evidence** | `TestTaggingProductionCopyWireShape` (matrix_test.go:436-459) drives the real `minio.Core.CopyObject` against an httptest peer: `metadata-directive="" tagging-directive="REPLACE"` plus the tagging timestamp header (log line 6). L17 now states both shapes and requires covering a peer that sends metadata REPLACE. | +| **R4** local revision inversion | **Resolved and correctly strengthened** | L31's uniform multi-pool requirement is necessary, and **my v1 note was wrong** — see correction 2. `TestLocalTaggingCommitCannotRegressRevision` encodes max-across-pools + 1ns in the response and every copy; it fails baseline (log line 9: stamp stays `01:00:00Z`). | +| **R5** duplicate-suppression wording | **Resolved** | L51 is explicit: valid source revision *strictly newer than the destination's stored tag revision*, `olderThan` semantics, client preconditions preserved, re-upload cost documented. | + +## R2 adjudication — why I withdraw it + +**My proposed restriction was incorrect.** `TestTaggingRepeatedValueNeedsRevisionDelivery` (matrix_test.go:422-435) runs both rows; baseline returns `replicateNone` for equal `""` **and** for equal `"key=same"` with a revision an hour newer (log lines 2-3). An empty-only condition fixes only the first row, so it does not repair X@T1 / delete@T2 / X@T3 with a reordered delete. The scenario is reachable: `er.PutObjectTags` never touches `fi.ModTime` (`cmd/erasure-object.go:2328-2332`), so the full-copy gate `oi1.ModTime.Unix() != oi2.LastModified.Unix()` (`:975-981`) never fires on tagging-only changes and the value comparison at `:1003` is decisive; concurrent workers for the same object are not serialized by revision, and at `:1600-1625` a `replicateNone` result is force-marked Completed, making the loss permanent and silent. + +**My cost rationale is refuted by source, not merely by assertion.** `queueReplicationHeal` returns at `cmd/bucket-replication.go:3768` for `Completed && VersionPurgeStatus.Empty() && !mustResync()`; `replicateObject` requeues only non-Completed at `:1306`. I also checked the feedback path I would have raised in its place: `mustReplicate` returns an empty decision for an incoming replication request (`:270-272`), so a forced COPY cannot schedule a new event at the destination — no active-active ping-pong. And the blast radius is **narrower than the plan claims**: `ObjectReplicationType` dispatches to `ri.replicateObject` (`:1233-1237`), which never calls `getReplicationAction`, so the extra COPY applies only to Metadata/Heal/ExistingObject types. + +**Concrete tradeoff against a HEAD revision protocol.** The sender already extends the HEAD (`sOpts.Set(xhttp.AmzTagDirective, "ACCESS")`, `:1595`), so the idea is not absurd — but the pinned SDK's `extractObjMetadata` (`minio-go@v7.3.1-0.20260910142817.../utils.go:232-277`) preserves only the whitelist plus `x-amz-meta-`/`X-Minio-Meta-`; any `x-minio-internal-*` response header is discarded. Exposing the revision therefore needs (a) a new target-side response header, in a client-visible namespace or behind an SDK whitelist change, (b) a sender-side read path, and (c) a fallback for peers that do not answer — and that fallback is the forced COPY anyway. Strictly more code, a new cross-version wire contract, and the same worst case. The plan's choice is right; L39 already states the residual cost honestly. + +## Corrections to my own v1 non-blocking claims + +1. **KMS / COPY (as you flagged).** My v1 line — "the metadata-COPY leg is **not** affected" — is wrong. The sender indeed forwards no public SSE header, but `CopyObjectHandler` applies the destination bucket's SSE config and `globalAutoEncryption` to `r.Header` at `cmd/object-handlers.go:1428-1433`, *before* `copyDstOpts` → `putOptsFromReq` → `putOpts` → `putOptsFromHeaders`, whose `crypto.S3KMS.IsRequested(hdr)` branch (`cmd/object-api-options.go:431-461`) returns an ObjectOptions carrying the legal-hold and retention timestamps but **not** `ReplicationSourceTaggingTimestamp`. So COPY does depend on R4 whenever the destination bucket has default KMS or auto-encryption is on. Plan L59 states this correctly; do not adopt my broader exclusion. +2. **Multi-pool merge.** My v1 R4 note claimed per-pool stamp differences merge safely via `mergedPoolObjectInfo`. They do not on ordinary reads: `z.GetObjectInfo` → `getLatestObjectInfoWithIdx` (`cmd/erasure-server-pool.go:1121`, `:1032-1072`) returns one pool's `ObjectInfo`, sorted by ModTime with a lowest-index tiebreak — and ModTime is identical across copies for tagging changes. `mergedPoolObjectInfo` is reached only from `replicaObjectInfo` and `updatePoolMetadata` (`cmd/erasure-server-pool-consistency.go:135-147`, `:169-174`). Since the replication sender reads through `GetObjectNInfo`, it can emit a stale primary-pool revision. L31's uniform value is required. + +## Non-blocking notes for v2 + +- **Tighten the cost statement** in L39 to Metadata/Heal/ExistingObject types only (`ObjectReplicationType` bypasses the predicate). It makes the accepted cost smaller and the test targets sharper. +- **Add a termination regression** asserting `mustReplicate` yields no decision for the incoming forced COPY (`:270-272`). That property, not the scanner gates alone, is what makes the rule terminating under bidirectional configurations. +- **Assert ModTime invariance across tagging** (`:2328-2332`). If a future change ever bumped ModTime on tagging, the equal-value skip disappears and the whole cost calculus shifts; a one-line assertion pins the premise. +- **Multi-pool guard mechanics.** `z.PutObjectTags` returns `copies[0]`'s result (`cmd/erasure-server-pool.go:3059-3067`) and is bypassed entirely for `SinglePool()` (`:3035-3036`). Keep the guard in `er.PutObjectTags` as the invariant holder, have it advance only when the passed value is not strictly newer than that set's stored value, and have the response report what was actually written. Transient per-set divergence from a direct-to-set write outside the pool lock re-converges on the next z-level write; say so rather than implying it cannot happen. +- **`replicateNone` branch side effect.** Revisioned objects now reach `applyAction` instead of the force-Completed path at `:1614-1624`, so a failed COPY becomes Failed + MRF rather than a phantom Completed, and `rinfo.ReplicationAction` flips `none`→`metadata`. Right direction; cover the metrics path once. +- **Sender parse asymmetry.** The receiver fails closed on a malformed tag timestamp (`cmd/object-api-options.go:421-424`) while the COPY sender swallows it (`cmd/bucket-replication.go:1695-1700`). L35/L37 covers this; just confirm the COPY call site is in scope, since it is a different function from `putReplicationOpts`. + +I made no edits, launched no agents, and executed nothing. No implementation is proposed or requested here, so I am not calling ExitPlanMode. \ No newline at end of file diff --git a/docs/investigations/r5/plan-v1.md b/docs/investigations/r5/plan-v1.md new file mode 100644 index 000000000..e4549729c --- /dev/null +++ b/docs/investigations/r5/plan-v1.md @@ -0,0 +1,75 @@ +# R5 repair plan v1 — tag deletion and ordered replication + +Status: proposed, no production implementation before real Opus consensus. +Base: `9ebe81c1b3611f9cc73e676b5b741c2be62c467a` (local and GitHub main checked). + +## Problem and necessity + +A tag value and its `x-minio-internal-tagging-timestamp` form one state, including an empty value. Successful DeleteObjectTagging currently leaves the old timestamp. A delayed, authenticated metadata COPY newer than that old timestamp restores deleted tags. The parent and this worktree reproduce this through signed HTTP with real single-drive and 16-drive storage. An empty incoming COPY is also ignored. + +The consequence is persistent incorrect user metadata on the source or replicas, affecting tag-based access/lifecycle behavior. Repair is justified before claiming replication correctness. It is not a reason to change the supported PGSTY dependency graph, storage format, encryption protocol, or Object Lock behavior. + +## Current chain and additional gaps + +1. `PutObjectTaggingHandler` stamps only when replication is selected. `DeleteObjectTaggingHandler` never stamps. Both storage methods write the value under the existing object lock; multi-pool PutObjectTags updates the addressed version in each copy. +2. `putReplicationOpts` sets the timestamp only inside nonempty UserTags. Multipart uses this builder and clears SourceMTime at initiation, so using initiation time as a tag fallback would be wrong. +3. Metadata COPY carries an explicit empty tag via `getCopyObjMetadata`, but only defaults its timestamp to object ModTime for nonempty tags. +4. `CopyObjectHandler` branches on nonempty tags; its REPLACE metadata map also loses the previous timestamp before comparison. +5. `PutObjectHandler` and `NewMultipartUploadHandler` parse the source timestamp but never persist it in metadata. Multipart completion already rechecks the upload's persisted metadata against the addressed destination version under the object lock. +6. `getReplicationAction` compares tag values/counts, not their ordering time. An empty-to-empty deletion revision can be declared complete without transmitting the revision. +7. `checkPreconditionsPUT` skips matching version/ETag for non-SSE-C replicas even when their tag revision is newer. This affects PUT and multipart initiation. Explicit client If-Match/If-None-Match checks must still apply. +8. `replicateObject`'s completion metadata callback copies nonempty `ri.UserTags` from the old queue snapshot. A deletion committed before this worker's current-object read can be overwritten at acknowledgment. A status update must retain the tags read under its own metadata lock. + +The existing storage fix (`3ce831925`) already supplies `reconcileStoredObjectTags` in PUT, COPY, multipart completion and all-pool reconciliation. Its strict ordering keeps stored state on equal timestamps and keeps a valid stored revision against missing/invalid incoming timestamps. Reuse these gates, do not replace them. + +## Proposed minimal changes + +### A. Produce local revisions + +In both PUT tagging and DELETE tagging handlers, allocate opts.UserDefined if needed and assign a single UTCNow RFC3339Nano tag revision for each authorized mutation, independent of current replication selection. Use the same time for ReplicationTimestamp when replication is selected. This also covers empty PUT tagging and mutations while replication is disabled. Persist through existing PutObjectTags/DeleteObjectTags locks. Ordinary COPY must write an empty REPLACE tag and a fresh tag timestamp too. + +This preserves the existing wall-clock conflict model, not a new distributed causal clock. The timestamp is generated before the storage lock as in existing PUT tagging. Reviewer should explicitly assess whether a commit-time generation change is necessary for this scoped repair; if necessary, revise before implementing. Clock skew and simultaneous conflicting equal revisions cannot be completely ordered by this protocol. + +### B. Transport complete state + +Move tag timestamp selection outside the nonempty-value branch in putReplicationOpts. Use recorded RFC3339Nano time when present, otherwise object ModTime (also for empty legacy objects). Malformed recorded timestamps fail option construction rather than being silently treated as fresh. + +Use the same selection for metadata COPY; a small shared timestamp helper is acceptable to prevent inconsistent error/fallback rules. Preserve explicit empty tag REPLACE metadata. Multipart initiation retains this timestamp even though SourceMTime is cleared. + +When getReplicationAction sees a recorded tag revision after the existing identity/full-copy checks, select metadata replication even if visible values match: HEAD does not expose that revision. Do not change the existing null-version resync exclusion. This is an extra COPY only for already-scheduled work with an explicit revision, including retry/heal; it does not add scans or network calls on ordinary object reads. Avoid a new HEAD protocol merely to save that COPY. + +Remove the stale ri.UserTags assignment from replication completion metadata write-back. The callback changes replication status only; existing metadata write-back preserves the current tags and timestamp. + +### C. Accept, order, and persist + +COPY captures the stored UserTags and timestamp before metadata reconstruction. For a trusted replication request with a nonzero source timestamp, install the incoming tag value (including empty) with that timestamp, then reuse reconcileStoredObjectTags against the captured state. A missing source timestamp preserves stored state for metadata COPY. Storage rechecks under its lock, including all pools. Equal timestamps keep stored state. Ordinary COPY generates a fresh revision for its chosen tags, including empty. + +Ensure the final encMetadata merge cannot restore a stale tag timestamp over the accepted pair (SSE-C rotation snapshots reserved keys). Reconcile the timestamp entry in encMetadata with the tag decision before merging, using the existing lock-timestamp pattern. + +PUT and multipart initiation persist a nonzero parsed trusted source timestamp into the existing metadata map before entering storage. Their existing lock/reconcile flags retain the newest state. Completion takes tag state from the persisted upload, not client-supplied completion headers, and orders it again against current state. + +For trusted REPLICA PUT/multipart initiation, a valid newer source tag timestamp makes matching version/ETag insufficient to skip the request. Preserve explicit If-Match/If-None-Match and existing SSE-C behavior. Duplicate/equal/older non-SSE-C writes may keep their existing no-op/412 behavior; the sender treats these as already delivered. Completion does not set PreserveETag and does not need a new duplicate exception. + +### D. Boundaries and compatibility + +R4 owns object-api-options.go SSE-KMS common-field preservation. R5 will not implement it. R4 will provide a reviewed patch for isolated combined KMS verification. R5 owns the handlers, sender and tag-related duplicate exception. + +No migration: historical deletions with missing/wrong timestamps have irrecoverably lost ordering information. We do not invent historical deletion times or rewrite production state. A fresh authenticated tagging mutation after upgrade produces an ordered state. Upgrade both sender and receiver for complete guarantees; older peers may continue to drop empty revisions. No main merge, push, release or deployment is authorized here. + +## Verification matrix + +- Re-run parent overlay on exact HEAD; preserve raw failures. Extend temporary reproduction for PUT/multipart lost persistence, empty-to-empty sender decision, and matching-content newer revision skip. +- Signed HTTP tag PUT/delete, active replication and no selected replication, empty PUT, repeated DELETE; check response and persisted tag/time, worker scheduling when active. +- COPY old/new/equal/missing/invalid source time, empty/nonempty, metadata COPY/REPLACE, explicit UUID/null version; protect unrelated/latest versions and local empty COPY. +- Production putReplicationOpts/SDK headers and actual metadata sender requests: explicit empty tombstone, legacy ModTime fallback, nanoseconds, malformed timestamp. Equal empty values must still send ordered deletion; exercise retry after failed send. +- PUT and multipart through signed requests/SDK: first receipt, newer removal, older replay after removal, duplicate receipt, missing timestamp. For multipart, mutate tags between initiation and completion and check final disk state. +- Multi-pool real-storage fixture with duplicate UUID/null versions, newer tag state in secondary pool, all-pool persistence/retirement; reuse current tag storage suite and failure-closed coverage. Include a deterministic update after handler snapshot to show storage lock recheck. +- Old queued replication event followed by deletion: process event and confirm source completion callback cannot restore tags; check tag/time after reread. +- Run related replication trust, Object Lock/SSE-C retransmit, tag storage and API precondition tests; targeted race tests, gofmt, git diff --check. No whole-repository tests in parallel with other R tasks without need. +- KMS combined dependent tests only after R4 reviewed change is available. Report R5-only and combined results separately. + +## Work and acceptance + +Estimated 2–4 engineer days including replication boundary tests and review. The patch should remain localized; added regression code is expected to exceed production LOC. Completion requires a reviewable diff, actual Opus model/effort record, same-plan consensus, meaningful persisted-state tests, and explicit local versus merge/release state. Not production multi-site acceptance. + +Review questions: Are A's existing local timestamp generation semantics sufficient here? Are B's forced metadata synchronization, stale ACK removal and C's narrow duplicate exception necessary and correctly scoped? Is any incoming/forwarding/multi-pool path still able to lose or revive the deletion revision? diff --git a/docs/investigations/r5/plan-v2.md b/docs/investigations/r5/plan-v2.md new file mode 100644 index 000000000..c403c6a90 --- /dev/null +++ b/docs/investigations/r5/plan-v2.md @@ -0,0 +1,81 @@ +# R5 repair plan v2 — tag deletion and ordered replication + +Status: proposed v2, responding to real Opus v1 REQUEST_CHANGES; no production implementation before consensus. +Base: `9ebe81c1b3611f9cc73e676b5b741c2be62c467a` (local and GitHub main checked). + +## Problem and necessity + +A tag value and its `x-minio-internal-tagging-timestamp` form one state, including an empty value. Successful DeleteObjectTagging currently leaves the old timestamp. A delayed, authenticated metadata COPY newer than that old timestamp restores deleted tags. The parent and this worktree reproduce this through signed HTTP with real single-drive and 16-drive storage. An empty incoming COPY is also ignored. + +The consequence is persistent incorrect user metadata on the source or replicas, affecting tag-based access/lifecycle behavior. Repair is justified before claiming replication correctness. It is not a reason to change the supported PGSTY dependency graph, storage format, encryption protocol, or Object Lock behavior. + +## Current chain and additional gaps + +1. `PutObjectTaggingHandler` stamps only when replication is selected. `DeleteObjectTaggingHandler` never stamps. Both storage methods write the value under the existing object lock; multi-pool PutObjectTags updates the addressed version in each copy. +2. `putReplicationOpts` sets the timestamp only inside nonempty UserTags. Multipart uses this builder and clears SourceMTime at initiation, so using initiation time as a tag fallback would be wrong. +3. Metadata COPY carries an explicit empty tag via `getCopyObjMetadata`, but only defaults its timestamp to object ModTime for nonempty tags. +4. `CopyObjectHandler` branches on nonempty tags. Production getCopyObjMetadata + SDK Core.CopyObject sends tagging REPLACE but no metadata directive, so its default metadata map retains the old timestamp. A peer using metadata REPLACE additionally loses that timestamp before the handler comparison. Cover both shapes; the empty-value skip affects both. +5. `PutObjectHandler` and `NewMultipartUploadHandler` parse the source timestamp but never persist it in metadata. Multipart completion already rechecks the upload's persisted metadata against the addressed destination version under the object lock. +6. `getReplicationAction` compares tag values/counts, not their ordering time. An empty-to-empty deletion revision can be declared complete without transmitting the revision. +7. `checkPreconditionsPUT` skips matching version/ETag for non-SSE-C replicas even when their tag revision is newer. This affects PUT and multipart initiation. Explicit client If-Match/If-None-Match checks must still apply. +8. `replicateObject`'s completion metadata callback copies nonempty `ri.UserTags` from the old queue snapshot. A deletion committed before this worker's current-object read can be overwritten at acknowledgment. A status update must retain the tags read under its own metadata lock. + +The existing storage fix (`3ce831925`) already supplies `reconcileStoredObjectTags` in PUT, COPY, multipart completion and all-pool reconciliation. Its strict ordering keeps stored state on equal timestamps and keeps a valid stored revision against missing/invalid incoming timestamps. Reuse these gates, do not replace them. + +## Proposed minimal changes + +### A. Produce local revisions + +In both PUT tagging and DELETE tagging handlers, allocate opts.UserDefined if needed and assign a single UTCNow RFC3339Nano tag revision for each authorized mutation, independent of current replication selection. Use the same time for ReplicationTimestamp when replication is selected. This also covers empty PUT tagging and mutations while replication is disabled. Persist through existing PutObjectTags/DeleteObjectTags locks. Ordinary COPY must write an empty REPLACE tag and a fresh tag timestamp too. + +Under the existing storage write lock, guard local tagging revisions against regression: a valid supplied tag revision not strictly after the valid stored revision is advanced to stored + 1ns. Do this in er.PutObjectTags; for multi-pool writes, z.PutObjectTags first computes one revision strictly beyond every addressed copy and passes that identical value to all sets. This is necessary because ordinary replication source reads/returned primary ObjectInfo can observe one physical pool; merely allowing different pool revisions with equal values can send a revision older than an already-replicated tombstone. Only explicit valid local tag revisions are advanced; replicated PUT/COPY/multipart retain strict source ordering, and direct storage calls without a supplied revision retain current legacy semantics. Generate before locking as today; the guard runs under the lock. Preserve the requested map from unintended shared mutation. This is a per-object monotonic guard in the existing RFC3339Nano domain, not a new wire clock. Equal independent remote conflicting revisions still keep stored state; arbitrary distributed clock skew is not totally ordered by this protocol. + +### B. Transport complete state + +Move tag timestamp selection outside the nonempty-value branch in putReplicationOpts. Use recorded RFC3339Nano time when present even for an empty value. Without a recorded revision, retain the existing ModTime fallback only for nonempty tags; empty + no revision sends no timestamp. This avoids inventing a tombstone for never-tagged/historically unordered objects. Malformed recorded timestamps fail option construction rather than being silently treated as fresh. + +Use the same selection for metadata COPY; a small shared timestamp helper is acceptable to prevent inconsistent error/fallback rules. Preserve explicit empty tag REPLACE metadata. Multipart initiation retains this timestamp even though SourceMTime is cleared. + +When getReplicationAction sees a recorded tag revision after the existing identity/full-copy checks, select metadata replication even if visible values match: HEAD does not expose that revision. Keep this for empty AND nonempty states. Example: destination key=X@T1, source deleted at T2 and re-added key=X@T3; if the equal nonempty state skips T3, delayed delete T2 incorrectly removes X. An empty-only condition does not fix ordered deletion/re-addition. Preserve existing null-version resync exclusion. Cost: every explicitly scheduled retry/heal/resync for an object with a recorded revision may require a metadata COPY, even if already converged. It does not create a background retry loop: queueReplicationHeal returns early for Completed objects without requested resync (bucket-replication.go around 3758), and replicateObject requeues only failed results (around 1305). Successful copies remain Completed. Never-tagged objects retain the old skip optimization under the preceding rule. Accept the extra metadata I/O during explicit resync as the smallest correctness-complete option; avoid introducing an authenticated HEAD revision protocol solely as an optimization. + +Remove the stale ri.UserTags assignment from replication completion metadata write-back. The callback changes replication status only; existing metadata write-back preserves the current tags and timestamp. + +### C. Accept, order, and persist + +COPY captures the stored UserTags and timestamp before metadata reconstruction. For a trusted replication request with a nonzero source timestamp, install the incoming tag value (including empty) with that timestamp, then reuse reconcileStoredObjectTags against the captured state. A missing source timestamp preserves stored state for metadata COPY. Storage rechecks under its lock, including all pools. Equal timestamps keep stored state. Ordinary COPY generates a fresh revision for its chosen tags, including empty. + +Ensure the final encMetadata merge cannot restore a stale tag timestamp over the accepted pair (SSE-C rotation snapshots reserved keys). Reconcile the timestamp entry in encMetadata with the tag decision before merging, using the existing lock-timestamp pattern. + +PUT and multipart initiation persist a nonzero parsed trusted source timestamp into the existing metadata map before entering storage. Their existing lock/reconcile flags retain the newest state. Completion takes tag state from the persisted upload, not client-supplied completion headers, and orders it again against current state. + +For trusted REPLICA PUT/multipart initiation, a valid source tag timestamp strictly newer than the destination's stored tag revision makes matching version/ETag insufficient to skip the request. Use the existing olderThan predicate (zero never wins, valid source beats missing/invalid stored). Preserve explicit If-Match/If-None-Match and existing SSE-C behavior. Duplicate/equal/older non-SSE-C writes may keep their existing no-op/412 behavior; the sender treats these as already delivered. Completion does not set PreserveETag and does not need a new duplicate exception. This narrow exception can re-upload data to carry a tag-only revision; normal metadata work uses COPY, while full retransmission must not silently acknowledge a newer revision it did not persist. + +### D. Boundaries and compatibility + +R4 owns object-api-options.go SSE-KMS common-field preservation. R5 will not implement it. R4 will provide a reviewed patch for isolated combined KMS verification. R5 owns the handlers, sender and tag-related duplicate exception. + +Compatibility: equal-timestamp COPY consistently keeps stored state, including unqualified and explicit null requests; this aligns with existing storage tie behavior and changes the old unqualified handler incoming-wins tie. Preserve the existing tag trust predicate (trusted replication marker) and stronger ReplicaLockReconcile predicate (trusted marker + REPLICA and addressed version), without expanding trust. Production replication supplies both. + +Scope limitations: ordinary full object PUT/multipart creation retain their existing nonempty ModTime fallback; this change does not order independent unversioned content replacements against one another. Existing tag-filter target eligibility can exclude post-deletion empty tags; target-selection semantics are not changed here. This work guarantees correct tag ordering along selected per-hop replication requests, not every replication-rule configuration. Destination bucket-default/global-auto KMS is applied before copyDstOpts (object-handlers.go 1425-1435), so COPY can depend on R4 even when the sender does not forward an explicit encryption header. PUT/multipart KMS also require R4. + +No migration: historical deletions with missing/wrong timestamps have irrecoverably lost ordering information. We do not invent historical deletion times or rewrite production state. A fresh authenticated tagging mutation after upgrade produces an ordered state. Upgrade both sender and receiver for complete guarantees; older peers may continue to drop empty revisions. No main merge, push, release or deployment is authorized here. + +## Verification matrix + +- Re-run parent overlay on exact HEAD; preserve raw failures. Extend temporary reproduction for PUT/multipart lost persistence, empty-to-empty sender decision, and matching-content newer revision skip. +- Signed HTTP tag PUT/delete, active replication and no selected replication, empty PUT, repeated DELETE; check response and persisted tag/time, worker scheduling when active. +- COPY old/new/equal/missing/invalid source time, empty/nonempty, metadata COPY/REPLACE, explicit UUID/null version; protect unrelated/latest versions and local empty COPY. +- Production putReplicationOpts/SDK headers and actual metadata sender requests: explicit empty tombstone, nonempty legacy ModTime fallback, no fabricated empty legacy revision, nanoseconds, malformed timestamp. Equal empty values must still send ordered deletion; equal nonempty values must send re-addition revisions to defeat intervening delayed deletions. Pin actual SDK metadata COPY headers, and cover peer metadata REPLACE independently. Exercise retry after failed send. +- PUT and multipart through signed requests/SDK: first receipt, newer removal, older replay after removal, duplicate receipt, missing timestamp. For multipart, mutate tags between initiation and completion and check final disk state. +- Multi-pool real-storage fixture with duplicate UUID/null versions, newer tag state in secondary pool, all-pool persistence/retirement; reuse current tag storage suite and failure-closed coverage. Include a deterministic update after handler snapshot to show storage lock recheck. A local DELETE-to-PUT inversion with supplied older timestamp must produce one revision greater than the maximum across pools, in both response and every stored copy. +- Old queued replication event followed by deletion: process event and confirm source completion callback cannot restore tags; check tag/time after reread. +- Run related replication trust, Object Lock/SSE-C retransmit, tag storage and API precondition tests; targeted race tests, gofmt, git diff --check. No whole-repository tests in parallel with other R tasks without need. +- KMS combined dependent tests only after R4 reviewed change is available. Report R5-only and combined results separately. + +## Work and acceptance + +Estimated 2–4 engineer days including replication boundary tests and review. The patch should remain localized; added regression code is expected to exceed production LOC. Completion requires a reviewable diff, actual Opus model/effort record, same-plan consensus, meaningful persisted-state tests, and explicit local versus merge/release state. Establishes per-hop behavior with deterministic clock/commit interleaves; not production multi-site or real host-clock-skew acceptance. + +## v2 review focus + +See `opus-v1-response.md` for every blocking/nonblocking disposition and exact counterarguments. R1/R3/R4/R5 accepted with concrete changes. R2 is disputed as proposed: empty-only forced transfer is insufficient for same-value re-addition after deletion, and Completed scanner gates bound work. Please adjudicate on this v2 hash, not on general preference for avoiding metadata I/O. Do not treat unresolved disagreement as consensus. diff --git a/docs/investigations/r5/related-test-names.txt b/docs/investigations/r5/related-test-names.txt new file mode 100644 index 000000000..c0a30ed69 --- /dev/null +++ b/docs/investigations/r5/related-test-names.txt @@ -0,0 +1,135 @@ +TestPeerBucketCorsReplicationOrdering +TestSiteReplicationMetaInfoPreservesCorsTombstone +TestSiteReplicationStatusDetectsCorsTimestampMismatch +TestSiteReplicationStatusCountsCorsPerSite +TestCORSReplicationStateOrdering +TestCORSReplicationStatusStateEquality +TestNewBucketCORSReplicationEvent +TestCorsReplicationDispatchStatusHealReload +TestMarshalUnmarshalReplicationMRFStats +TestEncodeDecodeReplicationMRFStats +TestMarshalUnmarshalBucketReplicationResyncStatus +TestEncodeDecodeBucketReplicationResyncStatus +TestMarshalUnmarshalReplicationState +TestEncodeDecodeReplicationState +TestMarshalUnmarshalTargetReplicationResyncStatus +TestEncodeDecodeTargetReplicationResyncStatus +TestCompositeReplicationStatus +TestReplicationResyncwrapper +TestReplicationValidationObjectUsesRulePrefix +TestGetReplicationActionEmptyObjectLockValues +TestReplicationActionForTargetRetentionRemoval +TestReplicationActionForTargetNullVersionResync +TestReplicationActionForTargetTimestampOnlyRemoval +TestMarshalUnmarshalBucketReplicationStat +TestEncodeDecodeBucketReplicationStat +TestMarshalUnmarshalBucketReplicationStats +TestEncodeDecodeBucketReplicationStats +TestMarshalUnmarshalReplicationLastHour +TestEncodeDecodeReplicationLastHour +TestMarshalUnmarshalReplicationLastMinute +TestEncodeDecodeReplicationLastMinute +TestMarshalUnmarshalReplicationLatency +TestEncodeDecodeReplicationLatency +TestMarshalUnmarshalReplicationQueueStats +TestEncodeDecodeReplicationQueueStats +TestAPISSECCompressionReplicaStaysReadable +TestSSECBatchReplicationCannotRead +TestAPIDeleteObjectVersionDenyAndReplicationCompatibility +TestAPISSECReplicaPartNumberReads +TestAPISSECReplicaMalformedPartIsRejected +TestPoolsDeleteVersionAPI +TestPoolsDeleteVersionUnreadablePool +TestPoolsDeleteVersionSingleCopy +TestPoolsDeleteVersionReplicationPurge +TestPoolsDeleteVersionCleanupFailure +TestPoolsDeleteVersionCallbacks +TestPoolsDeleteVersionSpecialCalls +TestPoolsDeleteUnversionedFanout +TestPoolsConditionalDeleteVersionSelection +TestPoolsConditionalDeleteDuplicateVersion +TestPoolsConditionalDeleteReportsOtherPoolFailure +TestPoolsConditionalDeleteSerializesPut +TestPoolsConditionalDeleteSerializesCompletion +TestPoolsReplicaSerializesMetadataAndHealing +TestPoolsConditionalDeletePreservesVersionHistory +TestPoolsReplicaIndependentLockWinners +TestPoolsReplicaSoleDrainingOwner +TestPoolsMetadataUpdateUsesMergedVersion +TestPoolsReplicaMetadataCopyReconcilesLockAndTags +TestPoolsReplicaCleanupFailureCanRetry +TestPoolsRetiringCopyPreservesSharedTierObject +TestPoolsDeleteVersionAfterInterruptedRebalance +TestPoolsDeleteDirectoryMarker +TestPoolsMultipartConditionalUsesLogicalLatest +TestPoolsMultipartConditionalHTTPMatrix +TestPoolsMultipartConditionalHTTPAbsentObject +TestPoolsMultipartConditionalHTTPNormalRouting +TestPoolsMultipartConditionalUnreadablePool +TestPoolsMultipartConditionalLatestVersionAndCallbackOnce +TestPoolsMultipartConditionalConcurrentCompletes +TestPoolsMultipartConditionMatrix +TestPoolsMultipartConditionBoundaries +TestPoolsMetadataUpdatePreservesTags +TestReplicaWritesPreserveTagOrdering +TestMergedPoolObjectInfoTagOrdering +TestPoolsMetadataCallbackReplacesTags +TestReconcileStoredObjectTagOrdering +TestPoolsMetadataUpdatePreservesAbsentTags +TestExtractReplicationMetadataHeaders +TestGetCopyObjectMetadataFromHeaderReplication +TestCloneRequestWithoutReplicationHeaders +TestIAMServiceAccountReplicationRejectsOtherCredentialKinds +TestIAMServiceAccountReplicationPreservesExpiration +TestPutOptsFromHeadersReplicationTimestamps +TestAPIGetObjectAttributesSSECReplicationAuthz +TestAPICopyObjectSSECKeyRotationReplicaKeepsFastPath +TestAPIFederatedCopyObjectRejectsRawSSECReplica +TestAPICopyObjectReplicaTaggingTimestampUnderKMS +TestCheckPreconditions +TestAPIPutObjectReplicationHeaderPoisoning +TestAPICopyObjectReplicationHeaderPoisoning +TestPostPolicyCannotForgeReplicationStatus +TestReplicationMRFDropsVisible +TestReplicationObjectDeleteWorkerAffinity +TestAPISSECReplicationTargetHead +TestAPISSECReplicaRetransmitOverExistingVersion +TestPutReplicationOptsRetentionRemoval +TestAPISSECReplicaWriteExemptionIsKeyedOnTheIncomingWrite +TestAPISSECReplicaRetransmitObjectLockOrdering +TestAPISSECReplicaRetransmitMultipartObjectLockOrdering +TestPutReplicationOptsRetentionRemovalTimestampOnly +TestAPIReplicaMarkerOnlyAppliesObjectLock +TestAPIReplicaMultipartNewerHoldSurvivesCompletion +TestAPITaggingReplicationOrdering +TestAPITaggingReplicationOrderingKMS +TestAPITaggingMultipartCommitRechecksRevision +TestAPILocalTaggingAlwaysAdvancesRevision +TestTaggingTimestampWire +TestAPIPoolsTaggingReplicaDeletion +TestAPITaggingSSECRotationPreservesDeletionRevision +TestTaggingRepeatedValueNeedsRevisionDelivery +TestTaggingProductionCopyWireShape +TestLocalTaggingCommitCannotRegressRevision +TestTaggingReplicaContentDuplicateGuard +TestAPITaggingUnqualifiedCopyOrdering +TestTaggingReplicationSenderRetryAndAcknowledgment +TestAPISSECReplicaSkipsDestinationTransforms +TestAPISSECMultipartReplicaRoundTripWithCompression +TestPutReplicationOptsRejectsCompressedSSEC +TestAPIReplicationTrustProtectsSSECReads +TestReplicationTrustControlsInternalOptionsAndEvents +TestAPIPutObjectReplicationTrust +TestAPISnowballReplicationTrustIsPerEntry +TestAPIDeleteObjectReplicationTrust +TestAPISSECMultipartReplicationTrust +TestAPIStreamingTrailerWithUntrustedReplicationHeaders +TestAPICopyObjectReplicaLegalHoldTimestamp +TestAPICopyObjectReplicaAbsentLockFieldsPreserveNewerState +TestAPICopyObjectReplicaRetentionRemovalKeepsOrderingTimestamp +TestAPICopyObjectReplicaObjectLockOrdering +TestAPICopyObjectReplicaRetentionRemovalUnderBucketKMS +TestAPICopyObjectReplicaLockTimestampSurvivesSSECKeyRotation +TestBucketPolicyReplicationKey +TestBucketPolicyReplicationStatusLegacyOrder +TestSiteReplicationStatusAccountsPerSiteAndSurvivesMalformedConfig diff --git a/docs/investigations/r5/reproduction.md b/docs/investigations/r5/reproduction.md new file mode 100644 index 000000000..2807406bb --- /dev/null +++ b/docs/investigations/r5/reproduction.md @@ -0,0 +1,38 @@ +# Current-baseline reproduction + +Base: `9ebe81c1b3611f9cc73e676b5b741c2be62c467a`. Go 1.27.1, macOS arm64. Production sources unchanged. Temporary overlay injects regression tests; real erasure disks persist object metadata. Capacity adapter changes only reported capacity to avoid the developer machine's unrelated disk-usage threshold. + +## Commands and raw evidence + +Raw directory: `/Users/vonng/tmp/silo-r5-20260915-77ad/`. + +```sh +GOMAXPROCS=2 go test -p 1 -overlay /Users/vonng/tmp/silo-r5-20260915-77ad/overlay.json ./cmd -run '^TestReviewR5' -count=1 -v +GOMAXPROCS=2 go test -p 1 -overlay /Users/vonng/tmp/silo-r5-20260915-77ad/overlay.json ./cmd -run '^TestReviewR5Queued' -count=1 -v +``` + +Both exit 1, as expected before repair. Files: `baseline.log`, `baseline-extended.log`, `baseline-ack.log`; the full injected source is `baseline_test.go`. + +## Observations + +| Regression | Observed result | +|---|---| +| Empty source tags with explicit revision | putReplicationOpts returns zero TaggingTimestamp | +| Signed HTTP DELETE on a versioned object with replication selected | 204, empty tags, one queued event, unchanged old timestamp | +| Delayed signed trusted COPY after DELETE | 200 and deleted tags restored | +| Newer empty signed COPY | 200, old nonempty tags/time remain | +| First signed replica PUT carrying tag timestamp | 200, timestamp absent in stored object | +| First signed replica multipart initiation carrying timestamp | 200, timestamp absent in persisted upload metadata | +| Equal empty source/target values, source has newer deletion revision | getReplicationAction returns none | +| Same ETag/version with newer trusted source tag revision | checkPreconditionsPUT skips request | +| Process an old queued tagging event after a stored deletion | replication completes; source ACK restores `key=queued` with the deletion timestamp | + +All HTTP/storage cases above ran on both ErasureSD (one real disk) and Erasure (16 real disks). The last case uses a local HTTP protocol peer for replication responses and the real source object layer. It manually persists the deletion revision before processing the old queue snapshot to isolate the ACK defect from the separate DELETE-handler defect. The worker reads current deleted tags, yet its completion callback restores stale queue tags. + +These are component/in-process HTTP integration results, not multi-site production acceptance. + +## Provenance + +Current git history attributes introduction of ReplicationSourceTaggingTimestamp in COPY to upstream `c4373ef29` (2021-09-18, multi-site replication). COPY sender timestamps were added in `3781a0f9a` (2023-12-13), with default tag timestamps in `64a8f2e55` (2025-02-04). Queue-snapshot tag reassignment traces to `fa6d082bf` (2023-09-16). The storage tag reconciliation fix `3ce831925` is already present in this baseline and does not cover the HTTP/transport or queue-ACK omissions. + +No historical state can prove a missing deletion time. The planned repair records future revisions; a production backfill would need separate authoritative evidence and authorization. diff --git a/docs/investigations/r5/validation-results.json b/docs/investigations/r5/validation-results.json new file mode 100644 index 000000000..868e69194 --- /dev/null +++ b/docs/investigations/r5/validation-results.json @@ -0,0 +1,88 @@ +{ + "environment": { + "go": "go1.27.1 darwin/arm64", + "GOMAXPROCS": "2", + "go_test_p": "1" + }, + "new_r5_tests": { + "count": 13, + "exit_code": 0, + "log": "/Users/vonng/tmp/silo-r5-20260915-77ad/fixed-targeted-latest.log", + "runtime_seconds": 9.161 + }, + "related_selection": { + "selected_top_level_tests": 135, + "first_capacity_adapted_run": { + "passed": 134, + "failed": 1, + "failure": "POST fixture still used host capacity; XMinioStorageFull", + "exit_code": 1, + "log": "/Users/vonng/tmp/silo-r5-20260915-77ad/related-suite-capacity-final.log" + }, + "remaining_post_and_strengthened_pool_test": { + "passed": 2, + "failed": 0, + "exit_code": 0, + "log": "/Users/vonng/tmp/silo-r5-20260915-77ad/final-post-and-pools.log" + }, + "all_135_selected_tests_passed_across_batches": true, + "unfiltered_full_cmd_package_pass": false + }, + "race": { + "command": [ + "go", + "test", + "-race", + "-p", + "1", + "./cmd", + "-run", + "^(TestAPITagging.*|TestAPIPoolsTaggingReplicaDeletion|TestAPILocalTaggingAlwaysAdvancesRevision|TestTagging.*|TestLocalTaggingCommitCannotRegressRevision|TestReplicaWritesPreserveTagOrdering|TestMergedPoolObjectInfoTagOrdering|TestReconcileStoredObjectTagOrdering)$", + "-count=1", + "-v" + ], + "environment": { + "GOMAXPROCS": "2" + }, + "log": "/Users/vonng/tmp/silo-r5-20260915-77ad/targeted-race-final.log", + "exit_code": 0, + "duration_seconds": 121.658, + "passed_top_level_tests": 16, + "race_diagnostics": 0 + }, + "verifiers": { + "command": [ + "make", + "verifiers", + "GOLANGCI=/Users/vonng/tmp/silo-r5-20260915-77ad/golangci-lint-serial" + ], + "environment": { + "GOMAXPROCS": "2" + }, + "log": "/Users/vonng/tmp/silo-r5-20260915-77ad/make-verifiers-serial.log", + "exit_code": 0, + "duration_seconds": 203.553, + "wrapper": "Same repository-pinned lint binary with --allow-serial-runners to wait for the shared host lint lock." + }, + "build": { + "command": [ + "make", + "build" + ], + "environment": { + "GOMAXPROCS": "2" + }, + "log": "/Users/vonng/tmp/silo-r5-20260915-77ad/make-build.log", + "exit_code": 0, + "duration_seconds": 35.043, + "built_worktree_base": "dbcf8dec589deb5d91e17d295cb70997635f5b55", + "source_manifest": "docs/investigations/r5/final-implementation-manifest.json", + "version_exit_code": 0, + "version_output": "silo version DEVELOPMENT.2026-09-15T15-56-31Z (commit-id=dbcf8dec589deb5d91e17d295cb70997635f5b55)\nRuntime: go1.27.1 darwin/arm64\nLicense: GNU AGPLv3 - https://www.gnu.org/licenses/agpl-3.0.html\nCopyright: 2015-2025 MinIO, Inc.\nModifications: Copyright 2025-2026 PGSTY\nSource compatibility: based on MinIO technology" + }, + "limits": [ + "Existing TestReplicationResync order-dependent panic reproduced on the unpatched production baseline; passes in isolation on both versions.", + "Existing test capacity uses recorded test-only overlays, with real I/O and errors preserved.", + "Unfiltered full cmd package and production multi-site validation remain unperformed." + ] +} diff --git a/docs/investigations/r5/verification.md b/docs/investigations/r5/verification.md new file mode 100644 index 000000000..b157b10f8 --- /dev/null +++ b/docs/investigations/r5/verification.md @@ -0,0 +1,57 @@ +# R5 local verification + +## Scope and source identity + +This is a local repair of ordered tag deletion along selected replication requests. It does not authorize or establish a main merge, push, release, deployment, historical-state migration, or production multi-site acceptance. + +- Research baseline: `9ebe81c1b3611f9cc73e676b5b741c2be62c467a`. +- Combined verification dependency: R4 `dbcf8dec589deb5d91e17d295cb70997635f5b55`; R5 does not modify `cmd/object-api-options.go`. +- Accepted plan: v2, SHA256 `5a782acf3f285b23d1ae43a73481c4eb772a9a6d917fc5a550ecfc7cbf7446ca`. +- Actual plan reviewer: `claude-opus-5`, explicit max effort; v1 requested changes, v2 approved with nonblocking notes and zero blockers. See `consensus.md`. +- Actual implementation reviewer: the same requested/observed model and effort, `GO_WITH_NONBLOCKING_NOTES`, zero blockers. Original review identity and hashes are in `opus-implementation-metadata.json`. +- `implementation-manifest.json` identifies the reviewed patch. `final-implementation-manifest.json` identifies the final source after a stronger multi-pool test assertion and gofumpt formatting. All seven production file hashes still match the review. + +## Executed regression checks + +All commands run from this worktree with `GOMAXPROCS=2` and `go test -p 1`, Go `go1.27.1 darwin/arm64`. Raw logs are in `/Users/vonng/tmp/silo-r5-20260915-77ad/`. + +| Check | Observed result | Evidence | +|---|---|---| +| New R5 tests before implementation | Reproduced real signed-HTTP deletion resurrection, empty COPY loss, full-write persistence/skip, equal-value sender skip, local revision inversion and stale source ACK | `baseline*.log`, `discussion-baseline.log`; `reproduction.md` | +| Latest complete new R5 selection | 13 top-level tests passed, 9.161s | `fixed-targeted-latest.log` | +| Related selection, host capacity adapted | 134 passed; one POST fixture still failed the host minimum-free threshold, 56.007s | `related-suite-capacity-final.log`; exact 135 names in `related-test-names.txt` | +| Remaining POST test plus strengthened multi-pool replay test | Both passed, 3.232s; completes the 135-name selection across the two batches | `final-post-and-pools.log` | +| Existing `TestReplicationResync` in isolation | Passed on baseline (2.191s) and R5 (1.776s) | `baseline-resync.log`, `fixed-resync-isolated.log` | +| Final R5 plus tag-storage race selection | 16 top-level tests passed, 25.584s runtime, no race diagnostics | `targeted-race-final.log`; exact command/exit in `final-check-results.json` | +| Repository verifiers | Passed: lint 0 issues, generated files unchanged, branding/compatibility and entrypoint checks passed | `make-verifiers-serial.log`, `verifiers-result.json` | +| Repository build and binary invocation | `make build` passed; the resulting `silo --version` exited 0 | `make-build.log`, `build-result.json`, `silo-version.log` | + +The selected regressions include existing replication trust/header poisoning, API preconditions, Object Lock, SSE-C retransmission, R4 KMS option/COPY tests, and pool metadata/cleanup/retry checks. The new R5 suite covers: + +- Local PUT tags, repeated DELETE, empty PUT and ordinary empty COPY; tag revisions advance even without selected replication, while local tagging preserves object ModTime. +- Empty/nonempty and newer/stale/equal/missing revisions through signed COPY with both metadata directives, PUT and multipart; UUID/null and unqualified COPY; unrelated newer versions survive. +- Multipart deletion committed between initiation and completion, with the upload's saved revision checked at initiation and ordered again at completion. +- Exact SDK sender headers, nanosecond precision, legacy fallback only for nonempty tags, and rejection of malformed recorded times. +- Equal-value metadata resend, failed COPY reporting/retry, no incoming-replica requeue, and a stale queued source ACK preserving the current deletion. +- Uniform local tag revisions beyond every physical pool, deterministic inverted request/commit timestamps, normal-routing readback after replay and inspection of every retained pool copy. +- Destination KMS encryption/readback and signed SSE-C key rotation with decrypted GET. These are local handler/storage fixtures, not an encrypted-source-to-encrypted-destination two-site deployment. + +## Baseline and environment failures retained + +The first broad selection panics at `TestReplicationResync` before any R5 test executes. Replacing all seven R5 production files with the R4 baseline, and hiding the two new tests in a Go overlay, reproduces the same panic after the same preceding tests (`baseline-related-suite.log`). The test passes alone on both versions. The remaining 135-name selection therefore runs separately; this is not reported as an unfiltered full-package pass. + +This host's used-space percentage makes existing allocation tests return `XMinioStorageFull`. The test-only overlays add the existing `tagTestCapacityDisk` via `r5Capacity` at the API/pool fixture boundaries and the final POST fixture. The adapter changes reported capacity only, delegates real I/O and propagates disk errors. The exact overlays, original/modified fixture hashes and diffs are retained as `capacity-fixture*` and `capacity-post*`. No fixture overlay or capacity-policy change enters production code. + +The first adapted link and first verifier run also failed actual `ENOSPC` when the volume had about 200–500 MiB available (`related-suite-capacity.log`, `make-verifiers.log`). Regenerable Go cache data untouched for three days was reclaimed with an exact manifest (`cache-reclaim.json`); subsequent successful checks are distinguished from those failures. A subsequent verifier caught gofumpt formatting in the new helper; that formatting was corrected before final validation. + +An earlier KMS multipart fixture used a single-PUT ETag with multipart data layout and failed decryption. Seeding a real multipart source fixed the fixture; the subsequent complete run passed plaintext readback. The failed log remains `fixed-targeted.log`, and this is not attributed to a production encryption change. + +## Local delivery and remaining integration gates + +Required scoped local checks are complete. `validation-results.json` records command results, and `evidence-manifest.json` identifies the raw files and binary by SHA256. The build compiled the working source identified by `final-implementation-manifest.json`; the Makefile stamped its pre-commit dependency ID `dbcf8dec5` into this local development binary. Final source identity is established by the file hashes, not by that pre-commit version label. + +An unfiltered full `cmd` run and real multi-site deployment remain future integration gates before any separately authorized merge/release. Known scope limits are retained in plan v2 and `implementation-review-response.md`: tag-filter target eligibility, historical missing revisions, malformed stored source times, legacy peers dropping empty revisions, and arbitrary distributed clock skew. + +The verifier uses the repository-pinned golangci-lint v2.13.1 through a local wrapper adding only `--allow-serial-runners`. This waits for the shared host lint lock instead of running another lint process concurrently. The first unscheduled attempt was rejected by that lock (`make-verifiers-success.log`; despite that filename, its recorded exit is 2). The final serialized run passed. The optional `typos` binary is unavailable and was skipped by the Makefile. + +R4 has since merged as `af2b1794d38d9e70e1d2c3ee692426e4b6cab4bd` (PR #193). `dependency-handoff.json` verifies that its production options file and both test bodies match the dependency used above. The other differences are test license headers and R4 review/validation documents. The R5 delivery base is this exact merged dependency, with the old unsigned `dbcf8dec5` ancestor removed. The original recorded plan/review baseline remains intact as historical evidence. The final local commit, clean-worktree check and post-rebase file-hash comparison are recorded outside the commit in `/Users/vonng/tmp/silo-r5-20260915-77ad/final-delivery.json`.