Skip to content

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

Merged
briankane merged 2 commits into
kubevela:mainfrom
guidewire-oss:fix/step-status-template-error
Aug 5, 2026
Merged

briankane merged 2 commits into
kubevela:mainfrom
guidewire-oss:fix/step-status-template-error

Conversation

@vaibhav0096

@vaibhav0096 vaibhav0096 commented Jul 31, 2026 •

Copy link
Copy Markdown
Contributor

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.

Written for commit 105d3d3. Summary will update on new commits.

Review in cubic

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

No issues found across 2 files

Re-trigger cubic

@vaibhav0096
vaibhav0096 force-pushed the fix/step-status-template-error branch from c7d0e78 to 0dfb79d Compare July 31, 2026 07:22
Signed-off-by: Reetika Malhotra <malhotra.reetika25@gmail.com>
Signed-off-by: Reetika Malhotra <malhotra.reetika25@gmail.com>
@vaibhav0096
vaibhav0096 force-pushed the fix/step-status-template-error branch from 0dfb79d to 105d3d3 Compare July 31, 2026 07:25
@codecov

codecov Bot commented Jul 31, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 68.17%. Comparing base (d7db9c4) to head (105d3d3).
⚠️ Report is 19 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #244      +/-   ##
==========================================
+ Coverage   62.49%   68.17%   +5.68%     
==========================================
  Files          62       67       +5     
  Lines        4415     5492    +1077     
==========================================
+ Hits         2759     3744     +985     
- Misses       1324     1358      +34     
- Partials      332      390      +58     
Flag Coverage Δ
e2etests 22.88% <0.00%> (?)
unit-test 62.09% <100.00%> (-0.41%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@vaibhav0096 vaibhav0096 closed this Aug 3, 2026
@vaibhav0096 vaibhav0096 reopened this Aug 3, 2026
@briankane
briankane merged commit 80b8833 into kubevela:main Aug 5, 2026
33 of 34 checks passed
@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown

Backport failed for release-0.5, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin release-0.5
git worktree add -d .worktree/backport-244-to-release-0.5 origin/release-0.5
cd .worktree/backport-244-to-release-0.5
git switch --create backport-244-to-release-0.5
git cherry-pick -x 80b8833884ca5b7c030f46167804c85adac90c40

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown

Backport failed for release-0.6, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin release-0.6
git worktree add -d .worktree/backport-244-to-release-0.6 origin/release-0.6
cd .worktree/backport-244-to-release-0.6
git switch --create backport-244-to-release-0.6
git cherry-pick -x 80b8833884ca5b7c030f46167804c85adac90c40

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

WorkflowStep reports Succeeded on retry when its CUE template fails to compile, and its actions never run

2 participants