minio/docs/investigations/r6/opus-v1-review.md

I'll start by reading AGENTS.md and the plan file, then dig into the actual source.

Let me locate the core source files and the evidence directory.

I've verified the plan against the actual source. Writing up the review now.

The Write tool is disabled in this session, so I can't create the plan file — the review is below in full.

---

Independent review — SILO R6 plan-v1.md

Plan hash reviewed: bcd023e2b00ad3dc709baa738bb02551d47aeebb49879b0f76f2789f4c4dff8b Source: 9ebe81c1b3611f9cc73e676b5b741c2be62c467a I did not run anything. All findings come from reading the source, the existing tests, PR #184's diff/prose, and the supplied baseline.log (read, not produced by me).

Verdict: REVISE

One blocking finding. The classification rule, the canonical-wire choice, the MRF 405 work, and the retry-budget work are technically correct and better grounded than PR #184. One explicitly prescribed detail in §A causes a silent regression on the canonical purge shape — the shape every live producer emits — and §C would not catch it. The correction is small and stays inside the plan's bounded scope.

---

Blocking

B1 — Pinning rinfo.ReplicationStatus = rinfo.PrevReplicationStatus for purges makes the persisted marker creation-status block get rewritten from the fan-out subset

§A prescribes this "on every exit" while also forbidding changes to getReplicationState merging. Together that is a regression, not preservation.

1. bucket-replication-utils.go:392-399 — targetState() never sets ReplicationStatus, so a canonical purge returns "" today.
2. bucket-replication-utils.go:92-101 — ReplicationStatusInternal() rebuilds "arn=STATUS;" only from rinfos.Targets, i.e. only fanned-out targets.
3. bucket-replication.go:508-543 — fan-out skips !Replicate, non-matching dobj.TargetArn, and nil clients. TargetArn is really set by queueReplicateDeletesWrapper (:2400-2409).
4. bucket-replication-utils.go:410-412 → drs, passed as DeleteReplication at bucket-replication.go:572-582.
5. erasure-object.go:2163-2177 → xl-storage-format-v2.go:1438-1446 — for a DeleteType version, if fi.DeleteMarkerReplicationStatus() is non-empty, MetaSys[ReplicationStatus] and MetaSys[ReplicationTimestamp] are overwritten.

Today that guard never fires for a canonical purge: the string is "arn1=;", replicationStatusesMap doesn't match it (:428-439), composite over the empty map is "" (:455-459) — so the on-disk creation block is left untouched, which preserves it perfectly including non-attempted targets. Under the plan it becomes non-empty and is rewritten:

• Status loss: "arn1=COMPLETED;arn2=COMPLETED;" with a fan-out covering only arn2 persists "arn2=COMPLETED;" — arn1 silently dropped. That is the exact failure the plan exists to fix, newly introduced on the main shape.
• Zero timestamp: rs.ReplicationTimeStamp = rinfos.ReplicationTimeStamp (:413) is never assigned in replicateDelete; it's only rescued by bucket-replication.go:568-570 when the composite changes. A repeated FAILED→FAILED purge writes 0001-01-01T00:00:00Z.
• Empty ReplicaStatus: under a legacy RoleArn config the composite can be REPLICA (bucket-replication-utils.go:497-503), taking xl-storage-format-v2.go:1398-1400, which writes ReplicaStatus — never populated by ObjectInfo.ReplicationState() (:569-587).

Only delete-marker versions are affected; ObjectType versions only touch VersionPurgeStatusKey (xl-storage-format-v2.go:1465-1476).

Minimal correction (strictly smaller than the plan): keep the classification and every exit fix, but for purges leave rinfo.ReplicationStatus at its zero value rather than pinning it. That is what the canonical path already does, and it preserves the on-disk block byte-for-byte including non-attempted targets, with no change to getReplicationState. Consequences that then become mandatory, not optional:

• purge exits write only VersionPurgeStatus — never Failed, Completed, or PrevReplicationStatus;
• the resync defer (:618-622) must be gated on the operation's own success. Under the plan as written, pinning Completed would stamp the reset marker for a failed purge on any marker whose creation was COMPLETED — the two changes are coupled and cannot land separately;
• the stats gate at :556 must be replaced by the per-target operation-status comparison §A already calls for, otherwise purges stop being reported at all.

Add a partial-fan-out test: two ARNs persisted, one excluded from fan-out, failed purge → marker metadata unchanged. §C item 5 fans out to both targets and cannot detect this.

---

Nonblocking

1. The wire version ID is load-bearing. For the legacy shape dobj.VersionID is empty, so the implementation must keep the existing versionID fallback (bucket-replication.go:609-612). A RemoveObject with empty VersionID against a versioned target creates a new marker (erasure-object.go:2126-2149) — the exact resurrection being fixed. Make §C1 assert the outgoing versionId and x-minio-source-deletemarker explicitly.
2. Retry increment must cover all three delete-path MRF sites: :487 (lock), :565 (aggregate failure), :2427 (queue full). §B names only the first two; missing :2427 leaves an unbounded loop once marker MRF is live. Mirror ri.RetryCount++ at :1310.
3. Stats will move. COMPLETE→COMPLETED makes ReplicationStats.Update reach its Completed case for Heal/ExistingObject deletes (bucket-replication-stats.go:184-201, replication.go:139-144). Today "COMPLETE" != "COMPLETED" so nothing is recorded. Assert expected counter deltas rather than discovering them.
4. §B's identity gate excludes null-version markers by construction — GetObjectInfo returns ObjectNotFound, not 405, when VersionID == "" (erasure-object.go:996-999), and ToObjectInfo leaves VersionID empty when versioned is false (erasure-metadata.go:120-123). No regression, but document it instead of implying MRF healing is complete.
5. Legacy-shape scope. I found no producer of that shape on this baseline: object-handlers.go:3225-3228, bucket-handlers.go:565-570, bucket-replication.go:3323-3327/:3775-3779 are mutually exclusive, and erasure-object.go:1752-1766 only sets DeleteMarkerVersionID when VersionID == "". DeletedObjectReplicationInfo isn't serialized, so it can't survive a restart. §A's legacy handling is upgrade/robustness work; the live value of R6 is mostly §B. Say so in the PR text so the fork doesn't inherit #184's overclaiming.
6. §C item 3 is testable but fiddly. globalLocalDrivesMap is populated by newErasureServerPools (erasure-server-pool.go:174-181), so save/load works — but loadMRF deletes the file after reading (:4013-4015) and queueMRFHeal dispatches a detached goroutine with a 1s per-entry context (:4067-4081). Re-persist between rounds and synchronize, don't sleep.
7. Confirm the fixtures run. The supplied baseline.log shows both TestReplicateDeleteMarkerPurge subtests aborting at replication-delete-marker_test.go:121 with "Storage reached its minimum free drive threshold" — an environment failure. §C items 2, 3, 5 all depend on those fixtures.

---

Independently confirmed as correct in the plan

• Observation 1 holds at every exit: :624 (early-return without sending), :645-649/:702-706/:712-716 (field selected on dobj.VersionID), :684-688 (HEAD-not-ready overwrites creation status unconditionally), :628 (completed-purge early-out only for non-empty VersionID), :618-622 (resync stamp keyed on ReplicationStatus). The supplied probe log agrees.
• Observation 2 is a real regression in PR #184. replicateDelete selects on dobj.VersionID != "" (:549-552), so with only the target-side fix a failed legacy purge aggregates to COMPLETED, emits ObjectReplicationComplete, and skips queueMRFSave (:563-566) — worse than baseline.
• Task-level classification is required; #184's per-target isDMPurge is wrong. VersionPurgeStatus() needs completed == len(ri.Targets) (bucket-replication-utils.go:122-139); a target classified as a creation never sets the purge field, so the composite can never reach COMPLETE. The task-level rule is also safe here — no current producer emits a creation with a non-empty composite purge status.
• The canonical-wire choice really does fix resurrection; #184 does not. With ReplicationDeleteMarker=true and an absent version the receiver re-creates the marker (xl-storage.go:1346-1349; xl-storage-format-v2.go:1517-1520). With false, the receiver returns VersionNotFound (erasure-object.go:2013-2027) and the handler answers 204 (object-handlers.go:3171-3192). Scope it honestly: this only changes legacy-shaped purges (canonical already sends false), and does nothing for a delayed creation after a purge — correctly filed as separate.
• §B's 405 premise is exact: erasure-object.go:996-1002 returns a populated ObjectInfo with toObjectErr(errMethodNotAllowed,…) → MethodNotAllowed{} (object-api-errors.go:96-102), preserved multi-pool too (erasure-server-pool.go:1063-1077).
• §B's retry gap is real: ToMRFEntry() (:1927-1937) never sets RetryCount, so the > mrfRetryLimit drop (:3883-3887) can never fire for deletes. No msgp code exists for the type, so the field is genuinely schema-free.
• Resync accounting already handles both shapes — resyncTargetSucceeded (:3154-3162) keys on roi.VersionPurgeStatus. The plan correctly leaves it alone.
• The pushback on #184's permanence/topology prose is correct. Canonical purge retries are not short-circuited on this baseline (:624 requires VersionID == ""; :628 exits only on VersionPurgeComplete). #184's Bugs 1 and 2 as narrated are legacy-shape-only.

---

Because B1 is a blocker, I am not stating consensus on this exact hash. Resolve B1 (and the coupled resync-defer/stats-gate items it forces) and the plan becomes technically acceptable in my judgement — implementation would still require the tests in §C plus the partial-fan-out regression above.