fix(generate): gate retry shims and their dependents with a status function - #645
Merged
Conversation
…nction A generated retry shim gated its re-invocation with a bare boolean expression (if: needs.<prev>.result == 'failure'). GitHub applies an implicit needs-success requirement to any job whose if has no status-check function, so the shim was skipped exactly when the previous attempt failed, which is the only time it should run. The rescue never fired on real GitHub; the live fleet caught it (base=failure, retry=skipped) while act, which evaluates if leniently, ran the shim and kept the e2e harness green. The same flaw hit a dependent of a retried deploy: its effective-result OR gate references the base and its shims, but with no status function the dependent was skipped the moment the base failed, before the OR could rescue it via a shim (#639 fixed the needs list, not the if gate). Emit the retry shim gate as ${{ !cancelled() && needs.<prev>.result == 'failure' }} and prefix a default-policy dependent of a retried dependency with !cancelled(). !cancelled() (not always()) is used so a cancelled run does not force a re-deploy; the == 'failure' clause already excludes a cancelled predecessor, so the status function only governs whole-run cancellation. The wrap is required because a bare YAML scalar may not begin with the "!" tag indicator. The no-retries and non-retried-dependent paths are byte-identical. Add a generation-correctness assertion pinning the shim and dependent if gates (and locking the bare form out), extend the censused depends_on assertions (GM5/GM7) with the status-function check, update the retries e2e scenario, and codify the general rule in CONTRIBUTING. Signed-off-by: Joshua Temple <joshua.temple@stablekernel.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
cascade's retry-rescue feature was broken on real GitHub. A generated retry shim gated its re-invocation with a bare boolean expression:
GitHub applies an implicit needs-success requirement to any job whose
ifcontains no status-check function (always(),!cancelled(),success(),failure(),cancelled()). A bare boolean does not lift that implicit gate, so whendeploy-appfails,deploy-app-retry-1is skipped rather than run. The rescue never fired.The same flaw hit a dependent of a retried deploy: its effective-result OR gate references the base plus its shims, but with no status function the dependent was skipped the moment the base failed, before the OR could rescue it. (#639 fixed the dependent's
needs:list but not itsifgate.)finalizealready usedif: always() && ...correctly; only the retry shims and dependents-of-retried-deploys were missed.Why it escaped: this emits valid YAML that actionlint accepts, and act evaluates
ifleniently enough to run the skipped-on-real-GitHub job, so unit tests and the e2e harness stayed green. Only the live fleet caught it (base=failure, retry=skipped).Fix
if: ${{ !cancelled() && needs.<prev>.result == 'failure' }}for every rung (retry-1, retry-2..N).ifwith!cancelled() &&, only when the dependency actually declares retries.!cancelled()rather thanalways(): a retry re-invokes a deployment, so a cancelled run must not force a re-deploy. The== 'failure'clause already excludes a cancelled predecessor (its result is'cancelled', not'failure'), so the status function only governs whole-run cancellation. The${{ }}wrap is required because a bare YAML scalar may not begin with the!tag indicator.The
needs:lists and thefinalize/state-writealways()gates are unchanged. The no-retries and non-retried-dependent paths are byte-identical.Census gap this closes
The generation-correctness census walks string-carrying manifest fields;
retriesis an int, so the shimifshape sat outside it, which is why the defect escaped. This PR adds a named correctness assertion for the shim and dependent gates and folds the status-function check into the censuseddepends_onassertions (GM5/GM7) so a regression reds.Verification
TestGenCorrectness_RetryShim_IfGateCarriesStatusFunction(red before, green after); extended GM5/GM7.73-deploy-retries.yamlasserts the wrapped${{ !cancelled() && ... }}form and locks the bare form out vianot_contains.go build ./...,go test ./...(3654 pass),go test ./internal/generate -race,golangci-lint run ./...all clean; e2e module builds and vets.CONTRIBUTING.md.