minio/docs/investigations/r4-r8-integration/evidence/opus-review.md

The plan file can't be written (Write is disabled this session), so here is the complete review inline.

# SILO R4–R8 integration review — independent, source-only

**Verdict: GO_WITH_NONBLOCKING_NOTES — 0 blocking findings.**

**Candidate identity:** head `055030ea53ca92ee22ce1e601ef4757c247edde8`, branch `codex/merge-r4-r8`, base `9f3037e941a49ab4cd8a0eed7c0f01083fbe4bbe`, worktree `/Users/vonng/tmp/silo-r4-r8-main-20260916-01a0a5ab/silo`. Scope of this review is a **main merge**, not a release or deployment. I executed nothing — no tests, no builds; every statement below is from reading source.

**Inspected scope:** `integration-code.diff` in full (3367 lines) and all 22 files in `reviewed-source.json`, then the complete surrounding functions in the worktree: `cmd/bucket-replication.go` (`replicateDelete`, `replicateDeleteToTarget`, `replicateObject`, `replicateAll`, `putReplicationOpts`, `getReplicationAction`, `queueMRFHeal`, `queueReplicaDeleteTask`, `queueMRFSave`, `queueReplicationHeal`), `bucket-replication-utils.go`, `bucket-replication-stats.go`, `xl-storage-format-v2.go DeleteVersion`, `erasure-object.go`, `erasure-multipart.go`, `erasure-server-pool.go`, `erasure-server-pool-consistency.go`, `object-handlers.go`, `object-handlers-common.go`, `object-multipart-handlers.go`, `handler-utils.go`, `object-api-options.go`, `bucket-handlers.go`, `common-main.go`, `server-main.go`, `internal/http/{server,listener}.go`, `internal/deadlineconn/deadlineconn.go`, all six new test files plus the base tests they interact with, `Makefile`, `.golangci.yml`, `AGENTS.md`, `CONTRIBUTING.md`, and the r5/r6/r8 plan + consensus records. Cross-checked against Go 1.27.1 `net/http/server.go` and pinned `minio-go/v7 v7.3.1-0.20260910142817-60bd07042d49`.

## Independent confirmation of the key claims

**R6 final empty creation-update invariant (the disproved-v2 point).** The v2 proof fails exactly as recorded: `replStatusRegex` (`bucket-replication-utils.go:168`) matches `arn1=;arn2=;` yielding `{arn1: ";arn2="}` — non-empty — while single-target `arn1=;` does not match at all. The v3 fix at `bucket-replication.go:575-582` sets `ReplicationStatusInternal=""`, `Targets=nil`, `ReplicaStatus=""`; `CompositeReplicationStatus` (`bucket-replication-utils.go:356-379`) then returns empty via both the internal string and the replica fallback, so `xlMetaV2.DeleteVersion` (`xl-storage-format-v2.go:1396-1405`, `1438-1447`) skips the creation/replica write while still writing `VersionPurgeStatusKey` (`1406-1408`, `1448-1450`). `ReplicationTimeStamp` is therefore inert (consensus N5 holds). I also confirmed the counterexample's precondition independently: `erasure-object.go:2099-2105` leaves `deleteMarker=true` when the stored marker carries no purge status, which is what sets `fi.Deleted=true` and reaches the rewriting branch. COMPLETE purges still remove the version (`1379-1393`, `1457-1459`).

**R6 classification / all exits.** `isVersionPurge()` (`1954-1956`) parses as `VersionID != "" || (DeleteMarkerVersionID != "" && !VersionPurgeStatus().Empty())`. Every live producer emits only one shape (`object-handlers.go:3232-3248`, `bucket-replication.go:3362-3378`, `3814-3836`), and the dir-object `nullVersionID` re-add (`bucket-handlers.go:552-555`, `679-681`) classifies as a purge under both old and new code, so no wire-form change there. All exits of `replicateDeleteToTarget` select the purge field consistently, HEAD probing is creation-only, `ReplicationDeleteMarker` is false for purges. `purgeReplicationStatus` maps only the legacy `COMPLETE` spelling; `ReplicationStats.Update` (`bucket-replication-stats.go:179-237`) consumes the passed status and never `rinfo.ReplicationStatus`, with `ri.Size == 0` giving count-only/zero-byte deltas. The `ResetStatusesMap` nil guard fixes a genuine nil-map assignment panic, since `ObjectToDelete.ReplicationState()` never initialises that map.

**R6 MRF recovery / budget.** `queueMRFHeal:4118-4125` parses as `(err != nil && !validMarker) || oi.Name == ""`; `decodeDirObject` is identity for both `obj` and `dir/`, matching `GetObjectInfo`'s decoded name, and `erasure-server-pool-consistency.go:143-145` is what returns a populated marker `ObjectInfo` with `MethodNotAllowed`. `RetryCount int` matches the persisted `MRFReplicateEntry.RetryCount` and `QueueReplicationHeal`'s parameter — no on-disk format change. All three increment sites feed the existing `> mrfRetryLimit` drop accounting (`3926-3931`), and the scanner fallback restarts with a fresh budget.

**R5 tag revision flow.** Sender: `replicationTaggingTimestamp` (`804-812`) serves both the full retransmit and the metadata-COPY branch, the latter now failing closed symmetrically (`1720-1725`). `getCopyObjMetadata:765-766` always emits `X-Amz-Tagging` (possibly empty) + `REPLACE`; minio-go `copyObjectDo` forwards empty header values; the receiver's `getRequestHeaderOrQueryValue` (`handler-utils.go:160-171`) treats presence-with-empty-value as authoritative — so an ordered deletion is genuinely representable on the wire. Receiver: `CopyObjectHandler:1799-1840` captures the stored stamp before REPLACE rebuilds the map, every branch writes or deletes the key explicitly, and the unconditional `delete(encMetadata, …)` is safe *because* of that, blocking the SSE-C rotation snapshot (`1655-1659`) from re-merging at `1910`. `PutObjectHandler:2323-2325` and `NewMultipartUploadHandler:315-318` mutate the same map that becomes `opts.UserDefined` (`object-api-options.go:451`), and the header is parsed only under trusted replication (`388-396`). Ordering is re-applied under the write lock by the existing `reconcileStoredObjectTags` callers (`erasure-object.go:136-139`, `1312-1315`; `erasure-multipart.go:1161-1189`; `erasure-server-pool.go:1443-1456`) — which is also what keeps the base R4 KMS test's `missing-timestamp` expectation intact. Dropping the `ri.UserTags` re-injection in the source ACK (`1294-1304`) is right: `cleanMetadata` strips the tagging key from `UserDefined`, so stored tags are now left alone, and the pools path re-derives them from merged `UserTags`.

**R7 boundary.** `replicationToInternalHeaders` has exactly six entries (`handler-utils.go:106-114`); `extractReplicationMetadataFromMime` restores only those and re-extracts no ordinary metadata, so the `aws-chunked` normalisation in `extractMetadata:225-241` is not undone. R5 touches neither, and R4's KMS options (`object-api-options.go:449-460`) still carry the three replication timestamps unmodified.

**R8 deadlines.** Against Go 1.27.1: header window set at `server.go:2038`/`2177`, whole-request deadline unconditionally at `1103`, `StateActive` at `2056-2058` firing after *every* successful `readRequest` because `readRequest` calls `setInfiniteReadLimit()` at `1067`. That is the one place where the naive reading of the `c.r.remain` comment is wrong — the pipelined/fully-buffered request does get the strict→rolling flip, so consensus N2 is correct. `startBackgroundRead` (`741`) and `hijackLocked` zero the deadline, which `infReads` honours — that is why background reads and hijacked grid/websocket conns still work. The strict cap only shortens, never extends; zero/past semantics unchanged; `readExplicit`/`readDeadlineStrict` only touched under `mu`. The h2 skip is defensive rather than load-bearing (net/http uses `skipHooks` for ALPN h2; the h2 server zeroes the conn deadline), and a nil `raw` fails the type assertion safely. Strict mode is opt-in, so every other `DeadlineConn` caller — the optional Linux internode dialer (`dial_linux.go:126-131`, currently disabled at `server-main.go:422`) and all outbound replication transports — keeps legacy rolling reads. The real fix is propagation: flag, field and `UseReadHeaderTimeout` already existed; `ctxt.ReadHeaderTimeout` was simply never populated before `common-main.go:448`.

**Full-package test state.** No duplicate symbols (`tagTestCapacityDisk` defined once in base `erasure-server-pool-tags_test.go:258`); no helper collisions in `internal/http`; `testdata/config/1.yaml`, `fmtGenFlags`, `serverCmd.Flags` all exist; `buildServerCtxt` mutates no globals. Globals are swapped/restored, and `prepareFS`/`prepareErasure`/`initAPIHandlerTest` re-run `initAllSubsystems` between backends, so leaked target-sys entries can't cross a fixture boundary. `logger.UpdateAuditWebhooks(ctx, nil)` really clears the list (`targets.go:227-273`), so the audit fixture is re-enterable across the SD and Erasure passes. `make verifiers` = lint + check-gen + rebrand-guard, and `.golangci.yml` enables no header linter.

## Blocking findings

**None.**

## Non-blocking notes (newly introduced here)

1. `bucket-replication.go:804-812` — an unparseable stored tagging timestamp now fails replication for *untagged* objects too. Trigger: a corrupt/legacy value, or the empty value `erasure-server-pool-consistency.go:211-214` can persist when a non-primary pool holds a malformed stamp. Fail-closed (FAILED + MRF retry), never silent divergence. Optional hardening: treat missing-or-empty as absent, keeping the hard error for non-empty malformed values.
2. `bucket-replication.go:1031-1036` — any version with a tag revision now resolves to `replicateMetadata` instead of `replicateNone`; since `object-handlers.go:1835-1836` gives every non-replica COPY a revision, large resyncs issue one extra metadata COPY per object. Steady state is unaffected (COMPLETED gate at `3810`). Accepted in r5 consensus; flagged as operational cost.
3. `bucket-replication.go:435-445`, `551-566`, `1958-1964` — purge audit status changes from legacy `COMPLETE` to `COMPLETED`, and per-target purge outcomes now reach `globalReplicationStats` where the empty creation status previously recorded nothing. Intended (r6 dispositions 1–2); worth one release-note line for audit/metric consumers.
4. `common-main.go:448` — now that `ReadHeaderTimeout` is populated it also participates in `net/http`'s `tlsHandshakeTimeout()` minimum. No change at defaults (30s vs 30s); a small configured header timeout also shortens the TLS handshake window. Documented in r8 consensus N1.
5. `replication-tagging-order_test.go:1805-1814` — `TestAPIPoolsTaggingReplicaDeletion` calls `globalBucketMetadataSys.Update` after `consistencyPools`, which (unlike `initAPIHandlerTest`) does not run `initAllSubsystems`; run in isolation with `-run` it would nil-dereference. Harmless in full-package CI. Smallest fix: use `initAPIHandlerTest`, as the neighbouring consistency tests do.
6. `replication-delete-mrf_test.go:1208-1215` — the audit cleanup cancels targets twice; non-panicking because `SafeClose(nil)` only prints a stack (`internal/ioutil/ioutil.go:463-471`), but it can add stderr noise.
7. Headers (already queued, header-only): the two new R6 test files use `Copyright (c) 2026 PGSTY` + SPDX-only, and the four new R8 test files carry `Copyright (c) 2015-2021 MinIO, Inc.` despite being new original work; CONTRIBUTING.md asks new original files to name their actual holders with AGPL boilerplate (the two new R5 files already comply). No verifier enforces headers, so this does not gate CI and is distinct from production semantics.

## Inherited / out-of-scope limitations (not introduced by this candidate)

Purge-target subset merging under narrowed fan-out; nil/missing target clients cannot restore already-lost tracking; the `ResetStatusesMap` key asymmetry between `targetResetHeader(arn)` writes and bare-`arn` reads (`bucket-replication-utils.go:392-399` vs `419-426`) — the candidate only adds the panic-preventing nil guard; the shared `replStatusRegex` still mis-parses serialized empty statuses (R6 deliberately works around it at the write site); unqualified (no `versionId`) replica PUT has no stored-tag reconcile (`object-handlers.go:2442`); TLS handshake *write* deadlines keep rolling behaviour (r8 N9); historical objects without a revision, tag-filter target selection and real multi-site clock skew remain unsolved (r5 consensus).

## Verification still owed (I ran nothing; nothing below is "passed")

**Required for this merge — repository gates only:** `make verifiers` (lint, check-gen, rebrand-guard), `make build` producing `silo`, and full-package tests, at minimum `./cmd`, `./internal/http`, `./internal/deadlineconn`; a Linux CI leg to compile and run the build-tagged `TestInternodeDialReadDeadline`; DCO sign-off/authorship checks on the cherry-picked commits. With >700 GiB free, no capacity overlay is needed.

**Optional, explicitly not a merge gate:** `SILO_TEST_LONG_UPLOAD=1` (>30s default-idle transfer regressions), multi-process/multi-node deployment acceptance, cross-region replication mesh. Their absence is a coverage boundary, not a defect; I do not treat missing production deployment as a blocker.