Report a non-success outcome when rad commands are killed

[nellshamrell]

Fixes #12756

Problem

The run-rad-commands composite action seeded its outcome accumulator to succeeded and wrote the result file from a trap write_result EXIT. Every other assignment to OVERALL_OUTCOME was a failure value (command_failed, disallowed_command), each followed by an explicit exit. So success was never earned — only seeded.

That works for a normal command failure: record() returns non-zero, a failure branch assigns command_failed, and the trap writes the truth. It does not work for a termination. A step timeout kills bash mid-command, so no failure branch ever runs — but the EXIT trap still fires and writes the stale succeeded seed. publish-deploy-status then maps that to RUN_STATE=succeeded, and a deploy that was killed is published as a successful one.

This is worse than the missing-file case the mapping was designed for: unknown → in_progress is the intended "no verdict" sentinel, and the trap defeats it by supplying a confident wrong verdict instead of no verdict.

#12746 adds a timeout-minutes bound to this step, which makes the affected path the normal shape of a slow-deploy failure rather than an edge case.

Fix

Invert the default so the accumulator is pessimistic:

• Seed OVERALL_OUTCOME="interrupted" / OVERALL_EXIT=1.
• Assign succeeded / 0 exactly once, after the command loop completes.

Every failure path exits before reaching the promotion, and an abnormal termination never reaches it at all, so a killed run now reports the seed. publish-deploy-status already maps unrecognized outcomes to failed via its *) arm, so the consumer needs no change.

The fix lives in the composite action rather than the workflow templates on purpose: templates are copied into user repositories and only update on regeneration, whereas composite actions resolve at the pinned {{RADIUS_REF}} and reach existing repositories automatically.

On cancellation: the issue asked whether a cancelled job reaches the same path. It does — cancellation kills the step identically — but both deploy workflow templates gate Publish deployed graph and status on if: ${{ !cancelled() }}, so a cancelled job publishes no status at all. There the seed only corrects the uploaded rad-commands-result artifact. The comments and README say so explicitly rather than overclaiming.

Tests

New command-outcome_test.sh extracts the accumulator prologue verbatim from action.yml (scoped to the Run rad commands step so an unrelated step can't silently redirect the extraction), then models a step timeout faithfully: it forks a subshell with the same options GitHub gives a shell: bash step, waits for the trap to be installed, asserts the result file is not written eagerly, and sends a real SIGTERM. It then asserts the published outcome is neither succeeded/success nor unknown (which would map to the neutral in_progress), plus static checks that succeeded is assigned exactly once and after the last failure assignment.

Mutation-tested against three regressions — restoring the optimistic seed, removing the promotion, and renaming command_failed — each fails with a specific diagnostic.

Also adds an interrupted → failed case to publish-deploy-status_test.sh, and wires the new suite into make test as test-command-outcome.

Validation: both suites pass; shellcheck 0.11.0 with the repo rcfile is clean; both action.yml files parse.

Docs

.github/extension/README.md now documents the pessimistic-accumulator contract next to the rad-commands-result output description, including the cancellation caveat.

[github-actions]

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

[Copilot]

Pull request overview

This PR fixes incorrect “success” reporting when the run-rad-commands composite action is terminated (for example by a step timeout-minutes) by making the result accumulator pessimistic by default and only assigning success after the command loop completes normally.

Changes:

• Seed OVERALL_OUTCOME/OVERALL_EXIT to a non-success (interrupted/1) and promote to succeeded/0 only after successful completion.
• Add a new bash regression test that models SIGTERM termination and asserts the emitted outcome is non-success/non-unknown; wire it into make test.
• Update docs and publish-status tests/comments to cover the new interrupted outcome behavior.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
build/test.mk	Adds test-command-outcome to the aggregate test target and defines the new target.
.github/extension/README.md	Documents the pessimistic accumulator contract and cancellation caveat for rad-commands-result.
.github/extension/actions/run-rad-commands/command-outcome_test.sh	New test to validate outcome handling on termination and enforce “success is earned” invariants.
.github/extension/actions/run-rad-commands/action.yml	Implements the pessimistic seed and explicit success promotion after the command loop.
.github/extension/actions/publish-deploy-status/publish-deploy-status_test.sh	Adds coverage asserting interrupted maps to failed run state.
.github/extension/actions/publish-deploy-status/action.yml	Updates mapping commentary to include the interrupted seed outcome.

---

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

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 53.96%. Comparing base (f841849) to head (7b839ca).

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #12759      +/-   ##
==========================================
+ Coverage   53.95%   53.96%   +0.01%     
==========================================
  Files         774      774              
  Lines       51991    51991              
==========================================
+ Hits        28050    28056       +6     
+ Misses      21313    21310       -3     
+ Partials     2628     2625       -3     

☔ 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.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[github-actions]

Unit Tests

    2 files  ±0    459 suites  ±0   7m 42s ⏱️ + 1m 14s
6 355 tests ±0  6 353 ✅ ±0  2 💤 ±0  0 ❌ ±0 
7 613 runs  ±0  7 611 ✅ ±0  2 💤 ±0  0 ❌ ±0 

Results for commit 7b839ca. ± Comparison against base commit f841849.

♻️ This comment has been updated with latest results.

[github-actions]

Functional Tests - upgrade-noncloud

3 tests  ±0   3 ✅ ±0   3m 32s ⏱️ ±0s
1 suites ±0   0 💤 ±0 
1 files   ±0   0 ❌ ±0 

Results for commit d33439c. ± Comparison against base commit b0875a4.

♻️ This comment has been updated with latest results.

[github-actions]

Functional Tests - corerp-cloud

32 tests   31 ✅  20m 59s ⏱️
 2 suites   1 💤
 1 files     0 ❌

Results for commit 7b839ca.

♻️ This comment has been updated with latest results.

[github-actions]

Functional Tests - statestore-noncloud

2 tests  ±0   2 ✅ ±0   3m 4s ⏱️ +7s
1 suites ±0   0 💤 ±0 
1 files   ±0   0 ❌ ±0 

Results for commit d3396bc. ± Comparison against base commit b4ebe2f.

♻️ This comment has been updated with latest results.

[nellshamrell]

Approver review

Disclosure: I wrote this PR, so treat this as a structured self-review to speed up a human approver, not as an independent approval. Findings were verified against the code and cross-checked with a second model.

⚠️ Read this first: main is currently broken, and it blocks this PR

While checking whether this branch still rebases cleanly, I found that main does not currently parse as valid bash. e30a2594d ("Live graph support", #12727) removed the closing } of cleanup() in run-rad-commands/action.yml:

cleanup() {
  if [ -n "$OIDC_REFRESH_PID" ]; then
    kill "$OIDC_REFRESH_PID" 2>/dev/null || true
    wait "$OIDC_REFRESH_PID" 2>/dev/null || true
  fi
  stop_live_deploy_progress || true
  radius_clear_artifact_runtime
  write_result
trap cleanup EXIT        # <-- no closing brace before this

Extracting the Run rad commands run block and syntax-checking it:

Ref	bash -n
origin/main (3c7dfecd6)	❌ exit 2 — syntax error: unexpected end of file from '{' command on line 32
this branch (3f2dcd33d)	✅ exit 0

trap cleanup EXIT is swallowed into the unterminated function body, so the whole step fails to parse. Every deploy running the composite action at main should be failing immediately. This is unrelated to this PR — but it must be fixed before this branch can rebase, and it deserves its own issue.

---

Orientation

Purpose. When the Run rad commands step dies without running one of its failure branches, the run reported succeeded. This makes the reported outcome honest.

Placement. The fix is in the composite action, not the workflow templates. That distinction is the whole delivery story: templates are copied into user repos and only change when regenerated, whereas composite actions resolve at the pinned {{RADIUS_REF}} and reach existing repositories immediately.

Mechanism. The result file is written by trap cleanup EXIT, so it is written on every exit — including ones where no failure branch ran. The accumulator was optimistic: succeeded was seeded, never earned. This inverts that. The seed becomes interrupted/1, and succeeded/0 is assigned once, after the command loop.

Failure surface. A wrong verdict here is worse than a missing one: publish-deploy-status already has unknown → in_progress as its designed "no verdict" sentinel, and the optimistic seed defeated it with a confident wrong answer.

Review focus. Three things decide whether this is safe: (1) can any failure path now fall through into the promotion and be overwritten, (2) does interrupted map correctly downstream, (3) is the new test real evidence.

What the code changes

240 lines across 6 files; 193 of them are the new test.

File	Change
run-rad-commands/action.yml	Seed interrupted/1 (182-183); promote to succeeded/0 (517-521)
publish-deploy-status/action.yml	Comment only
command-outcome_test.sh	New regression test
publish-deploy-status_test.sh	interrupted → failed case
README.md, build/test.mk	Docs; test-command-outcome target

Control flow

The promotion at line 520 is unconditional, so the safety of this change rests entirely on every failure path exiting before it. I verified all eight assignment sites reach an exit:

Line	Outcome	Exits
377	disallowed_command	exit 2 (383, after the JSON append)
388, 444, 459, 468, 482, 488, 509	command_failed	exit 1

Both success paths — the rad_commands loop (ending 476) and the default-deploy else (ending 514) — join at the fi on 515 and fall into the promotion. Ending the block on an assignment does not change the step's exit status, since a plain assignment returns 0.

seed: interrupted/1
  ├─ failure branch ──> assign outcome ──> exit ────────┐
  ├─ killed mid-command ─────────────────────────────┐  │
  └─ loop completes ──> promote succeeded/0 ─────┐   │  │
                                                 ▼   ▼  ▼
                                          trap cleanup EXIT → write_result

Key implementation decisions

Why seed a non-success value rather than write the file only on success?
The artifact is deliberately written on every exit so a failed run still produces a complete commands array. Writing only on success would trade a wrong verdict for a missing artifact and lose the per-command detail. Seeding pessimistically keeps the artifact unconditional and makes success the thing that must be earned. Sound.

Why no change to publish-deploy-status?
Its case already ends in *) RUN_STATE="failed", so interrupted maps to failed without touching the consumer. I checked for other programmatic readers of .outcome in this repo and found none. The alternative — adding an explicit interrupted) arm — would document intent but adds a second place to update. Reasonable as-is, and the new consumer test case pins the behavior.

Why static assertions alongside the dynamic test?
The dynamic test can only prove the seed is pessimistic; it cannot prove succeeded is assigned late. The static assertions (181-191) cover ordering. Together they cover more than either alone — though see the gap below.

Reachability: what actually triggers this today

This is the part I'd most want an approver to check, because the PR's own framing is narrower than the bug it fixes.

timeout-minutes is not configured anywhere in .github/extension/ today — the only occurrence is prose this PR adds. So the headline scenario is not currently reachable. What is reachable:

Trigger	Step result	Published status	Fixed here?
set -e abort on an unguarded non-zero command	failed	published	✅ the real win
Catchable signal (SIGTERM, OOM, runner reclaim)	failed	published	✅
Job timeout (360m default) / manual cancel	cancelled	suppressed by !cancelled()	artifact only
SIGKILL	failed	no trap runs at all	n/a
Step timeout-minutes	failed	published	✅ once #12746 lands

The step declares shell: bash, which GitHub runs as bash --noprofile --norc -eo pipefail {0} — -e is active. So any unguarded non-zero command aborts the script mid-run, fires the trap, and publishes. Under the old seed that published succeeded for a genuinely failed deploy. That path exists today and needs no new configuration.

This also makes the PR a natural complement to #12746 ("Bound the deploy step with a validated timeout"), which adds the timeout-minutes bound and notes that composite actions cannot carry one themselves (actions/runner#1979). #12746 makes the timeout possible; this makes its outcome honest.

Suggested change: the code comment (172-180) and README paragraph should lead with the general invariant — any termination that skips the failure branches — and treat timeout-minutes as one instance, ideally cross-referencing #12746. As written, a future maintainer could reasonably conclude this only matters if someone adds a timeout.

Compatibility

• Artifact enum extended. interrupted is a new value for a field the design note calls a stable, frontend-consumed contract. Behaviorally safe: the note says the frontend reads the conclusion from exitCode, which is 1.
• deploy-state.txt carries the raw outcome (publish-deploy-status/action.yml:201-203 writes $OUTCOME, not the mapped $RUN_STATE), so it now emits state=interrupted. Low risk — the README states deploy-progress.json is the only file the canvas reads, and the previous value was actively wrong rather than merely novel.
• No change to secret handling, permissions, or command validation.

Test evidence and gaps

The test is stronger than typical for shell CI. It extracts the accumulator prologue verbatim from action.yml rather than copying it, scoped to the single Run rad commands step with exactly-one assertions; waits on a readiness marker instead of racing; asserts the result file is not written eagerly (so the assertions can't pass against a file the kill had no part in); sends a real SIGTERM; mirrors GitHub's -eo pipefail; and asserts the contract (not succeeded/success/unknown, exitCode != 0) rather than the literal interrupted, so the seed can change without breaking. It also handles grep -c exiting 1 under set -e.

What it does not prove:

• That a normal successful run reaches the promotion. The dynamic test sources only the prologue and never runs the command loop; the promotion is covered by static grep alone.
• That every failure branch exits. See below.
• Real GitHub step-timeout lifecycle, or that artifact upload survives a timeout.

I'd stop short of the comment's claim that it "models a step timeout faithfully" — it models the SIGTERM/trap mechanism, not the full composite-step timeout lifecycle.

Findings

Blocking (external, not this PR's logic)

1. main is syntactically invalid (above). This branch cannot rebase onto it, and mergeable is already CONFLICTING.

Non-blocking

2. No test enforces "every failure path exits." That invariant is what makes the unconditional promotion safe, and it's stated as an assumption in the comment at 517-519. A future contributor adding a failure branch that assigns an outcome without exiting would have it silently overwritten by the promotion, and every existing test would still pass. This is the highest-value assertion still missing — worth a follow-up.
3. Design note is now stale. eng/design-notes/environments/2026-06-repo-radius-deploy-workflow.md:61 enumerates the outcome as succeeded, command_failed, or disallowed_command. That note is marked Status: Draft and has been amended by later implementation PRs (Add custom recipe pack support and delete workflows to Repo Radius #12367, feat(sync): recipe pack pins and unified defaults.yaml schema #12567, fix(azure): keep federated assertions valid for the whole run #12751), so it reads as living rather than historical — and it explicitly calls this a stable frontend-facing contract. interrupted should be added there in this PR.
4. Doc framing over-indexes on timeout-minutes (see Reachability).

Rebase hazard, once main is fixed

5. Live graph support #12727 changed cleanup() to call stop_live_deploy_progress and radius_clear_artifact_runtime, defined in deploy-progress/progress.sh, which is sourced at line 166 — above the RESULT_FILE= anchor where this test begins its extraction. After rebasing, the extracted prologue will reference functions the sandbox never defines, and radius_clear_artifact_runtime is unguarded, so the SIGTERM test will likely fail before reaching write_result. The extractor or harness will need to stub or include those. This is why I'd no longer describe the conflict as merely textual.

Strengths

• The load-bearing invariant is real and verified: no failure path can reach the promotion.
• The fix is placed where it propagates to existing repositories without regeneration.
• The test exercises shipped code rather than a copy, and asserts a contract rather than a literal.
• Failure modes were narrowed correctly: unknown was deliberately avoided because it maps to the neutral in_progress.

Risk register

Risk	Likelihood	Impact	Detectability	Mitigation
Future non-exiting failure branch overwritten by promotion	Low	High (silent wrong verdict)	Poor — no test	Finding 2
External consumer rejects unknown interrupted	Low	Medium	Medium	exitCode unchanged in meaning
Rebase breaks the prologue extraction	High	Low (CI catches it)	Good	Finding 5
Seed misread as "still running"	Very low	Low	Good	*) → failed; test pins it

Recommendation

Approve with follow-up — but do not merge yet.

The core change is small, the invariant it depends on is verified, and the test is genuine evidence rather than a formality. Nothing in the logic blocks approval. It cannot merge, though, until main is repaired and this branch is rebased, and that rebase is likely to require adapting the test to #12727's cleanup() dependencies.

I'd like to fold finding 3 (design note) into this PR since the note is a live contract doc, and take findings 2 and 4 either here or as a fast follow.

What would change this: evidence that a failure branch can reach the promotion, or that an external consumer treats the outcome as a closed enum, would turn this into Request changes.

Coverage and confidence

• Inspected: full diff vs merge base b4ebe2f28; the complete Run rad commands run block; publish-deploy-status mapping and status-file writers; both deploy workflow templates; the design note; main's versions of all five overlapping files.
• Checks run: command-outcome_test.sh ✅, publish-deploy-status_test.sh ✅, shellcheck (repo rcfile) ✅, bash -n on both branches' run blocks, PR checks (required checks green; the earlier statestore-noncloud failure was an unrelated control-plane 503 flake that also hit Add CI coverage for database.enabled=true install #12771 minutes later and passed on re-run).
• Not covered: behavior on a real GitHub-hosted runner under an actual step timeout, and the external frontend's parsing of the outcome field — neither is reproducible from this repo.
• Confidence: High on control flow and consumer mapping (directly verified); Medium on external artifact compatibility.

[radius-functional-tests]

Radius functional test overview

🔍 Go to test action run

Click here to see the test run details

Name	Value
Repository	radius-project/radius
Commit ref	7b839ca
Unique ID	func7cb39bb9a0
Image tag	pr-func7cb39bb9a0

• Dapr: 1.14.4
• Azure KeyVault CSI driver: 1.4.2
• Azure Workload identity webhook: 1.3.0
• Bicep recipe location ghcr.io/radius-project/dev/test/testrecipes/test-bicep-recipes/<name>:pr-func7cb39bb9a0
• Terraform recipe location http://tf-module-server.radius-test-tf-module-server.svc.cluster.local/<name>.zip (in cluster)
• applications-rp test image location: ghcr.io/radius-project/dev/applications-rp:pr-func7cb39bb9a0
• dynamic-rp test image location: ghcr.io/radius-project/dev/dynamic-rp:pr-func7cb39bb9a0
• controller test image location: ghcr.io/radius-project/dev/controller:pr-func7cb39bb9a0
• ucp test image location: ghcr.io/radius-project/dev/ucpd:pr-func7cb39bb9a0
• deployment-engine test image location: ghcr.io/radius-project/deployment-engine:latest

Test Status

⌛ Building Radius and pushing container images for functional tests...
✅ Container images build succeeded
⌛ Publishing Bicep Recipes for functional tests...
✅ Recipe publishing succeeded
⌛ Starting ucp-cloud functional tests...
⌛ Starting corerp-cloud functional tests...
✅ ucp-cloud functional tests succeeded
✅ corerp-cloud functional tests succeeded