Skip to content

fix(executor): prevent invalid TaskActions from staying nonterminal - #7967

Merged
popojk merged 1 commit into
flyteorg:mainfrom
1fanwang:1fannnw/retry-taskaction-validation-status
Sep 4, 2026
Merged

fix(executor): prevent invalid TaskActions from staying nonterminal#7967
popojk merged 1 commit into
flyteorg:mainfrom
1fanwang:1fannnw/retry-taskaction-validation-status

Conversation

@1fanwang

@1fanwang 1fanwang commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Tracking issue

None. This bug was found on current main.

Why are the changes needed?

An invalid TaskAction can stay nonterminal when Kubernetes rejects the status write that should mark it failed. The executor discards the write error and reports a successful reconciliation, so controller-runtime removes the request from its work queue until another event happens to enqueue it.

Before this change, one rejected status write can leave the TaskAction stuck. After this change, controller-runtime retries the reconciliation and can persist the terminal failure.

controller-runtime treats these outcomes differently: a non-nil error adds the request back to the rate-limited queue, while a successful empty result forgets it. See controller-runtime v0.24.1.

What changes were proposed in this pull request?

Return the status update error from the validation failure path. A successful status write still returns an empty result with no error. The regression test covers both outcomes.

How was this patch tested?

Scenario Result
Parent source with the regression test Failed because the status error was swallowed
Patched source Both the successful and failed status-write cases passed
Full controller package Passed
git restore --source=500b99b6ca64342f9a221f8c438feeecd1fa9b10 -- executor/pkg/controller/taskaction_controller.go
go test ./executor/pkg/controller -run TestReconcileTask_ValidationStatusUpdate -count=1 -v
git restore --source=689b8a67379292380093d298f211b920cb8821cb -- executor/pkg/controller/taskaction_controller.go
go test ./executor/pkg/controller -run TestReconcileTask_ValidationStatusUpdate -count=1 -v
go test ./executor/pkg/controller -count=1
Raw logs
=== RUN   TestReconcileTask_ValidationStatusUpdate
=== RUN   TestReconcileTask_ValidationStatusUpdate/success
=== RUN   TestReconcileTask_ValidationStatusUpdate/failure
    taskaction_validation_test.go:145: expected status update failed, got <nil>
--- FAIL: TestReconcileTask_ValidationStatusUpdate (0.04s)
    --- PASS: TestReconcileTask_ValidationStatusUpdate/success (0.04s)
    --- FAIL: TestReconcileTask_ValidationStatusUpdate/failure (0.00s)
FAIL
FAIL	github.com/flyteorg/flyte/v2/executor/pkg/controller	0.996s
FAIL

=== RUN   TestReconcileTask_ValidationStatusUpdate
=== RUN   TestReconcileTask_ValidationStatusUpdate/success
=== RUN   TestReconcileTask_ValidationStatusUpdate/failure
--- PASS: TestReconcileTask_ValidationStatusUpdate (0.04s)
    --- PASS: TestReconcileTask_ValidationStatusUpdate/success (0.04s)
    --- PASS: TestReconcileTask_ValidationStatusUpdate/failure (0.00s)
PASS
ok  	github.com/flyteorg/flyte/v2/executor/pkg/controller	1.075s

ok  	github.com/flyteorg/flyte/v2/executor/pkg/controller	10.683s

Labels

fixed

Setup process

make -C executor setup-envtest

Screenshots

Not applicable.

Check all the applicable boxes

  • Documentation is not affected.
  • All new and existing controller tests passed.
  • All commits are signed off.

Related PRs

None.

Stack

This PR is not stacked.

Docs link

Not applicable.

Signed-off-by: 1fanwang <1fannnw@gmail.com>
Copilot AI lite review requested due to automatic review settings September 3, 2026 07:48
@github-actions github-actions Bot added the flyte2 label Sep 3, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

The change is small, directly addresses the reported bug, and includes a focused regression test validating both success and failure paths.

Pull request overview

This PR fixes the TaskAction executor’s validation-failure path to propagate status subresource update errors back to controller-runtime, ensuring failed status writes are requeued (rate-limited) instead of being silently “forgotten” as a successful reconcile.

Changes:

  • Return the error from r.Status().Update(...) when validation fails, so controller-runtime requeues on write failure.
  • Add a regression test that covers both successful and failed status-update outcomes for the validation-failure path.
File summaries
File Description
executor/pkg/controller/taskaction_controller.go Returns the status update error on validation failure so reconcile requeues on write failures.
executor/pkg/controller/taskaction_validation_test.go Adds a regression test asserting status update errors are surfaced (and success remains nil-error).
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@1fanwang 1fanwang changed the title fix(executor): retry failed validation status writes fix(executor): prevent invalid TaskActions from staying nonterminal Sep 3, 2026

@AdilFayyaz AdilFayyaz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!

@popojk
popojk merged commit 17e9fcf into flyteorg:main Sep 4, 2026
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants