From 5d5ea18287cfbf9e274f4b78be40a0ae220cb55d Mon Sep 17 00:00:00 2001
From: Chris Lu <chrislusf@users.noreply.github.com>
Date: Mon, 7 Sep 2026 13:47:22 -0700
Subject: [PATCH] topology: fix fatal concurrent map read/write on
 VolumeLayout.crowded (#11216)

SetVolumeCrowded mutated the crowded map under accessLock.RLock(), while
GetWritableVolumeCount reads the same map under RLock() on the Assign hot
path. Two concurrent RLock holders with one writing and one reading the
map triggers a fatal "concurrent map read and map write" that kills the
master process (unrecoverable, bypasses recover).

Take the write lock in SetVolumeCrowded instead. This event path is a
low-frequency single consumer driven by the crowded-volume event loop,
and every other mutation of crowded already holds Lock(); setVolumeCrowded
takes no nested locks, so there is no deadlock path. The hot readers
(GetWritableVolumeCount, CloneWritableVolumes) keep using RLock.

Adds a -race regression test that fails (race detected) on the old RLock
and passes with the write lock.

Fixes #11211
---
 weed/topology/crowded_writable_test.go | 50 ++++++++++++++++++++++++++
 weed/topology/volume_layout.go         | 13 ++++---
 2 files changed, 58 insertions(+), 5 deletions(-)

diff --git a/weed/topology/crowded_writable_test.go b/weed/topology/crowded_writable_test.go
index ba87bcb6d..32e0a072f 100644
--- a/weed/topology/crowded_writable_test.go
+++ b/weed/topology/crowded_writable_test.go
@@ -1,6 +1,7 @@
 package topology
 
 import (
+	"sync"
 	"testing"
 	"time"
 
@@ -73,3 +74,52 @@ func TestCrowdedCountStillMatchesWritables(t *testing.T) {
 		t.Errorf("writable=%d crowded=%d, want 1 and 1", writable, crowded)
 	}
 }
+
+// SetVolumeCrowded mutates the crowded map while GetWritableVolumeCount reads
+// it on the Assign hot path. Under RLock both could run at once and the Go
+// runtime aborts with "concurrent map read and map write". Run with -race to
+// reproduce the original bug; the write lock makes this pass.
+func TestSetVolumeCrowdedNoRaceWithGetWritableVolumeCount(t *testing.T) {
+	rp, _ := super_block.NewReplicaPlacementFromString("000")
+	vl := NewVolumeLayout(rp, needle.EMPTY_TTL, types.HardDriveType, 10000, false)
+	// GetWritableVolumeCount iterates vl.writables, so give it something to
+	// read while the crowded map is being mutated.
+	vl.writables = []needle.VolumeId{1, 2, 3}
+
+	var wg sync.WaitGroup
+	stop := make(chan struct{})
+	for range 8 {
+		wg.Add(1)
+		go func() {
+			defer wg.Done()
+			for {
+				select {
+				case <-stop:
+					return
+				default:
+					vl.SetVolumeCrowded(needle.VolumeId(2))
+				}
+			}
+		}()
+	}
+	for range 8 {
+		wg.Add(1)
+		go func() {
+			defer wg.Done()
+			for {
+				select {
+				case <-stop:
+					return
+				default:
+					vl.GetWritableVolumeCount()
+				}
+			}
+		}()
+	}
+
+	// The race detector needs only a brief window to flag a concurrent
+	// map read/write; let the goroutines hammer the lock briefly.
+	time.Sleep(50 * time.Millisecond)
+	close(stop)
+	wg.Wait()
+}
diff --git a/weed/topology/volume_layout.go b/weed/topology/volume_layout.go
index 17897cbbf..8a36f79cc 100644
--- a/weed/topology/volume_layout.go
+++ b/weed/topology/volume_layout.go
@@ -1066,11 +1066,14 @@ func (vl *VolumeLayout) setVolumeCrowded(vid needle.VolumeId) {
 }
 
 func (vl *VolumeLayout) SetVolumeCrowded(vid needle.VolumeId) {
-	// since delete is guarded by accessLock.Lock(),
-	// and is always called in sequential order,
-	// RLock() should be safe enough
-	vl.accessLock.RLock()
-	defer vl.accessLock.RUnlock()
+	// setVolumeCrowded mutates the crowded map, and GetWritableVolumeCount
+	// reads it under RLock on the Assign hot path. Two concurrent RLock
+	// holders with one writing the map triggers a fatal
+	// "concurrent map read and map write". Take the write lock: this path
+	// is a low-frequency single consumer driven by the crowded-volume
+	// event loop, and every other mutation of crowded already holds Lock().
+	vl.accessLock.Lock()
+	defer vl.accessLock.Unlock()
 
 	vl.setVolumeCrowded(vid)
 }