service: Plug leakages of connection pools

While some of our service servers get the connection pool injected as
dependency, others construct them ad-hoc. These ad-hoc creations are
never cleaned up though and thus lead to Goroutine leakages.

Fix the leakage by always using the same injected client pool. Luckily,
all callsites do in fact use the same options to construct the pool and
thus we shouldn't see any change in behaviour, except for less
connections being create.

17 files changed, 68 insertions, 25 deletions

diff --git a/internal/git/remoterepo/repository_test.go b/internal/git/remoterepo/repository_test.go
index ddc63fb11..5bb233fdc 100644
--- a/internal/git/remoterepo/repository_test.go
+++ b/internal/git/remoterepo/repository_test.go
@@ -32,6 +32,7 @@ func TestRepository(t *testing.T) {
 			deps.GetTxManager(),
 			deps.GetGitCmdFactory(),
 			deps.GetCatfileCache(),
+			deps.GetConnsPool(),
 		))
 		gitalypb.RegisterCommitServiceServer(srv, commit.NewServer(
 			deps.GetCfg(),
diff --git a/internal/gitaly/maintenance/optimize_test.go b/internal/gitaly/maintenance/optimize_test.go
index 0281bf843..d764f59ee 100644
--- a/internal/gitaly/maintenance/optimize_test.go
+++ b/internal/gitaly/maintenance/optimize_test.go
@@ -8,6 +8,7 @@ import (
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/v14/client"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/backchannel"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/catfile"
@@ -34,7 +35,17 @@ func (mo *mockOptimizer) OptimizeRepository(ctx context.Context, req *gitalypb.O
 	gitCmdFactory := git.NewExecCommandFactory(mo.cfg)
 	catfileCache := catfile.NewCache(mo.cfg)
 	mo.t.Cleanup(catfileCache.Stop)
-	resp, err := repository.NewServer(mo.cfg, nil, l, transaction.NewManager(mo.cfg, backchannel.NewRegistry()), gitCmdFactory, catfileCache).OptimizeRepository(ctx, req)
+
+	connsPool := client.NewPool()
+	mo.t.Cleanup(func() { testhelper.MustClose(mo.t, connsPool) })
+
+	resp, err := repository.NewServer(mo.cfg, nil, l, transaction.NewManager(
+		mo.cfg,
+		backchannel.NewRegistry()),
+		gitCmdFactory,
+		catfileCache,
+		connsPool,
+	).OptimizeRepository(ctx, req)
 	assert.NoError(mo.t, err)
 	return resp, err
 }
diff --git a/internal/gitaly/service/conflicts/server.go b/internal/gitaly/service/conflicts/server.go
index 63f1279ee..0bd08481c 100644
--- a/internal/gitaly/service/conflicts/server.go
+++ b/internal/gitaly/service/conflicts/server.go
@@ -27,19 +27,23 @@ type server struct {
 }
 
 // NewServer creates a new instance of a grpc ConflictsServer
-func NewServer(cfg config.Cfg, hookManager hook.Manager, locator storage.Locator, gitCmdFactory git.CommandFactory, catfileCache catfile.Cache) gitalypb.ConflictsServiceServer {
+func NewServer(
+	cfg config.Cfg,
+	hookManager hook.Manager,
+	locator storage.Locator,
+	gitCmdFactory git.CommandFactory,
+	catfileCache catfile.Cache,
+	connsPool *client.Pool,
+) gitalypb.ConflictsServiceServer {
 	return &server{
 		cfg:           cfg,
 		hookManager:   hookManager,
 		locator:       locator,
 		gitCmdFactory: gitCmdFactory,
 		catfileCache:  catfileCache,
-		pool: client.NewPoolWithOptions(
-			client.WithDialer(client.HealthCheckDialer(client.DialContext)),
-			client.WithDialOptions(client.FailOnNonTempDialError()...),
-		),
-		updater: updateref.NewUpdaterWithHooks(cfg, hookManager, gitCmdFactory, catfileCache),
-		git2go:  git2go.NewExecutor(cfg, locator),
+		pool:          connsPool,
+		updater:       updateref.NewUpdaterWithHooks(cfg, hookManager, gitCmdFactory, catfileCache),
+		git2go:        git2go.NewExecutor(cfg, locator),
 	}
 }
 
diff --git a/internal/gitaly/service/conflicts/testhelper_test.go b/internal/gitaly/service/conflicts/testhelper_test.go
index 95797b072..31792b01c 100644
--- a/internal/gitaly/service/conflicts/testhelper_test.go
+++ b/internal/gitaly/service/conflicts/testhelper_test.go
@@ -64,6 +64,7 @@ func runConflictsServer(t testing.TB, cfg config.Cfg, hookManager hook.Manager)
 			deps.GetLocator(),
 			deps.GetGitCmdFactory(),
 			deps.GetCatfileCache(),
+			deps.GetConnsPool(),
 		))
 		gitalypb.RegisterRepositoryServiceServer(srv, repository.NewServer(
 			deps.GetCfg(),
@@ -72,6 +73,7 @@ func runConflictsServer(t testing.TB, cfg config.Cfg, hookManager hook.Manager)
 			deps.GetTxManager(),
 			deps.GetGitCmdFactory(),
 			deps.GetCatfileCache(),
+			deps.GetConnsPool(),
 		))
 		gitalypb.RegisterSSHServiceServer(srv, ssh.NewServer(
 			deps.GetCfg(),
diff --git a/internal/gitaly/service/operations/testhelper_test.go b/internal/gitaly/service/operations/testhelper_test.go
index c62315942..3e8996740 100644
--- a/internal/gitaly/service/operations/testhelper_test.go
+++ b/internal/gitaly/service/operations/testhelper_test.go
@@ -90,6 +90,7 @@ func runOperationServiceServer(t testing.TB, cfg config.Cfg, rubySrv *rubyserver
 			deps.GetTxManager(),
 			deps.GetGitCmdFactory(),
 			deps.GetCatfileCache(),
+			deps.GetConnsPool(),
 		))
 		gitalypb.RegisterRefServiceServer(srv, ref.NewServer(
 			deps.GetCfg(),
diff --git a/internal/gitaly/service/remote/server.go b/internal/gitaly/service/remote/server.go
index d466af948..a44c96916 100644
--- a/internal/gitaly/service/remote/server.go
+++ b/internal/gitaly/service/remote/server.go
@@ -30,6 +30,7 @@ func NewServer(
 	gitCmdFactory git.CommandFactory,
 	catfileCache catfile.Cache,
 	txManager transaction.Manager,
+	connsPool *client.Pool,
 ) gitalypb.RemoteServiceServer {
 	return &server{
 		cfg:           cfg,
@@ -37,10 +38,7 @@ func NewServer(
 		gitCmdFactory: gitCmdFactory,
 		catfileCache:  catfileCache,
 		txManager:     txManager,
-		conns: client.NewPoolWithOptions(
-			client.WithDialer(client.HealthCheckDialer(client.DialContext)),
-			client.WithDialOptions(client.FailOnNonTempDialError()...),
-		),
+		conns:         connsPool,
 	}
 }
 
diff --git a/internal/gitaly/service/remote/testhelper_test.go b/internal/gitaly/service/remote/testhelper_test.go
index bf36a936d..06eaa916e 100644
--- a/internal/gitaly/service/remote/testhelper_test.go
+++ b/internal/gitaly/service/remote/testhelper_test.go
@@ -31,6 +31,7 @@ func setupRemoteService(t *testing.T, opts ...testserver.GitalyServerOpt) (confi
 			deps.GetGitCmdFactory(),
 			deps.GetCatfileCache(),
 			deps.GetTxManager(),
+			deps.GetConnsPool(),
 		))
 	}, opts...)
 	cfg.SocketPath = addr
diff --git a/internal/gitaly/service/remote/update_remote_mirror_test.go b/internal/gitaly/service/remote/update_remote_mirror_test.go
index e56171609..7eeb8128f 100644
--- a/internal/gitaly/service/remote/update_remote_mirror_test.go
+++ b/internal/gitaly/service/remote/update_remote_mirror_test.go
@@ -564,6 +564,7 @@ func TestUpdateRemoteMirror(t *testing.T) {
 					cmdFactory,
 					deps.GetCatfileCache(),
 					deps.GetTxManager(),
+					deps.GetConnsPool(),
 				))
 			})
 
@@ -632,6 +633,7 @@ func TestSuccessfulUpdateRemoteMirrorRequest(t *testing.T) {
 			deps.GetGitCmdFactory(),
 			deps.GetCatfileCache(),
 			deps.GetTxManager(),
+			deps.GetConnsPool(),
 		))
 	})
 
@@ -734,6 +736,7 @@ func TestSuccessfulUpdateRemoteMirrorRequestWithWildcards(t *testing.T) {
 			deps.GetGitCmdFactory(),
 			deps.GetCatfileCache(),
 			deps.GetTxManager(),
+			deps.GetConnsPool(),
 		))
 	})
 
@@ -817,6 +820,7 @@ func TestUpdateRemoteMirrorInmemory(t *testing.T) {
 			deps.GetGitCmdFactory(),
 			deps.GetCatfileCache(),
 			deps.GetTxManager(),
+			deps.GetConnsPool(),
 		))
 	})
 
@@ -865,6 +869,7 @@ func TestSuccessfulUpdateRemoteMirrorRequestWithKeepDivergentRefs(t *testing.T)
 			deps.GetGitCmdFactory(),
 			deps.GetCatfileCache(),
 			deps.GetTxManager(),
+			deps.GetConnsPool(),
 		))
 	})
 
@@ -952,6 +957,7 @@ func TestFailedUpdateRemoteMirrorRequestDueToValidation(t *testing.T) {
 			deps.GetGitCmdFactory(),
 			deps.GetCatfileCache(),
 			deps.GetTxManager(),
+			deps.GetConnsPool(),
 		))
 	})
 
diff --git a/internal/gitaly/service/repository/apply_gitattributes_test.go b/internal/gitaly/service/repository/apply_gitattributes_test.go
index f85b6f456..c71b48b80 100644
--- a/internal/gitaly/service/repository/apply_gitattributes_test.go
+++ b/internal/gitaly/service/repository/apply_gitattributes_test.go
@@ -105,6 +105,7 @@ func TestApplyGitattributesWithTransaction(t *testing.T) {
 			deps.GetTxManager(),
 			deps.GetGitCmdFactory(),
 			deps.GetCatfileCache(),
+			deps.GetConnsPool(),
 		))
 	})
 
diff --git a/internal/gitaly/service/repository/fetch_bundle_test.go b/internal/gitaly/service/repository/fetch_bundle_test.go
index a81889000..25b2d9961 100644
--- a/internal/gitaly/service/repository/fetch_bundle_test.go
+++ b/internal/gitaly/service/repository/fetch_bundle_test.go
@@ -83,6 +83,7 @@ func TestServer_FetchBundle_transaction(t *testing.T) {
 			deps.GetTxManager(),
 			deps.GetGitCmdFactory(),
 			deps.GetCatfileCache(),
+			deps.GetConnsPool(),
 		))
 		gitalypb.RegisterHookServiceServer(srv, hook.NewServer(
 			deps.GetCfg(),
diff --git a/internal/gitaly/service/repository/fetch_remote_test.go b/internal/gitaly/service/repository/fetch_remote_test.go
index a64a51d87..85bcc3254 100644
--- a/internal/gitaly/service/repository/fetch_remote_test.go
+++ b/internal/gitaly/service/repository/fetch_remote_test.go
@@ -209,6 +209,7 @@ func TestFetchRemote_transaction(t *testing.T) {
 			deps.GetTxManager(),
 			deps.GetGitCmdFactory(),
 			deps.GetCatfileCache(),
+			deps.GetConnsPool(),
 		))
 	}, testserver.WithTransactionManager(txManager))
 
diff --git a/internal/gitaly/service/repository/fork_test.go b/internal/gitaly/service/repository/fork_test.go
index 8c7876882..092ec2e07 100644
--- a/internal/gitaly/service/repository/fork_test.go
+++ b/internal/gitaly/service/repository/fork_test.go
@@ -9,7 +9,7 @@ import (
 
 	"github.com/stretchr/testify/require"
 	gitalyauth "gitlab.com/gitlab-org/gitaly/v14/auth"
-	gclient "gitlab.com/gitlab-org/gitaly/v14/client"
+	"gitlab.com/gitlab-org/gitaly/v14/client"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/backchannel"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/cache"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git"
@@ -129,7 +129,7 @@ func newSecureRepoClient(t testing.TB, addr, token string, pool *x509.CertPool)
 		grpc.WithPerRPCCredentials(gitalyauth.RPCCredentialsV2(token)),
 	}
 
-	conn, err := gclient.Dial(addr, connOpts)
+	conn, err := client.Dial(addr, connOpts)
 	require.NoError(t, err)
 
 	return gitalypb.NewRepositoryServiceClient(conn), conn
@@ -228,9 +228,12 @@ func runSecureServer(t *testing.T, cfg config.Cfg, rubySrv *rubyserver.Server) s
 	catfileCache := catfile.NewCache(cfg)
 	t.Cleanup(catfileCache.Stop)
 
-	gitalypb.RegisterRepositoryServiceServer(server, NewServer(cfg, rubySrv, locator, txManager, gitCmdFactory, catfileCache))
+	connsPool := client.NewPool()
+	t.Cleanup(func() { testhelper.MustClose(t, connsPool) })
+
+	gitalypb.RegisterRepositoryServiceServer(server, NewServer(cfg, rubySrv, locator, txManager, gitCmdFactory, catfileCache, connsPool))
 	gitalypb.RegisterHookServiceServer(server, hookservice.NewServer(cfg, hookManager, gitCmdFactory, nil))
-	gitalypb.RegisterRemoteServiceServer(server, remote.NewServer(cfg, locator, gitCmdFactory, catfileCache, txManager))
+	gitalypb.RegisterRemoteServiceServer(server, remote.NewServer(cfg, locator, gitCmdFactory, catfileCache, txManager, connsPool))
 	gitalypb.RegisterSSHServiceServer(server, ssh.NewServer(cfg, locator, gitCmdFactory, txManager))
 	gitalypb.RegisterRefServiceServer(server, ref.NewServer(cfg, locator, gitCmdFactory, txManager, catfileCache))
 	gitalypb.RegisterCommitServiceServer(server, commit.NewServer(cfg, locator, gitCmdFactory, nil, catfileCache))
diff --git a/internal/gitaly/service/repository/search_files_test.go b/internal/gitaly/service/repository/search_files_test.go
index 425c805cb..25bd6c17c 100644
--- a/internal/gitaly/service/repository/search_files_test.go
+++ b/internal/gitaly/service/repository/search_files_test.go
@@ -10,6 +10,7 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/v14/client"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/backchannel"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/catfile"
@@ -210,6 +211,9 @@ func TestSearchFilesByContentFailure(t *testing.T) {
 	catfileCache := catfile.NewCache(cfg)
 	t.Cleanup(catfileCache.Stop)
 
+	connsPool := client.NewPool()
+	defer testhelper.MustClose(t, connsPool)
+
 	server := NewServer(
 		cfg,
 		nil,
@@ -217,6 +221,7 @@ func TestSearchFilesByContentFailure(t *testing.T) {
 		transaction.NewManager(cfg, backchannel.NewRegistry()),
 		gitCommandFactory,
 		catfileCache,
+		connsPool,
 	)
 
 	testCases := []struct {
@@ -335,6 +340,9 @@ func TestSearchFilesByNameFailure(t *testing.T) {
 	catfileCache := catfile.NewCache(cfg)
 	t.Cleanup(catfileCache.Stop)
 
+	connsPool := client.NewPool()
+	defer testhelper.MustClose(t, connsPool)
+
 	server := NewServer(
 		cfg,
 		nil,
@@ -342,6 +350,7 @@ func TestSearchFilesByNameFailure(t *testing.T) {
 		transaction.NewManager(cfg, backchannel.NewRegistry()),
 		gitCommandFactory,
 		catfileCache,
+		connsPool,
 	)
 
 	testCases := []struct {
diff --git a/internal/gitaly/service/repository/server.go b/internal/gitaly/service/repository/server.go
index e248c70d7..d211325e5 100644
--- a/internal/gitaly/service/repository/server.go
+++ b/internal/gitaly/service/repository/server.go
@@ -36,21 +36,19 @@ func NewServer(
 	txManager transaction.Manager,
 	gitCmdFactory git.CommandFactory,
 	catfileCache catfile.Cache,
+	connsPool *client.Pool,
 ) gitalypb.RepositoryServiceServer {
 	return &server{
 		ruby:          rs,
 		locator:       locator,
 		txManager:     txManager,
 		gitCmdFactory: gitCmdFactory,
-		conns: client.NewPoolWithOptions(
-			client.WithDialer(client.HealthCheckDialer(client.DialContext)),
-			client.WithDialOptions(client.FailOnNonTempDialError()...),
-		),
-		cfg:          cfg,
-		binDir:       cfg.BinDir,
-		loggingCfg:   cfg.Logging,
-		catfileCache: catfileCache,
-		git2go:       git2go.NewExecutor(cfg, locator),
+		conns:         connsPool,
+		cfg:           cfg,
+		binDir:        cfg.BinDir,
+		loggingCfg:    cfg.Logging,
+		catfileCache:  catfileCache,
+		git2go:        git2go.NewExecutor(cfg, locator),
 	}
 }
 
diff --git a/internal/gitaly/service/repository/testhelper_test.go b/internal/gitaly/service/repository/testhelper_test.go
index dbca91c4e..1e33f5ff9 100644
--- a/internal/gitaly/service/repository/testhelper_test.go
+++ b/internal/gitaly/service/repository/testhelper_test.go
@@ -115,6 +115,7 @@ func runRepositoryServerWithConfig(t testing.TB, cfg config.Cfg, rubySrv *rubyse
 			deps.GetTxManager(),
 			deps.GetGitCmdFactory(),
 			deps.GetCatfileCache(),
+			deps.GetConnsPool(),
 		))
 		gitalypb.RegisterHookServiceServer(srv, hookservice.NewServer(cfg, deps.GetHookManager(), deps.GetGitCmdFactory(), deps.GetPackObjectsCache()))
 		gitalypb.RegisterRemoteServiceServer(srv, remote.NewServer(
@@ -123,6 +124,7 @@ func runRepositoryServerWithConfig(t testing.TB, cfg config.Cfg, rubySrv *rubyse
 			deps.GetGitCmdFactory(),
 			deps.GetCatfileCache(),
 			deps.GetTxManager(),
+			deps.GetConnsPool(),
 		))
 		gitalypb.RegisterSSHServiceServer(srv, ssh.NewServer(
 			cfg,
diff --git a/internal/gitaly/service/setup/register.go b/internal/gitaly/service/setup/register.go
index 67286ffea..39010ff36 100644
--- a/internal/gitaly/service/setup/register.go
+++ b/internal/gitaly/service/setup/register.go
@@ -102,6 +102,7 @@ func RegisterAll(srv *grpc.Server, deps *service.Dependencies) {
 		deps.GetTxManager(),
 		deps.GetGitCmdFactory(),
 		deps.GetCatfileCache(),
+		deps.GetConnsPool(),
 	))
 	gitalypb.RegisterSSHServiceServer(srv, ssh.NewServer(
 		deps.GetCfg(),
@@ -124,6 +125,7 @@ func RegisterAll(srv *grpc.Server, deps *service.Dependencies) {
 		deps.GetLocator(),
 		deps.GetGitCmdFactory(),
 		deps.GetCatfileCache(),
+		deps.GetConnsPool(),
 	))
 	gitalypb.RegisterRemoteServiceServer(srv, remote.NewServer(
 		deps.GetCfg(),
@@ -131,6 +133,7 @@ func RegisterAll(srv *grpc.Server, deps *service.Dependencies) {
 		deps.GetGitCmdFactory(),
 		deps.GetCatfileCache(),
 		deps.GetTxManager(),
+		deps.GetConnsPool(),
 	))
 	gitalypb.RegisterServerServiceServer(srv, server.NewServer(deps.GetGitCmdFactory(), deps.GetCfg().Storages))
 	gitalypb.RegisterObjectPoolServiceServer(srv, objectpool.NewServer(
diff --git a/internal/praefect/info_service_test.go b/internal/praefect/info_service_test.go
index 7d1710ffd..2a0caa2b4 100644
--- a/internal/praefect/info_service_test.go
+++ b/internal/praefect/info_service_test.go
@@ -40,6 +40,7 @@ func TestInfoService_RepositoryReplicas(t *testing.T) {
 				deps.GetTxManager(),
 				deps.GetGitCmdFactory(),
 				deps.GetCatfileCache(),
+				deps.GetConnsPool(),
 			))
 		}, testserver.WithDisablePraefect())
 		cfgNodes = append(cfgNodes, &config.Node{