deploy-all.yml summary reports success on a cancelled run (only success-by-default check in the repo)

[cristim]

Split out of #1805 / PR #1809 rather than fixed there, because it is pre-existing and #1809 did not cause it.

This is a one-line fix at a single site. An earlier revision of this issue claimed it was a class issue across four deploy workflows. That was wrong, and the correction is below under "Scope" so nobody goes looking for a bug in the four files that do not have it.

The defect

deploy-all.yml's notify job runs if: always() and its Check for failures step tests the four deploy jobs for == "failure". When a run is cancelled mid-flight:

• determine-deployment.result = success
• deploy-*.result = cancelled, which is not "failure"

No clause matches, FAILED=false, and the summary prints:

✅ **All deployments completed successfully!**

for a run that was killed mid-apply. always() is documented to return true "even when canceled", so notify does run in that state.

This matters more than a cosmetic summary bug because deploy-azure.yml already states the house position on exactly this scenario: "cancelling mid-terraform apply is how you get a half-applied stack and a lease nobody releases." The run most likely to have left infrastructure in a broken state is the one that announces complete success.

Scope: one site, and it is the outlier

The root cause is polarity. Check for failures is a denylist — it enumerates the bad state (== "failure") and defaults to green on everything its author did not think of. cancelled is one such state.

All four per-cloud summary jobs use the opposite polarity, an allowlist on success:

File	Line	Condition
deploy-azure.yml	482	[ "…build-and-deploy.result" == "success" ] && [ "…test-deployment.result" == "success" ]
deploy-gcp.yml	307	[ "…build-and-deploy.result" == "success" ] && [ "…test-deployment.result" == "success" ]
deploy-aws-lambda.yml	487	[ "$DEPLOY_RESULT" = "success" ] && [ "$TEST_RESULT" = "success" ]
deploy-aws-fargate.yml	322	[ "…deploy.result" == "success" ] && [ "…test-deployment.result" == "success" ]

Each is green only when the results are literally success, so a cancelled result falls to the else branch. None of the four has this bug. Do not "fix" them.

deploy-all.yml's Check for failures is the only success-by-default check in the repo. Verified by enumerating every .result comparison in all 16 workflow files (.github/workflows/*.yml; the 17th directory entry is README.md, and there are no .yaml files). The only denylist hits in the entire directory are deploy-all.yml:314-317. Every other .result comparison allowlists on success.

Worth knowing for whoever takes this: ci.yml:740-744 already handles the cancellation case explicitly, with a contains(needs.*.result, 'failure') test followed by a separate contains(needs.*.result, 'cancelled') test. That is in-repo precedent for treating cancelled as its own state rather than folding it into failure.

If there is a class framing here it is the reverse of the original claim: deploy-all.yml is the outlier and should be brought in line with the polarity the other four already use.

The fix

Swap if: always() for if: ${{ !cancelled() }} on deploy-all.yml's notify job. GitHub documents this directly: "If you want to run a job or step regardless of its success or failure, use the recommended alternative: if: ${{ !cancelled() }}". On a cancelled run the summary job skips entirely, producing no summary rather than a false one, while still running on every failure.

The safety question, resolved

notify has needs: [determine-deployment, deploy-aws-lambda, deploy-aws-fargate, deploy-gcp, deploy-azure], and deploy-aws-fargate is permanently skipped (deploy-all.yml hardcodes deploy-aws-fargate=false). A job whose needs include a skipped job is normally skipped itself, and always() bypasses that. If !cancelled() did not also bypass it, notify would silently never run again.

It does bypass it. The documented rule chain, not an inference:

1. "If a job fails or is skipped, all jobs that need it are skipped unless the jobs use a conditional expression that causes the job to continue." The escape hatch is stated generically, not as always() specifically.
2. Expressions reference, status check functions: "A default status check of success() is applied unless you include one of these functions." The four are success(), always(), cancelled(), failure().

!cancelled() includes cancelled(), so the implicit success() is not applied and nothing requires the needs to have succeeded.

"Include" means contains, not equals — the docs settle this with their own example, failure() && steps.demo.conclusion == 'failure', a compound expression they describe as overriding the default status check. So a negated call qualifies.

One trap

The ${{ }} braces are not optional here, even though job-level if normally allows omitting them:

if: !cancelled()          # INVALID YAML - fails to parse
if: ${{ !cancelled() }}   # correct

A leading ! in a YAML scalar is a tag indicator. The failure is loud ("Invalid workflow file") but will not look like a YAML problem at first glance.

Completeness

!cancelled() fully closes this only if run-level cancellation is the sole way a deploy job reaches result == "cancelled". All four producers were checked:

• cancel-in-progress: true anywhere in this chain: no. deploy-gcp:29, deploy-aws-lambda:35, deploy-aws-fargate:30 and deploy-azure are all explicitly false; deploy-all.yml has no workflow-level concurrency. The only true values in the repo are in frontend-build, frontend-build-sentinel and frontend-e2e, disjoint from the deploy chain.
• matrix fail-fast: no strategy:/matrix: in any of the five files.
• timeout-minutes: none declared, and a timed-out job surfaces as failure, already caught.
• reruns / API cancellation: these are run-level cancellation, which is what cancelled() reports.

So the fix is complete, not partial.

Consequence to accept knowingly

On a cancelled run there will be no "Deployment Results" summary at all, rather than a false one. Nothing depends on notify (it is terminal, no job needs it) and the run already displays as cancelled, so this is the right trade. Do not later mistake the missing summary for a bug.

Separate, lower-priority follow-on — do not merge into the above

All four per-cloud summary jobs print "Deployment failed. Check logs for details." on a cancelled run. That is wrong in wording (the deployment was cancelled, not failed) but safe in direction, which is why they are not part of this bug.

If anyone chooses to improve that, the fix is a distinct third state (cancelled) in those conditionals, not the !cancelled() swap. Keep the two tasks separate; conflating them is how four correct files get churned on the deploy path for no gain.

Why not fixed in #1809

#1809 folded in the adjacent determine-deployment case because that PR caused it: adding an environment allowlist made determine-deployment able to fail for the first time, and the four deploy jobs are skipped (not failed) in that case. The cancellation path is unchanged from origin/main. deploy-all.yml's Check for failures carries a comment pointing here.

Note that deploy-all.yml has never executed (gh run list --workflow deploy-all.yml returns []), so none of this has ever actually mis-reported.

[cristim]

Correction: this is one instance in one workflow, not a class across four

This issue states the defect spans four workflows: deploy-all.yml plus deploy-azure.yml:453, deploy-aws-lambda.yml:441 and deploy-aws-fargate.yml:293. That is wrong.

Measured on origin/main, counting the actual == "failure" comparison rather than inferring from the presence of if: always():

workflow	== "failure" occurrences
deploy-all.yml	4
deploy-azure.yml	0
deploy-aws-lambda.yml	0
deploy-aws-fargate.yml	0
deploy-gcp.yml	0

The failure mode described here - a summary printing "✅ All deployments completed successfully!" for a run that was cancelled - exists only in deploy-all.yml. The other workflows do have if: always() summary jobs, but those post a summary; none of them contains a pass/fail check that can produce a false success verdict.

The three line references cited are if: always() job declarations, not instances of the bug. if: always() is the precondition for the defect, not the defect: it is what makes a summary job run after a cancellation. The defect is the separate == "failure" test that then concludes success. Those two were conflated when this was filed, which turned one instance into a claimed class of four.

What this changes, and what it does not

Scope shrinks substantially. This is a one-file fix in deploy-all.yml, not a four-workflow sweep. Anyone picking it up should not go looking for three more instances that do not exist. effort/s is now, if anything, generous.

The decision to decline it from PR #1809 still stands, on the surviving reason. That decline rested on two arguments and only one of them was the class claim. The load-bearing one holds: the four == "failure" tests are unchanged from origin/main, so PR #1809 did not cause this and it fails the caused-by-this-PR test. By contrast the determine-deployment case was folded into #1809, correctly, because that PR's new allowlist made that job able to fail for the first time.

The technical analysis in this issue remains valid and is the useful part: that !cancelled() must bypass the skipped-need auto-skip, that ${{ }} is mandatory because of the YAML tag-indicator trap, and that run-level cancellation is the only producer of cancelled in this chain. That reasoning is what makes the follow-up short, and none of it depended on the count.

Worth naming the failure mode, since it recurs: the count was asserted from a pattern that looked related (if: always()) rather than measured against the thing that actually defines the bug (== "failure"). A neighbouring construct is not an instance.

[cristim]

Correction, recorded rather than silently edited: the original filing claimed this was "a class issue across all four deploy workflows' summary jobs". That was wrong.

I verified all three siblings and none of them has this bug. They use the opposite polarity - an allowlist on == "success" (deploy-azure.yml:482, deploy-aws-lambda.yml:487, deploy-aws-fargate.yml:322) - so a cancelled result falls to their failure branch and errs safe. deploy-all.yml's Check for failures is the only denylist (== "failure"), and a denylist defaults green on every state its author did not enumerate. That is the whole bug, and it is one site.

The claim mattered because it would have sent whoever picked this up into three files looking for a bug that is not there, with the plausible outcome of "fixing" three correct summary jobs - churn on the deploy path for no gain. It also inverted the effort estimate: this reads as a four-workflow sweep but is one if: on one job. Title, scope section and the effort/* label are corrected accordingly, and the second-order wording issue in those three files is now called out as an explicitly separate task so the two do not get merged.

The same false claim was in the code comment in PR #1809 and is corrected there too.

[cristim]

Second correction: the sibling count was three, should be four. deploy-gcp.yml has a summary: job at :274 (if: always() at :278) with the same == "success" allowlist at :307. Same shape, same safe polarity - it was simply missing from the enumeration. The scope table and the follow-on section now list all four.

Impact was in the same direction as the original defect this issue corrects: the table said "None of the three has this bug. Do not 'fix' them" while leaving GCP unlisted, which is precisely the file someone might then have gone and "fixed".

Provenance, since it is the more useful part: the omission originated in my first filing of this issue, which named azure/lambda/fargate and left GCP out. The reviewer who refuted the class claim then re-derived the polarity of each entry correctly but inherited the membership of the set from my list rather than deriving it independently. So the error survived a review that was otherwise correct, because only one side of the comparison was re-derived. Textbook feedback_rederive_both_sides_of_a_comparison.

Also now verified rather than asserted: the repo-wide claim. Sweeping all 17 workflows for the denylist shape (.result … == "failure") returns hits only at deploy-all.yml:314-317. Every other .result comparison in the repo allowlists on success, so "the only success-by-default check in the repo" is exactly right, not merely true across the deploy chain. That sentence is now in the issue body with the method attached.