Site Replication State Locks: Snapshot Topology Before Storage I/O and Peer RPCs

This record covers an upstream-inherited concurrency deadlock and map-race family in SiteReplicationSys (cmd/site-replication.go, cmd/iam-revision.go, cmd/site-replication-metadata.go), tracked under #341.

Status: Merged to main in 745fbf043, 2f228284f, and ef6efd312; scheduled for the next post-20261109 Server release. SILO Server 20261109 and earlier—as well as upstream MinIO releases since 2022-04-24—are affected.

Short Answer

SiteReplicationSys embeds a sync.RWMutex protecting its in-memory peer configuration (c.enabled and c.state, including c.state.Peers). Inherited from upstream MinIO, background healing routines (healIAMSystem, healBuckets, healUsers, healPolicies, healUserMappings, healGroupMappings, healSTSUserMappings, healIAMDeletions), status collection (siteReplicationStatus), and live mutation hooks (MakeBucketHook) acquired c.RLock() and held it across local storage reads, peer HTTP RPCs, and helper calls that acquired c.RLock() a second time on the same goroutine.

In Go’s sync.RWMutex, recursive read locking is explicitly forbidden: as soon as any writer (c.Lock()—triggered by loadFromDisk, PeerStateUpdateHandler, SiteReplicationEdit, SiteReplicationAdd, or resync state updates) arrives and blocks waiting for active readers, every subsequent c.RLock() blocks behind the queued writer, including a second c.RLock() from the goroutine that already holds the first c.RLock(). Neither the outer reader nor the queued writer can ever advance, ctx cancellation cannot wake a goroutine blocked on sync.RWMutex, and every subsequent site-replication hook (IAMChangeHook, BucketMetaHook, SiteReplicationInfo) freezes permanently until process restart. Meanwhile, several peer handlers read or shallow-copied c.state.Peers without locking, creating concurrent map read/write panic hazards.

The corrected healing, status and hook paths use cloneState() and stateSnapshot() to copy srState and its Peers map in memory, then release c.RLock() before storage I/O, peer RPCs or subsystem calls.

Classification

1. Upstream Provenance and Call-Chain Anatomy

Upstream commit 3a6458066 (2022-04-24) established a recurring pattern across cmd/site-replication.go: acquire c.RLock(); defer c.RUnlock() at the top of an orchestration function so c.enabled and c.state.Peers stay stable throughout a multi-step sync pass, then invoke high-level helpers that also guard c with c.RLock().

Auditing every c.RLock() call graph in cmd/site-replication.go, cmd/iam-revision.go, and cmd/site-replication-metadata.go identified four distinct re-entrant deadlock chains plus unguarded map sharing:

1. IAM healing status & sync (healIAMSystem):
   • healIAMSystem acquired c.RLock() and called c.siteReplicationStatus(ctx, ...) -> c.SiteReplicationMetaInfo(ctx) -> c.RLock() (re-entrant lock #1).
   • Next, healIAMSystem called healUsers, healPolicies, healUserMappings, healGroupMappings, and healSTSUserMappings, each of which held c.RLock() while calling c.IAMChangeHook(...) -> c.isEnabled() / c.RLock() (re-entrant lock #2).
   • healIAMDeletions (cmd/iam-revision.go) likewise held c.RLock() across storage scans and peer RPCs.
2. Bucket healing (healBuckets):
   • healBuckets held c.RLock() across listing all local and remote buckets and called c.healBucket(...), which called c.healBucketReplicationConfig(...) -> c.RLock() and BucketMetaHook -> c.RLock().
3. Synchronous bucket creation hook (MakeBucketHook):
   • MakeBucketHook acquired c.RLock() and called c.BucketMetaHook(...) -> c.RLock(), blocking client CreateBucket requests directly whenever a site-replication config reload or peer update queued c.Lock().
4. Status collection and peer map races (siteReplicationStatus, peer handlers):
   • siteReplicationStatus held c.RLock() across concurrent remote peer HTTP status probes and local IAM/bucket metadata reads (SiteReplicationMetaInfo). Even without re-entrancy, a single slow or unreachable peer held c.RLock() for the full HTTP timeout, stalling config writers (c.Lock()) and queuing every S3 MakeBucket or IAM mutation behind it.
   • PeerBucketConfigureReplHandler re-entered c.RLock(), while peer state edit paths shallow-copied srState (state := c.state) and mutated state.Peers[k] = ... on the map still referenced by concurrent readers.

Why Context Cancellation Cannot Break the Deadlock

In Go’s runtime (sync/rwmutex.go), once a goroutine calls rw.Lock(), readerCount is decremented by rwmutexMaxReaders (1 << 30), causing all new rw.RLock() callers to park on readerSem until the writer finishes rw.Unlock(). Because Goroutine A already holds RLock #1 and parks unconditionally on readerSem at RLock #2, rw.Lock() never observes readerWait == 0, and RLock #2 has no select on ctx.Done(). Deterministic race-enabled tests (cmd/site-replication-status_test.go, cmd/iam-svcacct-replication-snapshot_test.go) reproduce the permanent hang in milliseconds as soon as a writer contends between RLock #1 and RLock #2.

2. Solution Design: Why Leaf Snapshots Are Necessary and Sufficient

Evaluated Alternatives

Core Implementation (cloneState & stateSnapshot)

Commits 745fbf043, 2f228284f, and ef6efd312 enforce a single structural invariant across SiteReplicationSys:

1. Independent topology snapshot (stateSnapshot / cloneState): Under c.RLock(), stateSnapshot() checks c.enabled, copies srState by value, clones the Peers map with maps.Clone, and releases the lock. The remaining srStateV1 fields are Name, ServiceAccountAccessKey, and UpdatedAt; there are no Sites, Buckets, or endpoint-slice fields. cloneState() performs the same value/map copy without checking the enabled flag.
2. Lock-free I/O and RPC execution on srState:
   • healIAMSystem, healUsers, healPolicies, healUserMappings, healGroupMappings, healSTSUserMappings, healIAMDeletions, healBuckets, healBucket, healBucketReplicationConfig, SiteReplicationMetaInfo, siteReplicationStatus, MakeBucketHook, BucketMetaHook, and IAMChangeHook take a snapshot upfront and execute all disk reads, IAM queries, and remote peer RPCs outside c.RLock().
   • siteReplicationStatus uses the same captured topology for dispatch, result indexing, credentials, error attribution, info.Sites, and info.UpdatedAt. It does not reacquire c.RLock() at completion to merge a newer peer topology into those results.
3. Copy-on-write peer state mutations: Snapshot readers own a separate Peers map. Editing paths such as SiteReplicationEdit and SiteReplicationRemove work on cloned state; persistence and reload paths publish state under c.Lock(). This separates the readers’ maps from later topology edits.

Why This Is Necessary and Sufficient

• Scope of the fix: The converted healing, status and peer-hook paths release the replication-state lock before storage I/O, peer RPCs or subsystem calls. This removes the demonstrated nested-reader lock cycle in those paths; it is not a guarantee that every subsystem is free of deadlocks.
• Necessary: Leaving even one outer caller holding c.RLock() (for example, healIAMSystem calling siteReplicationStatus, which we caught and closed in 2f228284f and ef6efd312) was enough for a single concurrent config writer to freeze site replication permanently.

3. Trade-Offs, Caveats, and Operational Costs

Verification

Regression tests in cmd/site-replication-status_test.go and cmd/iam-svcacct-replication-snapshot_test.go (executed under go test -race) deterministically interleave queued c.Lock() writers between snapshot acquisition and every IAM/bucket healing helper, status collector, peer hook, and cancellation path, confirming zero deadlocks and zero data races. See also Candidate Server Compatibility Notes and IAM Policy Reload Lock Scope (#251).