Moving retryable-err checks to errors.As, moving some to not-retryable (#1167)

Part 1 of 2 for solving retry storms, particularly around incorrectly-categorized
errors (e.g. limit exceeded) and service-busy.

This PR moves us to `errors.As` to support wrapped errors in the future, and
re-categorizes some incorrectly-retried errors. This is both useful on its own,
and helps make #1174 a smaller and clearer change.

Service-busy behavior is actually changed in #1174, this commit intentionally
maintains its current (flawed) behavior.

Author: Groxx

internal/internal_retry.go

@@ -24,6 +24,7 @@ package internal
 
 import (
 	"context"
+	"errors"
 	"time"
 
 	s "go.uber.org/cadence/.gen/go/shared"
@@ -62,26 +63,59 @@ func createDynamicServiceRetryPolicy(ctx context.Context) backoff.RetryPolicy {
 }
 
 func isServiceTransientError(err error) bool {
-	// Retrying by default so it covers all transport errors.
-	switch err.(type) {
-	case *s.BadRequestError,
-		*s.EntityNotExistsError,
-		*s.WorkflowExecutionAlreadyStartedError,
-		*s.WorkflowExecutionAlreadyCompletedError,
-		*s.DomainAlreadyExistsError,
-		*s.QueryFailedError,
-		*s.DomainNotActiveError,
-		*s.CancellationAlreadyRequestedError,
-		*s.ClientVersionNotSupportedError,
-		*s.LimitExceededError:
+	// check intentionally-not-retried error types via errors.As.
+	//
+	// sadly we cannot build this into a list of error values / types to range over, as that
+	// would turn the variable passed as the &target into e.g. an interface{} or an error,
+	// which is compatible with ALL errors, so all are .As(err, &target).
+	//
+	// so we're left with this mess.  it's not even generics-friendly.
+	// at least this pattern lets it be done inline without exposing the variable.
+	if target := (*s.AccessDeniedError)(nil); errors.As(err, &target) {
+		return false
+	}
+	if target := (*s.BadRequestError)(nil); errors.As(err, &target) {
+		return false
+	}
+	if target := (*s.CancellationAlreadyRequestedError)(nil); errors.As(err, &target) {
+		return false
+	}
+	if target := (*s.ClientVersionNotSupportedError)(nil); errors.As(err, &target) {
+		return false
+	}
+	if target := (*s.DomainAlreadyExistsError)(nil); errors.As(err, &target) {
+		return false
+	}
+	if target := (*s.DomainNotActiveError)(nil); errors.As(err, &target) {
+		return false
+	}
+	if target := (*s.EntityNotExistsError)(nil); errors.As(err, &target) {
+		return false
+	}
+	if target := (*s.FeatureNotEnabledError)(nil); errors.As(err, &target) {
+		return false
+	}
+	if target := (*s.LimitExceededError)(nil); errors.As(err, &target) {
+		return false
+	}
+	if target := (*s.QueryFailedError)(nil); errors.As(err, &target) {
+		return false
+	}
+	if target := (*s.WorkflowExecutionAlreadyCompletedError)(nil); errors.As(err, &target) {
+		return false
+	}
+	if target := (*s.WorkflowExecutionAlreadyStartedError)(nil); errors.As(err, &target) {
 		return false
 	}
 
-	if err == errShutdown {
+	// shutdowns are not retryable, of course
+	if errors.Is(err, errShutdown) {
 		return false
 	}
 
 	// s.InternalServiceError
-	// s.ServiceBusyError
+	// s.ServiceBusyError (must retry after a delay, but it is transient)
+	// server-side-only error types (as they should not reach clients)
+	// and all other `error` types
 	return true
 }

internal/internal_retry_test.go

@@ -0,0 +1,53 @@
+package internal
+
+import (
+	"errors"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	s "go.uber.org/cadence/.gen/go/shared"
+)
+
+func TestErrRetries(t *testing.T) {
+	t.Run("retryable", func(t *testing.T) {
+		for _, err := range []error{
+			// service-busy means "retry later", which is still transient/retryable.
+			// callers with retries MUST detect this separately and delay before retrying,
+			// and ideally we'll return a minimum time-to-wait in errors in the future.
+			&s.ServiceBusyError{},
+
+			&s.InternalServiceError{},  // fallback error type from server, "unknown" = "retry might work"
+			errors.New("unrecognized"), // fallback error type elsewhere, "unknown" = "retry might work"
+
+			// below are all server-side internal errors and should never be exposed, they're just included
+			// to be explicit about current behavior (without intentionally deciding on any of them).
+			// retry by default, like any other unknown error.  it shouldn't lead to incorrect behavior.
+			&s.CurrentBranchChangedError{},
+			&s.RetryTaskV2Error{},
+			&s.RemoteSyncMatchedError{},
+			&s.InternalDataInconsistencyError{},
+		} {
+			assert.True(t, isServiceTransientError(err), "%T should be transient", err)
+		}
+	})
+	t.Run("terminal", func(t *testing.T) {
+		for _, err := range []error{
+			&s.AccessDeniedError{},                      // access won't be granted immediately
+			&s.BadRequestError{},                        // bad requests don't become good
+			&s.CancellationAlreadyRequestedError{},      // can only cancel once
+			&s.ClientVersionNotSupportedError{},         // clients don't magically upgrade
+			&s.DomainAlreadyExistsError{},               // re-creating again won't work
+			&s.DomainNotActiveError{},                   // may actually succeed, but very unlikely to work immediately
+			&s.EntityNotExistsError{},                   // may actually succeed, but very unlikely to work immediately
+			&s.FeatureNotEnabledError{},                 // features won't magically enable
+			&s.LimitExceededError{},                     // adding +1 to a value does not move it below its limit
+			&s.QueryFailedError{},                       // arguable, this could be considered "unknown" and retryable
+			&s.WorkflowExecutionAlreadyCompletedError{}, // completed workflows won't uncomplete
+			&s.WorkflowExecutionAlreadyStartedError{},   // started workflows could complete quickly, but re-starting may not be desirable
+
+			errShutdown, // shutdowns can't be stopped
+		} {
+			assert.False(t, isServiceTransientError(err), "%T should be fatal", err)
+		}
+	})
+}

internal/workflow_shadower_worker_test.go

@@ -149,7 +149,7 @@ func (s *shadowWorkerSuite) TestStartShadowWorker_Failed_StartWorkflowError() {
 		Name: common.StringPtr(testDomain),
 	}, callOptions()...).Return(&shared.DescribeDomainResponse{}, nil).Times(1)
 	// first return a retryable error to check if retry policy is configured
-	s.mockService.EXPECT().StartWorkflowExecution(gomock.Any(), gomock.Any(), callOptions()...).Return(nil, &shared.ServiceBusyError{}).Times(1)
+	s.mockService.EXPECT().StartWorkflowExecution(gomock.Any(), gomock.Any(), callOptions()...).Return(nil, &shared.InternalServiceError{}).Times(1)
 	// then return a non-retryable error
 	s.mockService.EXPECT().StartWorkflowExecution(gomock.Any(), gomock.Any(), callOptions()...).Return(nil, &shared.BadRequestError{}).Times(1)