fix/webserver: recover gRPC handler panics and reject empty query nodes

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,13 +35,15 @@ 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{}),
 			tenant.UnaryServerInterceptor,
 			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.
 //

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)
 	}
 }