mirror of
https://github.com/temporalio/temporal.git
synced 2026-08-30 18:41:49 -07:00
Fix Nexus start-operation retry classification for wrapped service errors (#11820)
## What changed? `handleStartOperationError` (`components/nexusoperations/executors.go`), `callErrorToFailure`, and `newInvocationResult` (`chasm/lib/nexusoperation/task_handler_helpers.go`) all unwrap `callErr` into a typed `serviceerror.ServiceError` via `errors.As`/`errors.AsType`, then pass the *original wrapped* `callErr` — not the unwrapped `serviceErr` — to `common.IsRetryableRPCError`. Fix: pass the already-unwrapped `serviceErr` to `IsRetryableRPCError` at all three call sites — in both the workflow-based (`components/nexusoperations`) and CHASM standalone (`chasm/lib/nexusoperation`) Nexus operation state machines. These were the only call sites of `IsRetryableRPCError` outside its own definition/tests. ## Why? `IsRetryableRPCError` only recognizes a service error via a direct (non-unwrapping) type assertion or a gRPC status; neither sees through wrapping. So whenever the transport wraps the service error (e.g. `net/http.Client.Do` wraps `RoundTripper` errors in `*url.Error`), a transient error like `Unavailable` is always misclassified as non-retryable, permanently failing the Nexus operation instead of retrying it. Observed in practice: a Nexus `cancel` operation dispatched during a brief window where the internal service resolver had zero available frontend members failed with `Unavailable: no frontend host to route request to`. That error was wrapped by the HTTP round-tripper, misclassified as non-retryable, and permanently failed the operation — with ~19 of its 20-minute `scheduleToCloseTimeout` still unused, while sibling operations dispatched moments later succeeded normally. A single retry would have resolved it. ## How did you test it? - [x] built - [ ] run locally and tested manually - [x] covered by existing tests - [x] added new unit test(s) - [ ] added new functional test(s) ## Potential risks Low risk: the change only affects the retryability decision for the `serviceerror` branch of Nexus start-operation error handling, and only for errors that were previously misclassified (wrapped service errors). Correctly-classified cases (direct/unwrapped service errors, gRPC status errors) are unaffected since `serviceErr` and `callErr` resolve to the same classification for those.
This commit is contained in:
@@ -141,7 +141,7 @@ func cancelCallOutcomeTag(callCtx context.Context, callErr error) string {
|
||||
// Always returns a non-nil failure.
|
||||
func callErrorToFailure(callErr error) (*failurepb.Failure, bool, error) {
|
||||
if serviceErr, ok := errors.AsType[serviceerror.ServiceError](callErr); ok {
|
||||
retryable := common.IsRetryableRPCError(callErr)
|
||||
retryable := common.IsRetryableRPCError(serviceErr)
|
||||
failure := &failurepb.Failure{
|
||||
Message: fmt.Sprintf("%s: %s", strings.Replace(fmt.Sprintf("%T", serviceErr), "*serviceerror.", "", 1), serviceErr.Error()),
|
||||
FailureInfo: &failurepb.Failure_ServerFailureInfo{
|
||||
@@ -233,7 +233,7 @@ func newInvocationResult(
|
||||
}
|
||||
|
||||
if serviceErr, ok := errors.AsType[serviceerror.ServiceError](callErr); ok {
|
||||
retryable := common.IsRetryableRPCError(callErr)
|
||||
retryable := common.IsRetryableRPCError(serviceErr)
|
||||
failure := &failurepb.Failure{
|
||||
Message: fmt.Sprintf("%s: %s", strings.Replace(fmt.Sprintf("%T", serviceErr), "*serviceerror.", "", 1), serviceErr.Error()),
|
||||
FailureInfo: &failurepb.Failure_ServerFailureInfo{
|
||||
|
||||
37
chasm/lib/nexusoperation/task_handler_helpers_test.go
Normal file
37
chasm/lib/nexusoperation/task_handler_helpers_test.go
Normal file
@@ -0,0 +1,37 @@
|
||||
package nexusoperation
|
||||
|
||||
import (
|
||||
"net/url"
|
||||
"testing"
|
||||
|
||||
"github.com/stretchr/testify/require"
|
||||
"go.temporal.io/api/serviceerror"
|
||||
)
|
||||
|
||||
// wrappedUnavailable returns a transient serviceerror reachable only via Unwrap,
|
||||
// as an HTTP client wraps transport errors.
|
||||
func wrappedUnavailable() error {
|
||||
return &url.Error{
|
||||
Op: "Post",
|
||||
URL: "https://internal",
|
||||
Err: serviceerror.NewUnavailable("no frontend host to route request to"),
|
||||
}
|
||||
}
|
||||
|
||||
func TestCallErrorToFailure_RetriesTransientServiceErrorEvenWhenWrapped(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
failure, retryable, err := callErrorToFailure(wrappedUnavailable())
|
||||
require.NoError(t, err)
|
||||
require.True(t, retryable, "wrapped Unavailable must be classified as retryable")
|
||||
require.False(t, failure.GetServerFailureInfo().GetNonRetryable())
|
||||
}
|
||||
|
||||
func TestNewInvocationResult_RetriesTransientServiceErrorEvenWhenWrapped(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
result, err := newInvocationResult(nil, wrappedUnavailable())
|
||||
require.NoError(t, err)
|
||||
require.IsType(t, invocationResultRetry{}, result,
|
||||
"wrapped Unavailable must produce a retry result, not a terminal failure")
|
||||
}
|
||||
@@ -763,6 +763,9 @@ func getFieldNameFromStruct(structPtr any, fieldPtr any) (string, error) {
|
||||
}
|
||||
|
||||
// IsRetryableRPCError checks if the error is a retryable gRPC error.
|
||||
//
|
||||
// This does not unwrap err: If err may be wrapped (e.g. by an HTTP client, which wraps
|
||||
// RoundTripper errors in *url.Error), unwrap it yourself and pass the unwrapped error here.
|
||||
func IsRetryableRPCError(err error) bool {
|
||||
var st *status.Status
|
||||
stGetter, ok := err.(interface{ Status() *status.Status })
|
||||
|
||||
@@ -535,7 +535,7 @@ func (e taskExecutor) handleStartOperationError(env hsm.Environment, node *hsm.N
|
||||
|
||||
switch {
|
||||
case errors.As(callErr, &serviceErr):
|
||||
if !common.IsRetryableRPCError(callErr) {
|
||||
if !common.IsRetryableRPCError(serviceErr) {
|
||||
return handleNonRetryableStartOperationError(node, operation, callErr)
|
||||
}
|
||||
// Fall through all uncaught errors to retryable
|
||||
|
||||
@@ -5,6 +5,8 @@ import (
|
||||
"encoding/json"
|
||||
"errors"
|
||||
"fmt"
|
||||
"net/http"
|
||||
"net/url"
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
@@ -99,6 +101,9 @@ func TestProcessInvocationTask(t *testing.T) {
|
||||
startToCloseTimeout time.Duration
|
||||
schedToStartTimeout time.Duration
|
||||
destinationDown bool
|
||||
// httpCallerErr, when set, fails the outbound HTTP call with this error instead of
|
||||
// performing a real request.
|
||||
httpCallerErr error
|
||||
}{
|
||||
{
|
||||
name: "async start",
|
||||
@@ -379,6 +384,22 @@ func TestProcessInvocationTask(t *testing.T) {
|
||||
require.Empty(t, events)
|
||||
},
|
||||
},
|
||||
{
|
||||
name: "transient service error wrapped by HTTP caller",
|
||||
requestTimeout: time.Hour,
|
||||
httpCallerErr: &url.Error{
|
||||
Op: "Post",
|
||||
URL: "http://unavailable",
|
||||
Err: serviceerror.NewUnavailable("no frontend host to route request to"),
|
||||
},
|
||||
expectedMetricOutcome: "service-error:Unavailable",
|
||||
checkOutcome: func(t *testing.T, op nexusoperations.Operation, events []*historypb.HistoryEvent) {
|
||||
require.Equal(t, enumsspb.NEXUS_OPERATION_STATE_BACKING_OFF, op.State())
|
||||
require.False(t, op.LastAttemptFailure.GetServerFailureInfo().GetNonRetryable())
|
||||
require.Contains(t, op.LastAttemptFailure.Message, "Unavailable")
|
||||
require.Empty(t, events)
|
||||
},
|
||||
},
|
||||
{
|
||||
name: "invocation timeout by request timeout",
|
||||
requestTimeout: 2 * time.Millisecond,
|
||||
@@ -655,11 +676,17 @@ func TestProcessInvocationTask(t *testing.T) {
|
||||
Logger: log.NewNoopLogger(),
|
||||
EndpointRegistry: endpointReg,
|
||||
ClientProvider: func(ctx context.Context, namespaceID string, entry *persistencespb.NexusEndpointEntry, service string) (*nexusrpc.HTTPClient, error) {
|
||||
return nexusrpc.NewHTTPClient(nexusrpc.HTTPClientOptions{
|
||||
options := nexusrpc.HTTPClientOptions{
|
||||
BaseURL: "http://" + listenAddr,
|
||||
Service: service,
|
||||
Serializer: commonnexus.PayloadSerializer,
|
||||
})
|
||||
}
|
||||
if tc.httpCallerErr != nil {
|
||||
options.HTTPCaller = func(*http.Request) (*http.Response, error) {
|
||||
return nil, tc.httpCallerErr
|
||||
}
|
||||
}
|
||||
return nexusrpc.NewHTTPClient(options)
|
||||
},
|
||||
}))
|
||||
|
||||
|
||||
Reference in New Issue
Block a user