Merge branch 'pks-structerr-unknown-by-default' into 'master'

structerr: Convert `New()` to return `Unknown` error codes

See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5521

Merged-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Approved-by: karthik nayak <knayak@gitlab.com>
Approved-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Co-authored-by: Patrick Steinhardt <psteinhardt@gitlab.com>

13 files changed, 65 insertions, 55 deletions

diff --git a/STYLE.md b/STYLE.md
index 7f6c8c70b..81132e77c 100644
--- a/STYLE.md
+++ b/STYLE.md
@@ -88,6 +88,11 @@ func CheckFrobnicated(s string) error {
 }
 ```
 
+Unless the root cause of an error can be determined and categorized, you should
+use `structerr.New()` to generate an error with an `Unknown` error code. This
+code will automatically get overridden when the error is wrapped with a specific
+error code.
+
 ### Error wrapping
 
 You should use wrapping directives `"%w"` to wrap errors. This
diff --git a/internal/gitaly/service/diff/patch_id_test.go b/internal/gitaly/service/diff/patch_id_test.go
index 386ad45c9..ddf29bc9b 100644
--- a/internal/gitaly/service/diff/patch_id_test.go
+++ b/internal/gitaly/service/diff/patch_id_test.go
@@ -240,7 +240,7 @@ func TestGetPatchID(t *testing.T) {
 						NewRevision: []byte(fmt.Sprintf("%s:file", newCommit)),
 					},
 					//nolint:gitaly-linters
-					expectedErr: structerr.New("waiting for git-diff: exit status 128").
+					expectedErr: structerr.NewInternal("waiting for git-diff: exit status 128").
 						WithInterceptedMetadata("stderr", fmt.Sprintf("fatal: path 'file' does not exist in '%s'\n", oldCommit)),
 				}
 			},
@@ -258,7 +258,7 @@ func TestGetPatchID(t *testing.T) {
 						OldRevision: []byte(gittest.DefaultObjectHash.ZeroOID),
 						NewRevision: []byte(newRevision),
 					},
-					expectedErr: structerr.New("waiting for git-diff: exit status 128").
+					expectedErr: structerr.NewInternal("waiting for git-diff: exit status 128").
 						WithInterceptedMetadata("stderr", fmt.Sprintf("fatal: bad object %s\n", gittest.DefaultObjectHash.ZeroOID)),
 				}
 			},
diff --git a/internal/gitaly/service/operations/tags.go b/internal/gitaly/service/operations/tags.go
index 437f33d67..caa14794f 100644
--- a/internal/gitaly/service/operations/tags.go
+++ b/internal/gitaly/service/operations/tags.go
@@ -281,9 +281,9 @@ func (s *Server) createTag(
 	if makingTag {
 		tagObjectID, err := repo.WriteTag(ctx, targetObjectID, targetObjectType, tagName, message, committer, committerTime)
 		if err != nil {
-			var FormatTagError localrepo.FormatTagError
-			if errors.As(err, &FormatTagError) {
-				return nil, "", structerr.NewUnknown("Rugged::InvalidError: failed to parse signature - expected prefix doesn't match actual")
+			var formatTagErr localrepo.FormatTagError
+			if errors.As(err, &formatTagErr) {
+				return nil, "", structerr.NewInvalidArgument("formatting tag: %w", formatTagErr)
 			}
 
 			var MktagError localrepo.MktagError
diff --git a/internal/gitaly/service/remote/find_remote_root_ref_test.go b/internal/gitaly/service/remote/find_remote_root_ref_test.go
index 42e20dcfb..196a8fabc 100644
--- a/internal/gitaly/service/remote/find_remote_root_ref_test.go
+++ b/internal/gitaly/service/remote/find_remote_root_ref_test.go
@@ -125,7 +125,7 @@ func TestFindRemoteRootRef(t *testing.T) {
 						Repository: localRepo,
 						RemoteUrl:  "file://" + testhelper.TempDir(t),
 					},
-					expectedErr: structerr.New("exit status 128"),
+					expectedErr: structerr.NewInternal("exit status 128"),
 				}
 			},
 		},
diff --git a/internal/gitaly/service/repository/apply_gitattributes_test.go b/internal/gitaly/service/repository/apply_gitattributes_test.go
index 227d8e36e..a3ab4aca8 100644
--- a/internal/gitaly/service/repository/apply_gitattributes_test.go
+++ b/internal/gitaly/service/repository/apply_gitattributes_test.go
@@ -3,7 +3,6 @@ package repository
 import (
 	"bytes"
 	"context"
-	"errors"
 	"os"
 	"path/filepath"
 	"testing"
@@ -164,11 +163,11 @@ func TestApplyGitattributes_transactional(t *testing.T) {
 			desc:     "failing vote does not write gitattributes",
 			revision: []byte(commitWithGitattributes),
 			voteFn: func(t *testing.T, request *gitalypb.VoteTransactionRequest) (*gitalypb.VoteTransactionResponse, error) {
-				return nil, errors.New("foobar")
+				return nil, structerr.NewFailedPrecondition("foobar")
 			},
 			shouldExist: false,
 			expectedErr: func() error {
-				return structerr.NewUnknown("committing gitattributes: voting on locked file: preimage vote: rpc error: code = Unknown desc = foobar")
+				return structerr.NewFailedPrecondition("committing gitattributes: voting on locked file: preimage vote: rpc error: code = FailedPrecondition desc = foobar")
 			}(),
 			expectedVotes: 1,
 		},
diff --git a/internal/gitaly/service/repository/fetch_remote_test.go b/internal/gitaly/service/repository/fetch_remote_test.go
index 3ae37edca..01bb7feb5 100644
--- a/internal/gitaly/service/repository/fetch_remote_test.go
+++ b/internal/gitaly/service/repository/fetch_remote_test.go
@@ -641,7 +641,7 @@ func TestFetchRemote(t *testing.T) {
 								"refs/heads/main":   localDivergingID,
 								"refs/heads/branch": remoteUpdatedID,
 							},
-							expectedErr: structerr.New("fetch remote: exit status 1"),
+							expectedErr: structerr.NewInternal("fetch remote: exit status 1"),
 						},
 					},
 				}
@@ -678,7 +678,7 @@ func TestFetchRemote(t *testing.T) {
 								"refs/heads/main":  localDivergingID,
 								"refs/tags/v1.0.0": remoteTagID,
 							},
-							expectedErr: structerr.New("fetch remote: exit status 1"),
+							expectedErr: structerr.NewInternal("fetch remote: exit status 1"),
 						},
 					},
 				}
diff --git a/internal/gitaly/service/repository/object_format_test.go b/internal/gitaly/service/repository/object_format_test.go
index 80d41c36f..83053c899 100644
--- a/internal/gitaly/service/repository/object_format_test.go
+++ b/internal/gitaly/service/repository/object_format_test.go
@@ -151,7 +151,7 @@ func TestObjectFormat(t *testing.T) {
 					request: &gitalypb.ObjectFormatRequest{
 						Repository: repoProto,
 					},
-					expectedErr: structerr.New("detecting object hash: reading object format: exit status 128").WithInterceptedMetadata(
+					expectedErr: structerr.NewInternal("detecting object hash: reading object format: exit status 128").WithInterceptedMetadata(
 						"stderr",
 						fmt.Sprintf("error: invalid value for 'extensions.objectformat': 'blake2b'\n"+
 							"fatal: bad config line 5 in file %s\n", filepath.Join(repoPath, "config"),
diff --git a/internal/praefect/remove_all_test.go b/internal/praefect/remove_all_test.go
index 7b7d43586..add4fc3f3 100644
--- a/internal/praefect/remove_all_test.go
+++ b/internal/praefect/remove_all_test.go
@@ -15,15 +15,14 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/praefect/datastore"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/praefect/grpc-proxy/proxy"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/praefect/protoregistry"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/structerr"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testcfg"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testdb"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testserver"
 	"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
 	"google.golang.org/grpc"
-	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/credentials/insecure"
-	"google.golang.org/grpc/status"
 )
 
 func TestRemoveAllHandler(t *testing.T) {
@@ -32,7 +31,7 @@ func TestRemoveAllHandler(t *testing.T) {
 
 	const virtualStorage, relativePath = "virtual-storage", "relative-path"
 
-	errServedByGitaly := status.Error(codes.Unknown, "request passed to Gitaly")
+	errServedByGitaly := structerr.NewInternal("request passed to Gitaly")
 	db := testdb.New(t)
 
 	const gitaly1Storage = "gitaly-1"
diff --git a/internal/praefect/remove_repository_test.go b/internal/praefect/remove_repository_test.go
index 7bdce8a76..2d5c4d34d 100644
--- a/internal/praefect/remove_repository_test.go
+++ b/internal/praefect/remove_repository_test.go
@@ -22,16 +22,14 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testserver"
 	"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
 	"google.golang.org/grpc"
-	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/credentials/insecure"
-	"google.golang.org/grpc/status"
 )
 
 func TestRemoveRepositoryHandler(t *testing.T) {
 	t.Parallel()
 	ctx := testhelper.Context(t)
 
-	errServedByGitaly := status.Error(codes.Unknown, "request passed to Gitaly")
+	errServedByGitaly := structerr.NewInternal("request passed to Gitaly")
 	const virtualStorage, relativePath = "virtual-storage", "relative-path"
 
 	db := testdb.New(t)
diff --git a/internal/praefect/repository_exists_test.go b/internal/praefect/repository_exists_test.go
index ba005cc84..29f919c6b 100644
--- a/internal/praefect/repository_exists_test.go
+++ b/internal/praefect/repository_exists_test.go
@@ -13,18 +13,18 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/praefect/datastore"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/praefect/grpc-proxy/proxy"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/praefect/protoregistry"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/structerr"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testdb"
 	"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
 	"google.golang.org/grpc"
-	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/credentials/insecure"
-	"google.golang.org/grpc/status"
 )
 
 func TestRepositoryExistsHandler(t *testing.T) {
 	t.Parallel()
-	errServedByGitaly := status.Error(codes.Unknown, "request passed to Gitaly")
+
+	errServedByGitaly := structerr.NewInternal("request passed to Gitaly")
 
 	db := testdb.New(t)
 	for _, tc := range []struct {
diff --git a/internal/structerr/error.go b/internal/structerr/error.go
index fdaf132df..752075661 100644
--- a/internal/structerr/error.go
+++ b/internal/structerr/error.go
@@ -81,10 +81,17 @@ func newError(code codes.Code, format string, a ...any) Error {
 	formattedErr := fmt.Errorf(format, a...)
 
 	// When we wrap an Error, we retain its error code. The intent of this is to retain the most
-	// specific error code we have in the general case.
+	// specific error code we have in the general case. As `Unknown` does not really count as a
+	// specific error code, we will ignore these errors.
+	//
+	// Note that this impacts our middleware status handler, where we wrap non-context-errors
+	// via `structerr.NewInternal()`. The result is that the caller should never see any
+	// `Unknown` errors.
 	var wrappedErr Error
 	if errors.As(formattedErr, &wrappedErr) {
-		code = wrappedErr.code
+		if wrappedErr.code != codes.Unknown {
+			code = wrappedErr.code
+		}
 	}
 
 	return Error{
@@ -93,11 +100,12 @@ func newError(code codes.Code, format string, a ...any) Error {
 	}
 }
 
-// New returns a new Error with the default error code, which is Internal. When this function is
-// used to wrap another Error, then the error code of that wrapped Error will be retained. The
-// intent of this is to always retain the most specific error code in the general case.
+// New returns a new Error with an Unknown error code. This constructor should be used in the
+// general case where it is not clear what the specific error category is. As Unknown errors get
+// treated specially, they will be overridden when wrapped with an error that has a more specific
+// error code.
 func New(format string, a ...any) Error {
-	return newError(codes.Internal, format, a...)
+	return newError(codes.Unknown, format, a...)
 }
 
 // NewAborted constructs a new error code with the Aborted error code. Please refer to New for
@@ -184,12 +192,6 @@ func NewUnimplemented(format string, a ...any) Error {
 	return newError(codes.Unimplemented, format, a...)
 }
 
-// NewUnknown constructs a new error code with the Unknown error code. Please refer to New for
-// further details.
-func NewUnknown(format string, a ...any) Error {
-	return newError(codes.Unknown, format, a...)
-}
-
 // Error returns the error message of the Error.
 func (e Error) Error() string {
 	return e.err.Error()
diff --git a/internal/structerr/error_test.go b/internal/structerr/error_test.go
index 5b87cd98c..0cd2638db 100644
--- a/internal/structerr/error_test.go
+++ b/internal/structerr/error_test.go
@@ -30,7 +30,7 @@ func TestNew(t *testing.T) {
 		{
 			desc:         "New",
 			constructor:  New,
-			expectedCode: codes.Internal,
+			expectedCode: codes.Unknown,
 		},
 		{
 			desc:         "NewAborted",
@@ -102,11 +102,6 @@ func TestNew(t *testing.T) {
 			constructor:  NewUnimplemented,
 			expectedCode: codes.Unimplemented,
 		},
-		{
-			desc:         "NewUnknown",
-			constructor:  NewUnknown,
-			expectedCode: codes.Unknown,
-		},
 	} {
 		t.Run(tc.desc, func(t *testing.T) {
 			require.NotEqual(t, tc.expectedCode, unusedErrorCode)
@@ -153,6 +148,18 @@ func TestNew(t *testing.T) {
 				require.Equal(t, status.New(unusedErrorCode, "top-level: nested"), s)
 			})
 
+			t.Run("wrapping structerr with Unknown error", func(t *testing.T) {
+				err := tc.constructor("top-level: %w", newError(codes.Unknown, "unknown"))
+				require.EqualError(t, err, "top-level: unknown")
+				// We should be overriding the Unknown error code with the error
+				// code of the top-level error.
+				require.Equal(t, tc.expectedCode, status.Code(err))
+
+				s, ok := status.FromError(err)
+				require.True(t, ok)
+				require.Equal(t, status.New(tc.expectedCode, "top-level: unknown"), s)
+			})
+
 			t.Run("wrapping status.Error", func(t *testing.T) {
 				err := tc.constructor("top-level: %w", status.Error(unusedErrorCode, "nested"))
 				require.EqualError(t, err, "top-level: nested")
@@ -323,7 +330,7 @@ func TestError_Metadata(t *testing.T) {
 		err := New("message")
 		require.Equal(t, Error{
 			err:  errors.New("message"),
-			code: codes.Internal,
+			code: codes.Unknown,
 		}, err)
 		require.Equal(t, map[string]any{}, err.Metadata())
 	})
@@ -332,7 +339,7 @@ func TestError_Metadata(t *testing.T) {
 		err := New("message").WithMetadata("key", "value")
 		require.Equal(t, Error{
 			err:  errors.New("message"),
-			code: codes.Internal,
+			code: codes.Unknown,
 			metadata: []metadataItem{
 				{key: "key", value: "value"},
 			},
@@ -346,7 +353,7 @@ func TestError_Metadata(t *testing.T) {
 		err := New("message").WithMetadata("first", 1).WithMetadata("second", 2)
 		require.Equal(t, Error{
 			err:  errors.New("message"),
-			code: codes.Internal,
+			code: codes.Unknown,
 			metadata: []metadataItem{
 				{key: "first", value: 1},
 				{key: "second", value: 2},
@@ -362,7 +369,7 @@ func TestError_Metadata(t *testing.T) {
 		err := New("message").WithMetadata("first", "initial").WithMetadata("first", "overridden")
 		require.Equal(t, Error{
 			err:  errors.New("message"),
-			code: codes.Internal,
+			code: codes.Unknown,
 			metadata: []metadataItem{
 				{key: "first", value: "overridden"},
 			},
@@ -377,7 +384,7 @@ func TestError_Metadata(t *testing.T) {
 		toplevelErr := New("top-level: %w", nestedErr).WithMetadata("toplevel", "value")
 		require.Equal(t, Error{
 			err:  fmt.Errorf("top-level: %w", nestedErr),
-			code: codes.Internal,
+			code: codes.Unknown,
 			metadata: []metadataItem{
 				{key: "toplevel", value: "value"},
 			},
@@ -393,7 +400,7 @@ func TestError_Metadata(t *testing.T) {
 		toplevelErr := New("top-level: %w", nestedErr).WithMetadata("key", "top-level")
 		require.Equal(t, Error{
 			err:  fmt.Errorf("top-level: %w", nestedErr),
-			code: codes.Internal,
+			code: codes.Unknown,
 			metadata: []metadataItem{
 				{key: "key", value: "top-level"},
 			},
@@ -408,7 +415,7 @@ func TestError_Metadata(t *testing.T) {
 		toplevelErr := New("top-level: %w", nestedErr).WithMetadata("toplevel", "initial").WithMetadata("toplevel", "overridden")
 		require.Equal(t, Error{
 			err:  fmt.Errorf("top-level: %w", nestedErr),
-			code: codes.Internal,
+			code: codes.Unknown,
 			metadata: []metadataItem{
 				{key: "toplevel", value: "overridden"},
 			},
@@ -426,7 +433,7 @@ func TestError_Metadata(t *testing.T) {
 
 		require.Equal(t, Error{
 			err:  fmt.Errorf("top: %w", midlevelErr),
-			code: codes.Internal,
+			code: codes.Unknown,
 			metadata: []metadataItem{
 				{key: "toplevel", value: "value"},
 			},
@@ -462,7 +469,7 @@ func TestError_Details(t *testing.T) {
 			},
 			expectedErr: Error{
 				err:  errors.New("message"),
-				code: codes.Internal,
+				code: codes.Unknown,
 			},
 			expectedMessage: "message",
 		},
@@ -473,7 +480,7 @@ func TestError_Details(t *testing.T) {
 			},
 			expectedErr: Error{
 				err:  errors.New("message"),
-				code: codes.Internal,
+				code: codes.Unknown,
 				details: []proto.Message{
 					initialPayload,
 				},
@@ -490,7 +497,7 @@ func TestError_Details(t *testing.T) {
 			},
 			expectedErr: Error{
 				err:  errors.New("message"),
-				code: codes.Internal,
+				code: codes.Unknown,
 				details: []proto.Message{
 					initialPayload,
 					overridingPayload,
@@ -510,7 +517,7 @@ func TestError_Details(t *testing.T) {
 			},
 			expectedErr: Error{
 				err:  fmt.Errorf("top-level: %w", New("nested").WithDetail(initialPayload)),
-				code: codes.Internal,
+				code: codes.Unknown,
 				details: []proto.Message{
 					overridingPayload,
 				},
@@ -536,7 +543,7 @@ func TestError_Details(t *testing.T) {
 
 			s, ok := status.FromError(err)
 			require.True(t, ok)
-			require.Equal(t, codes.Internal, s.Code())
+			require.Equal(t, codes.Unknown, s.Code())
 			require.Equal(t, tc.expectedMessage, s.Message())
 			testhelper.ProtoEqual(t, anyDetails, s.Details())
 		})
diff --git a/internal/structerr/grpc_server_test.go b/internal/structerr/grpc_server_test.go
index b9c48d6b9..6d2bb39b3 100644
--- a/internal/structerr/grpc_server_test.go
+++ b/internal/structerr/grpc_server_test.go
@@ -139,12 +139,12 @@ func TestFieldsProducer(t *testing.T) {
 		{
 			desc:        "structured error",
 			returnedErr: New("message"),
-			expectedErr: status.Error(codes.Internal, "message"),
+			expectedErr: status.Error(codes.Unknown, "message"),
 		},
 		{
 			desc:        "structured error with metadata",
 			returnedErr: New("message").WithMetadata("key", "value"),
-			expectedErr: status.Error(codes.Internal, "message"),
+			expectedErr: status.Error(codes.Unknown, "message"),
 			expectedMetadata: []map[string]any{
 				{
 					"key": "value",
@@ -154,7 +154,7 @@ func TestFieldsProducer(t *testing.T) {
 		{
 			desc:        "structured error with nested metadata",
 			returnedErr: New("message: %w", New("nested").WithMetadata("nested", "value")).WithMetadata("key", "value"),
-			expectedErr: status.Error(codes.Internal, "message: nested"),
+			expectedErr: status.Error(codes.Unknown, "message: nested"),
 			expectedMetadata: []map[string]any{
 				{
 					"key":    "value",