ci: run status check always but consider cancellations failures #1174
Conversation
|
Auto-sync is disabled for ready for review pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
/ok to test |
cpcloud
force-pushed
the
run-always-but-fail-on-cancelled
branch
from
c8e96c3 to
0f834c2
Compare
|
/ok to test |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR attempts to fix a GitHub Actions workflow issue where cancelled jobs (triggered by critical failures in dependencies) would cause timeouts but paradoxically allow builds to pass. The change modifies the final status check job in ci.yml to: (1) always run regardless of dependency status (via if: always()), (2) explicitly check if any dependent job was cancelled, and (3) fail the build if cancellations are detected. The workflow uses a final gate job pattern where checks depends on test-linux-64, test-linux-aarch64, test-windows, and doc—this is common in repos that need a single mergeable status check across multiple parallel job matrices.
PR Description Notes:
- The PR description references "Details and links are in the comment" but no such comment is visible in the provided metadata.
Critical Issues
Syntax Error: Bash cannot evaluate GitHub Actions expressions directly
Lines 222-225 contain a fatal flaw—the code attempts to use GitHub Actions template syntax ${{ ... }} inside a bash if statement:
if ${{ needs.test-linux-64.result == 'cancelled' || ... }}; thenThis will fail because:
- GitHub Actions expressions are evaluated during workflow compilation, before the runner executes bash
- The bash interpreter will receive malformed syntax like
if true || false; then(literal boolean tokens, not valid bash) - The correct pattern is to interpolate each expression as a string and use bash string comparison:
if [[ "${{ needs.test-linux-64.result }}" == "cancelled" ]] || \
[[ "${{ needs.test-linux-aarch64.result }}" == "cancelled" ]] || \
[[ "${{ needs.test-windows.result }}" == "cancelled" ]] || \
[[ "${{ needs.doc.result }}" == "cancelled" ]]; thenAlternative approach: Use the if: condition at the step level with pure GitHub Actions expression syntax (no bash):
- name: Fail if any dependency was cancelled
if: |
needs.test-linux-64.result == 'cancelled' ||
needs.test-linux-aarch64.result == 'cancelled' ||
needs.test-windows.result == 'cancelled' ||
needs.doc.result == 'cancelled'
run: exit 1This will prevent merges when dependencies are cancelled, which is the intended behavior.
Confidence Score
1 out of 5 — The PR will not achieve its goal due to the syntax error. The logic is sound, but the implementation is broken and must be corrected before merge.
1 file reviewed, no comments
cpcloud
force-pushed
the
run-always-but-fail-on-cancelled
branch
from
0f834c2 to
83a09d3
Compare
|
/ok to test |
cpcloud
force-pushed
the
run-always-but-fail-on-cancelled
branch
from
83a09d3 to
0b1b1e0
Compare
|
/ok to test |
| run: | | ||
| # if any dependencies were cancelled, that's a failure | ||
| # | ||
| # see https://docs.github.com/en/actions/reference/workflows-and-actions/expressions#always | ||
| # and https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/collaborating-on-repositories-with-code-quality-features/troubleshooting-required-status-checks#handling-skipped-but-required-checks | ||
| # for why this cannot be encoded in the job-level `if:` field | ||
| # | ||
| # TL; DR: `$REASONS` | ||
| # | ||
| # The intersection of skipped-as-success and required status checks |
There was a problem hiding this comment.
Ah yes, I feel every proper GHA-based status check workflow needs a comment expressing the dismay and astonishment that came from this discovery. See also: https://github.com/NVIDIA/cccl/blob/main/.github/workflows/ci-workflow-pull-request.yml#L259-L263
🙂
|
Seems like cancellation is working: https://github.com/NVIDIA/cuda-python/actions/runs/18721400957/job/53395760809?pr=1174 I manually cancelled and that caused the status check to fail. |
|
/ok to test |
|
|
/ok to test |
1 similar comment
|
/ok to test |
cpcloud
force-pushed
the
run-always-but-fail-on-cancelled
branch
from
5bf077f to
41d1ce7
Compare
|
/ok to test |
cpcloud
force-pushed
the
run-always-but-fail-on-cancelled
branch
from
41d1ce7 to
9088268
Compare
cpcloud
changed the title
4 participants
This PR should address the issue of cancelled-due-to-failure-but-still-times-out shenanigans present in GitHub Actions. Details and links are in the comment.