master: keep a racing registration out of a dying collection (#10677)

weed/server/master_grpc_server_changed_volumes_test.go

@@ -203,3 +203,33 @@ func TestStaleFullListDoesNotEraseAFreshGrow(t *testing.T) {
 		t.Fatal("a confirmed volume survived a list that dropped it")
 	}
 }
+
+// A registration can resolve a collection's layout just before the collection
+// is deleted. Bits it sets afterwards would never be released, and the node's
+// held and servable digests would disagree forever.
+func TestRegistrationRacingCollectionDeleteDoesNotLeak(t *testing.T) {
+	topo, dn := changedTestCluster(t)
+	topo.SyncDataNodeRegistration([]*master_pb.VolumeInformationMessage{changedTestVolume(1, 1024)}, dn)
+
+	vi, err := storage.NewVolumeInfo(changedTestVolume(1, 1024))
+	if err != nil {
+		t.Fatal(err)
+	}
+	stale := topo.GetVolumeLayout("c", vi.ReplicaPlacement, vi.Ttl, types.ToDiskType(vi.DiskType))
+
+	topo.DeleteCollection("c")
+
+	// the interleaved registration must refuse the dropped layout
+	if stale.RegisterVolume(&vi, dn) {
+		t.Fatal("registered into a layout dropped with its collection")
+	}
+
+	// the node prunes its copy when the departure is named
+	topo.IncrementalSyncDataNodeRegistration(nil, []*master_pb.VolumeShortInformationMessage{
+		{Id: 1, Collection: "c", Version: 3},
+	}, dn)
+
+	if !dn.HasConsistentVolumeIndex() {
+		t.Fatal("the racing registration leaked lookup ownership")
+	}
+}

weed/topology/collection.go

@@ -75,10 +75,12 @@ func (c *Collection) DeleteVolumeLayout(rp *super_block.ReplicaPlacement, ttl *n
 	if diskType != types.HardDriveType {
 		keyString += string(diskType)
 	}
-	if vl, found := c.GetVolumeLayout(rp, ttl, diskType); found {
+	// Unpublish first so a racing registration re-resolves into a fresh layout.
+	vl, found := c.GetVolumeLayout(rp, ttl, diskType)
+	c.storageType2VolumeLayout.Delete(keyString)
+	if found {
 		vl.releaseLookupOwnership()
 	}
-	c.storageType2VolumeLayout.Delete(keyString)
 }
 
 func (c *Collection) Lookup(vid needle.VolumeId) []*DataNode {

weed/topology/topology.go

@@ -503,13 +503,16 @@ func (t *Topology) DeleteCollection(collectionName string) {
 	// holds a bit in its node's lookup digest. Left in place, those bits keep
 	// the node's held and servable digests apart forever, and the master asks
 	// for the full volume list on every heartbeat from then on.
-	if collection, found := t.FindCollection(collectionName); found {
+	// Unpublish first so a racing registration re-resolves into a fresh collection.
+	collection, found := t.FindCollection(collectionName)
+	t.collectionMap.Delete(collectionName)
+	if !found {
+		return
+	}
 	for _, vl := range collection.GetAllVolumeLayouts() {
 		vl.releaseLookupOwnership()
 	}
 }
-	t.collectionMap.Delete(collectionName)
-}
 
 func (t *Topology) DeleteLayout(collectionName string, rp *super_block.ReplicaPlacement, ttl *needle.TTL, diskType types.DiskType) {
 	collection, found := t.FindCollection(collectionName)
@@ -524,9 +527,14 @@ func (t *Topology) DeleteLayout(collectionName string, rp *super_block.ReplicaPl
 
 func (t *Topology) RegisterVolumeLayout(v storage.VolumeInfo, dn *DataNode) {
 	diskType := types.ToDiskType(v.DiskType)
+	for {
 		vl := t.GetVolumeLayout(v.Collection, v.ReplicaPlacement, v.Ttl, diskType)
-	vl.RegisterVolume(&v, dn)
+		if vl.RegisterVolume(&v, dn) {
 			vl.EnsureCorrectWritables(&v)
+			return
+		}
+		// Dropped with its collection; the next lookup creates a fresh one.
+	}
 }
 
 func (t *Topology) UnRegisterVolumeLayout(v storage.VolumeInfo, dn *DataNode) {
@@ -635,10 +643,12 @@ func (t *Topology) SyncDataNodeRegistration(volumes []*master_pb.VolumeInformati
 		// Without this, the volume stays visible in volume.list/admin UI yet
 		// LookupVolume returns "volume id not found".
 		if !vl.HasDataNode(v.Id, dn) {
-			// vl is already resolved above; call it directly instead of
+			if vl.RegisterVolume(&v, dn) {
-			// RegisterVolumeLayout, which would repeat the GetVolumeLayout lookup.
-			vl.RegisterVolume(&v, dn)
 				vl.EnsureCorrectWritables(&v)
+			} else {
+				// Dropped with its collection; re-resolve.
+				t.RegisterVolumeLayout(v, dn)
+			}
 			// Volumes new to the disk map were registered above, so reaching
 			// here means only the lookup index had lost it. Clients were told
 			// it went when the node dropped out, so the repair has to tell them
@@ -714,8 +724,9 @@ func (t *Topology) ApplyVolumeChanges(changed []*master_pb.VolumeInformationMess
 		// volume only that index had lost is an arrival as far as clients are
 		// concerned: they were told it went when the node dropped out.
 		becameServable := !vl.HasDataNode(vi.Id, dn)
-		if becameServable {
+		for becameServable && !vl.RegisterVolume(&vi, dn) {
-			vl.RegisterVolume(&vi, dn)
+			// Dropped with its collection; the next lookup creates a fresh one.
+			vl = t.GetVolumeLayout(vi.Collection, vi.ReplicaPlacement, vi.Ttl, types.ToDiskType(vi.DiskType))
 		}
 		if isNew || becameServable {
 			newVolumes = append(newVolumes, vi)

weed/topology/volume_index_digest.go

@@ -51,6 +51,7 @@ func (vl *VolumeLayout) releaseLookupOwnership() {
 		}
 	}
 	vl.vid2location = make(map[needle.VolumeId]*VolumeLocationList)
+	vl.dropped = true
 }
 
 // moveLookupOwnership transfers the digest bit for vid from the node a lookup

weed/topology/volume_layout.go

@@ -66,6 +66,8 @@ type VolumeLayout struct {
 	replicationAsMin bool
 	accessLock       sync.RWMutex
 	sizeTracking     map[needle.VolumeId]*volumeSizeTracking
+	// dropped: the layout went away with its collection; late registrations must re-resolve.
+	dropped bool
 }
 
 type VolumeLayoutStats struct {
@@ -119,10 +121,16 @@ func (vl *VolumeLayout) initSizeTracking(vid needle.VolumeId, size uint64, compa
 	}
 }
 
-func (vl *VolumeLayout) RegisterVolume(v *storage.VolumeInfo, dn *DataNode) {
+// RegisterVolume records a volume location. It refuses a layout dropped with
+// its collection — the caller must re-resolve, or the bits it sets would leak.
+func (vl *VolumeLayout) RegisterVolume(v *storage.VolumeInfo, dn *DataNode) bool {
 	vl.accessLock.Lock()
 	defer vl.accessLock.Unlock()
 
+	if vl.dropped {
+		return false
+	}
+
 	defer vl.rememberOversizedVolume(v, dn)
 
 	moveLookupOwnership(v.Id, vl.getOrCreateLocationList(v.Id).Set(dn), dn)
@@ -137,17 +145,17 @@ func (vl *VolumeLayout) RegisterVolume(v *storage.VolumeInfo, dn *DataNode) {
 				glog.V(1).Infof("vid %d removed from writable", v.Id)
 				vl.removeFromWritable(v.Id)
 				location.SetReadOnly(dn, true)
-				return
+				return true
 			}
 			location.SetReadOnly(dn, false)
 		} else {
 			glog.V(1).Infof("vid %d removed from writable", v.Id)
 			vl.removeFromWritable(v.Id)
 			location.SetReadOnly(dn, false)
-			return
+			return true
 		}
 	}
-
+	return true
 }
 
 func (vl *VolumeLayout) rememberOversizedVolume(v *storage.VolumeInfo, dn *DataNode) {
@@ -855,6 +863,9 @@ func (vl *VolumeLayout) SetVolumeAvailable(dn *DataNode, vid needle.VolumeId, is
 	if err != nil {
 		return false
 	}
+	if vl.dropped {
+		return false
+	}
 
 	// A disconnect during a long vacuum can drop the entry while the volume is
 	// still on the node; re-create it instead of dereferencing a nil location,