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.