Skip to content

Commit 671af69

Browse files
authored
ci: fix Check job status to detect skipped results (fixes #2208) (#2210)
* ci: replace 'cancelled||failure' aggregator with CCCL-pattern check_result Fixes #2208. The previous `Check job status` body only treated a dep's `result == 'cancelled' || == 'failure'` as failure, letting `'skipped'` slip through silently. When a `build-*` job fails, the dependent `test-*` job is set to `'skipped'` by default needs-failure propagation, and the aggregator passes -- exactly the case demonstrated by #2209. Adopt CCCL's `check_result` pattern: explicit `expected="success"` per dependency, with `expected="skipped"` for legitimate `[doc-only]` skips, and an early short-circuit for `[no-ci]`. Now any deviation from the expected status (including `'skipped'` from a failed upstream) fails the aggregator. Reference: NVIDIA/cccl ci-workflow-pull-request.yml L463-L526. * [demo] Re-inject env-vars exit 7 to validate gating-check fix Mirrors the #2209 reproducer on this branch's new aggregator. Expected outcome: - All Build * matrix entries fail at the env-vars build step. - Downstream Test * jobs are skipped (cascaded needs-failure). - Check job status now reports failure -- the symmetric, post-fix counterpart to #2209 (where Check job status reported success despite the same set of failures). DO NOT MERGE while this commit is present. Remove before merge. Refs #2208, #2209. * Revert "[demo] Re-inject env-vars exit 7 to validate gating-check fix" This reverts commit 811388c.
1 parent 32da37d commit 671af69

1 file changed

Lines changed: 37 additions & 47 deletions

File tree

.github/workflows/ci.yml

Lines changed: 37 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -432,52 +432,42 @@ jobs:
432432
steps:
433433
- name: Exit
434434
run: |
435-
# if any dependencies were cancelled or failed, that's a failure
436-
#
437-
# see https://docs.github.com/en/actions/reference/workflows-and-actions/expressions#always
438-
# 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
439-
# for why this cannot be encoded in the job-level `if:` field
440-
#
441-
# TL; DR: `$REASONS`
442-
#
443-
# The intersection of skipped-as-success and required status checks
444-
# creates a scenario where if you DON'T `always()` run this job, the
445-
# status check UI will block merging and if you DO `always()` run and
446-
# a dependency is _cancelled_ (due to a critical failure, which is
447-
# somehow not considered a failure ¯\_(ツ)_/¯) then the critically
448-
# failing job(s) will timeout causing a cancellation here and the
449-
# build to succeed which we don't want (originally this was just
450-
# 'exit 0')
451-
#
452-
# Note: When [doc-only] is in PR title, test jobs are intentionally
453-
# skipped and should not cause failure.
454-
#
455-
# detect-changes gates whether heavy test matrices run at all; if it
456-
# does not succeed, downstream test jobs are skipped rather than
457-
# failed, which would otherwise go unnoticed here. Require its
458-
# success explicitly so a broken gating step cannot masquerade as a
459-
# green CI run.
460-
doc_only=${{ needs.should-skip.outputs.doc-only }}
461-
if ${{ needs.detect-changes.result != 'success' }}; then
462-
exit 1
463-
fi
464-
if ${{ needs.doc.result == 'cancelled' || needs.doc.result == 'failure' }}; then
465-
exit 1
435+
# GitHub treats `result == 'skipped'` as success for required
436+
# status checks (see CCCL gate comment + cccl#605). The previous
437+
# `cancelled || failure` predicate let upstream build failures
438+
# propagate as `skipped` on downstream test jobs and silently
439+
# pass this aggregator. Adopt CCCL's `check_result` pattern:
440+
# require an explicit `expected` status per dependency, where
441+
# anything else (including `skipped` from a failed upstream)
442+
# fails the gate. `if: always()` on the job still ensures this
443+
# step runs even when needs are skipped.
444+
if [[ "${{ needs.should-skip.outputs.skip }}" == "true" ]]; then
445+
echo "[no-ci] - skipping aggregator checks"
446+
exit 0
466447
fi
467-
if ${{ needs.test-sdist-linux.result == 'cancelled' ||
468-
needs.test-sdist-linux.result == 'failure' ||
469-
needs.test-sdist-windows.result == 'cancelled' ||
470-
needs.test-sdist-windows.result == 'failure' }}; then
471-
exit 1
472-
fi
473-
if [[ "${doc_only}" != "true" ]]; then
474-
if ${{ needs.test-linux-64.result == 'cancelled' ||
475-
needs.test-linux-64.result == 'failure' ||
476-
needs.test-linux-aarch64.result == 'cancelled' ||
477-
needs.test-linux-aarch64.result == 'failure' ||
478-
needs.test-windows.result == 'cancelled' ||
479-
needs.test-windows.result == 'failure' }}; then
480-
exit 1
448+
449+
doc_only="${{ needs.should-skip.outputs.doc-only }}"
450+
status="success"
451+
check_result() {
452+
name=$1; expected=$2; result=$3
453+
echo "Checking $name: result='$result' (expected '$expected')"
454+
if [[ "$result" != "$expected" ]]; then
455+
echo "::error::$name did not match expected result"
456+
status="failed"
481457
fi
482-
fi
483-
exit 0
458+
}
459+
460+
# always expected to succeed (even in [doc-only] mode)
461+
check_result "should-skip" "success" "${{ needs.should-skip.result }}"
462+
check_result "detect-changes" "success" "${{ needs.detect-changes.result }}"
463+
check_result "doc" "success" "${{ needs.doc.result }}"
464+
465+
# [doc-only] flips these from 'success' to 'skipped'
466+
if [[ "$doc_only" == "true" ]]; then expected="skipped"; else expected="success"; fi
467+
check_result "test-sdist-linux" "$expected" "${{ needs.test-sdist-linux.result }}"
468+
check_result "test-sdist-windows" "$expected" "${{ needs.test-sdist-windows.result }}"
469+
check_result "test-linux-64" "$expected" "${{ needs.test-linux-64.result }}"
470+
check_result "test-linux-aarch64" "$expected" "${{ needs.test-linux-aarch64.result }}"
471+
check_result "test-windows" "$expected" "${{ needs.test-windows.result }}"
472+
473+
[[ "$status" == "success" ]]

0 commit comments

Comments
 (0)