grpc: Fix missing error checks for `Serve()` function

The `grpc.Server.Serve()` function will return an error when the gRPC
server has either failed unexpectedly or in case it wasn't properly shut
down. We don't check this error in many places though, which both causes
the errcheck linter to complain and hides issues we have with some of
our test setups.

Fix the missing error checks via a new `testhelper.MustServe()` helper
function that does the error checking for us. This surfaces some test
errors for tests that forget to shut down the server or that manually
close the listener, which should be handled by gRPC itself. All of those
errors are fixed while at it.

Drop the corresponding linting exceptions.

23 files changed, 56 insertions, 40 deletions

diff --git a/.golangci.yml b/.golangci.yml
index cbb624eb8..73200d48e 100644
--- a/.golangci.yml
+++ b/.golangci.yml
@@ -70,7 +70,6 @@ linters-settings:
       - (net.Listener).Close
       # Serve
       - (*gitlab.com/gitlab-org/gitaly/v15/internal/praefect.ServerFactory).Serve
-      - (*google.golang.org/grpc.Server).Serve
       - (*net/http.Server).Serve
   forbidigo:
     forbid:
diff --git a/client/dial_test.go b/client/dial_test.go
index b862b4c3e..e0870ee32 100644
--- a/client/dial_test.go
+++ b/client/dial_test.go
@@ -296,7 +296,7 @@ func TestDial_Correlation(t *testing.T) {
 		}
 		grpc_testing.RegisterTestServiceServer(grpcServer, svc)
 
-		go func() { assert.NoError(t, grpcServer.Serve(listener)) }()
+		go testhelper.MustServe(t, grpcServer, listener)
 
 		defer grpcServer.Stop()
 		ctx := testhelper.Context(t)
@@ -332,7 +332,7 @@ func TestDial_Correlation(t *testing.T) {
 		}
 		grpc_testing.RegisterTestServiceServer(grpcServer, svc)
 
-		go func() { assert.NoError(t, grpcServer.Serve(listener)) }()
+		go testhelper.MustServe(t, grpcServer, listener)
 		defer grpcServer.Stop()
 		ctx := testhelper.Context(t)
 
@@ -395,7 +395,7 @@ func TestDial_Tracing(t *testing.T) {
 	}
 	grpc_testing.RegisterTestServiceServer(grpcServer, svc)
 
-	go func() { require.NoError(t, grpcServer.Serve(listener)) }()
+	go testhelper.MustServe(t, grpcServer, listener)
 	defer grpcServer.Stop()
 	ctx := testhelper.Context(t)
 
@@ -543,7 +543,7 @@ func startTCPListener(tb testing.TB, factory func(credentials.TransportCredentia
 	address := fmt.Sprintf("%d", tcpPort)
 
 	grpcServer := factory(insecure.NewCredentials())
-	go grpcServer.Serve(listener)
+	go testhelper.MustServe(tb, grpcServer, listener)
 
 	return func() {
 		grpcServer.Stop()
@@ -558,7 +558,7 @@ func startUnixListener(tb testing.TB, factory func(credentials.TransportCredenti
 	require.NoError(tb, err)
 
 	grpcServer := factory(insecure.NewCredentials())
-	go grpcServer.Serve(listener)
+	go testhelper.MustServe(tb, grpcServer, listener)
 
 	return func() {
 		grpcServer.Stop()
@@ -583,7 +583,7 @@ func startTLSListener(tb testing.TB, factory func(credentials.TransportCredentia
 			MinVersion:   tls.VersionTLS12,
 		}),
 	)
-	go grpcServer.Serve(listener)
+	go testhelper.MustServe(tb, grpcServer, listener)
 
 	return func() {
 		grpcServer.Stop()
diff --git a/cmd/gitaly-ssh/auth_test.go b/cmd/gitaly-ssh/auth_test.go
index 8a5921432..db33fbab3 100644
--- a/cmd/gitaly-ssh/auth_test.go
+++ b/cmd/gitaly-ssh/auth_test.go
@@ -167,7 +167,7 @@ func runServer(t *testing.T, secure bool, cfg config.Cfg, connectionType string,
 	listener, err := net.Listen(connectionType, addr)
 	require.NoError(t, err)
 
-	go srv.Serve(listener)
+	go testhelper.MustServe(t, srv, listener)
 
 	port := 0
 	if connectionType != "unix" {
diff --git a/internal/backchannel/backchannel_test.go b/internal/backchannel/backchannel_test.go
index e8885629a..534b63962 100644
--- a/internal/backchannel/backchannel_test.go
+++ b/internal/backchannel/backchannel_test.go
@@ -78,7 +78,7 @@ func TestBackchannel_concurrentRequestsFromMultipleClients(t *testing.T) {
 	})
 
 	defer srv.Stop()
-	go srv.Serve(ln)
+	go testhelper.MustServe(t, srv, ln)
 	ctx := testhelper.Context(t)
 
 	start := make(chan struct{})
@@ -271,7 +271,7 @@ func Benchmark(b *testing.B) {
 					require.NoError(b, err)
 
 					defer srv.Stop()
-					go srv.Serve(ln)
+					go testhelper.MustServe(b, srv, ln)
 					ctx := testhelper.Context(b)
 
 					opts := []grpc.DialOption{grpc.WithBlock(), grpc.WithTransportCredentials(insecure.NewCredentials())}
diff --git a/internal/gitaly/client/dial_test.go b/internal/gitaly/client/dial_test.go
index 9c9f72f17..2ff52e9b8 100644
--- a/internal/gitaly/client/dial_test.go
+++ b/internal/gitaly/client/dial_test.go
@@ -44,7 +44,7 @@ func TestDial(t *testing.T) {
 	ln, err := net.Listen("tcp", "localhost:0")
 	require.NoError(t, err)
 
-	go srv.Serve(ln)
+	go testhelper.MustServe(t, srv, ln)
 	ctx := testhelper.Context(t)
 
 	t.Run("non-muxed conn", func(t *testing.T) {
diff --git a/internal/gitaly/server/auth_test.go b/internal/gitaly/server/auth_test.go
index 7cd8d9efa..a997282cb 100644
--- a/internal/gitaly/server/auth_test.go
+++ b/internal/gitaly/server/auth_test.go
@@ -225,7 +225,7 @@ func runServer(t *testing.T, cfg config.Cfg) string {
 	listener, err := net.Listen("unix", serverSocketPath)
 	require.NoError(t, err)
 	t.Cleanup(srv.Stop)
-	go srv.Serve(listener)
+	go testhelper.MustServe(t, srv, listener)
 
 	return "unix://" + serverSocketPath
 }
@@ -256,7 +256,7 @@ func runSecureServer(t *testing.T, cfg config.Cfg) string {
 
 	listener, hostPort := testhelper.GetLocalhostListener(t)
 	t.Cleanup(srv.Stop)
-	go srv.Serve(listener)
+	go testhelper.MustServe(t, srv, listener)
 
 	return hostPort
 }
diff --git a/internal/gitaly/server/server_factory_test.go b/internal/gitaly/server/server_factory_test.go
index 9a181089d..0137b62a5 100644
--- a/internal/gitaly/server/server_factory_test.go
+++ b/internal/gitaly/server/server_factory_test.go
@@ -44,6 +44,8 @@ func TestGitalyServerFactory(t *testing.T) {
 		if schema == starter.TLS {
 			srv, err := sf.CreateExternal(true)
 			require.NoError(t, err)
+			t.Cleanup(srv.Stop)
+
 			healthpb.RegisterHealthServer(srv, health.NewServer())
 
 			listener, err := net.Listen(starter.TCP, addr)
@@ -66,6 +68,8 @@ func TestGitalyServerFactory(t *testing.T) {
 		} else {
 			srv, err := sf.CreateExternal(false)
 			require.NoError(t, err)
+			t.Cleanup(srv.Stop)
+
 			healthpb.RegisterHealthServer(srv, health.NewServer())
 
 			listener, err := net.Listen(schema, addr)
@@ -299,7 +303,7 @@ func TestGitalyServerFactory_closeOrder(t *testing.T) {
 		require.NoError(t, err)
 		defer ln.Close()
 
-		go server.Serve(ln)
+		go testhelper.MustServe(t, server, ln)
 
 		conn, err := grpc.DialContext(ctx, ln.Addr().String(), grpc.WithTransportCredentials(insecure.NewCredentials()))
 		require.NoError(t, err)
diff --git a/internal/helper/chunk/chunker_test.go b/internal/helper/chunk/chunker_test.go
index ea759312e..e80be6419 100644
--- a/internal/helper/chunk/chunker_test.go
+++ b/internal/helper/chunk/chunker_test.go
@@ -85,7 +85,7 @@ func runServer(t *testing.T, s *server, opt ...grpc.ServerOption) (*grpc.Server,
 	lis, err := net.Listen("unix", serverSocketPath)
 	require.NoError(t, err)
 
-	go grpcServer.Serve(lis)
+	go testhelper.MustServe(t, grpcServer, lis)
 
 	return grpcServer, "unix://" + serverSocketPath
 }
diff --git a/internal/middleware/limithandler/middleware_test.go b/internal/middleware/limithandler/middleware_test.go
index d5aa4ee57..08c3f0cca 100644
--- a/internal/middleware/limithandler/middleware_test.go
+++ b/internal/middleware/limithandler/middleware_test.go
@@ -572,7 +572,7 @@ func runServer(t *testing.T, s pb.TestServer, opt ...grpc.ServerOption) (*grpc.S
 	lis, err := net.Listen("unix", serverSocketPath)
 	require.NoError(t, err)
 
-	go grpcServer.Serve(lis)
+	go testhelper.MustServe(t, grpcServer, lis)
 
 	return grpcServer, "unix://" + serverSocketPath
 }
diff --git a/internal/praefect/auth_test.go b/internal/praefect/auth_test.go
index 9f0cd9012..324392dce 100644
--- a/internal/praefect/auth_test.go
+++ b/internal/praefect/auth_test.go
@@ -169,7 +169,7 @@ func runServer(t *testing.T, token string, required bool) (*grpc.Server, string,
 
 	listener, err := net.Listen("unix", serverSocketPath)
 	require.NoError(t, err)
-	go srv.Serve(listener)
+	go testhelper.MustServe(t, srv, listener)
 
 	return srv, "unix://" + serverSocketPath, cleanup
 }
diff --git a/internal/praefect/delete_object_pool_test.go b/internal/praefect/delete_object_pool_test.go
index db63edddf..5e29bf0b1 100644
--- a/internal/praefect/delete_object_pool_test.go
+++ b/internal/praefect/delete_object_pool_test.go
@@ -45,10 +45,10 @@ func TestDeleteObjectPoolHandler(t *testing.T) {
 	secondaryLn, secondaryAddr := testhelper.GetLocalhostListener(t)
 
 	defer primarySrv.Stop()
-	go primarySrv.Serve(primaryLn)
+	go testhelper.MustServe(t, primarySrv, primaryLn)
 
 	defer secondarySrv.Stop()
-	go secondarySrv.Serve(secondaryLn)
+	go testhelper.MustServe(t, secondarySrv, secondaryLn)
 
 	db := testdb.New(t)
 	rs := datastore.NewPostgresRepositoryStore(db, nil)
@@ -96,7 +96,7 @@ func TestDeleteObjectPoolHandler(t *testing.T) {
 	}, struct{}{})
 
 	defer praefectSrv.Stop()
-	go praefectSrv.Serve(praefectLn)
+	go testhelper.MustServe(t, praefectSrv, praefectLn)
 
 	praefectConn, err := grpc.Dial(praefectAddr, grpc.WithTransportCredentials(insecure.NewCredentials()))
 	require.NoError(t, err)
diff --git a/internal/praefect/grpc-proxy/proxy/handler_ext_test.go b/internal/praefect/grpc-proxy/proxy/handler_ext_test.go
index 7e5234fcd..a20131ca5 100644
--- a/internal/praefect/grpc-proxy/proxy/handler_ext_test.go
+++ b/internal/praefect/grpc-proxy/proxy/handler_ext_test.go
@@ -344,7 +344,7 @@ func TestProxyErrorPropagation(t *testing.T) {
 			backendServer := grpc.NewServer(grpc.UnknownServiceHandler(func(interface{}, grpc.ServerStream) error {
 				return tc.backendError
 			}))
-			go func() { backendServer.Serve(backendListener) }()
+			go testhelper.MustServe(t, backendServer, backendListener)
 			defer backendServer.Stop()
 			ctx := testhelper.Context(t)
 
@@ -374,7 +374,7 @@ func TestProxyErrorPropagation(t *testing.T) {
 				})),
 			)
 
-			go func() { proxyServer.Serve(proxyListener) }()
+			go testhelper.MustServe(t, proxyServer, proxyListener)
 			defer proxyServer.Stop()
 
 			proxyClientConn, err := grpc.DialContext(ctx, "unix://"+proxyListener.Addr().String(), grpc.WithTransportCredentials(insecure.NewCredentials()))
@@ -488,7 +488,7 @@ func TestRegisterStreamHandlers(t *testing.T) {
 			proxy.RegisterStreamHandlers(server, grpc_testing.TestService_ServiceDesc.ServiceName, registeredHandlers)
 
 			listener := newListener(t)
-			go server.Serve(listener)
+			go testhelper.MustServe(t, server, listener)
 			defer server.Stop()
 
 			conn, err := client.Dial("tcp://"+listener.Addr().String(), []grpc.DialOption{grpc.WithBlock()})
diff --git a/internal/praefect/grpc-proxy/proxy/testhelper_test.go b/internal/praefect/grpc-proxy/proxy/testhelper_test.go
index bc7ffca5e..4a8dd5aa0 100644
--- a/internal/praefect/grpc-proxy/proxy/testhelper_test.go
+++ b/internal/praefect/grpc-proxy/proxy/testhelper_test.go
@@ -41,7 +41,7 @@ func newBackendPinger(tb testing.TB, ctx context.Context) (*grpc.ClientConn, *in
 	done := make(chan struct{})
 	go func() {
 		defer close(done)
-		srvr.Serve(listener)
+		require.NoError(tb, srvr.Serve(listener))
 	}()
 
 	cc, err := grpc.DialContext(
@@ -83,7 +83,7 @@ func newProxy(tb testing.TB, ctx context.Context, director proxy.StreamDirector,
 	listener := newListener(tb)
 	go func() {
 		defer close(done)
-		proxySrvr.Serve(listener)
+		require.NoError(tb, proxySrvr.Serve(listener))
 	}()
 
 	proxyCC, err := grpc.DialContext(
diff --git a/internal/praefect/middleware/errorhandler_test.go b/internal/praefect/middleware/errorhandler_test.go
index 9e221e741..e87015a6c 100644
--- a/internal/praefect/middleware/errorhandler_test.go
+++ b/internal/praefect/middleware/errorhandler_test.go
@@ -64,7 +64,7 @@ func TestStreamInterceptor(t *testing.T) {
 
 	mock.RegisterSimpleServiceServer(internalSrv, &simpleService{})
 
-	go internalSrv.Serve(lis)
+	go testhelper.MustServe(t, internalSrv, lis)
 	defer internalSrv.Stop()
 
 	srvOptions := []grpc.ServerOption{
@@ -92,7 +92,7 @@ func TestStreamInterceptor(t *testing.T) {
 
 	praefectSrv := grpc.NewServer(srvOptions...)
 	defer praefectSrv.Stop()
-	go praefectSrv.Serve(praefectLis)
+	go testhelper.MustServe(t, praefectSrv, praefectLis)
 
 	praefectCC, err := grpc.Dial("unix://"+praefectSocket, grpc.WithTransportCredentials(insecure.NewCredentials()))
 	defer testhelper.MustClose(t, praefectCC)
diff --git a/internal/praefect/node_test.go b/internal/praefect/node_test.go
index b9a4bfecc..452a13d1a 100644
--- a/internal/praefect/node_test.go
+++ b/internal/praefect/node_test.go
@@ -54,7 +54,7 @@ func TestDialNodes(t *testing.T) {
 		srv := grpc.NewServer()
 		grpc_health_v1.RegisterHealthServer(srv, healthSrv)
 		defer srv.Stop()
-		go srv.Serve(ln)
+		go testhelper.MustServe(t, srv, ln)
 
 		cfgNodes = append(cfgNodes, &config.Node{
 			Storage: n.storage,
diff --git a/internal/praefect/nodes/sql_elector_test.go b/internal/praefect/nodes/sql_elector_test.go
index b2c90ac21..adc79cb75 100644
--- a/internal/praefect/nodes/sql_elector_test.go
+++ b/internal/praefect/nodes/sql_elector_test.go
@@ -453,7 +453,7 @@ func TestConnectionMultiplexing(t *testing.T) {
 	ln, err := net.Listen("tcp", "localhost:0")
 	require.NoError(t, err)
 
-	go srv.Serve(ln)
+	go testhelper.MustServe(t, srv, ln)
 
 	db := testdb.New(t)
 	mgr, err := NewManager(
diff --git a/internal/praefect/remove_repository_test.go b/internal/praefect/remove_repository_test.go
index f9eaf057a..4d8917378 100644
--- a/internal/praefect/remove_repository_test.go
+++ b/internal/praefect/remove_repository_test.go
@@ -124,7 +124,7 @@ func TestRemoveRepositoryHandler(t *testing.T) {
 			)
 			defer srv.Stop()
 
-			go func() { srv.Serve(ln) }()
+			go testhelper.MustServe(t, srv, ln)
 
 			clientConn, err := grpc.DialContext(ctx, "unix:"+ln.Addr().String(), grpc.WithTransportCredentials(insecure.NewCredentials()))
 			require.NoError(t, err)
diff --git a/internal/praefect/replicator_pg_test.go b/internal/praefect/replicator_pg_test.go
index 4bd66365e..2ee5ef568 100644
--- a/internal/praefect/replicator_pg_test.go
+++ b/internal/praefect/replicator_pg_test.go
@@ -45,7 +45,7 @@ func TestReplicatorInvalidSourceRepository(t *testing.T) {
 		},
 	})
 	defer srv.Stop()
-	go srv.Serve(ln)
+	go testhelper.MustServe(t, srv, ln)
 
 	targetCC, err := client.Dial(ln.Addr().Network()+":"+ln.Addr().String(), nil)
 	require.NoError(t, err)
@@ -101,7 +101,7 @@ func TestReplicatorDestroy(t *testing.T) {
 				return stream.SendMsg(&gitalypb.RemoveRepositoryResponse{})
 			}))
 
-			go srv.Serve(ln)
+			go testhelper.MustServe(t, srv, ln)
 			defer srv.Stop()
 
 			clientConn, err := grpc.Dial(ln.Addr().String(), grpc.WithTransportCredentials(insecure.NewCredentials()))
diff --git a/internal/praefect/repository_exists_test.go b/internal/praefect/repository_exists_test.go
index 3eb445e7e..ba005cc84 100644
--- a/internal/praefect/repository_exists_test.go
+++ b/internal/praefect/repository_exists_test.go
@@ -103,7 +103,7 @@ func TestRepositoryExistsHandler(t *testing.T) {
 			)
 			defer srv.Stop()
 
-			go func() { srv.Serve(ln) }()
+			go testhelper.MustServe(t, srv, ln)
 
 			clientConn, err := grpc.DialContext(ctx, "unix://"+ln.Addr().String(), grpc.WithTransportCredentials(insecure.NewCredentials()))
 			require.NoError(t, err)
diff --git a/internal/praefect/server_test.go b/internal/praefect/server_test.go
index edbed63e0..15c69e35a 100644
--- a/internal/praefect/server_test.go
+++ b/internal/praefect/server_test.go
@@ -94,7 +94,7 @@ func TestNewBackchannelServerFactory(t *testing.T) {
 	ln, err := net.Listen("tcp", "localhost:0")
 	require.NoError(t, err)
 
-	go server.Serve(ln)
+	go testhelper.MustServe(t, server, ln)
 	ctx := testhelper.Context(t)
 
 	nodeSet, err := DialNodes(ctx, []*config.VirtualStorage{{
@@ -848,7 +848,7 @@ func TestProxyWrites(t *testing.T) {
 	listener, err := net.Listen("unix", socket)
 	require.NoError(t, err)
 
-	go server.Serve(listener)
+	go testhelper.MustServe(t, server, listener)
 	defer server.Stop()
 
 	client, _ := newSmartHTTPClient(t, "unix://"+socket)
@@ -1002,7 +1002,7 @@ func TestErrorThreshold(t *testing.T) {
 			listener, err := net.Listen("unix", socket)
 			require.NoError(t, err)
 
-			go server.Serve(listener)
+			go testhelper.MustServe(t, server, listener)
 			defer server.Stop()
 
 			conn, err := dial("unix://"+socket, []grpc.DialOption{grpc.WithTransportCredentials(insecure.NewCredentials())})
diff --git a/internal/sidechannel/proxy_test.go b/internal/sidechannel/proxy_test.go
index e61f8a9b1..398d9c780 100644
--- a/internal/sidechannel/proxy_test.go
+++ b/internal/sidechannel/proxy_test.go
@@ -206,6 +206,6 @@ func startStreamServer(t *testing.T, handler func(gitalypb.SSHService_SSHUploadP
 	require.NoError(t, err)
 
 	t.Cleanup(srv.Stop)
-	go srv.Serve(ln)
+	go testhelper.MustServe(t, srv, ln)
 	return ln.Addr().String()
 }
diff --git a/internal/sidechannel/sidechannel_test.go b/internal/sidechannel/sidechannel_test.go
index bcc24c7a8..b07031323 100644
--- a/internal/sidechannel/sidechannel_test.go
+++ b/internal/sidechannel/sidechannel_test.go
@@ -157,16 +157,15 @@ func startServer(t *testing.T, th testHandler, opts ...grpc.ServerOption) string
 	opts = append(opts, grpc.Creds(lm))
 
 	s := grpc.NewServer(opts...)
-	t.Cleanup(func() { s.Stop() })
+	t.Cleanup(s.Stop)
 
 	handler := &server{testHandler: th}
 	healthpb.RegisterHealthServer(s, handler)
 
 	lis, err := net.Listen("tcp", "localhost:0")
 	require.NoError(t, err)
-	t.Cleanup(func() { lis.Close() })
 
-	go func() { s.Serve(lis) }()
+	go testhelper.MustServe(t, s, lis)
 
 	return lis.Addr().String()
 }
diff --git a/internal/testhelper/testhelper.go b/internal/testhelper/testhelper.go
index 2599eed26..3cabbb07e 100644
--- a/internal/testhelper/testhelper.go
+++ b/internal/testhelper/testhelper.go
@@ -102,6 +102,20 @@ func MustClose(tb testing.TB, closer io.Closer) {
 	require.NoError(tb, closer.Close())
 }
 
+// Server is an interface for a server that can serve requests on a specific listener. This
+// interface is used by the MustServe helper function.
+type Server interface {
+	Serve(net.Listener) error
+}
+
+// MustServe starts to serve the given server with the listener. This function asserts that the
+// server was able to successfully serve and is useful in contexts where one wants to simply spawn a
+// server in a Goroutine.
+func MustServe(tb testing.TB, server Server, listener net.Listener) {
+	tb.Helper()
+	require.NoError(tb, server.Serve(listener))
+}
+
 // CopyFile copies a file at the path src to a file at the path dst
 func CopyFile(tb testing.TB, src, dst string) {
 	fsrc, err := os.Open(src)