fix: guard nil debug request in bulk check (#3174)

ivanauth · web-flow · commit e053f978b43f · 2026-07-06T17:09:54.000Z
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -9,6 +9,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 - Updated the Prometheus buckets for `grpc_server_handling_seconds` and `spicedb_datastore_query_latency` to be able to correlate them (https://github.com/authzed/spicedb/pull/3188)
 
 ### Fixed
+- Fixed a nil pointer dereference panic in `CheckBulkPermissions` that could occur under concurrent load when a tracing-enabled check shared a singleflight dispatch with a non-tracing bulk check. Debug-enabled checks are no longer singleflighted together with non-debug checks. (https://github.com/authzed/spicedb/pull/3174)
 - When SpiceDB loses a connection to a CockroachDB node, every read happening in the server blocks for a short period of time (https://github.com/authzed/spicedb/pull/3181)
 - LSP: hover and go-to-definition now resolve identifiers on the right-hand side of arrow expressions (`->`, `.any(...)`, `.all(...)`) (https://github.com/authzed/spicedb/pull/3157)
 - The `in_cidr` caveat now matches IPv4-mapped IPv6 addresses (e.g. `::ffff:10.1.2.3`) against IPv4 CIDRs, the same as the dotted form (https://github.com/authzed/spicedb/pull/3184)
diff --git a/internal/dispatch/singleflight/singleflight.go b/internal/dispatch/singleflight/singleflight.go
@@ -80,6 +80,18 @@ func (d *Dispatcher) DispatchCheck(ctx context.Context, req *v1.DispatchCheckReq
 		return d.delegate.DispatchCheck(ctx, req)
 	}
 
+	// Debug-enabled checks must not share a flight with non-debug checks: the
+	// dispatch key does not depend on req.Debug, so an identical concurrent
+	// request with a different debug level would otherwise collapse into the
+	// same flight. A non-debug leader would deny a tracing follower its trace,
+	// and a debug leader would leak its trace to a non-debug follower (the
+	// latter has panicked CheckBulkPermissions, see #3159). Dispatch directly
+	// so the response carries exactly the debug info this caller asked for.
+	if req.Debug != v1.DispatchCheckRequest_NO_DEBUG {
+		singleFlightCount.WithLabelValues("DispatchCheck", "debug").Inc()
+		return d.delegate.DispatchCheck(ctx, req)
+	}
+
 	sharedResp, isShared, err := d.checkGroup.Do(ctx, keyString, func(innerCtx context.Context) (*v1.DispatchCheckResponse, error) {
 		return d.delegate.DispatchCheck(innerCtx, req)
 	})
diff --git a/internal/dispatch/singleflight/singleflight_test.go b/internal/dispatch/singleflight/singleflight_test.go
@@ -130,6 +130,82 @@ func TestSingleFlightDispatcherDetectsLoop(t *testing.T) {
 	assertCounterWithLabel(t, reg, 2, "spicedb_dispatch_single_flight_total", "loop")
 }
 
+// TestSingleFlightDispatcherDoesNotShareDebugChecks ensures debug-enabled checks
+// are dispatched directly rather than sharing a flight with otherwise-identical
+// non-debug checks. The dispatch key ignores req.Debug, so sharing would leak the
+// leader's debug info to a non-debug follower (the cause of #3159) or deny a debug
+// follower its trace.
+func TestSingleFlightDispatcherDoesNotShareDebugChecks(t *testing.T) {
+	singleFlightCount = prometheus.NewCounterVec(singleFlightCountConfig, []string{"method", "shared"})
+	reg := registerMetricInGatherer(singleFlightCount)
+
+	// The delegate returns debug info only when the request asked for it, so the
+	// response carried back to each caller reveals whether a flight was shared
+	// across debug levels.
+	delegate := &debugAwareDispatcher{delay: 100 * time.Millisecond}
+	disp := New(delegate, &keys.DirectKeyHandler{})
+
+	req := &v1.DispatchCheckRequest{
+		ResourceRelation: tuple.RR("document", "view").ToCoreRR(),
+		ResourceIds:      []string{"foo", "bar"},
+		Subject:          tuple.ONRStringToCore("user", "tom", "..."),
+		Metadata: &v1.ResolverMeta{
+			AtRevision:     "1234",
+			TraversalBloom: v1.MustNewTraversalBloomFilter(defaultBloomFilterSize),
+			SchemaHash:     []byte(datalayer.NoSchemaHashForTesting),
+		},
+	}
+
+	var mu sync.Mutex
+	var nonDebugResponses, debugResponses []*v1.DispatchCheckResponse
+
+	wg := sync.WaitGroup{}
+	wg.Add(4)
+	// Two non-debug checks should share a single flight.
+	for range 2 {
+		go func() {
+			resp, _ := disp.DispatchCheck(t.Context(), req.CloneVT())
+			mu.Lock()
+			nonDebugResponses = append(nonDebugResponses, resp)
+			mu.Unlock()
+			wg.Done()
+		}()
+	}
+	// Two debug-enabled checks, identical key, must each dispatch directly. Cover
+	// both debug levels: WithTracing maps to basic debugging, while trace debugging
+	// is the deeper variant; the bypass keys on != NO_DEBUG so both must apply.
+	for _, debug := range []v1.DispatchCheckRequest_DebugSetting{
+		v1.DispatchCheckRequest_ENABLE_BASIC_DEBUGGING,
+		v1.DispatchCheckRequest_ENABLE_TRACE_DEBUGGING,
+	} {
+		go func() {
+			debugReq := req.CloneVT()
+			debugReq.Debug = debug
+			resp, _ := disp.DispatchCheck(t.Context(), debugReq)
+			mu.Lock()
+			debugResponses = append(debugResponses, resp)
+			mu.Unlock()
+			wg.Done()
+		}()
+	}
+
+	wg.Wait()
+
+	// One dispatch for the shared non-debug pair, plus one per debug check.
+	require.Equal(t, uint64(3), delegate.called.Load(), "should have dispatched 3 calls but did %d", delegate.called.Load())
+	// Two label series: shared=true (the shared non-debug pair) and shared=debug.
+	assertCounterWithLabel(t, reg, 2, "spicedb_dispatch_single_flight_total", "debug")
+
+	// A non-debug caller must never receive debug info, even when racing an
+	// identical debug check; a debug caller must always receive its own.
+	for _, resp := range nonDebugResponses {
+		require.Nil(t, resp.GetMetadata().GetDebugInfo(), "non-debug caller received leaked debug info")
+	}
+	for _, resp := range debugResponses {
+		require.NotNil(t, resp.GetMetadata().GetDebugInfo(), "debug caller lost its trace")
+	}
+}
+
 // this test makes sure that bloom filter information is carried from dispatcher to dispatcher
 func TestSingleFlightDispatcherDetectsLoopThroughDelegate(t *testing.T) {
 	singleFlightCount = prometheus.NewCounterVec(singleFlightCountConfig, []string{"method", "shared"})
@@ -515,6 +591,26 @@ func bloomFilterForRequest(t *testing.T, keyHandler *keys.DirectKeyHandler, req
 	return binaryBloom
 }
 
+// debugAwareDispatcher returns debug info only when the request asked for it,
+// so callers can assert that a non-debug caller never receives debug info from a
+// flight shared with a debug caller. Only DispatchCheck is exercised.
+type debugAwareDispatcher struct {
+	dispatch.Dispatcher
+	delay  time.Duration
+	called atomic.Uint64
+}
+
+func (d *debugAwareDispatcher) DispatchCheck(_ context.Context, req *v1.DispatchCheckRequest) (*v1.DispatchCheckResponse, error) {
+	time.Sleep(d.delay)
+	d.called.Add(1)
+
+	resp := &v1.DispatchCheckResponse{Metadata: &v1.ResponseMeta{DispatchCount: 1}}
+	if req.Debug != v1.DispatchCheckRequest_NO_DEBUG {
+		resp.Metadata.DebugInfo = &v1.DebugInformation{Check: &v1.CheckDebugTrace{}}
+	}
+	return resp, nil
+}
+
 type mockDispatcher struct {
 	f func()
 }
diff --git a/internal/services/v1/bulkcheck.go b/internal/services/v1/bulkcheck.go
@@ -186,6 +186,9 @@ func (bc *bulkChecker) checkBulkPermissions(ctx context.Context, req *v1.CheckBu
 				// Find the debug info that matches the resource ID.
 				var debugInfo *dispatchv1.DebugInformation
 				for _, di := range debugInfos {
+					if di.Check == nil || di.Check.Request == nil {
+						continue
+					}
 					if slices.Contains(di.Check.Request.ResourceIds, resourceID) {
 						debugInfo = di
 						break
diff --git a/internal/services/v1/bulkcheck_test.go b/internal/services/v1/bulkcheck_test.go
@@ -0,0 +1,161 @@
+package v1
+
+import (
+	"context"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+
+	v1 "github.com/authzed/authzed-go/proto/authzed/api/v1"
+
+	"github.com/authzed/spicedb/internal/datastore/dsfortesting"
+	"github.com/authzed/spicedb/internal/datastore/memdb"
+	"github.com/authzed/spicedb/internal/dispatch"
+	"github.com/authzed/spicedb/internal/testfixtures"
+	caveattypes "github.com/authzed/spicedb/pkg/caveats/types"
+	"github.com/authzed/spicedb/pkg/datalayer"
+	"github.com/authzed/spicedb/pkg/middleware/consistency"
+	dispatchv1 "github.com/authzed/spicedb/pkg/proto/dispatch/v1"
+)
+
+// debugInfoDispatcher is a dispatcher that returns a successful check result whose
+// metadata carries debug information with a nil Check.Request. This mirrors the
+// shape produced by combineResponseMetadata for combined trace nodes, which can
+// reach a non-tracing CheckBulkPermissions caller when an identical tracing-enabled
+// request shares its dispatch via singleflight.
+type debugInfoDispatcher struct {
+	dispatch.Dispatcher
+}
+
+func (d debugInfoDispatcher) DispatchCheck(_ context.Context, req *dispatchv1.DispatchCheckRequest) (*dispatchv1.DispatchCheckResponse, error) {
+	return &dispatchv1.DispatchCheckResponse{
+		ResultsByResourceId: memberResults(req.ResourceIds),
+		Metadata: &dispatchv1.ResponseMeta{
+			DispatchCount: 1,
+			DebugInfo: &dispatchv1.DebugInformation{
+				Check: &dispatchv1.CheckDebugTrace{
+					SubProblems: []*dispatchv1.CheckDebugTrace{{IsCachedResult: true}},
+				},
+			},
+		},
+	}, nil
+}
+
+// matchingDebugInfoDispatcher returns a successful check result whose debug
+// information carries a Check.Request matching the dispatched resource IDs. This
+// exercises the normal tracing path where CheckBulkPermissions synthesizes a
+// debug trace for each resource.
+type matchingDebugInfoDispatcher struct {
+	dispatch.Dispatcher
+}
+
+func (d matchingDebugInfoDispatcher) DispatchCheck(_ context.Context, req *dispatchv1.DispatchCheckRequest) (*dispatchv1.DispatchCheckResponse, error) {
+	return &dispatchv1.DispatchCheckResponse{
+		ResultsByResourceId: memberResults(req.ResourceIds),
+		Metadata: &dispatchv1.ResponseMeta{
+			DispatchCount: 1,
+			DebugInfo: &dispatchv1.DebugInformation{
+				Check: &dispatchv1.CheckDebugTrace{
+					Request: &dispatchv1.DispatchCheckRequest{
+						ResourceRelation: req.ResourceRelation,
+						ResourceIds:      req.ResourceIds,
+						Subject:          req.Subject,
+						ResultsSetting:   req.ResultsSetting,
+						Debug:            req.Debug,
+					},
+					Results: memberResults(req.ResourceIds),
+				},
+			},
+		},
+	}, nil
+}
+
+func memberResults(resourceIDs []string) map[string]*dispatchv1.ResourceCheckResult {
+	results := make(map[string]*dispatchv1.ResourceCheckResult, len(resourceIDs))
+	for _, resourceID := range resourceIDs {
+		results[resourceID] = &dispatchv1.ResourceCheckResult{
+			Membership: dispatchv1.ResourceCheckResult_MEMBER,
+		}
+	}
+	return results
+}
+
+func runBulkCheck(t *testing.T, d dispatch.Dispatcher, withTracing bool) *v1.CheckBulkPermissionsResponse {
+	t.Helper()
+	require := require.New(t)
+
+	uninitialized, err := dsfortesting.NewMemDBDatastoreForTesting(t, 0, 0, memdb.DisableGC)
+	require.NoError(err)
+
+	ds, _ := testfixtures.StandardDatastoreWithData(t, uninitialized)
+	dl := datalayer.NewDataLayer(ds)
+
+	ctx := datalayer.ContextWithDataLayer(consistency.ContextWithHandle(t.Context()), dl)
+
+	req := &v1.CheckBulkPermissionsRequest{
+		Items: []*v1.CheckBulkPermissionsRequestItem{
+			{
+				Resource: &v1.ObjectReference{
+					ObjectType: "document",
+					ObjectId:   "masterplan",
+				},
+				Permission: "view",
+				Subject: &v1.SubjectReference{
+					Object: &v1.ObjectReference{
+						ObjectType: "user",
+						ObjectId:   "eng_lead",
+					},
+				},
+			},
+		},
+		WithTracing: withTracing,
+	}
+
+	require.NoError(consistency.AddRevisionToContext(ctx, req, dl, "", consistency.TreatMismatchingTokensAsError))
+
+	bc := &bulkChecker{
+		maxAPIDepth:          50,
+		maxCaveatContextSize: 4096,
+		maxConcurrency:       1,
+		caveatTypeSet:        caveattypes.Default.TypeSet,
+		dispatch:             d,
+		dispatchChunkSize:    100,
+	}
+
+	resp, err := bc.checkBulkPermissions(ctx, req)
+	require.NoError(err)
+	return resp
+}
+
+// TestCheckBulkPermissionsWithNilDebugRequest ensures that a debug information entry
+// with a nil Check.Request does not panic CheckBulkPermissions when tracing was not
+// requested. See https://github.com/authzed/spicedb/issues/3159.
+func TestCheckBulkPermissionsWithNilDebugRequest(t *testing.T) {
+	require := require.New(t)
+
+	resp := runBulkCheck(t, debugInfoDispatcher{}, false)
+	require.Len(resp.Pairs, 1)
+
+	item := resp.Pairs[0].GetItem()
+	require.NotNil(item)
+	require.Equal(v1.CheckPermissionResponse_PERMISSIONSHIP_HAS_PERMISSION, item.Permissionship)
+	require.Nil(item.DebugTrace)
+}
+
+// TestCheckBulkPermissionsWithMatchingDebugRequest ensures that a debug information
+// entry whose Check.Request matches the resource is surfaced as a debug trace on the
+// result, exercising the positive branch of the resource-ID matching loop.
+func TestCheckBulkPermissionsWithMatchingDebugRequest(t *testing.T) {
+	require := require.New(t)
+
+	resp := runBulkCheck(t, matchingDebugInfoDispatcher{}, true)
+	require.Len(resp.Pairs, 1)
+
+	item := resp.Pairs[0].GetItem()
+	require.NotNil(item)
+	require.Equal(v1.CheckPermissionResponse_PERMISSIONSHIP_HAS_PERMISSION, item.Permissionship)
+	require.NotNil(item.DebugTrace)
+	require.NotNil(item.DebugTrace.Check)
+	require.Equal("masterplan", item.DebugTrace.Check.Resource.GetObjectId())
+	require.Equal("view", item.DebugTrace.Check.Permission)
+}
