Merge branch 'pks-ssh-upload-pack-canceled-fetches' into 'master'

ssh: Improve errors for fetches canceled by the user

Closes #4331

See merge request gitlab-org/gitaly!4731

6 files changed, 70 insertions, 2 deletions

diff --git a/internal/gitaly/service/ssh/testhelper_test.go b/internal/gitaly/service/ssh/testhelper_test.go
index 407c6dbe4..a23087122 100644
--- a/internal/gitaly/service/ssh/testhelper_test.go
+++ b/internal/gitaly/service/ssh/testhelper_test.go
@@ -25,7 +25,11 @@ func runSSHServer(t *testing.T, cfg config.Cfg, serverOpts ...testserver.GitalyS
 }
 
 func runSSHServerWithOptions(t *testing.T, cfg config.Cfg, opts []ServerOpt, serverOpts ...testserver.GitalyServerOpt) string {
-	return testserver.RunGitalyServer(t, cfg, nil, func(srv *grpc.Server, deps *service.Dependencies) {
+	return startSSHServerWithOptions(t, cfg, opts, serverOpts...).Address()
+}
+
+func startSSHServerWithOptions(t *testing.T, cfg config.Cfg, opts []ServerOpt, serverOpts ...testserver.GitalyServerOpt) testserver.GitalyServer {
+	return testserver.StartGitalyServer(t, cfg, nil, func(srv *grpc.Server, deps *service.Dependencies) {
 		gitalypb.RegisterSSHServiceServer(srv, NewServer(
 			deps.GetLocator(),
 			deps.GetGitCmdFactory(),
diff --git a/internal/gitaly/service/ssh/upload_pack.go b/internal/gitaly/service/ssh/upload_pack.go
index 89e67e9a9..3bca10976 100644
--- a/internal/gitaly/service/ssh/upload_pack.go
+++ b/internal/gitaly/service/ssh/upload_pack.go
@@ -151,6 +151,20 @@ func (s *server) sshUploadPack(ctx context.Context, req sshUploadPackRequest, st
 
 	if err := cmd.Wait(); err != nil {
 		status, _ := command.ExitStatus(err)
+
+		// A common error case is that the client is terminating the request prematurely,
+		// e.g. by killing their git-fetch(1) process because it's taking too long. This is
+		// an expected failure, but we're not in a position to easily tell this error apart
+		// from other errors returned by git-upload-pack(1). So we have to resort to parsing
+		// the error message returned by Git, and if we see that it matches we return an
+		// error with a `Canceled` error code.
+		//
+		// Note that we're being quite strict with how we match the error for now. We may
+		// have to make it more lenient in case we see that this doesn't catch all cases.
+		if stderrBuilder.String() == "fatal: the remote end hung up unexpectedly\n" {
+			return status, helper.ErrCanceledf("user canceled the fetch")
+		}
+
 		return status, fmt.Errorf("cmd wait: %w, stderr: %q", err, stderrBuilder.String())
 	}
 
diff --git a/internal/gitaly/service/ssh/upload_pack_test.go b/internal/gitaly/service/ssh/upload_pack_test.go
index 3739d2617..29c6d98ff 100644
--- a/internal/gitaly/service/ssh/upload_pack_test.go
+++ b/internal/gitaly/service/ssh/upload_pack_test.go
@@ -247,6 +247,42 @@ func TestUploadPackWithSidechannel_client(t *testing.T) {
 			),
 		},
 		{
+			desc: "request missing object",
+			request: &gitalypb.SSHUploadPackWithSidechannelRequest{
+				Repository: repo,
+			},
+			client: func(clientConn *sidechannel.ClientConn, _ func()) error {
+				gittest.WritePktlineString(t, clientConn, "want "+strings.Repeat("1", 40)+" multi_ack\n")
+				gittest.WritePktlineFlush(t, clientConn)
+				gittest.WritePktlineString(t, clientConn, "done\n")
+
+				require.NoError(t, clientConn.CloseWrite())
+
+				return nil
+			},
+			expectedErr: helper.ErrInternalf("cmd wait: exit status 128, stderr: %q",
+				"fatal: git upload-pack: not our ref "+strings.Repeat("1", 40)+"\n",
+			),
+		},
+		{
+			desc: "request invalidly formatted object",
+			request: &gitalypb.SSHUploadPackWithSidechannelRequest{
+				Repository: repo,
+			},
+			client: func(clientConn *sidechannel.ClientConn, _ func()) error {
+				gittest.WritePktlineString(t, clientConn, "want 1111 multi_ack\n")
+				gittest.WritePktlineFlush(t, clientConn)
+				gittest.WritePktlineString(t, clientConn, "done\n")
+
+				require.NoError(t, clientConn.CloseWrite())
+
+				return nil
+			},
+			expectedErr: helper.ErrInternalf("cmd wait: exit status 128, stderr: %q",
+				"fatal: git upload-pack: protocol error, expected to get object ID, not 'want 1111 multi_ack'\n",
+			),
+		},
+		{
 			desc: "missing input",
 			request: &gitalypb.SSHUploadPackWithSidechannelRequest{
 				Repository:  repo,
@@ -273,7 +309,7 @@ func TestUploadPackWithSidechannel_client(t *testing.T) {
 
 				return nil
 			},
-			expectedErr: helper.ErrInternalf("cmd wait: exit status 128, stderr: %q", "fatal: the remote end hung up unexpectedly\n"),
+			expectedErr: helper.ErrCanceledf("user canceled the fetch"),
 		},
 		{
 			desc: "garbage",
diff --git a/internal/helper/error.go b/internal/helper/error.go
index e649e5231..517fb1d70 100644
--- a/internal/helper/error.go
+++ b/internal/helper/error.go
@@ -60,6 +60,12 @@ func wrapError(code codes.Code, err error) error {
 	return err
 }
 
+// ErrCanceledf wraps a formatted error with codes.Canceled, unless the formatted error is a
+// wrapped gRPC error.
+func ErrCanceledf(format string, a ...interface{}) error {
+	return formatError(codes.Canceled, format, a...)
+}
+
 // ErrInternalf wraps a formatted error with codes.Internal, unless the formatted error is a
 // wrapped gRPC error.
 func ErrInternalf(format string, a ...interface{}) error {
diff --git a/internal/helper/error_test.go b/internal/helper/error_test.go
index 047aa6853..3de56385d 100644
--- a/internal/helper/error_test.go
+++ b/internal/helper/error_test.go
@@ -99,6 +99,11 @@ func TestErrorf(t *testing.T) {
 		expectedCode codes.Code
 	}{
 		{
+			desc:         "Canceledf",
+			errorf:       ErrCanceledf,
+			expectedCode: codes.Canceled,
+		},
+		{
 			desc:         "Internalf",
 			errorf:       ErrInternalf,
 			expectedCode: codes.Internal,
diff --git a/internal/testhelper/testserver/gitaly.go b/internal/testhelper/testserver/gitaly.go
index 2e8df3a60..1d90fabfe 100644
--- a/internal/testhelper/testserver/gitaly.go
+++ b/internal/testhelper/testserver/gitaly.go
@@ -54,6 +54,7 @@ func StartGitalyServer(t testing.TB, cfg config.Cfg, rubyServer *rubyserver.Serv
 
 	if !testhelper.IsPraefectEnabled() || disablePraefect {
 		return GitalyServer{
+			Server:   gitalySrv,
 			shutdown: gitalySrv.Stop,
 			address:  gitalyAddr,
 		}
@@ -61,6 +62,7 @@ func StartGitalyServer(t testing.TB, cfg config.Cfg, rubyServer *rubyserver.Serv
 
 	praefectServer := runPraefectProxy(t, cfg, gitalyAddr)
 	return GitalyServer{
+		Server: gitalySrv,
 		shutdown: func() {
 			praefectServer.Shutdown()
 			gitalySrv.Stop()
@@ -106,6 +108,7 @@ func runPraefectProxy(t testing.TB, gitalyCfg config.Cfg, gitalyAddr string) Pra
 // GitalyServer is a helper that carries additional info and
 // functionality about gitaly (+praefect) server.
 type GitalyServer struct {
+	Server   *grpc.Server
 	shutdown func()
 	address  string
 }