Skip to content

fix(server): handle transient errors before ToStatusError loses type … #15996

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
HsiuChuanHsu wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(server): handle transient errors before ToStatusError loses type … #15996

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions server/workflow/workflow_server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,13 @@ import (
sutils "github.com/argoproj/argo-workflows/v4/server/utils"
"github.com/argoproj/argo-workflows/v4/server/workflow/store"
argoutil "github.com/argoproj/argo-workflows/v4/util"
errorsutil "github.com/argoproj/argo-workflows/v4/util/errors"
"github.com/argoproj/argo-workflows/v4/util/fields"
"github.com/argoproj/argo-workflows/v4/util/instanceid"
"github.com/argoproj/argo-workflows/v4/util/logging"
"github.com/argoproj/argo-workflows/v4/util/logs"
retry "github.com/argoproj/argo-workflows/v4/util/retry"
waitutil "github.com/argoproj/argo-workflows/v4/util/wait"
"github.com/argoproj/argo-workflows/v4/workflow/common"
"github.com/argoproj/argo-workflows/v4/workflow/creator"
"github.com/argoproj/argo-workflows/v4/workflow/hydrator"
Expand Down Expand Up @@ -135,7 +138,12 @@ func (s *workflowServer) CreateWorkflow(ctx context.Context, req *workflowpkg.Wo
return workflow, nil
}

wf, err := wfClient.ArgoprojV1alpha1().Workflows(req.Namespace).Create(ctx, req.Workflow, metav1.CreateOptions{})
var wf *wfv1.Workflow
err = waitutil.Backoff(retry.DefaultRetry(ctx), func() (bool, error) {
var createErr error
wf, createErr = wfClient.ArgoprojV1alpha1().Workflows(req.Namespace).Create(ctx, req.Workflow, metav1.CreateOptions{})
return !errorsutil.IsTransientErr(ctx, createErr), createErr
})
logger := logging.RequireLoggerFromContext(ctx)
if err != nil {
if apierr.IsServerTimeout(err) && req.Workflow.GenerateName != "" && req.Workflow.Name != "" {
Expand Down Expand Up @@ -856,7 +864,11 @@ func (s *workflowServer) SubmitWorkflow(ctx context.Context, req *workflowpkg.Wo
return workflow, nil
}

wf, err = wfClient.ArgoprojV1alpha1().Workflows(req.Namespace).Create(ctx, wf, metav1.CreateOptions{})
err = waitutil.Backoff(retry.DefaultRetry(ctx), func() (bool, error) {
var createErr error
wf, createErr = wfClient.ArgoprojV1alpha1().Workflows(req.Namespace).Create(ctx, wf, metav1.CreateOptions{})
return !errorsutil.IsTransientErr(ctx, createErr), createErr
})
if err != nil {
return nil, sutils.ToStatusError(err, codes.InvalidArgument)
}
Comment on lines +867 to 874
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Wrong gRPC status code after retry exhaustion; also inconsistent with CreateWorkflow.

On retry exhaustion, err is a wrapped transient API error (e.g., quota conflict, service unavailable, server timeout). Mapping it to codes.InvalidArgument is misleading — the request body wasn't invalid — and it's inconsistent with the sibling CreateWorkflow path which maps create failures to codes.Internal and additionally emits a DeadlineExceeded hint on apierr.IsServerTimeout. CLIs acting on the status code (including the CLI retry behavior this PR aims to enable) will be confused by InvalidArgument here.

Recommend aligning both paths:

🔧 Proposed fix for SubmitWorkflow post-retry error handling 
 	err = waitutil.Backoff(retry.DefaultRetry(ctx), func() (bool, error) {
 		var createErr error
 		wf, createErr = wfClient.ArgoprojV1alpha1().Workflows(req.Namespace).Create(ctx, wf, metav1.CreateOptions{})
 		return !errorsutil.IsTransientErr(ctx, createErr), createErr
 	})
+	logger := logging.RequireLoggerFromContext(ctx)
 	if err != nil {
-		return nil, sutils.ToStatusError(err, codes.InvalidArgument)
+		if apierr.IsServerTimeout(err) && wf.GenerateName != "" && wf.Name != "" {
+			errWithHint := fmt.Errorf(`submit request failed due to timeout, but it's possible that workflow "%s" already exists. Original error: %w`, wf.Name, err)
+			logger.WithError(err).Error(ctx, errWithHint.Error())
+			return nil, sutils.ToStatusError(errWithHint, codes.DeadlineExceeded)
+		}
+		logger.WithError(err).Error(ctx, "Submit request failed")
+		return nil, sutils.ToStatusError(err, codes.Internal)
 	}
 	return wf, nil
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@server/workflow/workflow_server.go` around lines 867 - 874, The post-retry
error return in the SubmitWorkflow path incorrectly maps retry-exhausted
transient API errors to codes.InvalidArgument; update the error mapping to match
CreateWorkflow: convert the final create error to an Internal gRPC status via
sutils.ToStatusError(err, codes.Internal) and, if apierr.IsServerTimeout(ctx,
err) is true, wrap/augment the status with a DeadlineExceeded hint as
CreateWorkflow does. Locate the retry block around
wfClient.ArgoprojV1alpha1().Workflows(...).Create in workflow_server.go (the
SubmitWorkflow handler) and replace the final return that uses
codes.InvalidArgument with the same error translation logic used by
CreateWorkflow (including the apierr.IsServerTimeout check).

Expand Down
Loading