spanset: assert that batches don't access store local and unreplicated RangeID local keys #157153

iskettaneh · 2025-11-11T01:03:12Z

This PR adds the following test-only assertions:

Generated batches don't touch store-local keys.
Generated batches don't touch unreplicated RangeID local keys.

We disable the check in exactly 3 locations we know that we currently
touch those keys.

Fixes: #156537

Release note: None

cockroach-teamcity · 2025-11-11T01:03:37Z

This change is

github-actions · 2025-11-11T20:54:00Z

Potential Bug(s) Detected

The three-stage Claude Code analysis has identified potential bug(s) in this PR that may warrant investigation.

Next Steps:
Please review the detailed findings in the workflow run.

Note: When viewing the workflow output, scroll to the bottom to find the Final Analysis Summary.

After you review the findings, please tag the issue as follows:

If the detected issue is real or was helpful in any way, please tag the issue with O-AI-Review-Real-Issue-Found
If the detected issue was not helpful in any way, please tag the issue with O-AI-Review-Not-Helpful

github-actions · 2025-11-12T00:49:59Z

Potential Bug(s) Detected

The three-stage Claude Code analysis has identified potential bug(s) in this PR that may warrant investigation.

Next Steps:
Please review the detailed findings in the workflow run.

Note: When viewing the workflow output, scroll to the bottom to find the Final Analysis Summary.

After you review the findings, please tag the issue as follows:

If the detected issue is real or was helpful in any way, please tag the issue with O-AI-Review-Real-Issue-Found
If the detected issue was not helpful in any way, please tag the issue with O-AI-Review-Not-Helpful

pav-kv · 2025-11-14T12:40:05Z

pkg/kv/kvserver/spanset/spanset.go

+	forbiddenSpansMatchers []func(roachpb.Span) error
+	allowUndeclared        bool
+	allowForbidden         bool


Pre-existing, but made stronger in this PR: it seems that this type shouldn't mix the job of representing a set of spans and that of [test-only] verifying it / forbidding keys. There seems to be a clear subset of methods that add/canonicalize/access spans without having an opinion on them, and then the "check" methods that verify stuff. I would make the latter the responsibility of a wrapper (either existing one like spanSetBatch, or make a new one).

Ideally need a prereq PR/commit that does that separation.

Will do it in another PR

pkg/kv/kvserver/spanset/spanset.go

pkg/kv/kvserver/spanset/spanset_test.go

pkg/keys/keys.go

pkg/kv/kvserver/batcheval/cmd_truncate_log.go

pkg/kv/kvserver/replica_write.go

pkg/kv/kvserver/spanset/batch.go

pkg/kv/kvserver/spanset/spanset_test.go

iskettaneh

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @pav-kv)

pkg/kv/kvserver/replica_write.go

pkg/kv/kvserver/batcheval/cmd_truncate_log.go

pkg/kv/kvserver/spanset/batch.go

iskettaneh · 2025-11-19T00:19:30Z

pkg/kv/kvserver/spanset/spanset.go

+	forbiddenSpansMatchers []func(roachpb.Span) error
+	allowUndeclared        bool
+	allowForbidden         bool


Will do it in another PR

pkg/kv/kvserver/spanset/spanset.go

pkg/kv/kvserver/spanset/spanset_test.go

pav-kv

This is nearing the LGTM, thanks for making it cleaner each time.

pkg/kv/kvserver/spanset/spanset.go

pkg/kv/kvserver/spanset/spanset_test.go

pkg/kv/kvserver/spanset/spanset.go

pkg/kv/kvserver/replica_write.go

iskettaneh

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @pav-kv)

pkg/kv/kvserver/replica_write.go

pkg/kv/kvserver/spanset/spanset.go

pkg/kv/kvserver/spanset/spanset_test.go

pkg/kv/kvserver/spanset/spanset.go

pkg/kv/kvserver/spanset/batch.go

pkg/kv/kvserver/spanset/spanset_test.go

This commit introduces the concept of forbidden spans, which will allow us to assert our batches don't modify Raft's engine keys.

iskettaneh

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @pav-kv)

pkg/kv/kvserver/replica_write.go

pkg/kv/kvserver/spanset/batch.go

pkg/kv/kvserver/spanset/spanset.go

pkg/kv/kvserver/spanset/spanset_test.go

iskettaneh

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @pav-kv)

pkg/kv/kvserver/spanset/batch.go

pkg/kv/kvserver/spanset/spanset.go

pkg/kv/kvserver/spanset/spanset_test.go

pav-kv

The non-test parts LGTM, minor suggestions. Request to clean up the tests a bit, so that I can make a final (self-convincing) pass on them and LGTM tomorrow.

pav-kv · 2025-11-20T14:54:57Z

pkg/kv/kvserver/spanset/batch.go

+	// TODO(ibrahim): The fields spans, spansOnly, and ts don't seem to be used.
+	// Consider removing them and performing the necessary clean ups.


or making them used as intended

pkg/kv/kvserver/spanset/spanset.go

pkg/kv/kvserver/spanset/batch.go

pkg/kv/kvserver/replica_write.go

pkg/kv/kvserver/replica_test.go

Analogous to how we have a spanset helper contains() that understands the special span representation: [x-eps,x). This commit adds Overlaps that expects the same span representation. Moreover, this commit makes this special span representation explicit by introducing a new type called `TrickySpan`.

…d RangeID local keys This commit adds the following test-only assertions: 1) Generated batches don't touch store-local keys. 2) Generated batches don't touch unreplicated RangeID local keys. We disable the check in exactly 3 locations we know that we currently touch those keys.

pav-kv

LGTM, thanks for this change! Some last suggestions included, apply as many as makes sense.

pav-kv · 2025-11-21T00:02:16Z

pkg/kv/kvserver/spanset/spanset_test.go

+	var start roachpb.Key
+	var end roachpb.Key
+
+	if parts[0] != "X" {
+		start = roachpb.Key(parts[0])
+	}
+
+	if parts[1] != "X" {
+		end = roachpb.Key(parts[1])
+	}


style nit / opinion (feel free to ignore, just noticed the pattern in this PR, e.g. in overlapsUnreplicatedRangeIDLocalKeys too):

In simple code, vertical sparseness / empty lines is often not needed (makes "value" per screen lower). Empty lines is a tool to communicate/separate slightly more medium-size blocks of logically related code, offering some "structure" to the reader.

E.g. here I would recommend making it one block, because it's all about converting parts to span:

var span roachpb.Span if parts[0] != "X" { span.Key = roachpb.Key(parts[0]) } if parts[1] != "X" { span.EndKey = roachpb.Key(parts[1]) } return span

As an extension to this (more applies to other places: Contains/Overlaps and the two matcher funcs), comments are another "separator" which we can play with to show structure:

// Need to do this and that for some reasons. this() that() // And do something different, but still related to this block. still() related() // And here is a comment after a break. Which emphasizes the // 2-level structure. Above, there is a "block" with 2 sub-blocks. // Now, it's a new "higher-level" block. totally() different() thing()

pav-kv · 2025-11-21T00:44:18Z

pkg/kv/kvserver/replica_write.go

+	// are not completely within fullRangeIDLocalSpans, return an error as we
+	// collide with at least one unreplicated RangeIDLocal key.


Technically, it's possible that we don't collide. E.g. you can imagine a little span that begins before the RangeID-local space, and slightly moves into it without touching the first range's unreplicated.

We just make it a "policy" here that it shouldn't be possible, i.e. the spans are sensibly respecting the "tree" structure, and a span that begins outside the subtree and ends inside (or vice versa) doesn't make logical sense.

Dunno it there is a concise way to say this, see if you can compress it :)

pav-kv · 2025-11-21T00:55:57Z

pkg/kv/kvserver/replica_test.go

+		// Point spans not overlapping with Store-local span.
+		{span: s(roachpb.KeyMin, nil)},
+		{span: s(roachpb.Key(keys.LocalStorePrefix).Prevish(1), nil)},
+		{span: s(keys.LocalStoreMax.Next(), nil)},


Add LocalStoreMax here too, since it's not inclusive?

pav-kv · 2025-11-21T00:57:26Z

pkg/kv/kvserver/replica_test.go

+// TestOverlapsStoreLocalKeys verifies that the function
+// overlapsStoreLocalKeys() successfully catches any overlap with
+// store local keys.
+func TestOverlapsStoreLocalKeys(t *testing.T) {


Nice, thanks a lot for the clean up! Was able to review this test in a minute and spot an edge case to add.

pav-kv · 2025-11-21T00:59:00Z

pkg/kv/kvserver/replica_test.go

+			if tc.notOK {
+				require.Errorf(t, err, "expected error for span %s", tc.span)
+			} else {
+				require.NoErrorf(t, err, "expected no error for span %s", tc.span)
+			}


optional (2 patterns too):

require.Equal(t, tc.notOk, err != nil, tc.span)

Same above.

pav-kv · 2025-11-21T01:43:20Z

pkg/kv/kvserver/replica_test.go

+		// Tricky spans not overlapping with unreplicated local RangeID spans.
+		{span: s(nil, keys.LocalRangeIDPrefix.AsRawKey())},
+		{span: s(nil, keys.RangeForceFlushKey(1))},
+		{span: s(nil, keys.LocalRangeIDPrefix.AsRawKey().PrefixEnd().Next())},


Can add one without Next(), it should be legit too (IIUC it will take a different branch in the func).

pav-kv · 2025-11-21T01:45:08Z

pkg/kv/kvserver/replica_test.go

+		{span: s(nil, keys.RangeForceFlushKey(1))},
+		{span: s(nil, keys.LocalRangeIDPrefix.AsRawKey().PrefixEnd().Next())},
+
+		// Tricky spans overlapping with unreplicated local RangeID spans.


A couple of corner cases would be good: for the beginning and ending of the unreplicated span.

Applies to a few blocks here. Could be convenient to pre-compute a couple of vars for some unreplicated span and use it in a few places:

r1Unrepl := roachpb.Span{...} {span: s(r1Unrepl.Key.Prev(), nil)}, {span: s(r1Unrepl.EndKey, nil)}, {span: s(nil, r1Unrepl.EndKey), notOk: true}, // etc

pav-kv · 2025-11-21T02:31:27Z

pkg/kv/kvserver/spanset/spanset_test.go

+	}{
+		// s1 is a full span, and s2 is a tricky span with nil StartKey.
+		{s1: "b-d", s2: "X-a", contains: false, overlaps: false},
+		{s1: "b-d", s2: "X-b", contains: false, overlaps: false},


Add

{s1: "b-d", s2: "X-b\x00", contains: true, overlaps: true},

Maybe a few more corner cases with \x00 in other blocks below. Those are interesting cases, when we have a key and its Next(). Perhaps not in all groups, but in the point tricky spans for sure because there is a line checking IsPrev in the func.

pav-kv · 2025-11-21T11:18:30Z

pkg/kv/kvserver/spanset/batch.go

+// SpanSet wrapper with the forbidden span assertion disabled.
+// TODO(ibrahim): We eventually want to eliminate all the users of this
+// function.
+func DisableForbiddenSpanAssertionsOnBatch(rw storage.ReadWriter) storage.ReadWriter {


nit: the two "disable" methods could be named in a similar style e.g.

DisableUndeclaredSpanAssertions
DisableForbiddenSpanAssertions

or if you fancy brevity:

NoUndeclaredSpanAssertions
NoForbiddenSpanAssertions

pav-kv · 2025-11-21T11:28:03Z

pkg/kv/kvserver/spanset/spanset.go

 	s.allowUndeclared = true
 }
+
+// DisableForbiddenAssertions disables forbidden spans assertions.


Sync the comment with the name?

iskettaneh force-pushed the rse3 branch 2 times, most recently from 9f7f91b to ed7f5cd Compare November 11, 2025 02:33

iskettaneh requested a review from pav-kv November 11, 2025 02:33

iskettaneh marked this pull request as ready for review November 11, 2025 02:34

iskettaneh requested a review from a team as a code owner November 11, 2025 02:34

iskettaneh force-pushed the rse3 branch 2 times, most recently from 4d115e0 to 4fbc791 Compare November 11, 2025 20:47

github-actions bot added the o-AI-Review-Potential-Issue-Detected AI reviewer found potential issue. Never assign manually—auto-applied by GH action only. label Nov 11, 2025

iskettaneh force-pushed the rse3 branch from 4fbc791 to 16922d9 Compare November 12, 2025 00:40

iskettaneh force-pushed the rse3 branch 2 times, most recently from f9daad1 to cdb4edc Compare November 12, 2025 14:35

pav-kv reviewed Nov 18, 2025

View reviewed changes

iskettaneh force-pushed the rse3 branch 2 times, most recently from 6f5f0a9 to 8193ffc Compare November 19, 2025 00:08

iskettaneh requested a review from pav-kv November 19, 2025 00:08

iskettaneh commented Nov 19, 2025

View reviewed changes

pav-kv reviewed Nov 19, 2025

View reviewed changes

pkg/kv/kvserver/replica_write.go Outdated Show resolved Hide resolved

iskettaneh force-pushed the rse3 branch from 8193ffc to 7152b5c Compare November 19, 2025 19:58

iskettaneh commented Nov 19, 2025

View reviewed changes

iskettaneh requested a review from pav-kv November 19, 2025 20:06

pav-kv reviewed Nov 19, 2025

View reviewed changes

pkg/kv/kvserver/spanset/spanset_test.go Outdated Show resolved Hide resolved

kvserver/spanset: introduce forbidden spans matchers

d54cde3

This commit introduces the concept of forbidden spans, which will allow us to assert our batches don't modify Raft's engine keys.

iskettaneh force-pushed the rse3 branch from 7152b5c to 398f333 Compare November 19, 2025 22:43

iskettaneh requested a review from pav-kv November 20, 2025 14:04

iskettaneh commented Nov 20, 2025

View reviewed changes

pav-kv reviewed Nov 20, 2025

View reviewed changes

iskettaneh added 2 commits November 20, 2025 15:10

iskettaneh force-pushed the rse3 branch from 398f333 to c8e6a23 Compare November 20, 2025 21:18

pav-kv approved these changes Nov 21, 2025

View reviewed changes

		// TODO(ibrahim): The fields spans, spansOnly, and ts don't seem to be used.
		// Consider removing them and performing the necessary clean ups.

		// are not completely within fullRangeIDLocalSpans, return an error as we
		// collide with at least one unreplicated RangeIDLocal key.

spanset: assert that batches don't access store local and unreplicated RangeID local keys #157153

Are you sure you want to change the base?

spanset: assert that batches don't access store local and unreplicated RangeID local keys #157153

Conversation

iskettaneh commented Nov 11, 2025

Uh oh!

cockroach-teamcity commented Nov 11, 2025

Uh oh!

github-actions bot commented Nov 11, 2025

Potential Bug(s) Detected

Uh oh!

github-actions bot commented Nov 12, 2025

Potential Bug(s) Detected

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

iskettaneh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pav-kv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

iskettaneh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

iskettaneh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!