From ef5fee6c2858dd98971f0609a8668bc97fa562df Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Sun, 14 Jun 2026 06:36:47 -0700 Subject: [PATCH] fix(storage): delete/unmount every copy of a duplicate volume id (#9954) * fix(storage): delete and unmount every copy of a duplicate volume id NewStore has no cross-disk duplicate guard (unlike the Rust volume server, which refuses to start in that state), so a stale twin of a volume id can mount on a second disk after a disk repair. DeleteVolume and UnmountVolume returned after the first matching disk, leaving the twin to survive and re-register as the volume's content. Walk every disk and act on all copies, emitting one heartbeat delta per copy. * fix(storage): surface partial delete/unmount failures across duplicate copies Address review: if removing one copy of a duplicate volume id fails with a real error (disk IO, permissions), the loop logged it and could still return success once another copy was removed -- leaving the stale copy to re-register, the exact divergence this guards against. DeleteVolume and UnmountVolume now accumulate such errors and return them (still attempting every disk), so a copy left behind is never reported as success. Add a DeleteVolume duplicate-copies regression test. --- weed/storage/store.go | 102 ++++++++++++++--------- weed/storage/store_duplicate_vid_test.go | 51 ++++++++++++ 2 files changed, 114 insertions(+), 39 deletions(-) create mode 100644 weed/storage/store_duplicate_vid_test.go diff --git a/weed/storage/store.go b/weed/storage/store.go index 0f420203c..70ca2962a 100644 --- a/weed/storage/store.go +++ b/weed/storage/store.go @@ -1,6 +1,7 @@ package storage import ( + "errors" "fmt" "io" "path/filepath" @@ -751,64 +752,87 @@ func (s *Store) MountVolume(i needle.VolumeId) error { } func (s *Store) UnmountVolume(i needle.VolumeId) error { - v := s.findVolume(i) - if v == nil { - return nil - } - message := master_pb.VolumeShortInformationMessage{ - Id: uint32(v.Id), - Collection: v.Collection, - ReplicaPlacement: uint32(v.ReplicaPlacement.Byte()), - Version: uint32(v.Version()), - Ttl: v.Ttl.ToUint32(), - DiskType: string(v.location.DiskType), - DiskId: v.diskId, - } - + // A volume id can be mounted on more than one disk of this server (e.g. a stale + // twin re-attached after a disk repair, since NewStore has no cross-disk + // duplicate guard). Unmount every copy, not just the first match, so a stale + // twin cannot survive and re-register as the volume's content. A no-op unmount + // (no copy present) is not an error, matching the prior behavior. + var errs []error for _, location := range s.Locations { - err := location.UnloadVolume(i) - if err == nil { - glog.V(0).Infof("UnmountVolume %d", i) - s.DeletedVolumesChan <- &message - return nil - } else if err == ErrVolumeNotFound { + v, found := location.FindVolume(i) + if !found { continue } + message := master_pb.VolumeShortInformationMessage{ + Id: uint32(v.Id), + Collection: v.Collection, + ReplicaPlacement: uint32(v.ReplicaPlacement.Byte()), + Version: uint32(v.Version()), + Ttl: v.Ttl.ToUint32(), + DiskType: string(location.DiskType), + DiskId: v.diskId, + } + if err := location.UnloadVolume(i); err != nil { + if err == ErrVolumeNotFound { + continue + } + // Keep going so the other copies are still unmounted; surface the + // failure so a copy left mounted is not reported as success. + glog.Errorf("UnmountVolume %d on %s: %v", i, location.Directory, err) + errs = append(errs, err) + continue + } + glog.V(0).Infof("UnmountVolume %d disk_id:%d", i, v.diskId) + s.DeletedVolumesChan <- &message } - - return fmt.Errorf("volume %d not found on disk", i) + return errors.Join(errs...) } func (s *Store) DeleteVolume(i needle.VolumeId, onlyEmpty bool, keepRemoteData bool) error { - v := s.findVolume(i) - if v == nil { - return fmt.Errorf("delete volume %d not found on disk", i) - } - message := master_pb.VolumeShortInformationMessage{ - Id: uint32(v.Id), - Collection: v.Collection, - ReplicaPlacement: uint32(v.ReplicaPlacement.Byte()), - Version: uint32(v.Version()), - Ttl: v.Ttl.ToUint32(), - DiskType: string(v.location.DiskType), - DiskId: v.diskId, - } + // Delete every copy of the volume id across disks, not just the first match, so + // a stale twin (e.g. a re-attached disk; NewStore has no cross-disk duplicate + // guard) cannot survive a delete and re-register as the volume's content. + deletedAny := false + var errs []error for _, location := range s.Locations { + v, found := location.FindVolume(i) + if !found { + continue + } + message := master_pb.VolumeShortInformationMessage{ + Id: uint32(v.Id), + Collection: v.Collection, + ReplicaPlacement: uint32(v.ReplicaPlacement.Byte()), + Version: uint32(v.Version()), + Ttl: v.Ttl.ToUint32(), + DiskType: string(location.DiskType), + DiskId: v.diskId, + } err := location.DeleteVolume(i, onlyEmpty, keepRemoteData) if err == nil { - glog.V(0).Infof("DeleteVolume %d", i) + glog.V(0).Infof("DeleteVolume %d disk_id:%d", i, v.diskId) s.DeletedVolumesChan <- &message - return nil + deletedAny = true } else if err == ErrVolumeNotFound { continue } else if err == ErrVolumeNotEmpty { + // onlyEmpty: a non-empty copy aborts the delete rather than leaving a + // partial result across disks. return fmt.Errorf("DeleteVolume %d: %v", i, err) } else { + // A real failure on one disk must not be masked by another copy's + // success: a stale copy left on the failing disk would re-register. glog.Errorf("DeleteVolume %d: %v", i, err) + errs = append(errs, err) } } - - return fmt.Errorf("volume %d not found on disk", i) + if len(errs) > 0 { + return fmt.Errorf("DeleteVolume %d failed on some disks: %w", i, errors.Join(errs...)) + } + if !deletedAny { + return fmt.Errorf("delete volume %d not found on disk", i) + } + return nil } func (s *Store) ConfigureVolume(i needle.VolumeId, replication string) error { diff --git a/weed/storage/store_duplicate_vid_test.go b/weed/storage/store_duplicate_vid_test.go new file mode 100644 index 000000000..160c4a9ba --- /dev/null +++ b/weed/storage/store_duplicate_vid_test.go @@ -0,0 +1,51 @@ +package storage + +import ( + "testing" + + "github.com/seaweedfs/seaweedfs/weed/storage/needle" + "github.com/seaweedfs/seaweedfs/weed/storage/super_block" + "github.com/stretchr/testify/require" +) + +// A volume id can end up mounted on more than one disk of a server (a stale twin +// re-attached after a disk repair, since NewStore has no cross-disk duplicate +// guard). UnmountVolume must remove EVERY copy, not just the first match, or the +// stale twin survives and re-registers as the volume's content on the next +// heartbeat. +func TestUnmountVolumeRemovesAllDuplicateCopies(t *testing.T) { + store := newTestStore(t, 2) + const vid = needle.VolumeId(4242) + + store.Locations[0].SetVolume(vid, createTestVolume(vid, false)) + store.Locations[1].SetVolume(vid, createTestVolume(vid, false)) + + require.NoError(t, store.UnmountVolume(vid)) + + _, found0 := store.Locations[0].FindVolume(vid) + _, found1 := store.Locations[1].FindVolume(vid) + require.False(t, found0, "copy on disk 0 must be unmounted") + require.False(t, found1, "the stale twin on disk 1 must also be unmounted") +} + +// DeleteVolume must likewise destroy every copy of a duplicate volume id, not just +// the first match, so the stale twin cannot survive the delete. +func TestDeleteVolumeRemovesAllDuplicateCopies(t *testing.T) { + store := newTestStore(t, 2) + const vid = needle.VolumeId(4243) + + // Real volumes (not stubs) so Destroy can close and unlink them cleanly. + for _, loc := range store.Locations { + v, err := NewVolume(loc.Directory, loc.IdxDirectory, "", vid, NeedleMapInMemory, + &super_block.ReplicaPlacement{}, &needle.TTL{}, 0, needle.GetCurrentVersion(), 0, 0) + require.NoError(t, err) + loc.SetVolume(vid, v) + } + + require.NoError(t, store.DeleteVolume(vid, false, false)) + + _, found0 := store.Locations[0].FindVolume(vid) + _, found1 := store.Locations[1].FindVolume(vid) + require.False(t, found0, "copy on disk 0 must be deleted") + require.False(t, found1, "the stale twin on disk 1 must also be deleted") +}