ssh: Improve errors for fetches canceled by the userpks-ssh-upload-pack-canceled-fetches

In recent weeks we have started to see an uptick of incidents caused by
a single user that was repeatedly fetching from a specific repository
via the SSHUploadPackWithSidechannel RPC. The error has always been the
same:

    fatal: the remote hung up unexpectedly

This error is returned by git-upload-pack(1) in case it sees a short
read of data and ultimately indicates that the user has probably killed
git-fetch(1). This is not an actionable error on our side as it is a
totally expected outcome that a killed fetch will cause the fetch to
fail.

Now that we have a lot more tests in this area we can be reasonably sure
that short reads on an otherwise normally closed network connection is
seemingly the only condition that causes the above error. So let's put a
gRPC error code of `Canceled` on this error so that it doesn't cause our
alerts to go off anymore.

Changelog: fixed

2 files changed, 15 insertions, 1 deletions

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 a00424147..972a59080 100644
--- a/internal/gitaly/service/ssh/upload_pack_test.go
+++ b/internal/gitaly/service/ssh/upload_pack_test.go
@@ -311,7 +311,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",