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.