From 23bc70e88a359d5d3f470c497ea3bc738cf2dddf Mon Sep 17 00:00:00 2001 From: Joshua Temple Date: Fri, 17 Jul 2026 05:19:08 -0400 Subject: [PATCH] fix(generate): judge a retries ladder on its effective result A GitHub Actions job result is immutable, so a callback declaring retries whose base job failed reported needs..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 _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 --- CHANGELOG.md | 23 ++ docs/src/content/docs/reference/callbacks.md | 4 +- docs/src/content/docs/reference/manifest.md | 10 +- e2e/scenarios/73-deploy-retries.yaml | 36 ++++ internal/generate/effective_result_test.go | 213 +++++++++++++++++++ internal/generate/generator.go | 118 ++++++++-- 6 files changed, 387 insertions(+), 17 deletions(-) create mode 100644 internal/generate/effective_result_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 6683b115..6d8faf61 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,19 @@ A `Migration` section is added to any release that bumps `schema_version`. ### Fixed +- **generate:** A callback declaring `retries` is now judged on its ladder's + effective result, so a deploy that fails and is then rescued by a retry no + longer fails the run or gets denied in recorded state. A GitHub Actions job + result is immutable: once the base job ended in `failure`, `needs..result` + reported `failure` for the rest of the run even after a retry shim redeployed + the environment successfully. The finalize failure gate, the manifest update, + the run summary, and the native Deployment status all read that frozen result, + so a successful retry still went red, and the manifest recorded the + environment's sha and version while refusing to record the deploy that had in + fact happened. Those four now ask whether any attempt in the base-plus-retry + ladder succeeded. A manifest that does not use `retries` emits byte-identical + output. + - **release:** The stale-draft reaper now sees every release instead of only the 30 most recent. The release list is paginated by the GitHub API, and the listing read a single response without requesting a page size or following the @@ -204,6 +217,16 @@ A `Migration` section is added to any release that bumps `schema_version`. - **git:** Pass `--` before the tag pattern in prefix-scoped `git tag -l` lookups as defense in depth beneath the new prefix validation. +### Documentation + +- **manifest, callbacks:** `run_policy`, `on_failure`, and `retries` are now + documented as applying to trunk runs only. The promote generator never emitted + any of the three, so a promotion ran each deploy once regardless of what the + manifest declared. A promote-side retry needs a per-environment design, because + a promote deploy declaring `inputs` compiles to a matrix job whose single + aggregate result would make a retry redeploy every environment including the + ones that already succeeded ([#626](https://github.com/stablekernel/cascade/issues/626)). + ### Added - **test:** A durable emitted-field guard: a reflection walk over the diff --git a/docs/src/content/docs/reference/callbacks.md b/docs/src/content/docs/reference/callbacks.md index c16d425b..5675d606 100644 --- a/docs/src/content/docs/reference/callbacks.md +++ b/docs/src/content/docs/reference/callbacks.md @@ -523,7 +523,9 @@ Callback failures are handled by the `on_failure` policy: | `abort` | Fail the entire workflow | | `continue` | Other callbacks proceed | -With `retries: N`, failed callbacks retry up to N times before final failure. +With `retries: N`, a failed callback is retried up to N times before it counts as a final failure. Each retry is a separate job that re-invokes the same reusable workflow, and the chain stops at the first attempt that succeeds. A callback that fails and is then rescued by a retry counts as a success: the run stays green and the deploy is recorded in state. + +`run_policy`, `on_failure`, and `retries` apply to trunk runs (`orchestrate.yaml`). They are not emitted into `promote.yaml`; see [Policy fields](/reference/manifest/#policy-fields). ## Migrating from inline `run:`/`shell:` callbacks diff --git a/docs/src/content/docs/reference/manifest.md b/docs/src/content/docs/reference/manifest.md index 50bca6b3..a426cda9 100644 --- a/docs/src/content/docs/reference/manifest.md +++ b/docs/src/content/docs/reference/manifest.md @@ -345,7 +345,7 @@ ci: | `env_inputs` | emitted | map | {} | Per-environment input overrides. | | `run_policy` | emitted | string | `default` | Execution policy. See [Policy fields](#policy-fields). | | `on_failure` | emitted | string | `abort` | Failure handling. See [Policy fields](#policy-fields). | -| `retries` | emitted | int | 0 | Retry attempts (0-3). | +| `retries` | emitted | int | 0 | Retry attempts (0-3). Trunk runs only. See [Policy fields](#policy-fields). | ## builds @@ -1047,6 +1047,14 @@ versioning](/cascade/reference/versioning/#per-component-versioning). `run_policy`, `on_failure`, and `retries` apply to `validate`, each `builds` entry, and each `deploys` entry. +:::caution[Trunk runs only] +These three fields shape `orchestrate.yaml`, the workflow that runs on a trunk merge. They are **not** emitted into `promote.yaml`, so a promotion runs each deploy once, with no retry and no `on_failure` handling. + +`retries` is deliberately not emitted into `promote.yaml`. A promote deploy that declares `inputs` compiles to a matrix job fanned across environments, and a matrix job reports one aggregate result: `failure` if any environment failed. A retry gated on that aggregate would re-invoke the callback for **every** environment, redeploying the ones that already succeeded, and GitHub Actions offers no way for a calling workflow to re-run only the failed legs of a dependency's matrix. Retrying a healthy production environment because an unrelated environment failed is worse than not retrying, so a promote-side retry needs a design that retries per environment. Track it in [issue #626](https://github.com/stablekernel/cascade/issues/626). + +To make a promotion resilient to a transient failure, handle the retry inside the reusable workflow the deploy points at, where it can be scoped to the one environment that failed. +::: + | `run_policy` | Behavior | |--------------|----------| | `default` | Skip if any dependency was skipped. | diff --git a/e2e/scenarios/73-deploy-retries.yaml b/e2e/scenarios/73-deploy-retries.yaml index 9f4b7d0b..3a766f82 100644 --- a/e2e/scenarios/73-deploy-retries.yaml +++ b/e2e/scenarios/73-deploy-retries.yaml @@ -29,6 +29,30 @@ description: | occurrences. Asserting a per-shim conclusion here would silently read the wrong job, so it is left out rather than faked. + WHY base-fail-then-retry-SUCCEED IS NOT EXERCISED HERE, for the next author: + the case that matters most (the base attempt fails, a shim re-runs and + SUCCEEDS, so the environment really is deployed) cannot be expressed by this + harness, and the gap is structural rather than an omission: + + 1. Each shim is a separate GitHub Actions job, so it gets its own container + and its own workspace. A marker file written by the first attempt is gone + by the second, so the callback cannot count its own invocations locally. + 2. Artifacts and the cache are the canonical cross-job state channels, and + both are unavailable: act is launched without an artifact server path + (harness/act.go), so upload-artifact/download-artifact cannot round-trip. + 3. The callback body cannot self-discriminate which attempt it is either. The + generator emits identical with: inputs for the base job and every shim, and + inside a reusable workflow github.job resolves to the inner job id, which + is the same string for all three invocations. + + A callback that fails once and then succeeds therefore has no way to keep state + across attempts here. Rather than fabricate a scenario that cannot prove the + claim, the runtime half is left unexercised and the effective-result gates are + pinned as emitted text above, with the four ladder shapes (no shims, base + succeeds, a middle shim succeeds, all attempts fail) covered as unit tests in + internal/generate/effective_result_test.go. Restoring the executing proof needs + an artifact server in the harness, not a change to this scenario. + INNER-JOB-ID NAMESPACE TRAP, for the next author: expect.jobs entries are resolved by findJob (assert.go), which strips a "build-"/"deploy-" prefix and looks the remainder up among the INNER job ids of the called workflows. The @@ -95,9 +119,21 @@ steps: # chain is sequential rather than every shim firing off the original. - "if: needs.deploy-web.result == 'failure'" - "if: needs.deploy-web-retry-1.result == 'failure'" + # A job result is immutable, so needs.deploy-web.result stays + # 'failure' for the whole run even after a shim redeploys the + # environment successfully. Both the finalize failure gate and the + # manifest-update gate must therefore judge the ladder's effective + # result ("did any attempt succeed?") rather than the base job's + # frozen one. The runtime half of this cannot be exercised here (see + # the note below), so the expressions are pinned as emitted text. + - "WEB_RESULT: ${{ (needs.deploy-web.result == 'success' || needs.deploy-web-retry-1.result == 'success' || needs.deploy-web-retry-2.result == 'success') && 'success' || 'failure' }}" + - "!(needs.deploy-web-retry-1.result == 'success' || needs.deploy-web-retry-2.result == 'success')" not_contains: # retries: 2 emits exactly two shims. - "deploy-web-retry-3:" + # The manifest gate must never bind to the immutable base result: that + # is what denied a retry-rescued deploy in recorded state. + - "WEB_RESULT: ${{ needs.deploy-web.result }}" - name: "Orchestrate: the deploy callback genuinely fails at runtime" action: orchestrate diff --git a/internal/generate/effective_result_test.go b/internal/generate/effective_result_test.go new file mode 100644 index 00000000..e2af4fe4 --- /dev/null +++ b/internal/generate/effective_result_test.go @@ -0,0 +1,213 @@ +package generate + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stablekernel/cascade/internal/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// A GitHub Actions job result is immutable: once deploy-web ends in 'failure', +// needs.deploy-web.result reports 'failure' for the rest of the run even when a +// retry shim re-invokes the same workflow and succeeds. Any gate that reads the +// base job's result alone therefore contradicts what actually happened on the +// environment. These tests pin the effective result, meaning "did any attempt in +// the base-plus-shim ladder succeed?", across the four shapes a ladder can take. + +// retriesFixture writes a deploy callback workflow and returns a config whose +// single deploy declares the requested retry count. +func retriesFixture(t *testing.T, retries int) (*config.TrunkConfig, string) { + t.Helper() + + tmpDir := t.TempDir() + require.NoError(t, os.MkdirAll(filepath.Join(tmpDir, ".github/workflows"), 0o755)) + + deployWorkflow := ` +name: Deploy Web +on: + workflow_call: + inputs: + environment: + type: string +` + require.NoError(t, os.WriteFile( + filepath.Join(tmpDir, ".github/workflows/deploy.yaml"), + []byte(deployWorkflow), 0o644)) + + cfg := &config.TrunkConfig{ + TrunkBranch: "main", + Environments: config.EnvNames("dev"), + Deploys: []config.DeployConfig{ + { + Name: "web", + Workflow: ".github/workflows/deploy.yaml", + Triggers: []string{"src/**"}, + Retries: retries, + }, + }, + } + return cfg, tmpDir +} + +// TestEffectiveResult_FailureGate_ToleratesSucceedingRetry is the core F1 +// regression. With retries: 2, a run where deploy-web fails and +// deploy-web-retry-1 succeeds has genuinely deployed the environment, so the +// finalize failure gate must not fire. Before the fix the gate read only +// needs.deploy-web.result, which is frozen at 'failure', and the run went red +// even though the retry succeeded. +func TestEffectiveResult_FailureGate_ToleratesSucceedingRetry(t *testing.T) { + cfg, tmpDir := retriesFixture(t, 2) + + result, err := NewGenerator(cfg, tmpDir).Generate() + require.NoError(t, err) + + // The failure gate must consult the retry shims, not the base job alone. + assert.Contains(t, result, + "!(needs.deploy-web-retry-1.result == 'success' || needs.deploy-web-retry-2.result == 'success')", + "failure gate must exonerate a ladder in which some attempt succeeded") +} + +// TestEffectiveResult_FailureGate_LadderClauseIsParenthesized guards the gate's +// grouping. Callbacks are joined with " || ", so an unparenthesized ladder +// clause would read "A || B && !C" and rely on GitHub Actions binding && more +// tightly than ||. That happens to be true, but a gate deciding whether a run +// goes red must not rest on implicit precedence: the clause is grouped as a unit. +func TestEffectiveResult_FailureGate_LadderClauseIsParenthesized(t *testing.T) { + cfg, tmpDir := retriesFixture(t, 2) + // A second, retry-free callback forces the " || " join that makes grouping matter. + cfg.Builds = []config.BuildConfig{{ + Name: "app", + Workflow: ".github/workflows/deploy.yaml", + Triggers: []string{"src/**"}, + }} + + result, err := NewGenerator(cfg, tmpDir).Generate() + require.NoError(t, err) + + assert.Contains(t, result, + "(contains(fromJSON('[\"failure\", \"cancelled\"]'), needs.deploy-web.result) && "+ + "!(needs.deploy-web-retry-1.result == 'success' || needs.deploy-web-retry-2.result == 'success'))", + "the ladder clause must be parenthesized as a unit, not left to operator precedence") +} + +// TestEffectiveResult_ManifestGate_RecordsSucceedingRetry is the second half of +// F1. The manifest update gates the deploys.web.* yq edits on WEB_RESULT, which +// was bound to the immutable base result. A retry that deployed the environment +// was therefore denied in recorded state. +func TestEffectiveResult_ManifestGate_RecordsSucceedingRetry(t *testing.T) { + cfg, tmpDir := retriesFixture(t, 2) + + result, err := NewGenerator(cfg, tmpDir).Generate() + require.NoError(t, err) + + assert.NotContains(t, result, "WEB_RESULT: ${{ needs.deploy-web.result }}", + "WEB_RESULT must not bind to the immutable base result when retries are declared") + assert.Contains(t, result, + "WEB_RESULT: ${{ (needs.deploy-web.result == 'success' || needs.deploy-web-retry-1.result == 'success' || needs.deploy-web-retry-2.result == 'success') && 'success' || 'failure' }}", + "WEB_RESULT must reflect whether any attempt in the ladder succeeded") +} + +// TestEffectiveResult_ZeroRetries_IsUnchanged pins the frozen-schema promise: +// a manifest that does not use retries must emit exactly what it emits today. +// The effective-result expression must collapse to the bare base result when +// there are no shims to consult. +func TestEffectiveResult_ZeroRetries_IsUnchanged(t *testing.T) { + cfg, tmpDir := retriesFixture(t, 0) + + result, err := NewGenerator(cfg, tmpDir).Generate() + require.NoError(t, err) + + assert.Contains(t, result, "WEB_RESULT: ${{ needs.deploy-web.result }}", + "a retry-free deploy must keep the bare base result") + assert.Contains(t, result, + "contains(fromJSON('[\"failure\", \"cancelled\"]'), needs.deploy-web.result)", + "a retry-free deploy must keep today's failure gate verbatim") + // Scoped to the ladder: the state-push shell legitimately says "retrying". + assert.NotContains(t, result, "deploy-web-retry-", "a retry-free deploy must emit no ladder") +} + +// TestEffectiveResult_SkippedBaseIsNotAFailure guards the crux of the design. +// A shim whose predecessor did not fail is 'skipped', NOT 'failure', and a base +// job whose triggers did not match is also 'skipped'. Expressing effective +// failure as "not success" would turn both of those into spurious run failures. +// The gate must stay anchored on the base job's failure/cancelled states and +// only then ask whether a retry rescued it. +func TestEffectiveResult_SkippedBaseIsNotAFailure(t *testing.T) { + cfg, tmpDir := retriesFixture(t, 2) + + result, err := NewGenerator(cfg, tmpDir).Generate() + require.NoError(t, err) + + // The gate keeps the failure/cancelled anchor, so a skipped base (triggers + // did not match) never trips it, and it never degrades to a bare !success. + assert.Contains(t, result, + "contains(fromJSON('[\"failure\", \"cancelled\"]'), needs.deploy-web.result) && "+ + "!(needs.deploy-web-retry-1.result == 'success' || needs.deploy-web-retry-2.result == 'success')", + "effective failure must anchor on the base failing, then ask if a retry rescued it") + assert.NotContains(t, result, "!(needs.deploy-web.result == 'success')", + "a bare !success gate would turn a skipped base into a spurious failure") +} + +// TestEffectiveResult_AllAttemptsFail_StillFails is the negative control: the +// fix must not make a genuinely failed ladder look green. When the base and +// every shim fail, the gate must still fire and the manifest must still refuse +// to record the deploy. +func TestEffectiveResult_AllAttemptsFail_StillFails(t *testing.T) { + cfg, tmpDir := retriesFixture(t, 2) + + result, err := NewGenerator(cfg, tmpDir).Generate() + require.NoError(t, err) + + // The gate is still present and still exits non-zero. + assert.Contains(t, result, "- name: Check for Failures") + assert.Contains(t, result, "exit 1") + // The manifest write is still conditional rather than unconditional. + assert.Contains(t, result, `if [[ "$WEB_RESULT" == "success" ]]; then`) +} + +// TestEffectiveResult_Promote_DoesNotEmitRetryLadder pins the scope decision for +// F2, so that a later author who adds a promote ladder must confront the reason +// one is not there today rather than discovering it in production. +// +// retries is an orchestrate-only capability. A promote deploy that declares +// inputs compiles to a matrix job fanned across environments with +// fail-fast: false (see writeDeployJobs), and a matrix job exposes a single +// aggregate result: 'failure' if ANY leg failed. A caller-side shim gated on +// that aggregate re-invokes the reusable workflow for EVERY leg, redeploying the +// environments that already succeeded. GitHub Actions offers no caller-side way +// to re-run only the failed legs of a dependency's matrix, so a ladder here +// would redeploy healthy production environments on account of an unrelated +// environment's transient failure. That is worse than no retry. +// +// Emitting a ladder only on the non-matrix path was rejected too: it would make +// retries silently work or not work depending on whether the deploy happened to +// declare inputs. The documented claim is scoped to orchestrate instead. +func TestEffectiveResult_Promote_DoesNotEmitRetryLadder(t *testing.T) { + for _, retries := range []int{0, 2} { + cfg, tmpDir := retriesFixture(t, retries) + + result, err := NewPromoteGenerator(cfg, tmpDir).Generate() + require.NoError(t, err) + + assert.NotContains(t, result, "deploy-web-retry-", + "promote emits no retry ladder at retries=%d; see this test's rationale", retries) + } +} + +// TestEffectiveResult_Promote_RetriesDoNotPerturbOutput proves retries is inert +// on the promote path rather than partially wired: a manifest that declares +// retries must emit byte-identical promote output to one that does not. +func TestEffectiveResult_Promote_RetriesDoNotPerturbOutput(t *testing.T) { + withRetries, dirA := retriesFixture(t, 2) + without, dirB := retriesFixture(t, 0) + + a, err := NewPromoteGenerator(withRetries, dirA).Generate() + require.NoError(t, err) + b, err := NewPromoteGenerator(without, dirB).Generate() + require.NoError(t, err) + + assert.Equal(t, b, a, "retries must not perturb promote output while it is orchestrate-only") +} diff --git a/internal/generate/generator.go b/internal/generate/generator.go index bd01e627..d8ecdf47 100644 --- a/internal/generate/generator.go +++ b/internal/generate/generator.go @@ -1626,19 +1626,27 @@ func (g *Generator) writeNativeDeploymentSteps(sb *strings.Builder, sorted []str envExpr = fmt.Sprintf("${{ github.event.inputs.environment || '%s' }}", g.config.Environments[0].Name) } - // Collect deploy job IDs so the terminal status reflects the real deploy - // outcome. The deployment succeeds only when every deploy callback succeeded. - var deployJobs []string + // Collect deploy callbacks so the terminal status reflects the real deploy + // outcome. The deployment succeeds only when every deploy callback succeeded, + // judged on each callback's effective result so a deploy rescued by a retry + // shim reports the Deployment as successful rather than failed. + var deployJobs []CallbackInfo for _, jobID := range sorted { - if g.graph.Nodes[jobID].Type == config.CallbackTypeDeploy { - deployJobs = append(deployJobs, jobID) + if info := g.graph.Nodes[jobID]; info.Type == config.CallbackTypeDeploy { + deployJobs = append(deployJobs, info) } } resultExpr := "success" if len(deployJobs) > 0 { var conds []string - for _, jobID := range deployJobs { - conds = append(conds, fmt.Sprintf("needs.%s.result == 'success'", jobID)) + for _, info := range deployJobs { + cond := effectiveSuccessCond(info.JobID, info.Retries) + // Parenthesize a ladder's disjunction so it cannot bind loosely + // against the surrounding && chain. + if info.Retries > 0 { + cond = "(" + cond + ")" + } + conds = append(conds, cond) } resultExpr = fmt.Sprintf("${{ (%s) && 'success' || 'failure' }}", strings.Join(conds, " && ")) } @@ -1661,8 +1669,11 @@ func (g *Generator) writeSummaryStep(sb *strings.Builder, sorted []string) { if onFailure == "" { onFailure = config.OnFailureAbort } - // Use DisplayName for the table and JobID for the needs reference - fmt.Fprintf(sb, " echo \"| %s | ${{ needs.%s.result }} | %s |\" >> \"$GITHUB_STEP_SUMMARY\"\n", info.DisplayName, info.JobID, onFailure) + // Use DisplayName for the table and JobID for the needs reference. A + // callback with retries reports its ladder's effective result so the + // summary agrees with the run's verdict rather than reporting the + // immutable first-attempt failure of a deploy a retry went on to rescue. + fmt.Fprintf(sb, " echo \"| %s | %s | %s |\" >> \"$GITHUB_STEP_SUMMARY\"\n", info.DisplayName, effectiveResultExpr(info.JobID, info.Retries), onFailure) } // Add outputs section to summary (only if there are outputs with values) @@ -1726,11 +1737,14 @@ func (g *Generator) writeManifestUpdateStep(sb *strings.Builder, sorted []string fmt.Fprintf(sb, " ENVIRONMENT: ${{ github.event.inputs.environment || '%s' }}\n", g.config.Environments[0].Name) } - // Add env vars for each deploy result + // Add env vars for each deploy result. A deploy declaring retries reports its + // ladder's effective result: the base job's result is immutable, so a deploy + // that failed and was then rescued by a retry shim would otherwise be denied + // in recorded state even though the environment really was deployed. for _, d := range g.config.Deploys { envName := strings.ToUpper(strings.ReplaceAll(d.Name, "-", "_")) jobName := fmt.Sprintf("deploy-%s", d.Name) - fmt.Fprintf(sb, " %s_RESULT: ${{ needs.%s.result }}\n", envName, jobName) + fmt.Fprintf(sb, " %s_RESULT: %s\n", envName, effectiveResultExpr(jobName, d.Retries)) } // Add env vars for build artifact IDs. Only emitted when the build @@ -1941,7 +1955,7 @@ func (g *Generator) writeNotifyPrimaryStep(sb *strings.Builder) { func (g *Generator) writeFailureCheckStep(sb *strings.Builder, sorted []string) { // Collect callbacks with on_failure: abort (default behavior) - var abortCallbacks []string + var abortCallbacks []CallbackInfo for _, jobID := range sorted { info := g.graph.Nodes[jobID] onFailure := info.OnFailure @@ -1949,7 +1963,7 @@ func (g *Generator) writeFailureCheckStep(sb *strings.Builder, sorted []string) onFailure = config.OnFailureAbort } if onFailure == config.OnFailureAbort { - abortCallbacks = append(abortCallbacks, info.JobID) + abortCallbacks = append(abortCallbacks, info) } } @@ -1963,9 +1977,11 @@ func (g *Generator) writeFailureCheckStep(sb *strings.Builder, sorted []string) // Build condition that only checks abort callbacks. A cancelled predecessor // (e.g. a run superseded by a newer push under cancel-in-progress) is treated // the same as a failure so a mid-flight cancellation is not silently tolerated. + // A callback declaring retries is judged on its ladder's effective result, so + // an attempt that failed and was then rescued by a shim does not fail the run. var conditions []string - for _, jobName := range abortCallbacks { - conditions = append(conditions, failureOrCancelledCond(jobName)) + for _, info := range abortCallbacks { + conditions = append(conditions, effectiveFailureOrCancelledCond(info.JobID, info.Retries)) } fmt.Fprintf(sb, " if: %s\n", strings.Join(conditions, " || ")) @@ -1983,6 +1999,78 @@ func failureOrCancelledCond(jobName string) string { return fmt.Sprintf("contains(fromJSON('[\"failure\", \"cancelled\"]'), needs.%s.result)", jobName) } +// retrySucceededCond builds the "some shim rescued it" half of a ladder's +// effective result: a disjunction over each retry shim's success. It returns +// the empty string when the callback declares no retries, which is what lets +// the effective-result helpers collapse to their pre-retry form. +func retrySucceededCond(jobName string, retries int) string { + if retries <= 0 { + return "" + } + conds := make([]string, 0, retries) + for i := 1; i <= retries; i++ { + conds = append(conds, fmt.Sprintf("needs.%s-retry-%d.result == 'success'", jobName, i)) + } + return strings.Join(conds, " || ") +} + +// effectiveSuccessCond builds a condition that matches when ANY attempt in a +// callback's ladder succeeded: the base job, or any retry shim. +// +// A GitHub Actions job result is immutable. When a callback declares retries, +// a base job that fails and is then rescued by a shim leaves needs..result +// pinned at 'failure' for the whole run, even though the work completed. Reading +// the base result alone therefore reports the opposite of what happened. +// +// The disjunction is over success rather than over failure because a shim that +// never ran reports 'skipped', not 'failure': shims are gated on their +// predecessor failing, so once an attempt succeeds every later shim skips. Only +// 'success' is a positive signal that an attempt actually completed the work, so +// asking "did any attempt succeed?" is correct for every ladder shape, while +// asking "did the last attempt not fail?" would read a skipped shim as success. +func effectiveSuccessCond(jobName string, retries int) string { + cond := fmt.Sprintf("needs.%s.result == 'success'", jobName) + if rescued := retrySucceededCond(jobName, retries); rescued != "" { + cond += " || " + rescued + } + return cond +} + +// effectiveResultExpr renders a callback's effective result as a ${{ }} +// expression evaluating to the string 'success' or 'failure', suitable for an +// env: value that shell then compares against "success". +// +// With no retries it collapses to the bare needs..result, so a manifest +// that does not use retries emits byte-identical output. +func effectiveResultExpr(jobName string, retries int) string { + if retries <= 0 { + return fmt.Sprintf("${{ needs.%s.result }}", jobName) + } + return fmt.Sprintf("${{ (%s) && 'success' || 'failure' }}", effectiveSuccessCond(jobName, retries)) +} + +// effectiveFailureOrCancelledCond builds a condition that matches when a +// callback's ladder genuinely failed: the base job failed or was cancelled AND +// no retry shim rescued it. +// +// The failure/cancelled anchor is load-bearing and deliberately not rewritten as +// a negated success. A base job whose triggers did not match reports 'skipped', +// which is neither a failure nor something a retry can rescue; gating on +// !success would turn that routine skip into a spurious run failure. Anchoring +// on the base's failure/cancelled states first preserves today's semantics for +// every non-retry shape and adds only the "unless a retry rescued it" exemption. +// The returned clause is parenthesized as a unit whenever it carries the retry +// exemption. Callers join these with " || ", and although GitHub Actions binds +// && tighter than || (so the grouping would hold implicitly today), a gate that +// decides whether a run goes red should not rest on implicit precedence. +func effectiveFailureOrCancelledCond(jobName string, retries int) string { + cond := failureOrCancelledCond(jobName) + if rescued := retrySucceededCond(jobName, retries); rescued != "" { + cond = fmt.Sprintf("(%s && !(%s))", cond, rescued) + } + return cond +} + // ownRepoCLIArtifactName is the workflow-artifact name under which the build-cli // callback publishes the cascade binary built from the commit under release, and // which cascade's own-repo finalize job downloads to run the release. The