Uh oh!
There was an error while loading. Please reload this page.
fix(generate): include a dependency's retry shims in a dependent's needs - #639
Merged
Merged
Conversation
A dependent deploy's if: gate already read needs.<dep>-retry-N.result to judge its dependency's effective (ladder-wide) result, but its needs: list was built from GetDirectDependencies alone, which returns only the base dependency job ID. GitHub Actions can only resolve a needs.<job> reference for a job actually listed in needs:, so the emitted pair silently disagreed: actionlint rejects it at parse, and had it been accepted, a retry-rescued dependency (base fails, a shim succeeds) would leave the dependent skipped instead of running. Add retryShimJobIDs as the single source of truth for a job's shim IDs and route both the needs: construction (writeCallbackJob, writeFinalizeJob) and the if: gate (retrySucceededCond) through it, so the two can no longer drift apart. A dependency with no retries is unaffected: needs: still collapses to the bare dependency job ID. Signed-off-by: Joshua Temple <joshua.temple@stablekernel.com>
Contributor
All PR Validation checks passed. |
The notify.yaml callback stub used an inline run: step whose command contained a colon-space, which YAML reads as a nested mapping, so the generator rejected the stub while discovering its outputs. Use a block scalar to match the deploy stub in the same scenario. Signed-off-by: Joshua Temple <joshua.temple@stablekernel.com>
joshua-templeforce-pushed
the
fix/dependent-deploy-retry-shim-needs
branch
from
July 18, 2026 15:17
a36826f to
911d073CompareUh oh!
There was an error while loading. Please reload this page.
joshua-temple added a commit
that referenced
this pull request
Jul 20, 2026
…nction (#645) 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 freeto 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
A dependent deploy's
if:gate already readsneeds.<dep>-retry-N.resultto judge its dependency's effective (ladder-wide) result, so a retry-rescued dependency does not skip its dependents. But the dependent'sneeds:list was built fromGetDirectDependenciesalone, which returns only the base dependency's job ID, never its retry shims.GitHub Actions can only resolve a
needs.<job>reference for a job actually listed in that job'sneeds:. The emitted pair silently disagreed:deploy-notify.needs=[setup, deploy-app]deploy-notify.ifreferencesneeds.deploy-app-retry-1.resultactionlint rejects this at parse (
property "deploy-app-retry-1" is not defined in object type ...), and had it somehow been accepted, the reference resolves to an always-empty value at runtime, so a retry-rescued dependency (base fails, a shim succeeds) would leave the dependent silently skipped instead of running.The finalize job's
needs:already listed retry shims correctly; only a dependent deploy'sneeds:was missing them.Fix
internal/generate/generator.go:retryShimJobIDs(jobID, retries)as the single source of truth for a job's shim IDs.writeCallbackJob'sneeds:construction now appends each hard dependency's retry shims right after the dependency itself.retrySucceededCond(theif:gate builder) andwriteFinalizeJob's job collection now both derive from the same helper, so theneeds:list and theif:gate can no longer drift apart.needs:collapses to the bare dependency job ID, byte-identical to before.Verification
internal/generate/pass10_silent_output_test.go: addedTestGM7_DependentDeploy_NeedsIncludesRetryShims(red before the fix:needs: [setup, deploy-web]; green after:needs: [setup, deploy-web, deploy-web-retry-1, deploy-web-retry-2]) andTestGM7_DependentDeploy_NoRetries_NeedsByteIdentical(proves the no-retries path is unchanged).internal/generate/correctness_census_map_test.go:deploys[].depends_on[]now points atTestGM7_DependentDeploy_NeedsIncludesRetryShims.internal/generate/actionlint_feature_matrix_test.go: added theretries_dependent_deploycase, pairingretrieswithdepends_onso this exact combination stays under permanent actionlint guard (the pre-existingretriescase never exercised a dependent deploy, which is why the matrix hadn't caught this).e2e/scenarios/73-deploy-retries.yaml: extended with anotifydeploy thatdepends_onthe retriedwebdeploy. Asserts the emittedneeds:line carries the base dependency plus both retry shims, and thatnotifycorrectly skips in the non-rescue path (the scenario's callback always fails, so no rescue occurs). The true rescue-runtime proof (base fails, shim succeeds, dependent runs) is documented as out of reach for this harness for the same structural reason the existing ladder proof already states (no artifact server, no cross-job state channel, identicalgithub.jobacross shims); the needs:/if: agreement it depends on is instead pinned by the two unit tests above.docs/src/content/docs/internals/coverage-matrix.md: filled in the73-deploy-retriesscenario reference for "Callback retry wrapper", which was blank.go build ./... && go test ./... && go test ./... -race && golangci-lint run ./...all clean on the root module;go build ./... && go vet ./... && go test ./... -short && golangci-lint run ./...clean on thee2emodule.