ssh: Fix silent errors when SSHReceivePack fails

When git-receive-pack(1) returns an error, then we send that error code
to the client via an `SSHUploadPackResponse`. So while the client will
be notified of the actual error condition if they know to be on the
lookout for the error code, we will not make that error visible outside
of that particular exchange. This has two important consequences:

    1. We are effectively blind to errors in SSHUploadPack given that
       they also won't be visible in Prometheus. It's thus not possible
       to put SLOs on pushes.

    2. Praefect will not see any errors either and assume that the
       request was indeed successful. When the request has failed under
       the hood though it notices that there aren't any transactional
       votes and thus it will schedule a replication job as a safety
       guard.

This is in fact the same error as we have fixed in 9deaf47f1 (ssh: Log
error on git command failure, 2021-12-03), only that back then we fixed
it for SSHUploadPack.

Fix this by both sending the error code to the client and returning an
error from the RPC call is case git-receive-pack(1) fails.

Changelog: fixed

2 files changed, 16 insertions, 6 deletions

diff --git a/internal/gitaly/service/ssh/receive_pack.go b/internal/gitaly/service/ssh/receive_pack.go
index 5f58d061d..e792f7518 100644
--- a/internal/gitaly/service/ssh/receive_pack.go
+++ b/internal/gitaly/service/ssh/receive_pack.go
@@ -86,10 +86,18 @@ func (s *server) sshReceivePack(stream gitalypb.SSHService_SSHReceivePackServer,
 	}
 
 	if err := cmd.Wait(); err != nil {
-		if status, ok := command.ExitStatus(err); ok {
-			return stream.Send(&gitalypb.SSHReceivePackResponse{
-				ExitStatus: &gitalypb.ExitStatus{Value: int32(status)},
-			})
+		status, ok := command.ExitStatus(err)
+		if !ok {
+			return fmt.Errorf("extracting exit status: %w", err)
+		}
+
+		// When the command has failed we both want to send its exit status as well as
+		// return an error from this RPC call. Otherwise we'd fail the RPC, but return with
+		// an `OK` error code to the client.
+		if errSend := stream.Send(&gitalypb.SSHReceivePackResponse{
+			ExitStatus: &gitalypb.ExitStatus{Value: int32(status)},
+		}); errSend != nil {
+			ctxlogrus.Extract(ctx).WithError(errSend).Error("send final status code")
 		}
 
 		return fmt.Errorf("cmd wait: %v", err)
diff --git a/internal/gitaly/service/ssh/receive_pack_test.go b/internal/gitaly/service/ssh/receive_pack_test.go
index 625d1abd3..433704284 100644
--- a/internal/gitaly/service/ssh/receive_pack_test.go
+++ b/internal/gitaly/service/ssh/receive_pack_test.go
@@ -211,6 +211,7 @@ func TestReceivePack_client(t *testing.T) {
 	for _, tc := range []struct {
 		desc              string
 		writeRequest      func(*testing.T, io.Writer)
+		expectedErr       error
 		expectedErrorCode int32
 		expectedStderr    string
 	}{
@@ -225,6 +226,7 @@ func TestReceivePack_client(t *testing.T) {
 			writeRequest: func(t *testing.T, stdin io.Writer) {
 				gittest.WritePktlineString(t, stdin, "garbage")
 			},
+			expectedErr:       helper.ErrInternalf("cmd wait: exit status 128"),
 			expectedErrorCode: 128,
 			expectedStderr:    "fatal: protocol error: expected old/new/ref, got 'garbage'\n",
 		},
@@ -233,6 +235,7 @@ func TestReceivePack_client(t *testing.T) {
 			writeRequest: func(t *testing.T, stdin io.Writer) {
 				gittest.WritePktlinef(t, stdin, "%[1]s %[1]s refs/heads/main", gittest.DefaultObjectHash.ZeroOID)
 			},
+			expectedErr:       helper.ErrInternalf("cmd wait: exit status 128"),
 			expectedErrorCode: 128,
 			expectedStderr:    "fatal: the remote end hung up unexpectedly\n",
 		},
@@ -274,8 +277,7 @@ func TestReceivePack_client(t *testing.T) {
 			tc.writeRequest(t, stdin)
 			require.NoError(t, stream.CloseSend())
 
-			// Even if the request has failed we still don't see any errors at all.
-			require.NoError(t, <-errCh)
+			testhelper.RequireGrpcError(t, <-errCh, tc.expectedErr)
 			require.Equal(t, tc.expectedErrorCode, observedErrorCode)
 			require.Equal(t, tc.expectedStderr, stderr.String())
 		})