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.
2026-07-07 16:46:37 +00:00 · 2026-06-14 06:36:47 -07:00
parent 284796c7b6
commit ef5fee6c28
2 changed files with 114 additions and 39 deletions
@@ -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 {
@@ -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")
+}