Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
+528 −108
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d54cde3
kvserver/spanset: introduce forbidden spans matchers
iskettaneh Nov 11, 2025
ad834d9
spanset: Add Overlaps() helper function
iskettaneh Nov 11, 2025
c8e6a23
spanset: assert that batches don't access store local and unreplicate…
iskettaneh Nov 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions pkg/keys/constants.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,7 @@ var (
//
// LocalStorePrefix is the prefix identifying per-store data.
LocalStorePrefix = makeKey(LocalPrefix, roachpb.Key("s"))
LocalStoreMax = roachpb.Key(LocalStorePrefix).PrefixEnd()
// localStoreClusterVersionSuffix stores the cluster-wide version
// information for this store, updated any time the operator
// updates the minimum cluster version.
Expand Down
15 changes: 15 additions & 0 deletions pkg/keys/keys.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,21 @@ func DecodeRangeIDKey(
return roachpb.RangeID(rangeInt), infix, suffix, b, nil
}

// DecodeRangeIDPrefix parses a local range ID prefix into range ID.
func DecodeRangeIDPrefix(key roachpb.Key) (roachpb.RangeID, error) {
if !bytes.HasPrefix(key, LocalRangeIDPrefix) {
return 0, errors.Errorf("key %s does not have %s prefix", key, LocalRangeIDPrefix)
}
// Cut the prefix, the Range ID, and the infix specifier.
b := key[len(LocalRangeIDPrefix):]
_, rangeInt, err := encoding.DecodeUvarintAscending(b)
if err != nil {
return 0, err
}

return roachpb.RangeID(rangeInt), nil
Comment on lines +319 to +323
Copy link
Collaborator

@pav-kv pav-kv Nov 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can return without checking:

return roachpb.RangeID(rangeInt), err

Pretty sure DecodeUvarint already returns zero on error (it's conventional, so we can do it transitively), but even if not, no big deal.

}

// AbortSpanKey returns a range-local key by Range ID for an
// AbortSpan entry, with detail specified by encoding the
// supplied transaction ID.
Expand Down
2 changes: 1 addition & 1 deletion pkg/kv/kvserver/batcheval/cmd_add_sstable.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,7 +337,7 @@ func EvalAddSSTable(
// addition, and instead just use this key-only iterator. If a caller actually
// needs to know what data is there, it must issue its own real Scan.
if args.ReturnFollowingLikelyNonEmptySpanStart {
existingIter, err := spanset.DisableReaderAssertions(readWriter).NewMVCCIterator(
existingIter, err := spanset.DisableLatchAssertions(readWriter).NewMVCCIterator(
ctx,
storage.MVCCKeyIterKind, // don't care if it is committed or not, just that it isn't empty.
storage.IterOptions{
Expand Down
2 changes: 1 addition & 1 deletion pkg/kv/kvserver/batcheval/cmd_delete.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ func Delete(
// If requested, replace point tombstones with range tombstones.
if cArgs.EvalCtx.EvalKnobs().UseRangeTombstonesForPointDeletes && h.Txn == nil {
if err := storage.ReplacePointTombstonesWithRangeTombstones(
ctx, spanset.DisableReadWriterAssertions(readWriter),
ctx, spanset.DisableLatchAssertions(readWriter),
cArgs.Stats, args.Key, args.EndKey); err != nil {
return result.Result{}, err
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/kv/kvserver/batcheval/cmd_delete_range.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,7 +302,7 @@ func deleteRangeTransactional(
// If requested, replace point tombstones with range tombstones.
if cArgs.EvalCtx.EvalKnobs().UseRangeTombstonesForPointDeletes && h.Txn == nil {
if err := storage.ReplacePointTombstonesWithRangeTombstones(
ctx, spanset.DisableReadWriterAssertions(readWriter),
ctx, spanset.DisableLatchAssertions(readWriter),
cArgs.Stats, args.Key, args.EndKey); err != nil {
return result.Result{}, err
}
Expand Down
11 changes: 7 additions & 4 deletions pkg/kv/kvserver/batcheval/cmd_end_transaction.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -719,7 +719,7 @@ func resolveLocalLocksWithPagination(
// If requested, replace point tombstones with range tombstones.
if ok && evalCtx.EvalKnobs().UseRangeTombstonesForPointDeletes {
if err := storage.ReplacePointTombstonesWithRangeTombstones(
ctx, spanset.DisableReadWriterAssertions(readWriter),
ctx, spanset.DisableLatchAssertions(readWriter),
ms, update.Key, update.EndKey); err != nil {
return 0, 0, 0, errors.Wrapf(err,
"replacing point tombstones with range tombstones for write intent at %s on end transaction [%s]",
Expand Down Expand Up @@ -757,7 +757,7 @@ func resolveLocalLocksWithPagination(
// If requested, replace point tombstones with range tombstones.
if evalCtx.EvalKnobs().UseRangeTombstonesForPointDeletes {
if err := storage.ReplacePointTombstonesWithRangeTombstones(
ctx, spanset.DisableReadWriterAssertions(readWriter),
ctx, spanset.DisableLatchAssertions(readWriter),
ms, update.Key, update.EndKey); err != nil {
return 0, 0, 0, errors.Wrapf(err,
"replacing point tombstones with range tombstones for write intent range at %s on end transaction [%s]",
Expand Down Expand Up @@ -1340,8 +1340,10 @@ func splitTriggerHelper(
if err != nil {
return enginepb.MVCCStats{}, result.Result{}, errors.Wrap(err, "unable to fetch last replica GC timestamp")
}

if err := storage.MVCCPutProto(
ctx, batch, keys.RangeLastReplicaGCTimestampKey(split.RightDesc.RangeID), hlc.Timestamp{},
ctx, spanset.DisableForbiddenSpanAssertionsOnBatch(batch),
keys.RangeLastReplicaGCTimestampKey(split.RightDesc.RangeID), hlc.Timestamp{},
&replicaGCTS, storage.MVCCWriteOptions{Category: fs.BatchEvalReadCategory}); err != nil {
return enginepb.MVCCStats{}, result.Result{}, errors.Wrap(err, "unable to copy last replica GC timestamp")
}
Expand Down Expand Up @@ -1541,7 +1543,8 @@ func splitTriggerHelper(
// as all replicas will be responsible for writing it locally before
// applying the split.
if !rec.ClusterSettings().Version.IsActive(ctx, clusterversion.V25_4_WriteInitialTruncStateBeforeSplitApplication) {
if err := kvstorage.WriteInitialTruncState(ctx, batch, split.RightDesc.RangeID); err != nil {
if err := kvstorage.WriteInitialTruncState(ctx,
spanset.DisableForbiddenSpanAssertionsOnBatch(batch), split.RightDesc.RangeID); err != nil {
return enginepb.MVCCStats{}, result.Result{}, errors.Wrap(err, "unable to write initial Replica state")
}
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/kv/kvserver/batcheval/cmd_resolve_intent.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ func ResolveIntent(
// If requested, replace point tombstones with range tombstones.
if ok && cArgs.EvalCtx.EvalKnobs().UseRangeTombstonesForPointDeletes {
if err := storage.ReplacePointTombstonesWithRangeTombstones(ctx,
spanset.DisableReadWriterAssertions(readWriter), ms, update.Key, update.EndKey); err != nil {
spanset.DisableLatchAssertions(readWriter), ms, update.Key, update.EndKey); err != nil {
return result.Result{}, err
}
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/kv/kvserver/batcheval/cmd_resolve_intent_range.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ func ResolveIntentRange(
// If requested, replace point tombstones with range tombstones.
if cArgs.EvalCtx.EvalKnobs().UseRangeTombstonesForPointDeletes {
if err := storage.ReplacePointTombstonesWithRangeTombstones(ctx,
spanset.DisableReadWriterAssertions(readWriter), ms, args.Key, args.EndKey); err != nil {
spanset.DisableLatchAssertions(readWriter), ms, args.Key, args.EndKey); err != nil {
return result.Result{}, err
}
}
Expand Down
4 changes: 3 additions & 1 deletion pkg/kv/kvserver/batcheval/cmd_truncate_log.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,9 @@ func TruncateLog(
// are not tracked in the raft log delta. The delta will be adjusted below
// raft.
// We can pass zero as nowNanos because we're only interested in SysBytes.
ms, err := storage.ComputeStats(ctx, readWriter, start, end, 0 /* nowNanos */)
// TODO(#157895): Use the log engine here instead of the state machine engine.
Copy link
Collaborator

@pav-kv pav-kv Nov 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likely can unTODO this, since this PR is merged.

ms, err := storage.ComputeStats(ctx,
spanset.DisableForbiddenSpanAssertionsOnBatch(readWriter), start, end, 0 /* nowNanos */)
if err != nil {
return result.Result{}, errors.Wrap(err, "while computing stats of Raft log freed by truncation")
}
Expand Down
14 changes: 7 additions & 7 deletions pkg/kv/kvserver/replica_eval_context_span.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ func (rec *SpanSetReplicaEvalContext) IsFirstRange() bool {
func (rec SpanSetReplicaEvalContext) Desc() *roachpb.RangeDescriptor {
desc := rec.i.Desc()
rec.ss.AssertAllowed(spanset.SpanReadOnly,
roachpb.Span{Key: keys.RangeDescriptorKey(desc.StartKey)},
spanset.TrickySpan{Key: keys.RangeDescriptorKey(desc.StartKey)},
)
return desc
}
Expand Down Expand Up @@ -145,7 +145,7 @@ func (rec SpanSetReplicaEvalContext) CanCreateTxnRecord(
ctx context.Context, txnID uuid.UUID, txnKey []byte, txnMinTS hlc.Timestamp,
) (bool, kvpb.TransactionAbortedReason) {
rec.ss.AssertAllowed(spanset.SpanReadOnly,
roachpb.Span{Key: keys.TransactionKey(txnKey, txnID)},
spanset.TrickySpan{Key: keys.TransactionKey(txnKey, txnID)},
)
return rec.i.CanCreateTxnRecord(ctx, txnID, txnKey, txnMinTS)
}
Expand All @@ -157,7 +157,7 @@ func (rec SpanSetReplicaEvalContext) MinTxnCommitTS(
ctx context.Context, txnID uuid.UUID, txnKey []byte,
) hlc.Timestamp {
rec.ss.AssertAllowed(spanset.SpanReadOnly,
roachpb.Span{Key: keys.TransactionKey(txnKey, txnID)},
spanset.TrickySpan{Key: keys.TransactionKey(txnKey, txnID)},
)
return rec.i.MinTxnCommitTS(ctx, txnID, txnKey)
}
Expand All @@ -167,7 +167,7 @@ func (rec SpanSetReplicaEvalContext) MinTxnCommitTS(
// not be served.
func (rec SpanSetReplicaEvalContext) GetGCThreshold() hlc.Timestamp {
rec.ss.AssertAllowed(spanset.SpanReadOnly,
roachpb.Span{Key: keys.RangeGCThresholdKey(rec.GetRangeID())},
spanset.TrickySpan{Key: keys.RangeGCThresholdKey(rec.GetRangeID())},
)
return rec.i.GetGCThreshold()
}
Expand All @@ -191,7 +191,7 @@ func (rec SpanSetReplicaEvalContext) GetLastReplicaGCTimestamp(
ctx context.Context,
) (hlc.Timestamp, error) {
if err := rec.ss.CheckAllowed(spanset.SpanReadOnly,
roachpb.Span{Key: keys.RangeLastReplicaGCTimestampKey(rec.GetRangeID())},
spanset.TrickySpan{Key: keys.RangeLastReplicaGCTimestampKey(rec.GetRangeID())},
); err != nil {
return hlc.Timestamp{}, err
}
Expand Down Expand Up @@ -222,11 +222,11 @@ func (rec *SpanSetReplicaEvalContext) GetCurrentReadSummary(ctx context.Context)
// To capture a read summary over the range, all keys must be latched for
// writing to prevent any concurrent reads or writes.
desc := rec.i.Desc()
rec.ss.AssertAllowed(spanset.SpanReadWrite, roachpb.Span{
rec.ss.AssertAllowed(spanset.SpanReadWrite, spanset.TrickySpan{
Key: keys.MakeRangeKeyPrefix(desc.StartKey),
EndKey: keys.MakeRangeKeyPrefix(desc.EndKey),
})
rec.ss.AssertAllowed(spanset.SpanReadWrite, roachpb.Span{
rec.ss.AssertAllowed(spanset.SpanReadWrite, spanset.TrickySpan{
Key: desc.StartKey.AsRawKey(),
EndKey: desc.EndKey.AsRawKey(),
})
Expand Down
122 changes: 121 additions & 1 deletion pkg/kv/kvserver/replica_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2732,7 +2732,7 @@ func TestReplicaLatchingSplitDeclaresWrites(t *testing.T) {
{spanset.SpanReadWrite, roachpb.Key("b"), false},
{spanset.SpanReadWrite, roachpb.Key("d"), true},
} {
err := spans.CheckAllowed(tc.access, roachpb.Span{Key: tc.key})
err := spans.CheckAllowed(tc.access, spanset.TrickySpan{Key: tc.key})
if tc.expectAccess {
require.NoError(t, err)
} else {
Expand Down Expand Up @@ -15197,3 +15197,123 @@ func TestLeaderlessWatcherInit(t *testing.T) {
t.Fatalf("expected LeaderlessWatcher channel to be closed")
}
}

// TestOverlapsUnreplicatedRangeIDLocalKeys verifies that the function
// overlapsUnreplicatedRangeIDLocalKeys() successfully catches any overlap with
// unreplicated rangeID local keys.
func TestOverlapsUnreplicatedRangeIDLocalKeys(t *testing.T) {
defer leaktest.AfterTest(t)()
defer log.Scope(t).Close(t)

s := func(start, end roachpb.Key) roachpb.Span {
return roachpb.Span{Key: start, EndKey: end}
}

testCases := []struct {
span roachpb.Span
notOk bool
}{
// Full spans not overlapping with unreplicated local RangeID spans.
{span: s(roachpb.KeyMin, keys.LocalRangeIDPrefix.AsRawKey())},
{span: s(keys.RangeForceFlushKey(1), keys.RangeLeaseKey(1))},
Copy link
Collaborator

@pav-kv pav-kv Nov 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can add a "typical" one here: a full replicated span (which includes both ForceFlush and RangeLease keys).

{span: s(keys.LocalRangeIDPrefix.AsRawKey().PrefixEnd(), roachpb.KeyMax)},

// Full spans overlapping with unreplicated local RangeID spans.
{span: s(roachpb.KeyMin, keys.RaftTruncatedStateKey(1)), notOk: true},
{span: s(keys.LocalRangeIDPrefix.AsRawKey(), keys.LocalRangeIDPrefix.AsRawKey().PrefixEnd()),
notOk: true},
{span: s(keys.RaftTruncatedStateKey(1), keys.RaftTruncatedStateKey(2)), notOk: true},
{span: s(keys.RaftTruncatedStateKey(1), roachpb.KeyMax), notOk: true},

// Point spans not overlapping with unreplicated local RangeID spans.
{span: s(roachpb.KeyMin, nil)},
{span: s(keys.LocalRangeIDPrefix.AsRawKey().Prevish(1), nil)},
{span: s(keys.RangeForceFlushKey(1), nil)},
{span: s(keys.LocalRangeIDPrefix.AsRawKey().PrefixEnd(), nil)},
{span: s(roachpb.KeyMax, nil)},

// Point spans overlapping with unreplicated local RangeID spans.
{span: s(keys.RangeTombstoneKey(1), nil), notOk: true},
{span: s(keys.RaftTruncatedStateKey(1), nil), notOk: true},
{span: s(keys.RaftTruncatedStateKey(2), nil), notOk: true},

// 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())},
Copy link
Collaborator

@pav-kv pav-kv Nov 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// Tricky spans overlapping with unreplicated local RangeID spans.
Copy link
Collaborator

@pav-kv pav-kv Nov 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{span: s(nil, keys.RangeTombstoneKey(1).Next()), notOk: true},
{span: s(nil, keys.RaftTruncatedStateKey(1).Next()), notOk: true},
{span: s(nil, keys.RaftTruncatedStateKey(2).Next()), notOk: true},
}

for _, tc := range testCases {
t.Run("", func(t *testing.T) {
err := overlapsUnreplicatedRangeIDLocalKeys(spanset.TrickySpan(tc.span))
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)
}
})
}
}

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

@pav-kv pav-kv Nov 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

defer leaktest.AfterTest(t)()
defer log.Scope(t).Close(t)

s := func(start, end roachpb.Key) roachpb.Span {
return roachpb.Span{Key: start, EndKey: end}
}

testCases := []struct {
span roachpb.Span
notOK bool
}{
// Full spans not overlapping with Store-local span.
{span: s(roachpb.KeyMin, keys.LocalStorePrefix)},
{span: s(keys.LocalStoreMax, roachpb.KeyMax)},

// Full spans overlapping with Store-local span.
{span: s(roachpb.KeyMin, roachpb.Key(keys.LocalStorePrefix).Next()), notOK: true},
{span: s(keys.LocalStorePrefix, keys.LocalStoreMax), notOK: true},
{span: s(keys.StoreGossipKey(), keys.StoreIdentKey()), notOK: true},
{span: s(keys.LocalStoreMax.Prevish(1), roachpb.KeyMax), notOK: true},

// 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)},
Copy link
Collaborator

@pav-kv pav-kv Nov 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{span: s(roachpb.KeyMax, nil)},

// Point spans overlapping with Store-local span.
{span: s(keys.LocalStorePrefix, nil), notOK: true},
{span: s(keys.StoreGossipKey(), nil), notOK: true},
{span: s(keys.LocalStoreMax.Prevish(1), nil), notOK: true},

// Tricky spans with nil StartKey not overlapping with Store-local span.
{span: s(nil, keys.LocalStorePrefix)},
{span: s(nil, keys.LocalStoreMax.Next())},

// Tricky spans with nil StartKey overlapping with Store-local span.
{span: s(nil, roachpb.Key(keys.LocalStorePrefix).Next()), notOK: true},
{span: s(nil, keys.StoreGossipKey()), notOK: true},
{span: s(nil, keys.LocalStoreMax), notOK: true},
}

for _, tc := range testCases {
t.Run("", func(t *testing.T) {
err := overlapsStoreLocalKeys(spanset.TrickySpan(tc.span))
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)
}
Comment on lines +15312 to +15316
Copy link
Collaborator

@pav-kv pav-kv Nov 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

optional (2 patterns too):

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

Same above.

})
}
}
Loading
Loading