From d9e514be723f38990968b9e44e5f8df85cb7a8ce Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 28 Jul 2022 16:31:58 +0200 Subject: ssh: Recognize cancelled pushes in SSHReceivePack Recently we've fixed cases where fetches which are cancelled by the user will be logged as `Internal` errors and thus easily impact our SLOs. This shortcoming has been fixed has been fixed via a2fee3dd5 (ssh: Improve errors for fetches canceled by the user, 2022-07-20), but now that we started to report errors in SSHReceivePack we are likely to hit the same issue there. Let's apply the same fix to SSHReceivePack and special-case this specific error. --- internal/gitaly/service/ssh/receive_pack.go | 15 +++++++++++++++ internal/gitaly/service/ssh/receive_pack_test.go | 2 +- 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/internal/gitaly/service/ssh/receive_pack.go b/internal/gitaly/service/ssh/receive_pack.go index faf913a3f..929898f8d 100644 --- a/internal/gitaly/service/ssh/receive_pack.go +++ b/internal/gitaly/service/ssh/receive_pack.go @@ -3,6 +3,8 @@ package ssh import ( "errors" "fmt" + "io" + "strings" "sync" "github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus" @@ -55,9 +57,15 @@ func (s *server) sshReceivePack(stream gitalypb.SSHService_SSHReceivePackServer, stdout := streamio.NewSyncWriter(&m, func(p []byte) error { return stream.Send(&gitalypb.SSHReceivePackResponse{Stdout: p}) }) + + // We both need to listen in on the stderr stream in order to be able to judge what exactly + // is happening, but also relay the output to the client. We thus create a MultiWriter to + // enable both at the same time. + var stderrBuilder strings.Builder stderr := streamio.NewSyncWriter(&m, func(p []byte) error { return stream.Send(&gitalypb.SSHReceivePackResponse{Stderr: p}) }) + stderr = io.MultiWriter(&stderrBuilder, stderr) repoPath, err := s.locator.GetRepoPath(req.Repository) if err != nil { @@ -100,6 +108,13 @@ func (s *server) sshReceivePack(stream gitalypb.SSHService_SSHReceivePackServer, ctxlogrus.Extract(ctx).WithError(errSend).Error("send final status code") } + // Detect the case where the user has cancelled the push and log it with a proper + // gRPC error code. We can't do anything about this error anyway and it is a totally + // valid outcome. + if stderrBuilder.String() == "fatal: the remote end hung up unexpectedly\n" { + return helper.ErrCanceledf("user canceled the push") + } + 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 072f5345f..0cd757de0 100644 --- a/internal/gitaly/service/ssh/receive_pack_test.go +++ b/internal/gitaly/service/ssh/receive_pack_test.go @@ -235,7 +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"), + expectedErr: helper.ErrCanceledf("user canceled the push"), expectedErrorCode: 128, expectedStderr: "fatal: the remote end hung up unexpectedly\n", }, -- cgit v1.2.3