Merge pull request #7076 from dolthub/aaron/fix-some-spurious-retries…

…-in-remotestorage [no-release-notes] go/libraries/doltcore/remotestorage: Fix some spurious retries on authentication failures and the likes when interacting with remotestorage.
dolthub · Dec 1, 2023 · 8f0818c · 8f0818c
2 parents a2a2e90 + 5cfc7e4
commit 8f0818c
Show file tree

Hide file tree

Showing 4 changed files with 22 additions and 13 deletions.
diff --git a/go/libraries/doltcore/remotesrv/interceptors.go b/go/libraries/doltcore/remotesrv/interceptors.go
@@ -79,15 +79,13 @@ func (si *ServerInterceptor) authenticate(ctx context.Context) error {
 	ctx, err := si.AccessController.ApiAuthenticate(ctx)
 	if err != nil {
 		si.Lgr.Warnf("authentication failed: %s", err.Error())
-		status.Error(codes.Unauthenticated, "unauthenticated")
-		return err
+		return status.Error(codes.Unauthenticated, err.Error())
 	}
 
 	// Have a valid user in the context.  Check authorization.
 	if authorized, err := si.AccessController.ApiAuthorize(ctx); !authorized {
 		si.Lgr.Warnf("authorization failed: %s", err.Error())
-		status.Error(codes.PermissionDenied, "unauthorized")
-		return err
+		return status.Error(codes.PermissionDenied, err.Error())
 	}
 
 	// Access Granted.

diff --git a/go/libraries/doltcore/remotestorage/chunk_store.go b/go/libraries/doltcore/remotestorage/chunk_store.go
@@ -1015,9 +1015,11 @@ func (dcs *DoltChunkStore) uploadTableFileWithRetries(ctx context.Context, table
 		req := &remotesapi.GetUploadLocsRequest{RepoId: id, RepoToken: token, RepoPath: dcs.repoPath, TableFileDetails: []*remotesapi.TableFileDetails{tbfd}}
 		resp, err := dcs.csClient.GetUploadLocations(ctx, req)
 		if err != nil {
-			if err != nil {
-				return NewRpcError(err, "GetUploadLocations", dcs.host, req)
+			err := NewRpcError(err, "GetUploadLocations", dcs.host, req)
+			if err.IsPermanent() {
+				return backoff.Permanent(err)
 			}
+			return err
 		}
 
 		if resp.RepoToken != "" {

diff --git a/go/libraries/doltcore/remotestorage/error.go b/go/libraries/doltcore/remotestorage/error.go
@@ -38,6 +38,10 @@ func (rpce *RpcError) Error() string {
 	return rpce.originalErrMsg
 }
 
+func (rpce *RpcError) IsPermanent() bool {
+	return statusCodeIsPermanentError(rpce.status)
+}
+
 func (rpce *RpcError) FullDetails() string {
 	jsonStr, _ := GetJsonEncodedRequest(rpce)
 	return rpce.originalErrMsg + "\nhost:" + rpce.host + "\nrpc: " + rpce.rpc + "\nparams:" + jsonStr

diff --git a/go/libraries/doltcore/remotestorage/retry.go b/go/libraries/doltcore/remotestorage/retry.go
@@ -46,12 +46,18 @@ func processHttpResp(resp *http.Response, err error) error {
 
 // ProcessGrpcErr converts an error from a Grpc call into a RetriableCallState
 func processGrpcErr(err error) error {
-	st, ok := status.FromError(err)
-	if !ok {
-		return err
+	st, _ := status.FromError(err)
+	if statusCodeIsPermanentError(st) {
+		return backoff.Permanent(err)
 	}
+	return err
+}
 
-	switch st.Code() {
+func statusCodeIsPermanentError(s *status.Status) bool {
+	if s == nil {
+		return false
+	}
+	switch s.Code() {
 	case codes.InvalidArgument,
 		codes.NotFound,
 		codes.AlreadyExists,
@@ -60,8 +66,7 @@ func processGrpcErr(err error) error {
 		codes.Unimplemented,
 		codes.OutOfRange,
 		codes.Unauthenticated:
-		return backoff.Permanent(err)
+		return true
 	}
-
-	return err
+	return false
 }