master: survive a volume layout deleted twice (#11098)

* master: survive a layout deleted twice

Two volume servers dropping the last replica of volumes that share a layout
both find it empty and both delete it. The loser's lookup misses, and the
single-value type assertion on the result crashed the master before the
caller could look at the found flag.

Claude-Session: https://claude.ai/code/session_01WmX6Rchx298NQksHDXg7sk

* master: remove a layout and read it back in one step

DeleteVolumeLayout looked the layout up and then deleted it, so two deleters
could each release the lookup ownership of the same layout, or one could find
nothing to release at all. Have the map hand back what it removed.

Claude-Session: https://claude.ai/code/session_01WmX6Rchx298NQksHDXg7sk

weed/topology/collection.go

@@ -54,7 +54,10 @@ func (c *Collection) GetVolumeLayout(rp *super_block.ReplicaPlacement, ttl *need
 		keyString += string(diskType)
 	}
 	vl, ok := c.storageType2VolumeLayout.Find(keyString)
-	return vl.(*VolumeLayout), ok
+	if !ok {
+		return nil, false
+	}
+	return vl.(*VolumeLayout), true
 }
 
 func (c *Collection) GetAllVolumeLayouts() []*VolumeLayout {
@@ -76,10 +79,8 @@ func (c *Collection) DeleteVolumeLayout(rp *super_block.ReplicaPlacement, ttl *n
 		keyString += string(diskType)
 	}
 	// Unpublish first so a racing registration re-resolves into a fresh layout.
-	vl, found := c.GetVolumeLayout(rp, ttl, diskType)
+	if vl, found := c.storageType2VolumeLayout.Delete(keyString); found {
-	c.storageType2VolumeLayout.Delete(keyString)
+		vl.(*VolumeLayout).releaseLookupOwnership()
-	if found {
-		vl.releaseLookupOwnership()
 	}
 }
 

weed/topology/collection_layout_delete_test.go

@@ -0,0 +1,72 @@
+package topology
+
+import (
+	"sync"
+	"testing"
+
+	"github.com/seaweedfs/seaweedfs/weed/pb/master_pb"
+	"github.com/seaweedfs/seaweedfs/weed/sequence"
+	"github.com/seaweedfs/seaweedfs/weed/storage"
+	"github.com/seaweedfs/seaweedfs/weed/storage/needle"
+	"github.com/seaweedfs/seaweedfs/weed/storage/super_block"
+	"github.com/seaweedfs/seaweedfs/weed/storage/types"
+)
+
+func TestGetVolumeLayoutOfAbsentKey(t *testing.T) {
+	rp, _ := super_block.NewReplicaPlacementFromString("000")
+	c := NewCollection("c", 32*1024, false)
+
+	vl, found := c.GetVolumeLayout(rp, needle.EMPTY_TTL, types.HardDriveType)
+	if found || vl != nil {
+		t.Fatalf("absent layout: got (%v, %v), want (nil, false)", vl, found)
+	}
+}
+
+// Volume servers dropping the last replica of several volumes that share one
+// layout all see the layout go empty and all delete it. The losers used to
+// crash the master on a nil type assertion.
+func TestConcurrentLastReplicaRemoval(t *testing.T) {
+	const nodeCount = 8
+	ttl, _ := needle.ReadTTL("5m")
+	for round := 0; round < 500; round++ {
+		topo := NewTopology("weedfs", sequence.NewMemorySequencer(), 32*1024, 5, false)
+		rack := topo.GetOrCreateDataCenter("dc1").GetOrCreateRack("rack1")
+
+		// A second layout keeps the collection alive, so every remover reaches
+		// the layout deletion instead of stopping at a vanished collection.
+		keeper := rack.GetOrCreateDataNode("127.0.0.1", 9000, 0, "", "", map[string]uint32{"": 100})
+		topo.SyncDataNodeRegistration([]*master_pb.VolumeInformationMessage{{
+			Id: 999, Collection: "c", Version: uint32(needle.GetCurrentVersion()), Ttl: ttl.ToUint32(),
+		}}, keeper)
+
+		var wg sync.WaitGroup
+		start := make(chan struct{})
+		for i := 0; i < nodeCount; i++ {
+			dn := rack.GetOrCreateDataNode("127.0.0.1", 8080+i, 0, "", "", map[string]uint32{"": 100})
+			m := &master_pb.VolumeInformationMessage{
+				Id: uint32(i + 1), Collection: "c", Version: uint32(needle.GetCurrentVersion()),
+			}
+			topo.SyncDataNodeRegistration([]*master_pb.VolumeInformationMessage{m}, dn)
+			vi, err := storage.NewVolumeInfo(m)
+			if err != nil {
+				t.Fatalf("NewVolumeInfo: %v", err)
+			}
+			wg.Add(1)
+			go func() {
+				defer wg.Done()
+				<-start
+				topo.UnRegisterVolumeLayout(vi, dn)
+			}()
+		}
+		close(start)
+		wg.Wait()
+
+		c, found := topo.FindCollection("c")
+		if !found {
+			t.Fatalf("round %d: collection dropped while a layout still had volumes", round)
+		}
+		if layouts := c.GetAllVolumeLayouts(); len(layouts) != 1 {
+			t.Fatalf("round %d: got %d layouts, want only the one still holding a volume", round, len(layouts))
+		}
+	}
+}

weed/topology/topology.go

@@ -533,12 +533,11 @@ func (t *Topology) DeleteCollection(collectionName string) {
 	// the node's held and servable digests apart forever, and the master asks
 	// for the full volume list on every heartbeat from then on.
 	// Unpublish first so a racing registration re-resolves into a fresh collection.
-	collection, found := t.FindCollection(collectionName)
+	collection, found := t.collectionMap.Delete(collectionName)
-	t.collectionMap.Delete(collectionName)
 	if !found {
 		return
 	}
-	for _, vl := range collection.GetAllVolumeLayouts() {
+	for _, vl := range collection.(*Collection).GetAllVolumeLayouts() {
 		vl.releaseLookupOwnership()
 	}
 }

weed/util/concurrent_read_map.go

@@ -53,8 +53,12 @@ func (m *ConcurrentReadMap) Items() (itemsCopy []interface{}) {
 	return itemsCopy
 }
 
-func (m *ConcurrentReadMap) Delete(key string) {
+// Delete removes the key and returns what it held, so a caller that has to
+// wind the entry down does not race another deleter for it.
+func (m *ConcurrentReadMap) Delete(key string) (interface{}, bool) {
 	m.Lock()
+	value, ok := m.items[key]
 	delete(m.items, key)
 	m.Unlock()
+	return value, ok
 }