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

Split out of LeanerCloud/cloud-commitments-cli#1805 / PR LeanerCloud/cloud-commitments-cli#1809 rather than fixed there, because it is pre-existing and LeanerCloud/cloud-commitments-cli#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:

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 LeanerCloud/cloud-commitments-cli#1809

LeanerCloud/cloud-commitments-cli#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.