fix(ci): stop a skipped retry job taking the whole deploy chain with it - #1008
Merged
Conversation
Merging 2.2.0 to `main` shipped nothing. Push run 33547853809 reported `success` with every check green, and `ios-package`, `ios-deploy`, `android-package`, `android-deploy` and `github-release` were all skipped: no v2.2.0 tag, no GitHub release, nothing to TestFlight, no AAB. Nothing looked wrong, which is the worst part. The cause arrived with 2.2.0's own two-runner iOS integration change. `ios-integration-tests-retry` is skipped on every *healthy* run — the first runner passed, so there is nothing to retry — and a skip travels the whole dependency chain, not one hop. `ios-integration-tests-result` steps out of the way with its own `if: !cancelled()`, but that exempts that one job; the skip keeps going into the deploy jobs, whose `if:` carried no status function and so inherited GitHub's implicit `success()`. All five of `ios-package`'s direct needs succeeded and it skipped anyway. So the pipeline only deployed when the first iOS runner *failed*. The A/B is clean: run 33243607505 fired the same guards on the same event and deployed, and the sole structural difference is that the package jobs then needed `ios-integration-tests` directly, before any job that skips on a healthy run existed upstream of them. Including a status function drops the implicit `success()` and with it the inherited skip, which is why each dependency is now asserted by hand. A bare `!cancelled()` would have been strictly worse than the bug — it would push an integration suite that failed on two independent runners straight to TestFlight and Play — so every `needs:` entry gets an explicit `result == 'success'`, and the comment on `ios-package` spells out the rules for keeping the two lists in step. Verified against the full outcome matrix. The three cases that must ship do: first runner passes, first fails and the retry passes, and dispatch on main. The eight that must not, do not: pull request, a failed integration result, a failed linux-checks / ios-build / android-integration-tests, a cancelled run, a push to develop, and a skipped result job. actionlint reports no new findings.
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The workflow changes correctly prevent transitive skip propagation while still enforcing that all required needs jobs must have succeeded before packaging, deploying, or cutting a release.
Pull request overview
This PR fixes a GitHub Actions deploy-chain regression where a skipped retry job (ios-integration-tests-retry) could propagate “skip” through needs: and silently prevent packaging/deploy/release jobs from running—even when all required checks were green.
Changes:
- Adds explicit
!cancelled()plus per-needsresult == 'success'checks to all deploy-gate jobs to avoid inheriting implicitsuccess()skip propagation. - Ensures deploy/release jobs only run when the workflow is a push to
mainor aworkflow_dispatch, and only when every required upstream job actually succeeded. - Documents the rationale and maintenance rules for keeping
needs:andif:conditions in sync across the five gated jobs.
File summaries
| File | Description |
|---|---|
.github/workflows/default_workflow.yml |
Makes deploy/package/release gating robust against transitive “skipped” propagation by using explicit status functions and per-need success assertions. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This was referenced Sep 1, 2026
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.
Merging #988 shipped nothing. Push run 33547853809 reported
successwith every check green, and the entire deploy chain was skipped:No
v2.2.0tag, no GitHub release, nothing to TestFlight, no AAB. The run was green, so nothing looked wrong.Cause
2.2.0's own two-runner iOS integration change introduced it.
ios-integration-tests-retryis skipped whenever the first runner passes — the normal case — and a skip travels the whole dependency chain, not one hop.ios-integration-tests-resultsteps out of the way with its ownif: !cancelled(), but that exempts that job; the skip keeps going into the deploy jobs, whoseif:contained no status function and so inherited GitHub's implicitsuccess().All five of
ios-package's direct needs concludedsuccessand it skipped anyway. The pipeline only deployed when the first iOS runner failed.The A/B is clean: run 33243607505 fired byte-identical guards on the same event and deployed. The sole structural difference is that the package jobs then needed
ios-integration-testsdirectly, before any job that skips on a healthy run existed upstream of them.Ruled out with evidence: the
if: |block scalar (byte-identical where deploy worked), the guard predicate (event=push,head_branch=main), reusable-workflowuses:jobs misbehaving inneeds, andios-podfile-lock-guard(skipped in both runs — it is in nobody'sneeds, which also shows the taint travels only alongneedsedges).The fix
Including a status function drops the implicit
success()and with it the inherited skip. That is also why each dependency now has to be asserted by hand: a bare!cancelled()would be strictly worse than the bug, because it would push an integration suite that failed on two independent runners straight to TestFlight and Play.So every
needs:entry gets an explicitresult == 'success', and the comment onios-packagestates the rules for keeping the two lists in step —== 'success'not!= 'failure',!cancelled()notalways(), and the event test kept as an AND-conjunct.Verification
Outcome matrix, evaluated against the real conditions parsed out of the file:
workflow_dispatchon mainactionlint 1.7.7reports no new findings (8 pre-existingmacos-26unknown-label warnings on both baseline and patched). A structural check confirms all five guards mirror theirneeds:exactly, no dangling dependencies, and both atomicity comments intact.Merging this is what deploys 2.2.0
The push to
mainruns the pipeline with the fixed guards.mainis already at2.2.0+63and nov2.2.0tag exists, so it cuts fresh.Expect
android-deployto go green with the Play upload undone — that is the #942 tolerance, not success. Read the step summary, upload the AAB by hand, and read Play's warnings on the review step.Not included, deliberately
workflow_dispatchhas no ref test, and the trigger has no branch filter — a dispatch from any branch passes the guard and ships. This is not theoretical: run26086317616cut a release fromintegrate-cicd-deployment. Closing it is a one-line change to all five guards, but it belongs in its own commit so that reverting it cannot revert this fix.