minio/docs/investigations/r5/opus-implementation-review.md

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.