From 3911e4c5487fff548285fb4694c854206a1ecbd2 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Sun, 9 Aug 2026 22:20:34 -0700 Subject: [PATCH] master: keep a racing registration out of a dying collection (#10677) --- ...master_grpc_server_changed_volumes_test.go | 30 ++++++++++++++ weed/topology/collection.go | 6 ++- weed/topology/topology.go | 39 ++++++++++++------- weed/topology/volume_index_digest.go | 1 + weed/topology/volume_layout.go | 19 +++++++-- 5 files changed, 75 insertions(+), 20 deletions(-) diff --git a/weed/server/master_grpc_server_changed_volumes_test.go b/weed/server/master_grpc_server_changed_volumes_test.go index 3770732ab..5ece14aa7 100644 --- a/weed/server/master_grpc_server_changed_volumes_test.go +++ b/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") + } +} diff --git a/weed/topology/collection.go b/weed/topology/collection.go index c8c0841ba..52149308f 100644 --- a/weed/topology/collection.go +++ b/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 { diff --git a/weed/topology/topology.go b/weed/topology/topology.go index c94765a2b..6f7895006 100644 --- a/weed/topology/topology.go +++ b/weed/topology/topology.go @@ -503,12 +503,15 @@ 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 { - for _, vl := range collection.GetAllVolumeLayouts() { - vl.releaseLookupOwnership() - } - } + // 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() + } } func (t *Topology) DeleteLayout(collectionName string, rp *super_block.ReplicaPlacement, ttl *needle.TTL, diskType types.DiskType) { @@ -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) - vl := t.GetVolumeLayout(v.Collection, v.ReplicaPlacement, v.Ttl, diskType) - vl.RegisterVolume(&v, dn) - vl.EnsureCorrectWritables(&v) + for { + vl := t.GetVolumeLayout(v.Collection, v.ReplicaPlacement, v.Ttl, diskType) + 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 - // RegisterVolumeLayout, which would repeat the GetVolumeLayout lookup. - vl.RegisterVolume(&v, dn) - vl.EnsureCorrectWritables(&v) + if 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 { - vl.RegisterVolume(&vi, dn) + for becameServable && !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) diff --git a/weed/topology/volume_index_digest.go b/weed/topology/volume_index_digest.go index afb18293d..38ca57379 100644 --- a/weed/topology/volume_index_digest.go +++ b/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 diff --git a/weed/topology/volume_layout.go b/weed/topology/volume_layout.go index d6b257757..d0bb4f89e 100644 --- a/weed/topology/volume_layout.go +++ b/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,