Skip to content

fix(generate): judge a retries ladder on its effective result - #627

Merged
joshua-temple merged 1 commit into
mainfrom
fix/retries-effective-result
Jul 17, 2026
Merged

fix(generate): judge a retries ladder on its effective result#627
joshua-temple merged 1 commit into
mainfrom
fix/retries-effective-result

Conversation

@joshua-temple

Copy link
Copy Markdown
Collaborator

Problem

A GitHub Actions job result is immutable. When a deploy declares retries: N, a base job that fails and is then rescued by a retry shim leaves needs.deploy-web.result pinned at failure for the whole run, even though the environment really was deployed.

Four sinks read that frozen result: the finalize failure gate, the manifest update's <NAME>_RESULT, the run summary, and the native Deployment status. Verified on emitted output at origin/main:

167:  WEB_RESULT: ${{ needs.deploy-web.result }}
187:  if [[ "$WEB_RESULT" == "success" ]]; then
280:- name: Check for Failures
281:  if: contains(fromJSON('["failure","cancelled"]'), needs.deploy-web.result) ...

So the retry deployed, the manifest denied it, and the run went red. Update Manifest (161) runs before Check for Failures (280), so it first pushed a state commit recording the environment's sha and version without deploys.web, and then failed the run. The redness masked the wrong-but-green state write. docs/.../callbacks.md promised "retry up to N times before final failure"; the emitted output contradicted it.

e2e/scenarios/73 missed this because its callback always fails, so base-fail then retry-SUCCEED was never exercised.

Fix

An effective result per ladder: did any attempt succeed?

  • Success is a disjunction over success, not over failure. A shim that never ran reports skipped, not failure, so only success positively signals an attempt completed the work.
  • Effective failure keeps the failure/cancelled anchor and adds "unless a retry rescued it". A bare !success would turn a base job skipped by non-matching triggers into a spurious failure.
  • The ladder clause is parenthesized rather than resting on && binding tighter than ||.
  • With retries: 0 it collapses to the bare base result, so output is byte-identical.

Ordering is deliberately unchanged. Finalize runs under always() to record the state a run actually reached; gating the write correctly is what makes that record honest. Reordering would suppress the write that always() exists to guarantee.

retries in promote is scoped, not implemented. promote.go references none of Retries/OnFailure/RunPolicy. A promote deploy declaring inputs compiles to a matrix job fanned across environments whose result is a single aggregate, so a caller-side shim would redeploy environments that already succeeded, and GHA cannot re-run only the failed legs of a dependency's matrix. Retrying a healthy production environment because an unrelated one failed is worse than not retrying. Docs now scope the claim to trunk runs; design tracked in #626.

Verification

  • 8 new tests in internal/generate/effective_result_test.go, written first and confirmed failing: 4 red at baseline, with the zero-retries and all-attempts-fail controls green throughout. Cases pinned: N=0, base succeeds, a middle shim succeeds, all attempts fail, plus skipped-vs-failure and the precedence grouping.
  • Byte-identical proof: generated a non-retries manifest with pre-change and post-change binaries; diff -r reports identical. No golden churn occurred, because no golden fixture declares retries.
  • go build, go test (3371), go test -race (3371), golangci-lint: clean. e2e: build + vet clean. cascade verify --own-repo: 3 files, no drift. actionlint on emitted output: no expression errors.
  • e2e: the harness cannot express fail-once-then-succeed. Each shim is a separate job with its own workspace, act runs without an artifact server so artifacts/cache cannot carry state across jobs, and the callback cannot self-discriminate (identical with: inputs; github.job is the inner id for all three). Rather than fabricate a scenario, scenario 73 now pins the effective-result expressions as emitted text and records why the runtime half is absent.

A GitHub Actions job result is immutable, so a callback declaring retries
whose base job failed reported needs.<job>.result == 'failure' for the rest
of the run even after a retry shim re-invoked the workflow and succeeded.

Four sinks read that frozen result: the finalize failure gate, the manifest
update's <NAME>_RESULT, the run summary, and the native Deployment status.
A deploy rescued by a retry therefore failed the run, and because the
manifest update runs before the failure gate it first pushed a state commit
recording the environment's sha and version while refusing to record the
deploy that had actually happened.

Each now consults the ladder's effective result: did any attempt succeed.
The disjunction is over success rather than failure because a shim that
never ran reports 'skipped', not 'failure'. Effective failure keeps the
failure/cancelled anchor so a base job skipped by non-matching triggers
stays a routine skip instead of becoming a spurious failure, and the ladder
clause is parenthesized rather than resting on operator precedence.

The step order is deliberately unchanged: finalize runs under always() to
record the state a run actually reached, and gating the write correctly is
what makes that record honest. Reordering would suppress it instead.

A manifest without retries emits byte-identical output.

Signed-off-by: Joshua Temple <joshua.temple@stablekernel.com>
@joshua-temple
joshua-temple force-pushed the fix/retries-effective-result branch from 7d751f6 to 23bc70e Compare July 17, 2026 09:44
@joshua-temple
joshua-temple merged commit d9132df into main Jul 17, 2026
21 checks passed
@joshua-temple
joshua-temple deleted the fix/retries-effective-result branch July 17, 2026 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant