Skip to content

ci: retry Node setup on the manifest-miss legs; fix trunk-red's close rule #588

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
colinhacks wants to merge 1 commit into
base: main
Choose a base branch
from
+73 −9
Open

ci: retry Node setup on the manifest-miss legs; fix trunk-red's close rule #588

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 35 additions & 5 deletions .github/workflows/node-matrix.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,20 @@ jobs:
- uses: actions/checkout@93cb6efe18208431cddfb8368fd83d5badbf9bfd # v5.0.1
with:
persist-credentials: false
# actions/node-versions publishes no 23.x or 25.x, so four of these legs (23.6, 23.11,
# 25.0, 25.2) miss the manifest and setup-node falls back to fetching
# nodejs.org/dist/index.json live on every run. That request
# retries only on 502/503/504 (@actions/http-client), so a DNS timeout or connection
# reset kills the leg outright — and with no diagnostic at all, which is what made
# nubjs/nub#585 cost a full investigation to attribute. Upstream: actions/setup-node#1136.
# One retry turns that class of red back into the blip it is.
- uses: actions/setup-node@a0853c24544627f65ddf259abe73b1d18a591444 # v5
id: setup-node
continue-on-error: true
with:
node-version: ${{ matrix.node }}
- uses: actions/setup-node@a0853c24544627f65ddf259abe73b1d18a591444 # v5
if: steps.setup-node.outcome == 'failure'
with:
node-version: ${{ matrix.node }}
- uses: actions/download-artifact@d3f86a106a0bac45b974a628896c90dbdf5c8093 # v4
Expand Down Expand Up @@ -230,15 +243,32 @@ jobs:
needs: [smoke, real-apps]
if: always()
runs-on: ubuntu-latest
permissions: {}
permissions:
# Reading this run's own job list is what lets the gate NAME the leg that died. The
# `needs.*.result` rollup cannot: it collapses 14 smoke legs into one word, so a red
# here used to say "smoke failed" and nothing else — the reason nubjs/nub#585 took a
# whole investigation just to identify which version had broken.
actions: read
steps:
- shell: bash
env:
GH_TOKEN: ${{ github.token }}
run: |
set -euo pipefail
smoke='${{ needs.smoke.result }}'
apps='${{ needs.real-apps.result }}'
echo "smoke=$smoke real-apps=$apps"
# A skipped leg (PR that didn't trigger it) is fine; a failure is not.
[[ "$smoke" == "success" || "$smoke" == "skipped" ]] || { echo "smoke failed"; exit 1; }
[[ "$apps" == "success" || "$apps" == "skipped" ]] || { echo "real-apps failed"; exit 1; }
echo "node matrix gate: OK"
# A skipped leg (a PR that didn't trigger it) is fine. Everything else is red on
# purpose, including `cancelled` and `abandoned` (GitHub reclaimed the runner):
# both mean the leg went UNVERIFIED, and fail-closed is the only safe reading.
if [[ "$smoke" == "success" || "$smoke" == "skipped" ]] \
&& [[ "$apps" == "success" || "$apps" == "skipped" ]]; then
echo "node matrix gate: OK"
exit 0
fi
echo "::error::Node matrix red — smoke=$smoke real-apps=$apps. Failing legs:"
gh api "repos/$GITHUB_REPOSITORY/actions/runs/$GITHUB_RUN_ID/jobs?per_page=100" \
--paginate --jq '.jobs[]
| select(.conclusion != null and .conclusion != "success" and .conclusion != "skipped")
| " \(.conclusion)\t\(.name)\t\(.html_url)"' || echo " (job list unavailable)"
exit 1
42 changes: 38 additions & 4 deletions .github/workflows/trunk-red.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ on: # zizmor: ignore[dangerous-triggers] trunk-only notifier; reads CI conclusi
permissions:
contents: read
issues: write
# Re-checking each implicated workflow's latest main run before clearing the flag.
actions: read

# Serialize so two failing workflows finishing together can't both create the
# tracking issue (the find-or-create below is read-then-write, not atomic).
Expand Down Expand Up @@ -64,11 +66,43 @@ jobs:
short="${SHA:0:9}"

if [[ "$CONCLUSION" == "success" ]]; then
# Only the workflow that carries the macOS integration leg can clear
# the flag; a green Docker smoke says nothing about CI being fixed.
if [[ -n "$existing" && "$WORKFLOW" == "CI" ]]; then
[[ -n "$existing" ]] || exit 0

# Seven workflows can file this issue, so ONE of them going green does not mean
# trunk is green. The old rule closed on any `CI` success and cleared reds filed
# by the other six without ever verifying them — #550 was filed by WPT Worker
# and Node matrix, then closed by a CI green while both were still unchecked.
# Every red report opens with "`<workflow>` failed on main at `<sha>`.", so the
# issue already records which workflows are implicated: re-check each one's
# latest main run and clear the flag only once they are ALL green.
# shellcheck disable=SC2016 # the backticks are literal: they match the report's markdown
reported=$(gh issue view "$existing" --repo "$REPO" --json body,comments \
--jq '[.body] + [.comments[].body] | .[]' \
| sed -n 's/^`\([^`]*\)` failed on main at .*/\1/p' | sort -u)

@pullfrog pullfrog Bot Jul 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ This reported=$(...) pipeline is unguarded under set -euo pipefail. If gh issue view has a transient failure (rate limit, network blip), pipefail propagates its non-zero status even though sed/sort succeed on empty input, and set -e aborts the whole step. Contrast line 88's latest=$(gh run list ... || echo ""), which is explicitly guarded. The failure direction is safe (the step just fails and the issue is left untouched, self-healing on the next trigger), but the inconsistency is worth closing — e.g. append || true here too, or wrap the whole assignment. A legitimate empty match is already handled by the [[ -n "$reported" ]] fallback below.

[[ -n "$reported" ]] || reported="$WORKFLOW"

still_red=""
while IFS= read -r wf; do
[[ -n "$wf" ]] || continue
latest=$(gh run list --repo "$REPO" --workflow "$wf" --branch main --limit 1 \
--json conclusion --jq '.[0].conclusion // ""' 2>/dev/null || echo "")
# Only a terminal green clears a workflow. An IN-FLIGHT run ("") holds the
# issue open on purpose: during a merge burst everything reads as in-flight,
# and treating that as green would reinstate the very false-clear this fixes.
# This converges — every watched workflow re-fires trunk-red when it completes,
# so the last one to settle re-evaluates with all of them terminal. `cancelled`
# is a superseded run, which the job-level `if:` above already treats as noise.
case "$latest" in
success|cancelled|skipped) ;;
*) still_red="$still_red $wf" ;;
esac

@pullfrog pullfrog Bot Jul 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ Treating cancelled as clearing a workflow is safe for the common merge-burst case — a superseding push means --limit 1 returns the newer run, not the cancelled one, so the cancelled run is never what's read here. The narrow gap is a cancel with no superseding run yet (a maintainer manually cancels a stuck run, or a runner-availability timeout): that cancelled run genuinely is the latest, nothing verified the fix, and this clears the workflow anyway.

The comment above ("cancelled is a superseded run, which the job-level if: above already treats as noise") conflates two paths — the job-level if: gates whether this trigger event fires, not what gh run list returns for an unrelated already-red workflow. Worth either dropping cancelled from the clearing set (fail-closed, consistent with the gate change) or tightening the comment to name the manual-cancel gap.

Technical details 
# `cancelled` in the clearing set can false-clear on a non-superseded cancel

## Affected sites
- `.github/workflows/trunk-red.yml:96` — `success|cancelled|skipped) ;;` clears the workflow.
- `.github/workflows/trunk-red.yml:93-94` — comment justifies `cancelled` as "superseded run," but that reasoning only covers the merge-burst case.

## Required outcome
- A workflow whose latest `main` run is `cancelled` *without* a newer superseding run should not be read as green.

## Suggested approach (optional)
- Drop `cancelled` from line 96 so it falls to `*)` (still-red) — the fail-closed reading, matching the node-matrix-gate change in this same PR. Superseded-run convergence still holds because the newer run re-fires trunk-red when it settles.
- Or keep it and reword the comment to state the manual/timeout-cancel gap explicitly rather than implying it's fully covered.

done <<< "$reported"

if [[ -z "${still_red// /}" ]]; then
gh issue close "$existing" --repo "$REPO" \
--comment "Green again on \`$short\` — $RUN_URL"
else
echo "still red:$still_red — leaving #$existing open"
fi
exit 0
fi
Expand All @@ -79,7 +113,7 @@ jobs:

Trunk-only legs (the macOS integration suite) do not run on pull requests, so a
regression can land green and only surface here. This issue tracks the red and
closes automatically when CI passes on main again."
closes automatically once every workflow reported above is green on main again."

if [[ -n "$existing" ]]; then
gh issue comment "$existing" --repo "$REPO" --body "$body"
Expand Down
Loading