ssh: Log error on git command failure

If the `git upload-pack` process forked by `SSHUploadPack` or
`git upload-archive` from `SSHUploadArchive` exits with a non-zero
return code, the result of executing `stream.Send` to respond to the
client will be returned, causing the request to be seen as successful
by Gitaly.

Clients will correctly receive the non-zero return code, but a status of
`OK` will be logged and the `grpc_server_handled_total` metric will
increment for `grpc_code=OK`.

This commit corrects `SSHUploadPack` and `SSHArchivePack` to return
an error when git fails.

Changelog: fixed

4 files changed, 47 insertions, 7 deletions

diff --git a/internal/git/localrepo/remote_extra_test.go b/internal/git/localrepo/remote_extra_test.go
index 2cfa720a0..acb91442d 100644
--- a/internal/git/localrepo/remote_extra_test.go
+++ b/internal/git/localrepo/remote_extra_test.go
@@ -132,7 +132,7 @@ func TestRepo_FetchInternal(t *testing.T) {
 		)
 		require.EqualError(t, err, "exit status 128")
 		require.IsType(t, err, localrepo.ErrFetchFailed{})
-		require.Equal(t, "fatal: couldn't find remote ref refs/does/not/exist\nfatal: the remote end hung up unexpectedly\n", stderr.String())
+		require.Contains(t, stderr.String(), "fatal: couldn't find remote ref refs/does/not/exist\nfatal: the remote end hung up unexpectedly\n")
 	})
 
 	t.Run("with env", func(t *testing.T) {
diff --git a/internal/gitaly/service/ssh/upload_archive.go b/internal/gitaly/service/ssh/upload_archive.go
index ea5de6984..9ca40558d 100644
--- a/internal/gitaly/service/ssh/upload_archive.go
+++ b/internal/gitaly/service/ssh/upload_archive.go
@@ -68,9 +68,12 @@ func (s *server) sshUploadArchive(stream gitalypb.SSHService_SSHUploadArchiveSer
 
 	if err := cmd.Wait(); err != nil {
 		if status, ok := command.ExitStatus(err); ok {
-			return stream.Send(&gitalypb.SSHUploadArchiveResponse{
+			if sendErr := stream.Send(&gitalypb.SSHUploadArchiveResponse{
 				ExitStatus: &gitalypb.ExitStatus{Value: int32(status)},
-			})
+			}); sendErr != nil {
+				return sendErr
+			}
+			return fmt.Errorf("SSHUploadPack: %v", err)
 		}
 		return fmt.Errorf("wait cmd: %v", err)
 	}
diff --git a/internal/gitaly/service/ssh/upload_pack.go b/internal/gitaly/service/ssh/upload_pack.go
index d2ac79a7a..656346d9f 100644
--- a/internal/gitaly/service/ssh/upload_pack.go
+++ b/internal/gitaly/service/ssh/upload_pack.go
@@ -125,9 +125,12 @@ func (s *server) sshUploadPack(stream gitalypb.SSHService_SSHUploadPackServer, r
 		wg.Wait()
 
 		if status, ok := command.ExitStatus(err); ok {
-			return stream.Send(&gitalypb.SSHUploadPackResponse{
+			if sendErr := stream.Send(&gitalypb.SSHUploadPackResponse{
 				ExitStatus: &gitalypb.ExitStatus{Value: int32(status)},
-			})
+			}); sendErr != nil {
+				return sendErr
+			}
+			return fmt.Errorf("SSHUploadPack: %v", err)
 		}
 		return fmt.Errorf("cmd wait: %v", err)
 	}
diff --git a/internal/gitaly/service/ssh/upload_pack_test.go b/internal/gitaly/service/ssh/upload_pack_test.go
index caa7bee00..599f85c13 100644
--- a/internal/gitaly/service/ssh/upload_pack_test.go
+++ b/internal/gitaly/service/ssh/upload_pack_test.go
@@ -3,7 +3,6 @@ package ssh
 import (
 	"bytes"
 	"fmt"
-	"io"
 	"os"
 	"os/exec"
 	"path/filepath"
@@ -153,7 +152,7 @@ func requireFailedSSHStream(t *testing.T, recv func() (int32, error)) {
 
 	select {
 	case <-done:
-		require.Equal(t, io.EOF, err)
+		testhelper.RequireGrpcCode(t, err, codes.Internal)
 		require.NotEqual(t, 0, code, "exit status")
 	case <-time.After(10 * time.Second):
 		t.Fatal("timeout waiting for SSH stream")
@@ -545,6 +544,41 @@ func TestUploadPackCloneFailure(t *testing.T) {
 	require.Error(t, err, "clone didn't fail")
 }
 
+func TestUploadPackCloneGitFailure(t *testing.T) {
+	cfg, repo, _ := testcfg.BuildWithRepo(t)
+
+	serverSocketPath := runSSHServer(t, cfg)
+
+	client, conn := newSSHClient(t, serverSocketPath)
+	defer conn.Close()
+
+	configPath := filepath.Join(cfg.Storages[0].Path, repo.RelativePath, "config")
+	gitconfig, err := os.Create(configPath)
+	require.NoError(t, err)
+
+	// Writing an invalid config will allow repo to pass the `IsGitDirectory` check but still
+	// trigger an error when git tries to access the repo.
+	_, err = gitconfig.WriteString("Not a valid git config")
+	require.NoError(t, err)
+
+	require.NoError(t, gitconfig.Close())
+
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+	stream, err := client.SSHUploadPack(ctx)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if err = stream.Send(&gitalypb.SSHUploadPackRequest{Repository: repo}); err != nil {
+		t.Fatal(err)
+	}
+	require.NoError(t, stream.CloseSend())
+
+	err = testPostUploadPackFailedResponse(t, stream)
+	testhelper.RequireGrpcCode(t, err, codes.Internal)
+}
+
 func testPostUploadPackFailedResponse(t *testing.T, stream gitalypb.SSHService_SSHUploadPackClient) error {
 	var err error
 	var res *gitalypb.SSHUploadPackResponse