sidechannel: Check `Close()` errors returned by the waiter

We don't properly check the error when closing the `SidechannelWaiter`
in many places. Fix this and drop the corresponding linting exceptions.

12 files changed, 53 insertions, 23 deletions

diff --git a/.golangci.yml b/.golangci.yml
index cb43b9861..cbb624eb8 100644
--- a/.golangci.yml
+++ b/.golangci.yml
@@ -56,10 +56,7 @@ linters-settings:
       - (*database/sql.DB).Close
       - (*database/sql.Rows).Close
       - (*gitlab.com/gitlab-org/gitaly/v15/client.Pool).Close
-      - (*gitlab.com/gitlab-org/gitaly/v15/client.SidechannelWaiter).Close
-      - (*gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/hook.SidechannelWaiter).Close
       - (*gitlab.com/gitlab-org/gitaly/v15/internal/sidechannel.ServerConn).Close
-      - (*gitlab.com/gitlab-org/gitaly/v15/internal/sidechannel.Waiter).Close
       - (*gitlab.com/gitlab-org/gitaly/v15/internal/streamcache.pipe).Close
       - (*gitlab.com/gitlab-org/gitaly/v15/internal/streamcache.pipeReader).Close
       - (*google.golang.org/grpc.ClientConn).Close
diff --git a/client/dial_test.go b/client/dial_test.go
index 758ff1013..b862b4c3e 100644
--- a/client/dial_test.go
+++ b/client/dial_test.go
@@ -247,7 +247,7 @@ func TestDialSidechannel(t *testing.T) {
 
 				return nil
 			})
-			defer scw.Close()
+			defer testhelper.MustClose(t, scw)
 
 			req := &healthpb.HealthCheckRequest{Service: "test sidechannel"}
 			_, err = healthpb.NewHealthClient(conn).Check(ctx, req)
diff --git a/client/upload_pack.go b/client/upload_pack.go
index 863465068..aeda65f6b 100644
--- a/client/upload_pack.go
+++ b/client/upload_pack.go
@@ -43,14 +43,24 @@ func UploadPack(ctx context.Context, conn *grpc.ClientConn, stdin io.Reader, std
 
 // UploadPackWithSidechannel proxies an SSH git-upload-pack (git fetch)
 // session to Gitaly using a sidechannel for the raw data transfer.
-func UploadPackWithSidechannel(ctx context.Context, conn *grpc.ClientConn, reg *SidechannelRegistry, stdin io.Reader, stdout, stderr io.Writer, req *gitalypb.SSHUploadPackWithSidechannelRequest) (int32, error) {
+func UploadPackWithSidechannel(
+	ctx context.Context,
+	conn *grpc.ClientConn,
+	reg *SidechannelRegistry,
+	stdin io.Reader,
+	stdout, stderr io.Writer,
+	req *gitalypb.SSHUploadPackWithSidechannelRequest,
+) (int32, error) {
 	ctx, cancel := context.WithCancel(ctx)
 	defer cancel()
 
 	ctx, wt := reg.Register(ctx, func(c SidechannelConn) error {
 		return stream.ProxyPktLine(c, stdin, stdout, stderr)
 	})
-	defer wt.Close()
+	defer func() {
+		// We aleady check the error further down.
+		_ = wt.Close()
+	}()
 
 	sshClient := gitalypb.NewSSHServiceClient(conn)
 	if _, err := sshClient.SSHUploadPackWithSidechannel(ctx, req); err != nil {
diff --git a/cmd/gitaly-hooks/hooks.go b/cmd/gitaly-hooks/hooks.go
index 39d0e4ba4..11601d29a 100644
--- a/cmd/gitaly-hooks/hooks.go
+++ b/cmd/gitaly-hooks/hooks.go
@@ -334,7 +334,10 @@ func handlePackObjectsWithSidechannel(ctx context.Context, payload git.HooksPayl
 	if err != nil {
 		return fmt.Errorf("SetupSidechannel: %w", err)
 	}
-	defer wt.Close()
+	defer func() {
+		// We aleady check the error further down.
+		_ = wt.Close()
+	}()
 
 	var glID, glUsername, gitProtocol string
 
@@ -376,5 +379,13 @@ func handlePackObjectsWithSidechannel(ctx context.Context, payload git.HooksPayl
 		return fmt.Errorf("call PackObjectsHookWithSidechannel: %w", err)
 	}
 
-	return wt.Wait()
+	if err := wt.Wait(); err != nil {
+		return err
+	}
+
+	if err := wt.Close(); err != nil {
+		return fmt.Errorf("closing sidechannel: %w", err)
+	}
+
+	return nil
 }
diff --git a/internal/gitaly/hook/sidechannel_test.go b/internal/gitaly/hook/sidechannel_test.go
index 243e0de09..0cba065c9 100644
--- a/internal/gitaly/hook/sidechannel_test.go
+++ b/internal/gitaly/hook/sidechannel_test.go
@@ -47,7 +47,7 @@ func testSidechannelWithRuntimeDir(t *testing.T, runtimeDir string) {
 		},
 	)
 	require.NoError(t, err)
-	defer wt.Close()
+	defer testhelper.MustClose(t, wt)
 
 	require.DirExists(t, wt.socketDir)
 
@@ -85,7 +85,6 @@ func testSidechannelCleanupWithRuntimeDir(t *testing.T, runtimeDir string) {
 		func(c *net.UnixConn) error { return nil },
 	)
 	require.NoError(t, err)
-	defer wt.Close()
 
 	require.DirExists(t, wt.socketDir)
 	_ = wt.Close()
diff --git a/internal/gitaly/service/hook/pack_objects_test.go b/internal/gitaly/service/hook/pack_objects_test.go
index 0bf4262da..403f3fe77 100644
--- a/internal/gitaly/service/hook/pack_objects_test.go
+++ b/internal/gitaly/service/hook/pack_objects_test.go
@@ -134,7 +134,7 @@ func testServerPackObjectsHookSeparateContextWithRuntimeDir(t *testing.T, ctx co
 		},
 	)
 	require.NoError(t, err)
-	defer wt1.Close()
+	defer testhelper.MustClose(t, wt1)
 
 	go func() {
 		defer wg.Done()
@@ -174,7 +174,7 @@ func testServerPackObjectsHookSeparateContextWithRuntimeDir(t *testing.T, ctx co
 		},
 	)
 	require.NoError(t, err)
-	defer wt2.Close()
+	defer testhelper.MustClose(t, wt2)
 
 	go func() {
 		defer wg.Done()
@@ -240,7 +240,7 @@ func testServerPackObjectsHookUsesCache(t *testing.T, ctx context.Context, runti
 			},
 		)
 		require.NoError(t, err)
-		defer wt.Close()
+		defer testhelper.MustClose(t, wt)
 
 		client, conn := newHooksClient(t, cfg.SocketPath)
 		defer conn.Close()
@@ -346,7 +346,7 @@ func testServerPackObjectsHookWithSidechannelWithRuntimeDir(t *testing.T, ctx co
 				},
 			)
 			require.NoError(t, err)
-			defer wt.Close()
+			defer testhelper.MustClose(t, wt)
 
 			client, conn := newHooksClient(t, cfg.SocketPath)
 			defer conn.Close()
@@ -539,7 +539,7 @@ func testServerPackObjectsHookWithSidechannelCanceledWithRuntimeDir(t *testing.T
 		},
 	)
 	require.NoError(t, err)
-	defer wt.Close()
+	defer testhelper.MustClose(t, wt)
 
 	cfg.SocketPath = runHooksServer(t, cfg, nil)
 	repo, _ := gittest.CreateRepository(ctx, t, cfg, gittest.CreateRepositoryConfig{
diff --git a/internal/gitaly/service/smarthttp/upload_pack_test.go b/internal/gitaly/service/smarthttp/upload_pack_test.go
index 02ca6296e..1da5527da 100644
--- a/internal/gitaly/service/smarthttp/upload_pack_test.go
+++ b/internal/gitaly/service/smarthttp/upload_pack_test.go
@@ -6,6 +6,7 @@ import (
 	"bytes"
 	"context"
 	"encoding/binary"
+	"errors"
 	"fmt"
 	"io"
 	"path/filepath"
@@ -613,7 +614,6 @@ func makePostUploadPackWithSidechannelRequest(ctx context.Context, t *testing.T,
 
 		return <-errC
 	})
-	defer waiter.Close()
 
 	rpcRequest := &gitalypb.PostUploadPackWithSidechannelRequest{
 		Repository:       in.GetRepository(),
@@ -622,7 +622,11 @@ func makePostUploadPackWithSidechannelRequest(ctx context.Context, t *testing.T,
 	}
 	_, err := client.PostUploadPackWithSidechannel(ctxOut, rpcRequest)
 	if err == nil {
-		require.NoError(t, waiter.Close())
+		testhelper.MustClose(t, waiter)
+	} else if err := waiter.Close(); err != nil && !errors.Is(err, sidechannel.ErrCallbackDidNotRun) {
+		// When the request failed the sidechannel may not even have been used, so we need
+		// to catch the `ErrCallbackDidNotRun` error here.
+		require.NoError(t, err)
 	}
 
 	return responseBuffer, err
diff --git a/internal/praefect/server_factory_test.go b/internal/praefect/server_factory_test.go
index 7fd850e21..598197311 100644
--- a/internal/praefect/server_factory_test.go
+++ b/internal/praefect/server_factory_test.go
@@ -165,7 +165,7 @@ func TestServerFactory(t *testing.T) {
 
 			return nil
 		})
-		defer waiter.Close()
+		defer testhelper.MustClose(t, waiter)
 
 		_, err = gitalypb.NewSmartHTTPServiceClient(cc).PostUploadPackWithSidechannel(ctx,
 			&gitalypb.PostUploadPackWithSidechannelRequest{Repository: repo},
diff --git a/internal/sidechannel/proxy.go b/internal/sidechannel/proxy.go
index 6da61274f..ee92baebf 100644
--- a/internal/sidechannel/proxy.go
+++ b/internal/sidechannel/proxy.go
@@ -18,14 +18,19 @@ func NewUnaryProxy(registry *Registry) grpc.UnaryClientInterceptor {
 		}
 
 		ctx, waiter := RegisterSidechannel(ctx, registry, proxy(ctx))
-		defer waiter.Close()
+		defer func() {
+			// We aleady check the error further down.
+			_ = waiter.Close()
+		}()
 
 		if err := invoker(ctx, method, req, reply, cc, opts...); err != nil {
 			return err
 		}
+
 		if err := waiter.Close(); err != nil && err != ErrCallbackDidNotRun {
 			return fmt.Errorf("sidechannel: proxy callback: %w", err)
 		}
+
 		return nil
 	}
 }
diff --git a/internal/sidechannel/proxy_test.go b/internal/sidechannel/proxy_test.go
index ed2f3e2a8..e61f8a9b1 100644
--- a/internal/sidechannel/proxy_test.go
+++ b/internal/sidechannel/proxy_test.go
@@ -169,7 +169,7 @@ func testStreamProxy(t *testing.T, closeWrite bool) {
 
 	conn, registry := dial(t, proxyAddr)
 	ctx, waiter := RegisterSidechannel(ctx, registry, testProxyClient(closeWrite))
-	defer waiter.Close()
+	defer testhelper.MustClose(t, waiter)
 
 	client, err := gitalypb.NewSSHServiceClient(conn).SSHUploadPack(ctx)
 	require.NoError(t, err)
diff --git a/internal/sidechannel/registry_test.go b/internal/sidechannel/registry_test.go
index 8a286b617..d1a37b927 100644
--- a/internal/sidechannel/registry_test.go
+++ b/internal/sidechannel/registry_test.go
@@ -13,6 +13,7 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
 )
 
 func TestRegistry(t *testing.T) {
@@ -25,7 +26,7 @@ func TestRegistry(t *testing.T) {
 			<-triggerCallback
 			return nil
 		})
-		defer waiter.Close()
+		defer testhelper.MustClose(t, waiter)
 
 		require.Equal(t, 1, registry.waiting())
 
@@ -57,7 +58,7 @@ func TestRegistry(t *testing.T) {
 
 					return conn.CloseWrite()
 				})
-				defer waiter.Close()
+				defer testhelper.MustClose(t, waiter)
 
 				require.NoError(t, registry.receive(waiter.id, client))
 				require.NoError(t, waiter.Close())
diff --git a/internal/sidechannel/sidechannel_test.go b/internal/sidechannel/sidechannel_test.go
index 7043e1642..bcc24c7a8 100644
--- a/internal/sidechannel/sidechannel_test.go
+++ b/internal/sidechannel/sidechannel_test.go
@@ -187,7 +187,10 @@ func call(ctx context.Context, conn *grpc.ClientConn, registry *Registry, handle
 	client := healthpb.NewHealthClient(conn)
 
 	ctxOut, waiter := RegisterSidechannel(ctx, registry, handler)
-	defer waiter.Close()
+	defer func() {
+		// We aleady check the error further down.
+		_ = waiter.Close()
+	}()
 
 	if _, err := client.Check(ctxOut, &healthpb.HealthCheckRequest{}); err != nil {
 		return err