diff --git a/CHANGELOG.md b/CHANGELOG.md index 638e37b488..1ee5f93a30 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). - Caveats: compiled caveats (and their CEL environments) are now cached per schema version — hung off the stored schema (`ReadOnlyStoredSchema`) and rebuilt only when the schema changes — rather than rebuilt on every check, reducing check cost for schemas with many caveats (https://github.com/authzed/spicedb/pull/3166) ### Fixed +- Bulk export and import streams are no longer cancelled by the streaming timeout. These methods legitimately go quiet between batches on large datasets, so the interceptor was terminating healthy streams (clients saw `RST_STREAM NO_ERROR`); they are now exempt while all other streaming methods keep the timeout. (https://github.com/authzed/spicedb/pull/3143) - 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) - Fixed a nil pointer dereference panic in the Postgres FDW (https://github.com/authzed/spicedb/pull/3235) - CockroachDB: deletes performed by CockroachDB's row-level TTL job for expired relationships are no longer emitted as `DELETE` events by the Watch API. On CockroachDB ≥ 24.1, SpiceDB sets the `ttl_disable_changefeed_replication` storage parameter on the relationship tables at startup (if it lacks `ALTER TABLE` privileges, it logs a warning with the statement to run manually); on older versions a startup warning is logged and TTL deletes continue to be emitted. Note that the parameter affects any changefeed over these tables — external changefeeds that want TTL deletes can opt back in with `ignore_disable_changefeed_replication`. Delete-only transactions also no longer write an internal transaction-metadata marker row, reducing write amplification. (https://github.com/authzed/spicedb/pull/3210) diff --git a/internal/middleware/streamtimeout/streamtimeout.go b/internal/middleware/streamtimeout/streamtimeout.go index 8f09fdb177..110aac104e 100644 --- a/internal/middleware/streamtimeout/streamtimeout.go +++ b/internal/middleware/streamtimeout/streamtimeout.go @@ -12,14 +12,44 @@ import ( "github.com/authzed/spicedb/pkg/spiceerrors" ) +// Option configures MustStreamServerInterceptor. +type Option func(*options) + +type options struct { + exemptMethods map[string]struct{} +} + +// WithExemptMethods exempts the given gRPC full method names (e.g. +// "/authzed.api.v1.PermissionsService/ExportBulkRelationships") from the +// streaming timeout. Use for methods where long server-side gaps between +// sends are expected by design — bulk export, bulk import — and where the +// interceptor's "client has hung up" purpose does not apply. +func WithExemptMethods(methods ...string) Option { + return func(o *options) { + if o.exemptMethods == nil { + o.exemptMethods = make(map[string]struct{}, len(methods)) + } + for _, m := range methods { + o.exemptMethods[m] = struct{}{} + } + } +} + // MustStreamServerInterceptor returns a new stream server interceptor that cancels the context // after a timeout if no new data has been received. -func MustStreamServerInterceptor(timeout time.Duration) grpc.StreamServerInterceptor { +func MustStreamServerInterceptor(timeout time.Duration, opts ...Option) grpc.StreamServerInterceptor { if timeout <= 0 { panic("timeout must be >= 0 for streaming timeout interceptor") } + o := options{} + for _, opt := range opts { + opt(&o) + } return func(srv any, stream grpc.ServerStream, info *grpc.StreamServerInfo, handler grpc.StreamHandler) error { + if _, exempt := o.exemptMethods[info.FullMethod]; exempt { + return handler(srv, stream) + } ctx := stream.Context() withCancel, internalCancelFn := context.WithCancelCause(ctx) timer := time.AfterFunc(timeout, func() { diff --git a/internal/middleware/streamtimeout/streamtimeout_test.go b/internal/middleware/streamtimeout/streamtimeout_test.go index 533180967c..0c625ef076 100644 --- a/internal/middleware/streamtimeout/streamtimeout_test.go +++ b/internal/middleware/streamtimeout/streamtimeout_test.go @@ -87,3 +87,56 @@ func (s *testSuite) TestStreamTimeout() { s.Require().LessOrEqual(maxCounter, int32(6), "stream was not properly canceled: %d", maxCounter) } } + +type exemptTestServer struct { + testpb.UnimplementedTestServiceServer +} + +func (t exemptTestServer) PingList(_ *testpb.PingListRequest, server testpb.TestService_PingListServer) error { + // Sleep well past the 50ms timeout before sending anything. If the + // interceptor exemption works, the handler receives the original stream + // whose context is not bound to the timer, so Context().Err() stays nil. + // If the exemption is broken, the interceptor wraps the stream with a + // cancelable context that fires at 50ms — Context().Err() then returns + // the DeadlineExceeded cause and this test fails (the smoking gun that + // distinguishes the fix from a no-op). + time.Sleep(150 * time.Millisecond) + if err := server.Context().Err(); err != nil { + return fmt.Errorf("expected uncanceled context on exempt method, got %w", err) + } + if err := server.Send(&testpb.PingListResponse{Counter: 1}); err != nil { + return err + } + return nil +} + +type exemptTestSuite struct { + *testpb.InterceptorTestSuite +} + +func TestStreamTimeoutExemptedMethods(t *testing.T) { + s := &exemptTestSuite{ + InterceptorTestSuite: &testpb.InterceptorTestSuite{ + TestService: &exemptTestServer{}, + ServerOpts: []grpc.ServerOption{ + grpc.StreamInterceptor(MustStreamServerInterceptor( + 50*time.Millisecond, + WithExemptMethods(testpb.TestService_PingList_FullMethodName), + )), + }, + }, + } + suite.Run(t, s) +} + +func (s *exemptTestSuite) TestExemptedMethodIsNotCanceled() { + stream, err := s.Client.PingList(s.SimpleCtx(), &testpb.PingListRequest{Value: "exempt"}) + s.Require().NoError(err) + + resp, err := stream.Recv() + s.Require().NoError(err, "exempt method must not be canceled by streamtimeout") + s.Require().Equal(int32(1), resp.Counter) + + _, err = stream.Recv() + s.Require().ErrorContains(err, "EOF") +} diff --git a/internal/services/v1/experimental.go b/internal/services/v1/experimental.go index bec4f995a4..bf22f99dde 100644 --- a/internal/services/v1/experimental.go +++ b/internal/services/v1/experimental.go @@ -107,7 +107,17 @@ func NewExperimentalServer(dispatch dispatch.Dispatcher, permServerConfig Permis grpcvalidate.StreamServerInterceptor(validator), handwrittenvalidation.StreamServerInterceptor, usagemetrics.StreamServerInterceptor(), - streamtimeout.MustStreamServerInterceptor(config.StreamReadTimeout), + streamtimeout.MustStreamServerInterceptor( + config.StreamReadTimeout, + // Bulk export/import are designed to run for arbitrarily long + // periods on large datasets; their inter-batch gaps regularly + // exceed the streaming-api timeout that catches hung clients + // on the bounded streaming reads. + streamtimeout.WithExemptMethods( + v1.ExperimentalService_BulkExportRelationships_FullMethodName, + v1.ExperimentalService_BulkImportRelationships_FullMethodName, + ), + ), perfinsights.StreamServerInterceptor(permServerConfig.PerformanceInsightMetricsEnabled), ), }, diff --git a/internal/services/v1/relationships.go b/internal/services/v1/relationships.go index 32a621bf39..9c33887384 100644 --- a/internal/services/v1/relationships.go +++ b/internal/services/v1/relationships.go @@ -185,7 +185,17 @@ func NewPermissionsServer( grpcvalidate.StreamServerInterceptor(validator), handwrittenvalidation.StreamServerInterceptor, usagemetrics.StreamServerInterceptor(), - streamtimeout.MustStreamServerInterceptor(configWithDefaults.StreamingAPITimeout), + streamtimeout.MustStreamServerInterceptor( + configWithDefaults.StreamingAPITimeout, + // Bulk export/import are designed to run for arbitrarily long + // periods on large datasets; their inter-batch gaps regularly + // exceed the streaming-api timeout that catches hung clients + // on the bounded streaming reads. + streamtimeout.WithExemptMethods( + v1.PermissionsService_ExportBulkRelationships_FullMethodName, + v1.PermissionsService_ImportBulkRelationships_FullMethodName, + ), + ), perfinsights.StreamServerInterceptor(configWithDefaults.PerformanceInsightMetricsEnabled), ), },