Fix: report step failure when the CUE template fails to compile

Merged
VA
3mo ago
+167-2

Description of your changes

The check that surfaces a step template error read exec.stepStatus — the previous reconcile's phase — where exec.wfStatus, this reconcile's, was intended. wfStatus is re-seeded to the neutral Succeeded default in the task generator on every reconcile; stepStatus is loaded from the persisted status.

So the first reconcile caught the error and recorded Failed, and every later reconcile read that Failed, skipped the check, and returned the untouched Succeeded default — reporting success for a step whose actions never ran. Recording the failure disabled the check that found it. For a standalone WorkflowRun the false succeeded was permanent, since a finished run is skipped on reconcile.

Reading wfStatus leaves the in-flight paths alone: Wait() and Suspend() set it to Running and Suspending while CompileString evaluates the template, so a mid-flight step still bypasses the check.

Fixes kubevela/kubevela#7275

I have:

  • Read and followed KubeVela's contribution process.
  • Related Docs updated properly. In a new feature or configuration option, an update to the documentation is necessary.
  • Run make reviewable to ensure this PR is ready for review.
  • Added backport release-x.y labels to auto-backport this PR if necessary.

How has this code been tested

New test TestTemplateErrorPersistsAcrossReconciles in pkg/tasks/custom/task_test.go. It runs a step whose template fails to evaluate, then runs it a second time with the first pass's status fed back in as options.StepStatus, and asserts the second pass is still Failed with a non-empty message. On master the second pass returns succeeded with an empty message; with this change it stays Failed.

Also added TestInFlightStatusSurvivesSecondReconcile, which asserts a Wait() step stays Running and a Suspend() step stays Suspending across two passes.

go test ./pkg/tasks/... passes. Originally found in a live cluster: a WorkflowStepDefinition with an unused CUE import (rejected by CUE 0.14.x) whose only action deleted a pod. The run reported succeeded while the pod was untouched.

Special notes for your reviewer

Two things are needed to reach this bug, which is why no existing test caught it. The status has to be fed back in as options.StepStatus, and the runner has to be regenerated for the second pass — the executor struct, and therefore the optimistic seed on wfStatus, is created per generator call rather than per Run call. Reusing one runner carries pass 1's Failed into pass 2 and hides the defect. Real reconciles regenerate runners (controllers/workflowrun_controller.go:134). The existing failed-after-retries case loops but omits the first ingredient.

Why this is narrow. On the first reconcile stepStatus and wfStatus both hold the same default, so behaviour there is unchanged. The fix only affects reconciles after the first, making them behave as the first already does — so it cannot introduce a failure mode that does not already occur.

One behaviour change worth calling out. A step with a persistently broken template now consumes errorRetryTimes and fails the workflow, instead of reporting success on the retry. A run that completes in seconds today will retry and then fail. That is the intent, but it will be visible to anyone tracking run duration or alerting on workflow failures.

On the phase condition itself. I read it as protecting steps that are legitimately mid-flight, but that is inference. Deleting the condition outright leaves the wait and suspend tests passing, because those steps leave a clean taskv and never reach the taskv.Err() != nil half. I kept it since it is clearly deliberate and harmless, but if you know the original intent I would rather match it.

Origin. Introduced in 7d94489 (#162, "Chore: refactor the cue engine with cuex"), which replaced an explicit doSteps() call and its unconditional error handling with the post-hoc taskv.Err() inspection.

Summary by cubic

Fixes steps incorrectly reported as succeeded after a CUE template compile error. We now read wfStatus (this reconcile) instead of stepStatus (previous reconcile) so compile failures are surfaced every time.

  • Bug Fixes

    • Gate template-error reporting on exec.wfStatus rather than exec.stepStatus.
    • Preserve in-flight states: Wait() stays Running and Suspend() stays Suspending.
    • Added tests: TestTemplateErrorPersistsAcrossReconciles, TestInFlightStatusSurvivesSecondReconcile.
    • e2e mock server now returns valid JSON with application/json to avoid json.Unmarshal template errors.

  • Migration

    • Steps with persistently broken templates now consume errorRetryTimes and fail the workflow, instead of falsely succeeding.