diff options
| author | Patrick Steinhardt <psteinhardt@gitlab.com> | 2023-07-04 10:55:43 +0300 |
|---|---|---|
| committer | Patrick Steinhardt <psteinhardt@gitlab.com> | 2023-07-06 08:14:00 +0300 |
| commit | 236406e6177cc4199c084c7c21134d43ce94aeeb (patch) | |
| tree | aff78de9e14801348614b8f3b086066f767f5c15 | |
| parent | f0578c2dbb79ee9dc8c2ef33809521c7a9e3390d (diff) | |
ref: Change error code for locked refs in DeleteRefs to Aborted
When trying to delete a reference that is already locked we return a
structured error in DeleteRefs with `codes.FailedPrecondition`. This
fails the litmus test documented in `grpc/codes` though:
> A litmus test that may help a service implementor in deciding
> between FailedPrecondition, Aborted, and Unavailable:
>
> (a) Use Unavailable if the client can retry just the failing call.
> (b) Use Aborted if the client should retry at a higher-level
> (e.g., restarting a read-modify-write sequence).
> (c) Use FailedPrecondition if the client should not retry until
> the system state has been explicitly fixed. E.g., if an "rmdir"
> fails because the directory is non-empty, FailedPrecondition
> should be returned since the client should not retry unless
> they have first fixed up the directory by deleting files from it.
> (d) Use FailedPrecondition if the client performs conditional
> REST Get/Update/Delete on a resource and the resource on the
> server does not match the condition. E.g., conflicting
> read-modify-write on the same resource.
FailedPrecondition should be returned only when the client should not
retry, but given that we are looking at a locking issue that is usually
caused by concurrent calls holding the lock this is wrong. Instead,
Aborted is a better fit as a retry should typically work alright.
Refactor the code to do so.
| -rw-r--r-- | internal/gitaly/service/ref/delete_refs.go | 2 | ||||
| -rw-r--r-- | internal/gitaly/service/ref/delete_refs_test.go | 2 |
2 files changed, 2 insertions, 2 deletions
diff --git a/internal/gitaly/service/ref/delete_refs.go b/internal/gitaly/service/ref/delete_refs.go index 538521f0e..99d1b6eca 100644 --- a/internal/gitaly/service/ref/delete_refs.go +++ b/internal/gitaly/service/ref/delete_refs.go @@ -80,7 +80,7 @@ func (s *server) DeleteRefs(ctx context.Context, in *gitalypb.DeleteRefsRequest) if err := updater.Prepare(); err != nil { var alreadyLockedErr updateref.AlreadyLockedError if errors.As(err, &alreadyLockedErr) { - return nil, structerr.NewFailedPrecondition("cannot lock references").WithDetail( + return nil, structerr.NewAborted("cannot lock references").WithDetail( &gitalypb.DeleteRefsError{ Error: &gitalypb.DeleteRefsError_ReferencesLocked{ ReferencesLocked: &gitalypb.ReferencesLockedError{ diff --git a/internal/gitaly/service/ref/delete_refs_test.go b/internal/gitaly/service/ref/delete_refs_test.go index 7b5dda94f..4d87a8c90 100644 --- a/internal/gitaly/service/ref/delete_refs_test.go +++ b/internal/gitaly/service/ref/delete_refs_test.go @@ -237,7 +237,7 @@ func TestDeleteRefs_refLocked(t *testing.T) { response, err := client.DeleteRefs(ctx, request) require.Nil(t, response) - detailedErr := structerr.NewFailedPrecondition("cannot lock references").WithDetail( + detailedErr := structerr.NewAborted("cannot lock references").WithDetail( &gitalypb.DeleteRefsError{ Error: &gitalypb.DeleteRefsError_ReferencesLocked{ ReferencesLocked: &gitalypb.ReferencesLockedError{ |