kvstorage: assert each engine only access its spans #160043

iskettaneh · 2025-12-22T22:34:04Z

The commit adds engine assertions to assert that Log engine only accesses log engine spans, and state engine only accesses state engine spans.

Fixes: #158281

Release notes: None

blathers-crl · 2025-12-22T22:34:07Z

Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

cockroach-teamcity · 2025-12-22T22:34:23Z

This change is

RaduBerinde

@RaduBerinde made 4 comments.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @iskettaneh, @jbowens, and @pav-kv).

pkg/storage/engine.go line 926 at r1 (raw file):

// EngineWithoutRW is the interface that wraps the engine functions.
type EngineWithoutRW interface {

[nit] I don't think this interface is very useful.. The wrapper can just not use the Reader/Writer functionality.

pkg/kv/kvserver/kvstorage/storage.go line 129 at r3 (raw file):

// engine reside in the same physical engine.
func MakeEngines(eng storage.Engine) Engines {
	if util.RaceEnabled {

Should this be buildutil.CrdbTestBuild? Race gets much more limited coverage (e.g. nothing in roachtests)

pkg/kv/kvserver/spanset/engine.go line 23 at r1 (raw file):

// do not belong to it.
type SpanSetEngine struct {
	storage.Reader

[nit] It would be more clear if we just embedded the ReadWriter (from this package) and initialized it with makeSpanSetReadWriterAt

pkg/kv/kvserver/spanset/engine.go line 39 at r1 (raw file):

	return &SpanSetEngine{
		e:      engine,
		Reader: NewReader(engine, spans, hlc.Timestamp{}),

[nit] It should be enough to create a reader-writer

pav-kv

@pav-kv made 1 comment.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @iskettaneh, @jbowens, and @RaduBerinde).

pkg/storage/engine.go line 926 at r1 (raw file):

The wrapper can just not use the Reader/Writer functionality.

In what sense? A lot of code passes around Engine as Reader/Writer. In those places where engines.LogEngine() or engines.StateEngine() is transparently converted to Reader/Writer, we would like the wrapped readers and writers.

iskettaneh

@iskettaneh made 2 comments and resolved 2 discussions.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jbowens, @pav-kv, and @RaduBerinde).

pkg/kv/kvserver/kvstorage/storage.go line 129 at r3 (raw file):

Previously, RaduBerinde wrote…

Should this be buildutil.CrdbTestBuild? Race gets much more limited coverage (e.g. nothing in roachtests)

I am following the same pattern that we do for Batch Eval assertions. But I get your point. I could leave a TODO to do that and make sure that it doesn't cause any test regressions due to the extra assertions. But I am not very worried because a lot of our "unit" tests use a full testCluster, which is a full CRDB cluster running locally.

pkg/storage/engine.go line 926 at r1 (raw file):

Previously, pav-kv (Pavel Kalinnikov) wrote…

The wrapper can just not use the Reader/Writer functionality.

In what sense? A lot of code passes around Engine as Reader/Writer. In those places where engines.LogEngine() or engines.StateEngine() is transparently converted to Reader/Writer, we would like the wrapped readers and writers.

Also, we already have SpansetReader and SpansetWriter (which has reader/writer assertions), and we didn't want to reimplement them here.

iskettaneh

@iskettaneh made 1 comment.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jbowens, @pav-kv, and @RaduBerinde).

pkg/storage/engine.go line 926 at r1 (raw file):

Previously, iskettaneh wrote…

Also, we already have SpansetReader and SpansetWriter (which has reader/writer assertions), and we didn't want to reimplement them here.

I see what happened! Initially when I was prototyping this change, I anonymous embedded Engine and ReadWriter fields. This caused a conflict. However, now that I only anonymous embedded the ReadWriter, there is no duplicate and I can keep the Engine interface unchanged. Thanks for pointing this out.

pav-kv

Looks good, dumping comments for now. Need to look more carefully at the second commit again.

pkg/kv/kvserver/spanset/engine.go

pkg/kv/kvserver/kvstorage/storage.go

pkg/kv/kvserver/spanset/batch.go

pav-kv · 2025-12-23T18:50:58Z

pkg/kv/kvserver/spanset/batch.go

 // wrapper with the latch assertion disabled.
 func DisableUndeclaredSpanAssertions(rw storage.ReadWriter) storage.ReadWriter {
 	switch v := rw.(type) {
+	case *storage.OpLoggerBatch:


Is this fix related to the PR, or a drive-by?

This is related to the PR. In the replica_write, we first get the Batch (now includes the forbidden assertions), then we wrap it by OpLoggerBatch, then we wrap it by the spanset batch (for undeclared assertions).

Oof. How do we know we haven't missed anything here? Do we need to assume we know all the possible wrappers?

I wonder if there is any other robust way to do this.

then we wrap it by the spanset batch (for undeclared assertions)

Are you saying we wrap into spanset types twice, with OpLoggerBatch in the middle? But then aren't we dropping the OpLoggerBatch from the chain when we ultimately return a fork of the bottom-most wrapper (or is it the topmost)?

Need a good comment somewhere.

A few comments about this:

How do we know if we haven't missed anything? Good question, but keep in mind that if we miss anything, we will "forget" to disable the assertion (not the opposite). So safety is still here. Worst thing that could happen is that a test fails, and we realize that we forgot some wrapper.

This is only temporary: Note that disabling forbidden spans is only temporary (we will eventually move all of the callers to their appropriate engine). Eventually, we will only have the disable undeclared spans calls. For these ones, we can drop the OpLoggerBatch because it will be wrapped by the undeclared span check last.

But then aren't we dropping the OpLoggerBatch from the chain? Good catch. I created a similar trick to what we have done before using a "shallowcopy"

So in summary I did the following:

I removed the OpLoggerBatch case from DisableUndeclaredSpanAssertions (similar to how it was before).

There is an existing TODO for DisableForbiddenSpanAssertions to fix all of its callers. After that, we can remove this function.

Fixed OpLoggerBatch being dropped when calling the DisableForbiddenSpanAssertions.

iskettaneh

@iskettaneh made 6 comments and resolved 1 discussion.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jbowens, @pav-kv, and @RaduBerinde).

pkg/kv/kvserver/kvstorage/storage.go

pkg/kv/kvserver/spanset/batch.go

iskettaneh · 2025-12-23T19:28:03Z

pkg/kv/kvserver/spanset/batch.go

 // wrapper with the latch assertion disabled.
 func DisableUndeclaredSpanAssertions(rw storage.ReadWriter) storage.ReadWriter {
 	switch v := rw.(type) {
+	case *storage.OpLoggerBatch:


This is related to the PR. In the replica_write, we first get the Batch (now includes the forbidden assertions), then we wrap it by OpLoggerBatch, then we wrap it by the spanset batch (for undeclared assertions).

pkg/kv/kvserver/spanset/engine.go

pkg/kv/kvserver/kvstorage/storage.go

pkg/kv/kvserver/kvstorage/destroy_test.go

pkg/kv/kvserver/spanset/batch.go

pav-kv · 2025-12-23T19:48:32Z

pkg/kv/kvserver/spanset/batch.go

 // wrapper with the latch assertion disabled.
 func DisableUndeclaredSpanAssertions(rw storage.ReadWriter) storage.ReadWriter {
 	switch v := rw.(type) {
+	case *storage.OpLoggerBatch:


Oof. How do we know we haven't missed anything here? Do we need to assume we know all the possible wrappers?

I wonder if there is any other robust way to do this.

then we wrap it by the spanset batch (for undeclared assertions)

Are you saying we wrap into spanset types twice, with OpLoggerBatch in the middle? But then aren't we dropping the OpLoggerBatch from the chain when we ultimately return a fork of the bottom-most wrapper (or is it the topmost)?

Need a good comment somewhere.

pav-kv · 2025-12-23T19:49:05Z

pkg/kv/kvserver/spanset/batch.go

+		result := DisableUndeclaredSpanAssertions(v.Batch)
+		return result


nit: variable not needed, return

same below

This commit adds SpansetEngine, which is a wrapper around Engine that will be used to assert that each engine only accesses its keys.

This commit changes the way we disable spanset assertionsby doing it recursively. This is because the SpansetBatch follows the decorator design pattern, and allows for multiple wrappers on top of each other. Note that the same recursive disabling was already happening in: DisableReadWriterAssertions and DisableReaderAssertions functions. Also, this commit adds DisableWriterAssertions so that we have the functions for reader, writer, and readwriter.

The commit adds engine assertions to assert that Log engine only accesses log engine spans, and state engine only accesses state engine spans.

iskettaneh

Thank you for the review!

@iskettaneh made 7 comments and resolved 2 discussions.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jbowens, @pav-kv, and @RaduBerinde).

pkg/kv/kvserver/kvstorage/storage.go

iskettaneh · 2025-12-23T20:56:31Z

pkg/kv/kvserver/spanset/batch.go

 // wrapper with the latch assertion disabled.
 func DisableUndeclaredSpanAssertions(rw storage.ReadWriter) storage.ReadWriter {
 	switch v := rw.(type) {
+	case *storage.OpLoggerBatch:


A few comments about this:

How do we know if we haven't missed anything? Good question, but keep in mind that if we miss anything, we will "forget" to disable the assertion (not the opposite). So safety is still here. Worst thing that could happen is that a test fails, and we realize that we forgot some wrapper.

This is only temporary: Note that disabling forbidden spans is only temporary (we will eventually move all of the callers to their appropriate engine). Eventually, we will only have the disable undeclared spans calls. For these ones, we can drop the OpLoggerBatch because it will be wrapped by the undeclared span check last.

But then aren't we dropping the OpLoggerBatch from the chain? Good catch. I created a similar trick to what we have done before using a "shallowcopy"

So in summary I did the following:

I removed the OpLoggerBatch case from DisableUndeclaredSpanAssertions (similar to how it was before).

There is an existing TODO for DisableForbiddenSpanAssertions to fix all of its callers. After that, we can remove this function.

Fixed OpLoggerBatch being dropped when calling the DisableForbiddenSpanAssertions.

pkg/kv/kvserver/spanset/engine.go

pkg/kv/kvserver/kvstorage/destroy_test.go

iskettaneh force-pushed the rse7 branch 3 times, most recently from 9c7f70c to 14b0dbd Compare December 23, 2025 15:20

iskettaneh changed the title ~~kvserver: add spanSetWriteBatch to spanset package~~ kvstorage: assert each engine only access its spans Dec 23, 2025

iskettaneh requested a review from pav-kv December 23, 2025 15:21

iskettaneh marked this pull request as ready for review December 23, 2025 15:22

iskettaneh requested review from a team as code owners December 23, 2025 15:22

iskettaneh requested a review from jbowens December 23, 2025 15:22

iskettaneh force-pushed the rse7 branch 2 times, most recently from 8a9c977 to 61ec9a6 Compare December 23, 2025 16:01

RaduBerinde reviewed Dec 23, 2025

View reviewed changes

pav-kv reviewed Dec 23, 2025

View reviewed changes

iskettaneh force-pushed the rse7 branch from 61ec9a6 to 5cf9cfd Compare December 23, 2025 18:09

iskettaneh commented Dec 23, 2025

View reviewed changes

iskettaneh requested review from RaduBerinde and pav-kv December 23, 2025 18:10

iskettaneh force-pushed the rse7 branch from 5cf9cfd to 5d4ad86 Compare December 23, 2025 18:23

iskettaneh commented Dec 23, 2025

View reviewed changes

pav-kv reviewed Dec 23, 2025

View reviewed changes

iskettaneh force-pushed the rse7 branch from 5d4ad86 to a168e34 Compare December 23, 2025 19:28

iskettaneh commented Dec 23, 2025

View reviewed changes

iskettaneh requested a review from pav-kv December 23, 2025 19:28

pav-kv reviewed Dec 23, 2025

View reviewed changes

iskettaneh added 3 commits December 23, 2025 15:29

kvserver: introduce SpansetEngine

7c59b65

This commit adds SpansetEngine, which is a wrapper around Engine that will be used to assert that each engine only accesses its keys.

kvstorage: assert each engine only access its spans

bf5aaec

The commit adds engine assertions to assert that Log engine only accesses log engine spans, and state engine only accesses state engine spans.

iskettaneh force-pushed the rse7 branch from a168e34 to bf5aaec Compare December 23, 2025 20:56

iskettaneh commented Dec 23, 2025

View reviewed changes

iskettaneh requested a review from pav-kv December 23, 2025 20:56

		result := DisableUndeclaredSpanAssertions(v.Batch)
		return result

kvstorage: assert each engine only access its spans #160043

Are you sure you want to change the base?

kvstorage: assert each engine only access its spans #160043

Uh oh!

Conversation

iskettaneh commented Dec 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

blathers-crl bot commented Dec 22, 2025

Uh oh!

cockroach-teamcity commented Dec 22, 2025

Uh oh!

RaduBerinde left a comment

Choose a reason for hiding this comment

Uh oh!

pav-kv left a comment

Choose a reason for hiding this comment

Uh oh!

iskettaneh left a comment

Choose a reason for hiding this comment

Uh oh!

iskettaneh left a comment

Choose a reason for hiding this comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iskettaneh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iskettaneh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

iskettaneh commented Dec 22, 2025 •

edited

Loading