fix(ec): remove shared EC index only when no shard remains node-wide (#9955)

* fix(ec): remove the shared EC index only when no shard remains node-wide deleteEcShardIdsForEachLocation removed the shared .ecx/.ecj/.vif index as soon as a single disk's shard count hit 0, even when a sibling disk of the same node still held shards of the volume (split-disk reconciled layout) -- orphaning those shards without their index. Split the non-teardown delete into two passes: delete the requested shard files (and now-orphaned per-disk bitrot sidecars) on every disk, then remove the shared index only once no shard of the volume remains on ANY disk. This brings the Go volume server in line with the Rust one, which already gates the index removal on a node-wide check. * refactor(ec): reuse checkEcVolumeStatus across the two delete passes Address review: cache hasEcxFile/hasIdxFile from the node-wide count pass and pass them to removeEcSharedIndexFiles instead of re-listing each location's directory. * fix(ec): clean an orphaned EC .vif even when its .ecx is already gone Address review: removeEcSharedIndexFiles returned early on !hasEcxFile, so a node-wide teardown left a stale EC .vif behind when its .ecx was already removed. Decouple the .vif removal (gated on !hasIdxFile) from .ecx presence so the generation metadata doesn't leak once no shard remains node-wide.
2026-08-16 20:26:45 +00:00 · 2026-06-14 06:36:50 -07:00
parent ef5fee6c28
commit c7781bfca2
2 changed files with 155 additions and 31 deletions
@@ -0,0 +1,87 @@
+package weed_server
+
+import (
+	"context"
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/seaweedfs/seaweedfs/weed/pb/volume_server_pb"
+	"github.com/seaweedfs/seaweedfs/weed/stats"
+	"github.com/seaweedfs/seaweedfs/weed/storage"
+	"github.com/seaweedfs/seaweedfs/weed/storage/erasure_coding"
+	"github.com/seaweedfs/seaweedfs/weed/storage/needle"
+	"github.com/seaweedfs/seaweedfs/weed/storage/types"
+	"github.com/seaweedfs/seaweedfs/weed/util"
+	"github.com/stretchr/testify/require"
+)
+
+// The non-teardown EC shard delete must not remove the shared .ecx/.ecj index while
+// a sibling disk still holds shards of the same volume (split-disk layout) — doing so
+// orphans those shards. The shared index is removed only once no shard remains across
+// any disk of the node.
+func TestEcShardDeleteKeepsSharedIndexWhileSiblingHasShards(t *testing.T) {
+	tempDir := t.TempDir()
+	dir0 := filepath.Join(tempDir, "disk0")
+	dir1 := filepath.Join(tempDir, "disk1")
+	for _, d := range []string{dir0, dir1} {
+		require.NoError(t, os.MkdirAll(d, 0o755))
+	}
+	const collection = "ec-shared-index"
+	vid := needle.VolumeId(77)
+
+	store := storage.NewStore(nil, "localhost", 8080, 18080, "http://localhost:8080", "store-id",
+		[]string{dir0, dir1}, []int32{100, 100}, []util.MinFreeSpace{{}, {}}, "",
+		storage.NeedleMapInMemory, []types.DiskType{types.HardDriveType, types.HardDriveType}, nil, 3, stats.DefaultDiskIOProbeConfig())
+	done := make(chan struct{})
+	go func() {
+		for {
+			select {
+			case <-store.NewEcShardsChan:
+			case <-store.NewVolumesChan:
+			case <-store.DeletedVolumesChan:
+			case <-store.DeletedEcShardsChan:
+			case <-store.StateUpdateChan:
+			case <-done:
+				return
+			}
+		}
+	}()
+	t.Cleanup(func() {
+		store.Close()
+		close(done)
+	})
+
+	base0 := erasure_coding.EcShardFileName(collection, dir0, int(vid))
+	// Shared index lives on disk0.
+	require.NoError(t, os.WriteFile(base0+".ecx", make([]byte, 16), 0o644))
+	require.NoError(t, os.WriteFile(base0+".ecj", nil, 0o644))
+	require.NoError(t, os.WriteFile(base0+".vif", []byte("x"), 0o644))
+	plant := func(dir string, ids ...int) {
+		base := erasure_coding.EcShardFileName(collection, dir, int(vid))
+		for _, id := range ids {
+			require.NoError(t, os.WriteFile(base+erasure_coding.ToExt(id), []byte("s"), 0o644))
+		}
+	}
+	plant(dir0, 0, 5)
+	plant(dir1, 7, 12)
+
+	vs := &VolumeServer{store: store}
+	del := func(ids ...uint32) {
+		_, err := vs.VolumeEcShardsDelete(context.Background(), &volume_server_pb.VolumeEcShardsDeleteRequest{
+			VolumeId:   uint32(vid),
+			Collection: collection,
+			ShardIds:   ids,
+		})
+		require.NoError(t, err)
+	}
+
+	// Delete disk0's shards; disk1 still holds 7 and 12, so the shared index stays.
+	del(0, 5)
+	require.False(t, util.FileExists(base0+erasure_coding.ToExt(0)), "deleted shard file should be gone")
+	require.True(t, util.FileExists(base0+".ecx"), "shared .ecx must be preserved while a sibling disk holds shards")
+
+	// Delete disk1's shards; no shard remains node-wide, so the shared index is removed.
+	del(7, 12)
+	require.False(t, util.FileExists(base0+".ecx"), "shared .ecx must be removed once no shard remains node-wide")
+}
@@ -457,6 +457,8 @@ func (vs *VolumeServer) VolumeEcShardsDelete(ctx context.Context, req *volume_se

 	glog.V(0).Infof("ec volume %s shard delete %v", bName, req.ShardIds)

+	// Pass 1: delete the requested shard files (and any now-orphaned per-disk bitrot
+	// sidecars) on every disk.
 	for diskId, location := range vs.store.Locations {
 		if err := deleteEcShardIdsForEachLocation(bName, location, req.ShardIds); err != nil {
 			glog.Errorf("deleteEcShards from disk_id:%d %s %s.%v: %v", diskId, location.Directory, bName, req.ShardIds, err)
@@ -464,6 +466,34 @@ func (vs *VolumeServer) VolumeEcShardsDelete(ctx context.Context, req *volume_se
 		}
 	}

+	// Pass 2: the shared .ecx/.ecj index (and the .vif) is removed only when NO shard
+	// of this volume remains on ANY disk of this node. A per-disk check would orphan a
+	// sibling disk's shards (split-disk reconciled volumes) by deleting their index.
+	nodeWideShards := 0
+	type ecLocationStatus struct {
+		location   *storage.DiskLocation
+		hasEcxFile bool
+		hasIdxFile bool
+	}
+	statuses := make([]ecLocationStatus, 0, len(vs.store.Locations))
+	for _, location := range vs.store.Locations {
+		hasEcxFile, hasIdxFile, existingShardCount, err := checkEcVolumeStatus(bName, location)
+		if err != nil {
+			return nil, err
+		}
+		nodeWideShards += existingShardCount
+		statuses = append(statuses, ecLocationStatus{location, hasEcxFile, hasIdxFile})
+	}
+	if nodeWideShards == 0 {
+		// Reuse the status from the count pass above so the directory listing is not
+		// repeated per location.
+		for _, st := range statuses {
+			if err := removeEcSharedIndexFiles(bName, st.location, st.hasEcxFile, st.hasIdxFile); err != nil {
+				return nil, err
+			}
+		}
+	}
+
 	return &volume_server_pb.VolumeEcShardsDeleteResponse{}, nil
 }

@@ -491,21 +521,17 @@ func deleteEcShardIdsForEachLocation(bName string, location *storage.DiskLocatio
 		return nil
 	}

-	hasEcxFile, hasIdxFile, existingShardCount, err := checkEcVolumeStatus(bName, location)
+	_, _, existingShardCount, err := checkEcVolumeStatus(bName, location)
 	if err != nil {
 		return err
 	}

 	if existingShardCount == 0 {
-		// The whole EC generation is gone on this disk.
-
-		// Remove the bitrot checksum sidecar(s) (.ecsum and any .ecsum.v<N>)
-		// from both dirs. This is gated on the shards being gone, NOT on a
-		// stray .ecx still being present: a sidecar whose shards have all been
-		// deleted is orphaned and must go even when no .ecx remains, or it
-		// leaks. The per-shard-id delete that ec.rebuild uses for
-		// copied-survivor cleanup leaves shards behind, so this guard does not
-		// fire there.
+		// This disk's shards for the volume are gone. Remove the bitrot checksum
+		// sidecar(s) (.ecsum and any .ecsum.v<N>) here, since they protect this
+		// disk's shards and are now orphaned. The shared .ecx/.ecj/.vif index is
+		// NOT removed here: a sibling disk may still hold shards that need it; the
+		// caller removes the shared index only once no shard remains node-wide.
 		if err := removeBitrotSidecars(dataBaseFilename); err != nil {
 			return err
 		}
@@ -514,34 +540,45 @@ func deleteEcShardIdsForEachLocation(bName string, location *storage.DiskLocatio
 				return err
 			}
 		}
+	}

-		if hasEcxFile {
-			// Remove .ecx/.ecj from both idx and data directories
-			// since they may be in either location depending on when -dir.idx was configured.
-			// A surviving stale .ecx is the orphan-index condition this path prevents,
-			// so surface a real removal failure instead of reporting cleanup as success.
-			for _, p := range []string{indexBaseFilename + ".ecx", indexBaseFilename + ".ecj"} {
+	return nil
+}
+
+// removeEcSharedIndexFiles removes the shared .ecx/.ecj index (and the .vif when no
+// .idx is present) for an EC volume on one disk. The caller invokes it only after
+// the whole node's shards for the volume are gone, so a sibling disk's shards are
+// never orphaned by deleting their index. A surviving stale .ecx is the orphan-index
+// condition this prevents, so a real removal failure is surfaced. hasEcxFile and
+// hasIdxFile come from the caller's checkEcVolumeStatus so the directory is not
+// re-listed here.
+func removeEcSharedIndexFiles(bName string, location *storage.DiskLocation, hasEcxFile, hasIdxFile bool) error {
+	indexBaseFilename := path.Join(location.IdxDirectory, bName)
+	dataBaseFilename := path.Join(location.Directory, bName)
+	if hasEcxFile {
+		// .ecx/.ecj may be in either dir depending on when -dir.idx was configured.
+		for _, p := range []string{indexBaseFilename + ".ecx", indexBaseFilename + ".ecj"} {
+			if err := removeFileIfExists(p); err != nil {
+				return err
+			}
+		}
+		if location.IdxDirectory != location.Directory {
+			for _, p := range []string{dataBaseFilename + ".ecx", dataBaseFilename + ".ecj"} {
 				if err := removeFileIfExists(p); err != nil {
 					return err
 				}
 			}
-			if location.IdxDirectory != location.Directory {
-				for _, p := range []string{dataBaseFilename + ".ecx", dataBaseFilename + ".ecj"} {
-					if err := removeFileIfExists(p); err != nil {
-						return err
-					}
-				}
-			}
-
-			if !hasIdxFile {
-				// .vif is used for ec volumes and normal volumes
-				if err := removeFileIfExists(dataBaseFilename + ".vif"); err != nil {
-					return err
-				}
-			}
 		}
 	}
-
+	// Remove the .vif when no .idx is present (so this is not a live normal/tiered
+	// volume), independent of .ecx presence: the caller only reaches here once no
+	// shard remains node-wide, so an EC .vif left without its .ecx is stale
+	// generation metadata that would otherwise leak.
+	if !hasIdxFile {
+		if err := removeFileIfExists(dataBaseFilename + ".vif"); err != nil {
+			return err
+		}
+	}
 	return nil
 }