fix(replication): verify-before-destroy in VolumeCopy, check.disk, and over-replication trim (#9943)

* volume: verify before destroy in VolumeCopy and replication repair

Four data-safety fixes around copy/repair paths that could destroy or
resurrect data before verifying the source or survivors.

(a) VolumeCopy no longer deletes a pre-existing local replica up front.
The delete is deferred until ReadVolumeFileStatus on the source succeeds,
so a transient source outage (or a retry after one) can no longer wipe a
healthy destination replica. Gated on source readability only; size/count
comparisons are intentionally not used because they invert legitimately
after divergent vacuum/compaction. Mirrored in the Rust volume server.

(b) volume.check.disk no longer resurrects vacuumed-deleted needles. A
key present-and-live on the source but entirely absent on the target is
ambiguous: it may be a genuine missing write, or a needle deleted on the
target and then vacuumed (its index entry and any tombstone are gone). An
individual needle AppendAtNs has no monotonic relation to a vacuum
watermark, so the old cutoff heuristic could not tell them apart. Without
positive proof the absence is a missing write, the safe default is to NOT
push it back. Tradeoff: a real missing write may go unrepaired until a
tombstone-aware path exists, but we never raise back deleted data.

(c) Over-replication trim no longer resurrects needles or removes the
wrong replica. The pre-delete sync now runs read-only (divergence check
only) instead of writing the doomed replica's needles into the survivor.
pickOneReplicaToDelete only ever removes the smallest of multiple healthy
writable replicas; it refuses the trim when doing so would leave only
read-only/integrity-flagged survivors, since file_count>0 alone cannot
prove the survivor's .dat is readable.

(d) Incomplete-volume (.note) cleanup keeps the shared .vif when an .ecx
for the same vid coexists on the disk, so removing an interrupted regular
copy cannot strip a coexisting EC volume's info file. VolumeCopy now
surfaces .note write/remove errors instead of ignoring them. In the Rust
volume server (where a persisting note is actually reachable) the .note
check moves below the empty-stub sweep and EC validation, keeps the .vif
on EC coexistence, and the mount path fails when a .note still persists.

* shell: scope the over-replication writable-survivor guard to the trim path only

The writable-survivor guard (never trim down to a read-only survivor) lived
inside the shared pickOneReplicaToDelete, so it also gated the misplaced-volume
relocation via pickOneMisplacedVolume -- a misplaced read-only volume (e.g. a
full one) would silently stop being rebalanced. Extract pickSmallestReplica
for the relocation path (which deletes-and-recreates and must act on read-only
replicas), and keep the writable-survivor guard only in pickOneReplicaToDelete
used by the over-replication trim.

* seaweed-volume: recompute keep_vif after invalid-EC cleanup in the .note path

keep_vif used the pre-validation ecx_exists snapshot, so when the EC-validation
step above removed the invalid .ecx/shards, the .note cleanup still preserved a
now-orphaned .vif. Re-check .ecx existence at cleanup time, matching the Go
hasEcxFile re-check.

* shell: keep placement when picking an over-replication victim to delete

The trim picked the smallest writable replica without regard to placement, so
it could delete the only replica in a required failure domain (e.g. with "100"
and replicas dc1 + two in dc2, deleting dc1 leaves both survivors in dc2).
Prefer a writable replica whose removal still satisfies placement, falling back
to the smallest writable only when none does.

weed/shell/command_volume_check_disk.go

@@ -22,7 +22,6 @@ import (
 	"github.com/seaweedfs/seaweedfs/weed/pb/volume_server_pb"
 	"github.com/seaweedfs/seaweedfs/weed/server/constants"
 	"github.com/seaweedfs/seaweedfs/weed/storage/needle_map"
-	"github.com/seaweedfs/seaweedfs/weed/storage/volume_replica"
 	"google.golang.org/grpc"
 )
 
@@ -44,6 +43,12 @@ type volumeCheckDisk struct {
 	syncDeletions      bool
 	fixReadOnly        bool
 	nonRepairThreshold float64
+	// resurrectMissingNeedles controls whether a needle present on the source
+	// but entirely absent on the target is pushed back. Default false: an
+	// absent needle is indistinguishable from a vacuumed delete, so the safe
+	// default never raises deleted data. No caller sets this true today; it is
+	// the seam for a future tombstone-aware repair path.
+	resurrectMissingNeedles bool
 
 	ewg *ErrorWaitGroup
 }
@@ -511,9 +516,8 @@ func (vcd *volumeCheckDisk) doVolumeCheckDisk(minuend, subtrahend *needle_map.Me
 	// hash join, can be more efficient
 	var missingNeedles []needle_map.NeedleValue
 	var partiallyDeletedNeedles []needle_map.NeedleValue
+	var skippedAbsentNeedles int
 	var counter int
-	doCutoffOfLastNeedle := true
-	cutoffFromAtNs := uint64(vcd.now.UnixNano())
 
 	minuend.DescendingVisit(func(minuendValue needle_map.NeedleValue) error {
 		counter++
@@ -521,27 +525,34 @@ func (vcd *volumeCheckDisk) doVolumeCheckDisk(minuend, subtrahend *needle_map.Me
 			if minuendValue.Size.IsDeleted() {
 				return nil
 			}
-			if doCutoffOfLastNeedle {
-				if needleMeta, err := volume_replica.ReadNeedleMeta(vcd.grpcDialOption(), pb.NewServerAddressFromDataNode(source.location.dataNode), source.info.Id, minuendValue); err == nil {
-					// needles older than the cutoff time are not missing yet
-					if needleMeta.AppendAtNs > cutoffFromAtNs {
-						return nil
-					}
-					doCutoffOfLastNeedle = false
-				}
+			// A key present-and-live on the source but entirely absent on the
+			// target is ambiguous: either a genuine missing write, or a needle
+			// that was deleted on the target and then vacuumed away (its index
+			// entry, including any tombstone, is gone after vacuum). An
+			// individual needle's AppendAtNs has no monotonic relation to a
+			// vacuum watermark, so it cannot distinguish the two. Without
+			// positive proof the absence is a missing write (rather than a
+			// vacuumed delete), the safe default is to NOT resurrect: a real
+			// missing write may go unrepaired until a tombstone-aware path
+			// exists, but we never raise back data the operator deleted.
+			if !vcd.resurrectMissingNeedles {
+				skippedAbsentNeedles++
+				return nil
 			}
 			missingNeedles = append(missingNeedles, minuendValue)
 		} else {
 			if minuendValue.Size.IsDeleted() && !subtrahendValue.Size.IsDeleted() {
 				partiallyDeletedNeedles = append(partiallyDeletedNeedles, minuendValue)
 			}
-			if doCutoffOfLastNeedle {
-				doCutoffOfLastNeedle = false
-			}
 		}
 		return nil
 	})
 
+	if skippedAbsentNeedles > 0 {
+		vcd.write("volume %d %s: not resurrecting %d needle(s) absent on %s (cannot prove they are missing writes vs vacuumed deletes)",
+			source.info.Id, source.location.dataNode.Id, skippedAbsentNeedles, target.location.dataNode.Id)
+	}
+
 	vcd.write("volume %d %s has %d entries, %s missed %d and partially deleted %d entries",
 		source.info.Id, source.location.dataNode.Id, counter, target.location.dataNode.Id, len(missingNeedles), len(partiallyDeletedNeedles))
 

weed/shell/command_volume_check_disk_test.go

@@ -6,8 +6,66 @@ import (
 	"time"
 
 	"github.com/seaweedfs/seaweedfs/weed/pb/master_pb"
+	"github.com/seaweedfs/seaweedfs/weed/storage/needle_map"
+	"github.com/seaweedfs/seaweedfs/weed/storage/types"
+	"google.golang.org/grpc"
+	"google.golang.org/grpc/credentials/insecure"
 )
 
+// TestDoVolumeCheckDiskDoesNotResurrectAbsentNeedle verifies that a needle
+// present-and-live on the source but entirely absent on the target is NOT
+// pushed back by default. Such an absence is indistinguishable from a needle
+// that was deleted on the target and then vacuumed away, so resurrecting it
+// would raise back data the operator deleted.
+func TestDoVolumeCheckDiskDoesNotResurrectAbsentNeedle(t *testing.T) {
+	sourceDB, targetDB := needle_map.NewMemDb(), needle_map.NewMemDb()
+	defer sourceDB.Close()
+	defer targetDB.Close()
+
+	// Source has a live needle; target lacks it (e.g. deleted+vacuumed there).
+	if err := sourceDB.Set(types.NeedleId(1001), types.ToOffset(8), types.Size(123)); err != nil {
+		t.Fatalf("seed source: %v", err)
+	}
+
+	var buf bytes.Buffer
+	vcd := &volumeCheckDisk{
+		commandEnv: &CommandEnv{
+			option: &ShellOptions{
+				GrpcDialOption: grpc.WithTransportCredentials(insecure.NewCredentials()),
+			},
+		},
+		writer:             &buf,
+		now:                time.Now(),
+		applyChanges:       false, // simulation: should not even read the source blob
+		nonRepairThreshold: 1,
+		// resurrectMissingNeedles left false: the safe default.
+	}
+
+	// Source points at an unreachable address: with the fix no blob read is
+	// attempted, so this is never contacted. The pre-fix code queues the
+	// absent needle and tries to read it from here, surfacing as an error.
+	source := &VolumeReplica{
+		location: &location{"dc1", "r1", &master_pb.DataNodeInfo{Id: "127.0.0.1:1"}},
+		info:     &master_pb.VolumeInformationMessage{Id: 7},
+	}
+	target := &VolumeReplica{
+		location: &location{"dc1", "r2", &master_pb.DataNodeInfo{Id: "127.0.0.1:2"}},
+		info:     &master_pb.VolumeInformationMessage{Id: 7},
+	}
+
+	// With the safe default, the absent needle is skipped before any network
+	// read, so this returns cleanly with no changes. On the pre-fix code the
+	// needle is queued and a source blob read is attempted, which has no server
+	// to reach and surfaces as an error (or a resurrection) instead.
+	hasChanges, err := vcd.doVolumeCheckDisk(sourceDB, targetDB, source, target)
+	if err != nil {
+		t.Fatalf("doVolumeCheckDisk returned error: %v", err)
+	}
+	if hasChanges {
+		t.Fatalf("absent-on-target needle was resurrected; expected no changes")
+	}
+}
+
 type testCommandVolumeCheckDisk struct {
 	commandVolumeCheckDisk
 }

weed/shell/command_volume_fix_replication.go

@@ -219,7 +219,12 @@ func collectVolumeReplicaLocations(topologyInfo *master_pb.TopologyInfo) (map[ui
 
 type SelectOneVolumeFunc func(replicas []*VolumeReplica, replicaPlacement *super_block.ReplicaPlacement) *VolumeReplica
 
-func checkOneVolume(a *VolumeReplica, b *VolumeReplica, writer io.Writer, commandEnv *CommandEnv) (err error) {
+// checkOneVolume compares the index of replica a against b. With
+// applyChanges=false it is a read-only divergence check; the over-replication
+// trim must use that mode so it does not push the soon-to-be-deleted replica's
+// needles into the survivor (which would resurrect data and is the opposite of
+// a safe trim).
+func checkOneVolume(a *VolumeReplica, b *VolumeReplica, writer io.Writer, commandEnv *CommandEnv, applyChanges bool) (err error) {
 	aDB, bDB := needle_map.NewMemDb(), needle_map.NewMemDb()
 	defer func() {
 		aDB.Close()
@@ -232,7 +237,7 @@ func checkOneVolume(a *VolumeReplica, b *VolumeReplica, writer io.Writer, comman
 		now:        time.Now(),
 
 		verbose:            false,
-		applyChanges:       true,
+		applyChanges:       applyChanges,
 		syncDeletions:      false,
 		nonRepairThreshold: float64(1),
 	}
@@ -261,6 +266,10 @@ func (c *commandVolumeFixReplication) deleteOneVolume(commandEnv *CommandEnv, wr
 		replicaPlacement, _ := super_block.NewReplicaPlacementFromByte(byte(replicas[0].info.ReplicaPlacement))
 
 		replica := selectOneVolumeFn(replicas, replicaPlacement)
+		if replica == nil {
+			fmt.Fprintf(writer, "skip trimming volume %d: no safe replica to delete (would leave only read-only survivors)\n", vid)
+			continue
+		}
 
 		// check collection name pattern
 		if *c.collectionPattern != "" {
@@ -302,7 +311,9 @@ func (c *commandVolumeFixReplication) deleteOneVolume(commandEnv *CommandEnv, wr
 				if replicaB.location.dataNode == replica.location.dataNode {
 					continue
 				}
-				if checkErr = checkOneVolume(replica, replicaB, writer, commandEnv); checkErr != nil {
+				// Read-only divergence check only: never write the doomed
+				// replica's needles into a survivor while trimming.
+				if checkErr = checkOneVolume(replica, replicaB, writer, commandEnv, false); checkErr != nil {
 					fmt.Fprintf(writer, "sync volume %d on %s and %s: %v\n", replica.info.Id, replica.location.dataNode.Id, replicaB.location.dataNode.Id, checkErr)
 					break
 				}
@@ -625,8 +636,20 @@ func countReplicas(replicas []*VolumeReplica) (diffDc, diffRack, diffNode map[st
 	return
 }
 
-func pickOneReplicaToDelete(replicas []*VolumeReplica, replicaPlacement *super_block.ReplicaPlacement) *VolumeReplica {
-	slices.SortFunc(replicas, func(a, b *VolumeReplica) int {
+// pickOneReplicaToDelete selects the replica to trim when over-replicated.
+// It only ever removes the smallest of multiple healthy writable replicas: a
+// ReadOnly/integrity-flagged replica is never chosen for deletion, and the
+// trim is refused (returns nil) when removing a writable replica would leave
+// only ReadOnly survivors. VolumeStatus file_count>0 alone cannot prove the
+// survivors' .dat is readable, so we do not over-claim survivor health.
+// pickSmallestReplica returns the smallest replica (ties broken by oldest then
+// lowest compact revision), or nil for an empty set.
+func pickSmallestReplica(replicas []*VolumeReplica) *VolumeReplica {
+	if len(replicas) == 0 {
+		return nil
+	}
+	sorted := slices.Clone(replicas)
+	slices.SortFunc(sorted, func(a, b *VolumeReplica) int {
 		if a.info.Size != b.info.Size {
 			return int(a.info.Size - b.info.Size)
 		}
@@ -638,9 +661,39 @@ func pickOneReplicaToDelete(replicas []*VolumeReplica, replicaPlacement *super_b
 		}
 		return 0
 	})
+	return sorted[0]
+}
 
-	return replicas[0]
-
+func pickOneReplicaToDelete(replicas []*VolumeReplica, replicaPlacement *super_block.ReplicaPlacement) *VolumeReplica {
+	// Over-replication trim: only ever remove a writable replica, and only
+	// when another writable one survives, so a healthy copy is never deleted
+	// down to a read-only (e.g. full or integrity-flagged) survivor.
+	var writable []*VolumeReplica
+	for _, r := range replicas {
+		if !r.info.ReadOnly {
+			writable = append(writable, r)
+		}
+	}
+	if len(writable) < 2 {
+		return nil
+	}
+	// Prefer a writable replica whose removal still satisfies placement, so the
+	// trim does not strip the only replica in a required failure domain. Fall
+	// back to the smallest writable if none keeps placement (a later misplaced
+	// cycle then re-balances).
+	var placementSafe []*VolumeReplica
+	for i, r := range replicas {
+		if r.info.ReadOnly {
+			continue
+		}
+		if !isMisplaced(otherThan(replicas, i), replicaPlacement) {
+			placementSafe = append(placementSafe, r)
+		}
+	}
+	if len(placementSafe) > 0 {
+		return pickSmallestReplica(placementSafe)
+	}
+	return pickSmallestReplica(writable)
 }
 
 // check and fix misplaced volumes
@@ -669,6 +722,10 @@ func otherThan(replicas []*VolumeReplica, index int) (others []*VolumeReplica) {
 
 func pickOneMisplacedVolume(replicas []*VolumeReplica, replicaPlacement *super_block.ReplicaPlacement) (toDelete *VolumeReplica) {
 
+	// Relocation, not over-replication: pick the smallest replica to delete
+	// and recreate at a correct placement. Unlike the trim this must still act
+	// on read-only replicas (e.g. a full but misplaced volume), so it does not
+	// use pickOneReplicaToDelete's writable-survivor guard.
 	var deletionCandidates []*VolumeReplica
 	for i := 0; i < len(replicas); i++ {
 		others := otherThan(replicas, i)
@@ -676,10 +733,10 @@ func pickOneMisplacedVolume(replicas []*VolumeReplica, replicaPlacement *super_b
 			deletionCandidates = append(deletionCandidates, replicas[i])
 		}
 	}
-	if len(deletionCandidates) > 0 {
-		return pickOneReplicaToDelete(deletionCandidates, replicaPlacement)
+	if toDelete = pickSmallestReplica(deletionCandidates); toDelete != nil {
+		return toDelete
 	}
 
-	return pickOneReplicaToDelete(replicas, replicaPlacement)
+	return pickSmallestReplica(replicas)
 
 }

weed/shell/command_volume_fix_replication_test.go

@@ -439,6 +439,127 @@ func TestPickingMisplacedVolumeToDelete(t *testing.T) {
 
 }
 
+func TestPickOneReplicaToDeleteSkipsReadOnlySurvivor(t *testing.T) {
+	replicaPlacement, _ := super_block.NewReplicaPlacementFromString("001")
+
+	// One writable replica and one read-only replica. The trim must never
+	// delete the only writable replica while the survivor is read-only:
+	// VolumeStatus alone cannot prove the read-only .dat is readable, so the
+	// trim is refused.
+	t.Run("refuses to delete the only writable replica while a read-only survivor remains", func(t *testing.T) {
+		replicas := []*VolumeReplica{
+			{
+				location: &location{"dc1", "r1", &master_pb.DataNodeInfo{Id: "dn-writable"}},
+				info:     &master_pb.VolumeInformationMessage{Size: 90, ReadOnly: false},
+			},
+			{
+				location: &location{"dc1", "r2", &master_pb.DataNodeInfo{Id: "dn-readonly"}},
+				info:     &master_pb.VolumeInformationMessage{Size: 100, ReadOnly: true},
+			},
+		}
+		if got := pickOneReplicaToDelete(replicas, replicaPlacement); got != nil {
+			t.Fatalf("expected no replica to delete, got %s", got.location.dataNode.Id)
+		}
+	})
+
+	// All survivors read-only: never trim.
+	t.Run("refuses when all replicas are read-only", func(t *testing.T) {
+		replicas := []*VolumeReplica{
+			{
+				location: &location{"dc1", "r1", &master_pb.DataNodeInfo{Id: "dn1"}},
+				info:     &master_pb.VolumeInformationMessage{Size: 100, ReadOnly: true},
+			},
+			{
+				location: &location{"dc1", "r2", &master_pb.DataNodeInfo{Id: "dn2"}},
+				info:     &master_pb.VolumeInformationMessage{Size: 100, ReadOnly: true},
+			},
+		}
+		if got := pickOneReplicaToDelete(replicas, replicaPlacement); got != nil {
+			t.Fatalf("expected no replica to delete, got %s", got.location.dataNode.Id)
+		}
+	})
+
+	// Two healthy writable replicas plus a read-only one: trim the smallest
+	// writable, never the read-only one.
+	t.Run("trims the smallest writable replica and never the read-only one", func(t *testing.T) {
+		replicas := []*VolumeReplica{
+			{
+				location: &location{"dc1", "r1", &master_pb.DataNodeInfo{Id: "dn-big"}},
+				info:     &master_pb.VolumeInformationMessage{Size: 100, ReadOnly: false},
+			},
+			{
+				location: &location{"dc1", "r2", &master_pb.DataNodeInfo{Id: "dn-small"}},
+				info:     &master_pb.VolumeInformationMessage{Size: 90, ReadOnly: false},
+			},
+			{
+				location: &location{"dc1", "r3", &master_pb.DataNodeInfo{Id: "dn-readonly"}},
+				info:     &master_pb.VolumeInformationMessage{Size: 80, ReadOnly: true},
+			},
+		}
+		got := pickOneReplicaToDelete(replicas, replicaPlacement)
+		if got == nil {
+			t.Fatalf("expected a writable replica to delete, got nil")
+		}
+		if got.location.dataNode.Id != "dn-small" {
+			t.Fatalf("expected to delete smallest writable dn-small, got %s", got.location.dataNode.Id)
+		}
+	})
+}
+
+// The over-replication writable-survivor guard must NOT leak into the misplaced
+// relocation path: a misplaced volume whose replicas are all read-only (e.g. a
+// full volume) must still be relocated, picking the smallest replica to delete
+// and recreate at a correct placement.
+func TestPickOneMisplacedVolumeRelocatesReadOnlyReplicas(t *testing.T) {
+	replicaPlacement, _ := super_block.NewReplicaPlacementFromString("001")
+	replicas := []*VolumeReplica{
+		{
+			location: &location{"dc1", "r1", &master_pb.DataNodeInfo{Id: "dn1"}},
+			info:     &master_pb.VolumeInformationMessage{Size: 100, ReadOnly: true},
+		},
+		{
+			location: &location{"dc1", "r2", &master_pb.DataNodeInfo{Id: "dn2"}},
+			info:     &master_pb.VolumeInformationMessage{Size: 99, ReadOnly: true},
+		},
+	}
+	got := pickOneMisplacedVolume(replicas, replicaPlacement)
+	if got == nil {
+		t.Fatal("misplaced read-only volume must still be relocated, got nil")
+	}
+	if got.location.dataNode.Id != "dn2" {
+		t.Fatalf("expected to relocate smallest replica dn2, got %s", got.location.dataNode.Id)
+	}
+}
+
+// The over-replication trim must not strip the only replica in a required
+// failure domain: with "100" and replicas in dc1 + two in dc2, deleting the
+// smallest (dc1) leaves both survivors in dc2 and violates placement, so the
+// trim must instead delete a dc2 writable replica.
+func TestPickOneReplicaToDeletePreservesPlacement(t *testing.T) {
+	replicaPlacement, _ := super_block.NewReplicaPlacementFromString("100")
+	replicas := []*VolumeReplica{
+		{
+			location: &location{"dc1", "r1", &master_pb.DataNodeInfo{Id: "dc1-writable"}},
+			info:     &master_pb.VolumeInformationMessage{Size: 90, ReadOnly: false},
+		},
+		{
+			location: &location{"dc2", "r2", &master_pb.DataNodeInfo{Id: "dc2-readonly"}},
+			info:     &master_pb.VolumeInformationMessage{Size: 80, ReadOnly: true},
+		},
+		{
+			location: &location{"dc2", "r3", &master_pb.DataNodeInfo{Id: "dc2-writable"}},
+			info:     &master_pb.VolumeInformationMessage{Size: 100, ReadOnly: false},
+		},
+	}
+	got := pickOneReplicaToDelete(replicas, replicaPlacement)
+	if got == nil {
+		t.Fatal("expected a replica to delete")
+	}
+	if got.location.dataNode.Id != "dc2-writable" {
+		t.Fatalf("expected to delete dc2-writable to preserve placement, got %s", got.location.dataNode.Id)
+	}
+}
+
 func TestSatisfyReplicaCurrentLocation(t *testing.T) {
 
 	var tests = []testcase{