diff --git a/grpc/defaults/server.go b/grpc/defaults/server.go index 714563d48..281136218 100644 --- a/grpc/defaults/server.go +++ b/grpc/defaults/server.go @@ -1,14 +1,20 @@ package defaults import ( + "context" + "fmt" + "runtime" "sync" grpcprom "github.com/grpc-ecosystem/go-grpc-middleware/providers/prometheus" + "github.com/grpc-ecosystem/go-grpc-middleware/v2/interceptors/recovery" "github.com/prometheus/client_golang/prometheus" sglog "github.com/sourcegraph/log" "go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc" "google.golang.org/grpc" + "google.golang.org/grpc/codes" "google.golang.org/grpc/reflection" + "google.golang.org/grpc/status" "github.com/sourcegraph/zoekt/grpc/internalerrs" "github.com/sourcegraph/zoekt/grpc/messagesize" @@ -19,6 +25,8 @@ import ( func NewServer(logger sglog.Logger, additionalOpts ...grpc.ServerOption) *grpc.Server { metrics := serverMetricsOnce() + recoveryOpt := recovery.WithRecoveryHandlerContext(panicRecoveryHandler(logger)) + opts := []grpc.ServerOption{ grpc.StatsHandler(otelgrpc.NewServerHandler()), grpc.ChainStreamInterceptor( @@ -27,6 +35,7 @@ func NewServer(logger sglog.Logger, additionalOpts ...grpc.ServerOption) *grpc.S metrics.StreamServerInterceptor(), messagesize.StreamServerInterceptor, internalerrs.LoggingStreamServerInterceptor(logger), + recovery.StreamServerInterceptor(recoveryOpt), ), grpc.ChainUnaryInterceptor( propagator.UnaryServerPropagator(tenant.Propagator{}), @@ -34,6 +43,7 @@ func NewServer(logger sglog.Logger, additionalOpts ...grpc.ServerOption) *grpc.S metrics.UnaryServerInterceptor(), messagesize.UnaryServerInterceptor, internalerrs.LoggingUnaryServerInterceptor(logger), + recovery.UnaryServerInterceptor(recoveryOpt), ), } @@ -51,6 +61,30 @@ func NewServer(logger sglog.Logger, additionalOpts ...grpc.ServerOption) *grpc.S return s } +// panicRecoveryHandler converts a recovered handler panic into an Internal +// error. Shard searches already recover their own panics in searchOneShard, so +// this only sees bugs in the layer between gRPC and the shard searchers. The +// panic value is logged rather than returned, since it is internal detail. +func panicRecoveryHandler(logger sglog.Logger) recovery.RecoveryHandlerFuncContext { + return func(ctx context.Context, p any) error { + stack := make([]byte, 64<<10) + stack = stack[:runtime.Stack(stack, false)] + + method, ok := grpc.Method(ctx) + if !ok { + method = "unknown" + } + + logger.Error("recovered from panic in gRPC handler", + sglog.String("method", method), + sglog.String("panic", fmt.Sprint(p)), + sglog.String("stacktrace", string(stack)), + ) + + return status.Error(codes.Internal, "internal error") + } +} + // serviceMetricsOnce returns a singleton instance of the server metrics // that are shared across all gRPC servers that this process creates. // diff --git a/query/query_proto.go b/query/query_proto.go index 621a12a7b..38696a7c6 100644 --- a/query/query_proto.go +++ b/query/query_proto.go @@ -56,6 +56,10 @@ func QToProto(q Q) *webserverv1.Q { } func QFromProto(p *webserverv1.Q) (Q, error) { + if p == nil { + return nil, fmt.Errorf("query node is missing") + } + switch v := p.Query.(type) { case *webserverv1.Q_RawConfig: return RawConfigFromProto(v.RawConfig), nil @@ -96,7 +100,7 @@ func QFromProto(p *webserverv1.Q) (Q, error) { case *webserverv1.Q_Meta: return MetaFromProto(v.Meta) default: - panic(fmt.Sprintf("unknown query node %T", p.Query)) + return nil, fmt.Errorf("unknown query node %T", p.Query) } }