Merge #143583 #143599

143583: storage: test that batch commit doesn't touch SSTs r=iskettaneh a=iskettaneh

This commit adds simple test where it injects errors on any operation touching any SSTable file before attempting to commit a batch, and asserts that the batch can commit successfully without needing any SSTable.

References: #143135

Release note: None

143599: batcheval: prevent excising non-user keys r=iskettaneh a=iskettaneh

This commit performs some checks in the Excise command evaluation. It prevents the command to run against non-user keys. Excising non-global keys is not allowed as it would leave the replica in a bad state. For example, we don't want to allow excising a range descriptor.

References: #143135

Release note: None

Co-authored-by: Ibrahim Kettaneh <[email protected]>

Author: craig[bot]

pkg/kv/kvserver/batcheval/BUILD.bazel

@@ -112,6 +112,7 @@ go_test(
         "cmd_delete_range_gchint_test.go",
         "cmd_delete_range_test.go",
         "cmd_end_transaction_test.go",
+        "cmd_excise_test.go",
         "cmd_export_test.go",
         "cmd_get_test.go",
         "cmd_is_span_empty_test.go",

pkg/kv/kvserver/batcheval/cmd_excise.go

@@ -14,6 +14,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/kvserverpb"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/storage"
+	"github.com/cockroachdb/errors"
 )
 
 func init() {
@@ -22,10 +23,33 @@ func init() {
 
 // EvalExcise evaluates a Excise command.
 func EvalExcise(
-	_ context.Context, _ storage.ReadWriter, cArgs CommandArgs, _ kvpb.Response,
+	_ context.Context, _ storage.ReadWriter, cArgs CommandArgs, resp kvpb.Response,
 ) (result.Result, error) {
 	args := cArgs.Args.(*kvpb.ExciseRequest)
-	start, end := storage.MVCCKey{Key: args.Key}, storage.MVCCKey{Key: args.EndKey}
+	start, end := args.Key, args.EndKey
+
+	// Verify that the start and end keys are for global key space. Excising
+	// non-global keys is not allowed as it would leave the replica in a bad
+	// state. For example, we don't want to allow excising a range descriptor.
+	rStart, err := keys.Addr(start)
+	if err != nil {
+		return result.Result{}, err
+	}
+
+	rEnd, err := keys.Addr(end)
+	if err != nil {
+		return result.Result{}, err
+	}
+
+	if !start.Equal(rStart.AsRawKey()) {
+		return result.Result{},
+			errors.Errorf("excise can only be run against global keys, but found start key: %s", start)
+	}
+
+	if !end.Equal(rEnd.AsRawKey()) {
+		return result.Result{},
+			errors.Errorf("excise can only be run against global keys, but found end key: %s", end)
+	}
 
 	// Since we can't know the exact range stats, mark it as an estimate.
 	cArgs.Stats.ContainsEstimates++
@@ -36,7 +60,7 @@ func EvalExcise(
 	return result.Result{
 		Replicated: kvserverpb.ReplicatedEvalResult{
 			Excise: &kvserverpb.ReplicatedEvalResult_Excise{
-				Span:          roachpb.Span{Key: start.Key, EndKey: end.Key},
+				Span:          roachpb.Span{Key: start, EndKey: end},
 				LockTableSpan: roachpb.Span{Key: ltStart, EndKey: ltEnd},
 			},
 		},

pkg/kv/kvserver/batcheval/cmd_excise_test.go

@@ -0,0 +1,95 @@
+// Copyright 2025 The Cockroach Authors.
+//
+// Use of this software is governed by the CockroachDB Software License
+// included in the /LICENSE file.
+
+package batcheval_test
+
+import (
+	"context"
+	"testing"
+
+	"github.com/cockroachdb/cockroach/pkg/keys"
+	"github.com/cockroachdb/cockroach/pkg/kv/kvpb"
+	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/batcheval"
+	"github.com/cockroachdb/cockroach/pkg/roachpb"
+	"github.com/cockroachdb/cockroach/pkg/storage/enginepb"
+	"github.com/cockroachdb/cockroach/pkg/util/hlc"
+	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
+	"github.com/cockroachdb/cockroach/pkg/util/log"
+	"github.com/cockroachdb/cockroach/pkg/util/timeutil"
+	"github.com/stretchr/testify/require"
+)
+
+// TestExciseEval tests basic Excise evaluation.
+func TestExciseEval(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	defer log.Scope(t).Close(t)
+
+	ctx := context.Background()
+	clock := hlc.NewClockForTesting(timeutil.NewManualTime(timeutil.Now()))
+	evalCtx := (&batcheval.MockEvalCtx{Clock: clock}).EvalContext()
+
+	testCases := []struct {
+		name      string
+		startKey  roachpb.Key
+		endKey    roachpb.Key
+		expectErr string
+	}{
+		{
+			// Valid range.
+			startKey:  roachpb.Key("a"),
+			endKey:    roachpb.Key("z"),
+			expectErr: "",
+		},
+		{
+			// Only the start key is a non-user key.
+			startKey:  keys.RangeDescriptorKey(roachpb.RKey("a")),
+			endKey:    roachpb.Key("z"),
+			expectErr: "excise can only be run against global keys",
+		},
+		{
+			// Only the end key is a non-user key.
+			startKey:  roachpb.Key("a"),
+			endKey:    keys.RangeDescriptorKey(roachpb.RKey("z")),
+			expectErr: "excise can only be run against global keys",
+		},
+		{
+			// Both keys are non-user keys.
+			startKey:  keys.RangeDescriptorKey(roachpb.RKey("a")),
+			endKey:    keys.RangeDescriptorKey(roachpb.RKey("z")),
+			expectErr: "excise can only be run against global keys",
+		},
+	}
+
+	for _, tc := range testCases {
+		resp := &kvpb.ExciseResponse{}
+		res, err := batcheval.EvalExcise(ctx, nil, batcheval.CommandArgs{
+			EvalCtx: evalCtx,
+			Stats:   &enginepb.MVCCStats{},
+			Args: &kvpb.ExciseRequest{
+				RequestHeader: kvpb.RequestHeader{
+					Key:    tc.startKey,
+					EndKey: tc.endKey,
+				},
+			},
+		}, resp)
+
+		// If there are no errors, we expect the result to be populated.
+		if tc.expectErr == "" {
+			require.NoError(t, err)
+
+			userSpan := roachpb.Span{Key: tc.startKey, EndKey: tc.endKey}
+			ltStart, _ := keys.LockTableSingleKey(tc.startKey, nil)
+			ltEnd, _ := keys.LockTableSingleKey(tc.endKey, nil)
+			LockTableSpan := roachpb.Span{Key: ltStart, EndKey: ltEnd}
+
+			require.NotNil(t, res.Replicated.Excise)
+			require.Equal(t, userSpan, res.Replicated.Excise.Span)
+			require.Equal(t, LockTableSpan, res.Replicated.Excise.LockTableSpan)
+		} else {
+			require.Regexp(t, tc.expectErr, err)
+			require.Nil(t, res.Replicated.Excise)
+		}
+	}
+}

pkg/storage/BUILD.bazel

@@ -217,6 +217,7 @@ go_test(
         "@com_github_cockroachdb_pebble//sstable",
         "@com_github_cockroachdb_pebble//sstable/block",
         "@com_github_cockroachdb_pebble//vfs",
+        "@com_github_cockroachdb_pebble//vfs/errorfs",
         "@com_github_cockroachdb_redact//:redact",
         "@com_github_kr_pretty//:pretty",
         "@com_github_stretchr_testify//assert",

pkg/storage/batch_test.go

@@ -12,6 +12,7 @@ import (
 	"fmt"
 	"reflect"
 	"strconv"
+	"strings"
 	"sync/atomic"
 	"testing"
 
@@ -27,6 +28,8 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/util/protoutil"
 	"github.com/cockroachdb/errors"
 	"github.com/cockroachdb/pebble"
+	"github.com/cockroachdb/pebble/vfs"
+	"github.com/cockroachdb/pebble/vfs/errorfs"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -1010,3 +1013,61 @@ func TestBatchReader(t *testing.T) {
 	require.False(t, r.Next())
 	require.NoError(t, r.Error())
 }
+
+// TestBatchCommitDoesntTouchSST tests that committing a writeBatch doesn't
+// touch SST files.
+func TestBatchCommitDoesntTouchSST(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	defer log.Scope(t).Close(t)
+
+	// Create an atomic variable that will cause an error when SST operations are
+	// performed.
+	var failSSTOps atomic.Bool
+
+	// Create a custom injector that blocks SST operations when failSSTOps is
+	// true.
+	injector := errorfs.InjectorFunc(func(op errorfs.Op) error {
+		if strings.Contains(op.Path, ".sst") && failSSTOps.Load() {
+			return errors.Newf("blocking SST operation: %+v", op)
+		}
+		return nil
+	})
+
+	// Create the wrapped filesystem, and create a db.
+	memFS := vfs.NewMem()
+	wrappedFS := errorfs.Wrap(memFS, injector)
+	env := mustInitTestEnv(t, wrappedFS, "")
+	db, err := Open(context.Background(), env, cluster.MakeClusterSettings())
+	require.NoError(t, err)
+	defer db.Close()
+
+	// Initialize the db with some data.
+	initBatch := db.NewBatch()
+	defer initBatch.Close()
+
+	// Perform some operations.
+	require.NoError(t, initBatch.PutUnversioned(mvccKey("key1").Key, []byte("val1")))
+	require.NoError(t, initBatch.PutUnversioned(mvccKey("key2").Key, []byte("val2")))
+	require.NoError(t, initBatch.PutUnversioned(mvccKey("key3").Key, []byte("val3")))
+	require.NoError(t, initBatch.PutUnversioned(mvccKey("key4").Key, []byte("val4")))
+	require.NoError(t, initBatch.Commit(true /* sync */))
+
+	// Force a flush to create an SST file.
+	require.NoError(t, db.Flush())
+
+	// Create a new batch for testing.
+	testingBatch := db.NewBatch()
+	defer testingBatch.Close()
+
+	// Perform some operations.
+	require.Equal(t, []byte("val1"), mvccGetRaw(t, testingBatch, mvccKey("key1")))
+	require.Equal(t, []byte(nil), mvccGetRaw(t, testingBatch, mvccKey("non-existent-key")))
+	_, err = Scan(context.Background(), testingBatch, localMax, roachpb.KeyMax, 0)
+	require.NoError(t, err)
+	require.NoError(t, testingBatch.ClearUnversioned(mvccKey("key4").Key, ClearOptions{}))
+
+	// Before committing, enable SST operation errors and make sure the commit
+	// succeeds.
+	failSSTOps.Store(true)
+	require.NoError(t, testingBatch.Commit(true /* sync */))
+}