Translate relevant errors to non retryable internal Nexus handler err…

…ors (#1833)
temporalio · Feb 20, 2025 · c99ec47 · c99ec47
1 parent e44e74e
commit c99ec47
Show file tree

Hide file tree

Showing 4 changed files with 22 additions and 12 deletions.
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -87,6 +87,9 @@ jobs:
       - name: Docker compose - integration tests
         if: ${{ matrix.testDockerCompose }}
         run: go run . integration-test
+        env:
+          # TODO(antlai-temporal): Remove this flag once server 1.27 released.
+          DISABLE_SERVER_1_27_TESTS: "1"
         working-directory: ./internal/cmd/build
 
   cloud-test:

diff --git a/internal/cmd/build/main.go b/internal/cmd/build/main.go
@@ -140,6 +140,10 @@ func (b *builder) integrationTest() error {
 	// Start dev server if wanted
 	if *devServerFlag {
 		devServer, err := testsuite.StartDevServer(context.Background(), testsuite.DevServerOptions{
+			// TODO: Use stable release once server 1.27.0 is out.
+			CachedDownload: testsuite.CachedDownload{
+				Version: "v1.3.0-versioning.0",
+			},
 			ClientOptions: &client.Options{
 				HostPort:  "127.0.0.1:7233",
 				Namespace: "integration-test-namespace",
@@ -158,11 +162,8 @@ func (b *builder) integrationTest() error {
 				"--dynamic-config-value", "worker.buildIdScavengerEnabled=true",
 				"--dynamic-config-value", "worker.removableBuildIdDurationSinceDefault=1",
 				"--dynamic-config-value", "system.enableDeployments=true",
-				// All of the below is required for Nexus tests.
-				"--http-port", "7243",
-				"--dynamic-config-value", "system.enableNexus=true",
-				// SDK tests use arbitrary callback URLs, permit that on the server.
-				"--dynamic-config-value", `component.callbacks.allowedAddresses=[{"Pattern":"*","AllowInsecure":true}]`,
+				"--http-port", "7243", // Nexus tests use the HTTP port directly
+				"--dynamic-config-value", `component.callbacks.allowedAddresses=[{"Pattern":"*","AllowInsecure":true}]`, // SDK tests use arbitrary callback URLs, permit that on the server
 			},
 		})
 		if err != nil {

diff --git a/internal/internal_nexus_task_handler.go b/internal/internal_nexus_task_handler.go
@@ -503,9 +503,9 @@ func convertKnownErrors(err error) error {
 	// Not using errors.As to be consistent ApplicationError checking with the rest of the SDK.
 	if appErr, ok := err.(*ApplicationError); ok && appErr.NonRetryable() {
 		return &nexus.HandlerError{
-			// TODO(bergundy): Change this to a non retryable internal error after the 1.27.0 server release.
-			Type:  nexus.HandlerErrorTypeBadRequest,
-			Cause: appErr,
+			Type:          nexus.HandlerErrorTypeInternal,
+			Cause:         appErr,
+			RetryBehavior: nexus.HandlerErrorRetryBehaviorNonRetryable,
 		}
 	}
 	return convertServiceError(err)
@@ -533,8 +533,7 @@ func convertServiceError(err error) error {
 	case codes.InvalidArgument:
 		return &nexus.HandlerError{Type: nexus.HandlerErrorTypeBadRequest, Cause: err}
 	case codes.AlreadyExists, codes.FailedPrecondition, codes.OutOfRange:
-		// TODO(bergundy): Change this to a non retryable internal error after the 1.27.0 server release.
-		return &nexus.HandlerError{Type: nexus.HandlerErrorTypeBadRequest, Cause: err}
+		return &nexus.HandlerError{Type: nexus.HandlerErrorTypeInternal, Cause: err, RetryBehavior: nexus.HandlerErrorRetryBehaviorNonRetryable}
 	case codes.Aborted, codes.Unavailable:
 		return &nexus.HandlerError{Type: nexus.HandlerErrorTypeUnavailable, Cause: err}
 	case codes.Canceled, codes.DataLoss, codes.Internal, codes.Unknown, codes.Unauthenticated, codes.PermissionDenied:

diff --git a/test/nexus_test.go b/test/nexus_test.go
@@ -28,6 +28,7 @@ import (
 	"errors"
 	"fmt"
 	"net/http"
+	"os"
 	"slices"
 	"testing"
 	"time"
@@ -309,7 +310,10 @@ func TestNexusSyncOperation(t *testing.T) {
 		_, err := nexus.ExecuteOperation(ctx, nc, syncOp, "already-started", nexus.ExecuteOperationOptions{})
 		var handlerErr *nexus.HandlerError
 		require.ErrorAs(t, err, &handlerErr)
-		require.Equal(t, nexus.HandlerErrorTypeBadRequest, handlerErr.Type)
+		require.Equal(t, nexus.HandlerErrorTypeInternal, handlerErr.Type)
+		if os.Getenv("DISABLE_SERVER_1_27_TESTS") == "" {
+			require.Equal(t, nexus.HandlerErrorRetryBehaviorNonRetryable, handlerErr.RetryBehavior)
+		}
 		require.Contains(t, handlerErr.Cause.Error(), "faking workflow already started")
 
 		require.EventuallyWithT(t, func(t *assert.CollectT) {
@@ -339,7 +343,10 @@ func TestNexusSyncOperation(t *testing.T) {
 		_, err := nexus.ExecuteOperation(ctx, nc, syncOp, "non-retryable-application-error", nexus.ExecuteOperationOptions{})
 		var handlerErr *nexus.HandlerError
 		require.ErrorAs(t, err, &handlerErr)
-		require.Equal(t, nexus.HandlerErrorTypeBadRequest, handlerErr.Type)
+		require.Equal(t, nexus.HandlerErrorTypeInternal, handlerErr.Type)
+		if os.Getenv("DISABLE_SERVER_1_27_TESTS") == "" {
+			require.Equal(t, nexus.HandlerErrorRetryBehaviorNonRetryable, handlerErr.RetryBehavior)
+		}
 		require.Contains(t, handlerErr.Cause.Error(), "fake app error for test")
 
 		require.EventuallyWithT(t, func(t *assert.CollectT) {