diff --git a/server_interceptors.go b/server_interceptors.go index 03ecfa9..dead495 100644 --- a/server_interceptors.go +++ b/server_interceptors.go @@ -16,7 +16,15 @@ import ( "google.golang.org/grpc" ) -func recoverWithSentry(hub *sentry.Hub, ctx context.Context, o *options) { +// recoverWithSentry recovers from a panic in the handler, reports it to Sentry +// and, unless Repanic is enabled, translates it into a codes.Internal error via +// outErr (and marks the transaction as failed). +// +// Without this translation the interceptor would fall through to its zero-value +// return — a nil error — making gRPC treat the RPC as successful: a streaming +// handler reports OK with a missing/partial response, and a unary handler +// returns a nil response that fails to marshal with an opaque Internal error. +func recoverWithSentry(hub *sentry.Hub, ctx context.Context, o *options, tx *sentry.Span, outErr *error) { if err := recover(); err != nil { eventID := hub.RecoverWithContext(ctx, err) if eventID != nil && o.WaitForDelivery { @@ -26,6 +34,13 @@ func recoverWithSentry(hub *sentry.Hub, ctx context.Context, o *options) { if o.Repanic { panic(err) } + + if outErr != nil { + *outErr = status.Errorf(codes.Internal, "%v", err) + } + if tx != nil { + tx.Status = sentry.SpanStatusInternalError + } } } @@ -34,7 +49,7 @@ func UnaryServerInterceptor(opts ...Option) grpc.UnaryServerInterceptor { return func(ctx context.Context, req interface{}, info *grpc.UnaryServerInfo, - handler grpc.UnaryHandler) (interface{}, error) { + handler grpc.UnaryHandler) (resp interface{}, err error) { hub := sentry.GetHubFromContext(ctx) if hub == nil { @@ -67,9 +82,9 @@ func UnaryServerInterceptor(opts ...Option) grpc.UnaryServerInterceptor { // TODO: Perhaps makes sense to use SetRequestBody instead? hub.Scope().SetExtra("requestBody", req) } - defer recoverWithSentry(hub, ctx, o) + defer recoverWithSentry(hub, ctx, o, tx, &err) - resp, err := handler(ctx, req) + resp, err = handler(ctx, req) if err != nil && o.ReportOn(err) { tags := grpc_tags.Extract(ctx) for k, v := range tags.Values() { @@ -92,7 +107,7 @@ func StreamServerInterceptor(opts ...Option) grpc.StreamServerInterceptor { return func(srv interface{}, ss grpc.ServerStream, info *grpc.StreamServerInfo, - handler grpc.StreamHandler) error { + handler grpc.StreamHandler) (err error) { ctx := ss.Context() hub := sentry.GetHubFromContext(ctx) @@ -125,9 +140,9 @@ func StreamServerInterceptor(opts ...Option) grpc.StreamServerInterceptor { stream := grpc_middleware.WrapServerStream(ss) stream.WrappedContext = ctx - defer recoverWithSentry(hub, ctx, o) + defer recoverWithSentry(hub, ctx, o, tx, &err) - err := handler(srv, stream) + err = handler(srv, stream) if err != nil && o.ReportOn(err) { tags := grpc_tags.Extract(ctx) for k, v := range tags.Values() { diff --git a/server_interceptors_test.go b/server_interceptors_test.go index 674068e..38b0efd 100644 --- a/server_interceptors_test.go +++ b/server_interceptors_test.go @@ -10,6 +10,7 @@ import ( "google.golang.org/grpc" "google.golang.org/grpc/codes" "google.golang.org/grpc/metadata" + "google.golang.org/grpc/status" ) // mockUnaryHandler is a mock handler for testing unary interceptors @@ -44,12 +45,12 @@ type mockServerStream struct { ctx context.Context } -func (m *mockServerStream) SetHeader(metadata.MD) error { return nil } -func (m *mockServerStream) SendHeader(metadata.MD) error { return nil } -func (m *mockServerStream) SetTrailer(metadata.MD) {} -func (m *mockServerStream) Context() context.Context { return m.ctx } -func (m *mockServerStream) SendMsg(interface{}) error { return nil } -func (m *mockServerStream) RecvMsg(interface{}) error { return nil } +func (m *mockServerStream) SetHeader(metadata.MD) error { return nil } +func (m *mockServerStream) SendHeader(metadata.MD) error { return nil } +func (m *mockServerStream) SetTrailer(metadata.MD) {} +func (m *mockServerStream) Context() context.Context { return m.ctx } +func (m *mockServerStream) SendMsg(interface{}) error { return nil } +func (m *mockServerStream) RecvMsg(interface{}) error { return nil } func TestUnaryServerInterceptor_Configuration(t *testing.T) { // Test that the interceptor can be created with different options @@ -162,6 +163,105 @@ func TestStreamServerInterceptor_Configuration(t *testing.T) { } } +// tracedIncomingContext returns a server-side context carrying a valid +// sentry-trace header, mirroring a real request whose caller propagates the +// trace. Without a trace header, StartTransaction is invoked with a nil +// SpanOption from ContinueFromGrpcMetadata, which is an unrelated concern. +func tracedIncomingContext() context.Context { + md := metadata.New(map[string]string{ + sentry.SentryTraceHeader: "1234567890abcdef1234567890abcdef-1234567890abcdef-1", + }) + return metadata.NewIncomingContext(context.Background(), md) +} + +func TestUnaryServerInterceptor_RecoversPanicAsInternal(t *testing.T) { + // With the default options (Repanic disabled), a panicking handler must be + // recovered and returned as a codes.Internal error rather than a nil error. + interceptor := UnaryServerInterceptor() + handler := &mockUnaryHandler{panic: true} + info := &grpc.UnaryServerInfo{FullMethod: "/test.Service/Method"} + + resp, err := interceptor(tracedIncomingContext(), "request", info, handler.handle) + + if resp != nil { + t.Errorf("Expected nil response on panic, got %v", resp) + } + if got := status.Code(err); got != codes.Internal { + t.Fatalf("Expected codes.Internal, got %v (err=%v)", got, err) + } +} + +func TestUnaryServerInterceptor_RepanicsWhenEnabled(t *testing.T) { + // With Repanic enabled the panic must propagate to the caller unchanged. + interceptor := UnaryServerInterceptor(WithRepanicOption(true)) + handler := &mockUnaryHandler{panic: true} + info := &grpc.UnaryServerInfo{FullMethod: "/test.Service/Method"} + + defer func() { + if r := recover(); r == nil { + t.Error("Expected interceptor to re-panic, but it did not") + } + }() + _, _ = interceptor(tracedIncomingContext(), "request", info, handler.handle) +} + +func TestUnaryServerInterceptor_PassesThroughError(t *testing.T) { + // A normal handler error must be returned unchanged. + interceptor := UnaryServerInterceptor() + handler := &mockUnaryHandler{err: status.Error(codes.NotFound, "missing")} + info := &grpc.UnaryServerInfo{FullMethod: "/test.Service/Method"} + + _, err := interceptor(tracedIncomingContext(), "request", info, handler.handle) + + if got := status.Code(err); got != codes.NotFound { + t.Fatalf("Expected codes.NotFound to pass through, got %v (err=%v)", got, err) + } +} + +func TestStreamServerInterceptor_RecoversPanicAsInternal(t *testing.T) { + // With the default options (Repanic disabled), a panicking handler must be + // recovered and returned as a codes.Internal error rather than a nil error. + interceptor := StreamServerInterceptor() + handler := &mockStreamHandler{panic: true} + info := &grpc.StreamServerInfo{FullMethod: "/test.Service/Stream"} + stream := &mockServerStream{ctx: tracedIncomingContext()} + + err := interceptor(nil, stream, info, handler.handle) + + if got := status.Code(err); got != codes.Internal { + t.Fatalf("Expected codes.Internal, got %v (err=%v)", got, err) + } +} + +func TestStreamServerInterceptor_RepanicsWhenEnabled(t *testing.T) { + // With Repanic enabled the panic must propagate to the caller unchanged. + interceptor := StreamServerInterceptor(WithRepanicOption(true)) + handler := &mockStreamHandler{panic: true} + info := &grpc.StreamServerInfo{FullMethod: "/test.Service/Stream"} + stream := &mockServerStream{ctx: tracedIncomingContext()} + + defer func() { + if r := recover(); r == nil { + t.Error("Expected interceptor to re-panic, but it did not") + } + }() + _ = interceptor(nil, stream, info, handler.handle) +} + +func TestStreamServerInterceptor_PassesThroughError(t *testing.T) { + // A normal handler error must be returned unchanged. + interceptor := StreamServerInterceptor() + handler := &mockStreamHandler{err: status.Error(codes.NotFound, "missing")} + info := &grpc.StreamServerInfo{FullMethod: "/test.Service/Stream"} + stream := &mockServerStream{ctx: tracedIncomingContext()} + + err := interceptor(nil, stream, info, handler.handle) + + if got := status.Code(err); got != codes.NotFound { + t.Fatalf("Expected codes.NotFound to pass through, got %v (err=%v)", got, err) + } +} + func TestContinueFromGrpcMetadata(t *testing.T) { tests := []struct { name string