volume: an already-deleted EC needle is not a delete failure (#11071)

* volume: an already-deleted EC needle is not a delete failure

Deleting a needle that is already gone is what the caller asked for, and the
non-EC paths have always said so: BatchDelete reports StatusNotModified when
DeleteVolumeNeedle finds nothing to do, and DeleteHandler answers 404 from its
ReadVolumeNeedle pre-check. The EC branches had no such case, so ErrorDeleted
fell through to a generic failure -- 500 from both, and DeleteHandler also
counted it in VolumeServerFileWriteFailures, inflating a failure metric on a
replayed or duplicated delete.

The filer already tolerates this by string-matching "already deleted" on the
result, which leaves an error message load-bearing; the status is now right at
the source instead.

Claude-Session: https://claude.ai/code/session_01P3pE6J2UPFp6G3ksfMV4s1

* volume: close the EC fixture's disk location

Close stops the location's disk-space goroutine and releases the mounted EC
volume's file handles, which otherwise live until the test binary exits.

Claude-Session: https://claude.ai/code/session_01P3pE6J2UPFp6G3ksfMV4s1

weed/server/volume_grpc_batch_delete.go

@@ -2,11 +2,13 @@ package weed_server
 
 import (
 	"context"
+	"errors"
 	"net/http"
 	"time"
 
 	"github.com/seaweedfs/seaweedfs/weed/operation"
 	"github.com/seaweedfs/seaweedfs/weed/pb/volume_server_pb"
+	"github.com/seaweedfs/seaweedfs/weed/storage"
 	"github.com/seaweedfs/seaweedfs/weed/storage/needle"
 )
 
@@ -116,7 +118,15 @@ func (vs *VolumeServer) BatchDelete(ctx context.Context, req *volume_server_pb.B
 				)
 			}
 		} else {
-			if size, err := vs.store.DeleteEcShardNeedle(ecVolume, n, n.Cookie); err != nil {
+			size, err := vs.store.DeleteEcShardNeedle(ecVolume, n, n.Cookie)
+			if errors.Is(err, storage.ErrorDeleted) {
+				// Already gone, which is what the caller asked for. The
+				// non-EC branch above reports that as StatusNotModified.
+				resp.Results = append(resp.Results, &volume_server_pb.DeleteResult{
+					FileId: fid,
+					Status: http.StatusNotModified},
+				)
+			} else if err != nil {
 				resp.Results = append(resp.Results, &volume_server_pb.DeleteResult{
 					FileId: fid,
 					Status: http.StatusInternalServerError,

weed/server/volume_grpc_batch_delete_ec_test.go

@@ -0,0 +1,132 @@
+package weed_server
+
+import (
+	"context"
+	"net"
+	"net/http"
+	"net/http/httptest"
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/prometheus/client_golang/prometheus/testutil"
+	"google.golang.org/grpc/peer"
+
+	"github.com/seaweedfs/seaweedfs/weed/pb/volume_server_pb"
+	"github.com/seaweedfs/seaweedfs/weed/security"
+	"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"
+)
+
+// ecVolumeWithDeletedNeedle mounts an EC volume holding one needle that has
+// already been deleted. A runtime EC delete is recorded in the .ecj journal
+// rather than by rewriting the sealed .ecx, and the load reads .ecj back into
+// the in-memory deleted set, so locating the needle is enough to see it is
+// gone -- no shard payload is needed.
+func ecVolumeWithDeletedNeedle(t *testing.T, vid needle.VolumeId, id types.NeedleId) *storage.Store {
+	t.Helper()
+	dir := t.TempDir()
+	base := filepath.Join(dir, vid.String())
+
+	entry := make([]byte, types.NeedleIdSize+types.OffsetSize+types.SizeSize)
+	types.NeedleIdToBytes(entry[0:types.NeedleIdSize], id)
+	types.OffsetToBytes(entry[types.NeedleIdSize:types.NeedleIdSize+types.OffsetSize], types.Offset{})
+	types.SizeToBytes(entry[types.NeedleIdSize+types.OffsetSize:], types.Size(100))
+
+	journal := make([]byte, types.NeedleIdSize)
+	types.NeedleIdToBytes(journal, id)
+
+	for name, content := range map[string][]byte{
+		base + ".ecx":  entry,
+		base + ".ecj":  journal,
+		base + ".ec00": make([]byte, 8),
+		base + ".vif":  {},
+	} {
+		if err := os.WriteFile(name, content, 0o644); err != nil {
+			t.Fatalf("write %s: %v", name, err)
+		}
+	}
+
+	location := storage.NewDiskLocation(dir, 100, util.MinFreeSpace{}, dir, types.HardDriveType, nil, stats.DiskIOProbeConfig{})
+	// Close stops the location's disk-space goroutine and releases the mounted
+	// EC volume's file handles. Registered before the mount so it also covers a
+	// failure there.
+	t.Cleanup(location.Close)
+	if _, err := location.LoadEcShard("", vid, erasure_coding.ShardId(0)); err != nil {
+		t.Fatalf("load ec shard: %v", err)
+	}
+
+	state, err := storage.NewState(dir)
+	if err != nil {
+		t.Fatalf("new store state: %v", err)
+	}
+	return &storage.Store{Locations: []*storage.DiskLocation{location}, State: state}
+}
+
+// Deleting a needle that is already gone is not a failure. The non-EC branch
+// has always answered StatusNotModified; the EC branch used to answer 500,
+// so a duplicate or replayed delete was booked as a server error.
+func TestBatchDelete_AlreadyDeletedEcNeedleIsNotAnError(t *testing.T) {
+	const vid = needle.VolumeId(7)
+	const needleId = types.NeedleId(1)
+
+	vs := &VolumeServer{
+		store: ecVolumeWithDeletedNeedle(t, vid, needleId),
+		guard: security.NewGuard(nil, "", 0, "", 0),
+	}
+
+	ctx := peer.NewContext(context.Background(), &peer.Peer{
+		Addr: &net.TCPAddr{IP: net.ParseIP("127.0.0.1"), Port: 12345},
+	})
+
+	resp, err := vs.BatchDelete(ctx, &volume_server_pb.BatchDeleteRequest{
+		FileIds:         []string{"7,0100000000"},
+		SkipCookieCheck: true,
+	})
+	if err != nil {
+		t.Fatalf("BatchDelete: %v", err)
+	}
+	if len(resp.Results) != 1 {
+		t.Fatalf("got %d results, want 1: %+v", len(resp.Results), resp.Results)
+	}
+
+	got := resp.Results[0]
+	if got.Status != http.StatusNotModified {
+		t.Fatalf("status = %d (error %q), want %d — an already-deleted needle is not a server error",
+			got.Status, got.Error, http.StatusNotModified)
+	}
+	if got.Error != "" {
+		t.Fatalf("error = %q, want empty", got.Error)
+	}
+}
+
+// The HTTP delete handler has the same asymmetry, and one extra consequence:
+// writeDeleteResult counts a failed delete in VolumeServerFileWriteFailures, so
+// a replayed delete of an EC needle used to inflate a failure metric. The
+// non-EC path answers 404 from its ReadVolumeNeedle pre-check instead.
+func TestDeleteHandler_AlreadyDeletedEcNeedleIsNotAWriteFailure(t *testing.T) {
+	const vid = needle.VolumeId(8)
+	const needleId = types.NeedleId(1)
+
+	vs := &VolumeServer{
+		store: ecVolumeWithDeletedNeedle(t, vid, needleId),
+		guard: security.NewGuard(nil, "", 0, "", 0),
+	}
+
+	before := testutil.ToFloat64(stats.VolumeServerFileWriteFailures)
+
+	w := httptest.NewRecorder()
+	vs.DeleteHandler(w, httptest.NewRequest(http.MethodDelete, "/8,0100000000", nil))
+
+	if w.Code != http.StatusNotFound {
+		t.Fatalf("status = %d (%s), want %d — an already-deleted needle is not a server error",
+			w.Code, w.Body.String(), http.StatusNotFound)
+	}
+	if after := testutil.ToFloat64(stats.VolumeServerFileWriteFailures); after != before {
+		t.Fatalf("VolumeServerFileWriteFailures moved %v -> %v; a delete that found nothing to do is not a write failure", before, after)
+	}
+}

weed/server/volume_server_handlers_write.go

@@ -12,6 +12,7 @@ import (
 	"github.com/seaweedfs/seaweedfs/weed/glog"
 	"github.com/seaweedfs/seaweedfs/weed/operation"
 	"github.com/seaweedfs/seaweedfs/weed/stats"
+	"github.com/seaweedfs/seaweedfs/weed/storage"
 	"github.com/seaweedfs/seaweedfs/weed/storage/needle"
 	"github.com/seaweedfs/seaweedfs/weed/topology"
 	"github.com/seaweedfs/seaweedfs/weed/util/buffer_pool"
@@ -101,6 +102,15 @@ func (vs *VolumeServer) DeleteHandler(w http.ResponseWriter, r *http.Request) {
 
 	if hasEcVolume {
 		count, err := vs.store.DeleteEcShardNeedle(ecVolume, n, cookie)
+		if errors.Is(err, storage.ErrorDeleted) {
+			// Already gone. The non-EC path below answers 404 from its
+			// ReadVolumeNeedle pre-check rather than counting a write failure,
+			// and callers fold that 404 into success.
+			m := make(map[string]uint32)
+			m["size"] = 0
+			writeJsonQuiet(w, r, http.StatusNotFound, m)
+			return
+		}
 		writeDeleteResult(err, count, w, r)
 		return
 	}