feat(connector): handle TaskExecution_RETRYABLE_FAILED phase

[pingsutw]

Tracking issue

Why are the changes needed?

The webapi connector plugin currently only handles TaskExecution_FAILED and TaskExecution_ABORTED as failure phases. The TaskExecution_RETRYABLE_FAILED phase (value 8 in flyteidl2/core/execution.proto) is silently ignored, falling through to the default branch which returns an "unknown execution phase" system error. As a result, a connector that legitimately reports a retryable failure surfaces as a system error to propeller and the task's retry policy is never honored.

What changes were proposed in this pull request?

In flyteplugins/go/tasks/plugins/webapi/connector/plugin.go:

1. ResourceWrapper.IsTerminal() now treats TaskExecution_RETRYABLE_FAILED as terminal alongside SUCCEEDED/FAILED/ABORTED, so the framework skips an extra Get round-trip to the connector once it has returned this phase.
2. The phase switch in Status maps TaskExecution_RETRYABLE_FAILED to core.PhaseInfoRetryableFailure(...), so propeller retries per the task's retry policy instead of marking the node as permanently failed.

How was this patch tested?

Built locally with go build ./flyteplugins/go/tasks/plugins/webapi/connector/.... No existing tests covered the previous default-branch behavior; happy to add a unit test for both IsTerminal() and the phase mapping if reviewers prefer.

Labels

• added

Setup process

N/A

Screenshots

N/A

Check all the applicable boxes

• I updated the documentation accordingly.
• All new and existing tests passed.
• All commits are signed-off.

Related PRs

Stack

Docs link

[Copilot]

Pull request overview

This PR updates the WebAPI connector task plugin to correctly recognize and translate the TaskExecution_RETRYABLE_FAILED phase from connector responses, ensuring Propeller treats it as a retryable task failure (honoring task retry policy) instead of surfacing an “unknown execution phase” system error.

Changes:

• Treat TaskExecution_RETRYABLE_FAILED as terminal in ResourceWrapper.IsTerminal() to avoid extra connector Get polling once the phase is reached.
• Map TaskExecution_RETRYABLE_FAILED to core.PhaseInfoRetryableFailure(...) in Status() so the failure is retryable.

Comments suppressed due to low confidence (2)

flyteplugins/go/tasks/plugins/webapi/connector/plugin.go:366

• The new TaskExecution_RETRYABLE_FAILED → PhaseInfoRetryableFailure mapping isn’t covered by the existing plugin_test.go status-phase tests (which already cover FAILED/ABORTED). Please add a test that asserts Status() returns pluginsCore.PhaseRetryableFailure for this phase so the retry behavior remains protected.

	case flyteIdl.TaskExecution_FAILED:
		return core.PhaseInfoFailure(errorCode, fmt.Sprintf("failed to run the job: %s", resource.Message), taskInfo), nil
	case flyteIdl.TaskExecution_RETRYABLE_FAILED:
		return core.PhaseInfoRetryableFailure(errorCode, fmt.Sprintf("failed to run the job: %s", resource.Message), taskInfo), nil

flyteplugins/go/tasks/plugins/webapi/connector/plugin.go:366

• Both TaskExecution_FAILED and TaskExecution_RETRYABLE_FAILED currently produce the same reason string prefix ("failed to run the job: …"), which makes it hard to tell from logs/events whether the failure is retryable vs permanent. Consider making the retryable-failure reason explicitly distinguishable (e.g., include "retryable" in the message).

	case flyteIdl.TaskExecution_FAILED:
		return core.PhaseInfoFailure(errorCode, fmt.Sprintf("failed to run the job: %s", resource.Message), taskInfo), nil
	case flyteIdl.TaskExecution_RETRYABLE_FAILED:
		return core.PhaseInfoRetryableFailure(errorCode, fmt.Sprintf("failed to run the job: %s", resource.Message), taskInfo), nil

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.