From db9b3145e200a261b83cfbd7f6849ef4c4316b3d Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Mon, 2 Nov 2026 05:13:58 -0800 Subject: [PATCH] master: keep under-replicated volumes out of empty-volume cleanup The cleanup only looked at the copies the master lists. With a replica offline the listed copies can be empty and quiet while the missing one still holds data, and deleting them would leave that copy as the only one when it returns. Low-disk replicas now report readOnlyCanDelete and pass the eligibility check, which made this reachable for more volumes. Co-Authored-By: Claude Opus 5.5 --- weed/topology/topology_vacuum.go | 5 ++++ weed/topology/topology_vacuum_test.go | 41 +++++++++++++++++++++++++++ 2 files changed, 46 insertions(+) diff --git a/weed/topology/topology_vacuum.go b/weed/topology/topology_vacuum.go index 83144cea1d..ef103e4657 100644 --- a/weed/topology/topology_vacuum.go +++ b/weed/topology/topology_vacuum.go @@ -478,6 +478,11 @@ func (t *Topology) deleteEmptyVolumes(grpcDialOption grpc.DialOption, vl *Volume quietSeconds := int64(quietPeriod / time.Second) nowUnixSeconds := time.Now().Unix() for vid, locationList := range todoVolumeMap { + // A missing replica may still hold data; deleting the listed copies + // would leave it as the only one when it returns. + if len(locationList.list) < vl.rp.GetCopyCount() { + continue + } eligible := true for _, dn := range locationList.list { v, err := dn.GetVolumesById(vid) diff --git a/weed/topology/topology_vacuum_test.go b/weed/topology/topology_vacuum_test.go index 106011d594..897a2b0ae8 100644 --- a/weed/topology/topology_vacuum_test.go +++ b/weed/topology/topology_vacuum_test.go @@ -211,6 +211,47 @@ func TestDeleteEmptyVolumesSkipsVidWithLiveReplica(t *testing.T) { } } +func TestDeleteEmptyVolumesSkipsUnderReplicatedVid(t *testing.T) { + fake := &fakeVolumeDeleteServer{} + grpcPort, dialOption := startFakeVolumeServer(t, fake) + + topo := NewTopology("weedfs", sequence.NewMemorySequencer(), 32*1024, 5, false) + dn := topo.GetOrCreateDataCenter("dc1").GetOrCreateRack("rack1"). + GetOrCreateDataNode("127.0.0.1", 8080, grpcPort, "127.0.0.1", "dn-online", map[string]uint32{"": 10}) + rp, err := super_block.NewReplicaPlacementFromString("001") + if err != nil { + t.Fatal(err) + } + v := storage.VolumeInfo{ + Id: needle.VolumeId(1), + Collection: "c", + ModifiedAtSecond: time.Now().Unix() - 7200, + Version: needle.GetCurrentVersion(), + ReplicaPlacement: rp, + Ttl: needle.EMPTY_TTL, + ReadOnly: true, + ReadOnlyCanDelete: true, + } + dn.UpdateVolumes([]storage.VolumeInfo{v}) + topo.RegisterVolumeLayout(v, dn) + + ll := NewVolumeLocationList() + ll.list = append(ll.list, dn) + todo := map[needle.VolumeId]*VolumeLocationList{v.Id: ll} + + vl := topo.GetVolumeLayout("c", rp, needle.EMPTY_TTL, types.ToDiskType("")) + topo.deleteEmptyVolumes(dialOption, vl, todo, time.Hour) + + if _, ok := todo[v.Id]; !ok { + t.Fatal("vid left the sweep map while a replica is missing") + } + fake.mu.Lock() + defer fake.mu.Unlock() + if len(fake.deletes) != 0 { + t.Fatalf("VolumeDelete calls = %d, want 0 (the missing replica may still hold data)", len(fake.deletes)) + } +} + // With every copy eligible a copy whose delete RPC fails keeps the vid in the // sweep map; the deleted copy is gone but the vid falls through to the normal // compaction path for its surviving replicas.