Build/Test Tools: Batch timeout-annotation checks via GraphQL (alternative to #639) - #640
Draft
desrosj wants to merge 15 commits into
Draft
Build/Test Tools: Batch timeout-annotation checks via GraphQL (alternative to #639)#640desrosj wants to merge 15 commits into
desrosj wants to merge 15 commits into
Conversation
…ils. When the "Prepare notifications" job in the reusable Slack notifications workflow throws an unhandled error, the "failure" and "cancelled" jobs were still running because their `if` conditions included `|| failure()` and `|| cancelled()`. Referencing those status-check functions disables GitHub Actions' default behavior of skipping a job when a job it `needs` did not succeed, so these jobs ran anyway and posted to Slack using `needs.prepare.outputs.payload`, which is empty because `prepare` never reached the step that sets it. This is why timed out (and some failed) workflow runs have been posting empty-looking Slack messages. This was most recently observed in a run where the "Determine whether the workflow timed out" step threw `TypeError: Cannot read properties of undefined (reading 'conclusion')` from `jobs.some(...)`, because `github.paginate()` can merge in an undefined entry for `jobs` when a page response is missing its `jobs` array. - Remove `|| failure()` and `|| cancelled()` from the "failure" and "cancelled" jobs' conditions so they consistently rely on the default "skip if `needs` did not succeed" behavior, matching "success", "fixed", and "timeout" already do. - Add a defensive `job &&` guard in the timeout-detection script so a malformed pagination page can't crash the job. - Add a new "notify-prepare-failure" job that posts a distinct alert to Slack whenever `prepare` fails, so a broken notification pipeline is surfaced instead of failing silently. See #65845.
…meout. The previous `job &&` guard only prevented a crash when `github.paginate()` merged in a missing entry for a page of `listJobsForWorkflowRunAttempt` results; it didn't address the underlying under-detection risk. Treating a missing page as "no timed out job here" can silently misclassify a run as `cancelled` instead of `timeout` if the job that actually timed out was on that page. This job only runs once every job it `needs` has reached a terminal state, but the REST API's view of job data can briefly lag behind that. Poll `listJobsForWorkflowRunAttempt` (up to 5 attempts, 3s apart) until every job in the list has a recorded conclusion and no entries are missing, instead of trusting the first response. Also request `per_page: 100` to avoid needing multiple pages for typical job counts in the first place. See #65845.
- Inline the retry delay since it was only used once. - Reduce the max attempts from 5 to 3. - Back off exponentially starting at 1 second (1s, then 2s) between attempts instead of a fixed delay. See #65845.
Root-caused with a reproduction against the real @octokit/plugin-paginate-rest
package: this endpoint's `{ total_count, jobs: [...] } ` response body is
unconditionally normalized by `github.paginate()` before any custom map
function sees it, replacing `response.data` with the `jobs` array itself.
Reading `response.data.jobs` off of that (as this script did) is therefore
always `undefined`, on every page, deterministically -- not an occasional
race tied to jobs still being cancelled, as previously assumed. That
`undefined` then gets merged into the results by `paginate()`, crashing
`.some()` when it's reached.
Because this was never a timing issue, the previous polling/backoff retry
loop couldn't fix it (it always saw the same undefined entry and always
fell through to 'false'). Since this script has been returning 'false' for
every timed-out run since it was introduced, timeouts have likely never
been correctly classified as 'timeout' -- only ever falling through as a
plain 'cancelled' notification instead.
Fix: stop passing a map function that reads `.jobs` and let `paginate()`
return the array it already normalized for us. Verified against the real
normalize-paginated-list-response.js logic for both a single-page and a
multi-page response.
See #65845.
…lusion. Live-tested by forcing an actual job timeout (temporary 1-minute timeout-minutes override) on a fork: every job that hit its own timeout was reported with `conclusion: 'cancelled'` via listJobsForWorkflowRunAttempt, identical to a job cancelled for any other reason (e.g. fail-fast cancelling a sibling after another job failed). `timed_out` is a documented value for the `conclusion` field, but GitHub Actions does not appear to ever set it. The real signal survives elsewhere: the timed-out job's check-run annotations still record "The job has exceeded the maximum execution time of ...", confirmed against the live test run's actual API response. Switch detection to check each cancelled job's annotations for that message via `checks.listAnnotations`, short-circuiting on the first match. Only runs for jobs already filtered to `conclusion === 'cancelled'`, so this adds no extra API calls on a normal (non-cancelled) run. Also: - Add `checks: read` to the "prepare" job's permissions, required for the annotations call. - Revert the temporary debugging changes used to force and inspect the live timeout (per-job 1-minute timeout override, always()-gated ifs to run on a fork, and the console.log of the raw jobs array). See #65845.
Re-apply the same live-test setup as before (1-minute forced job timeout, always()-gated ifs so the notification jobs run on a fork PR) to validate the new annotation-based timeout detection end to end, plus logging for each cancelled job's annotation check and the final result. This is temporary and will be reverted once validated.
…ler. The first live-test push failed with a startup_failure before any jobs ran: "The nested job 'prepare' is requesting 'checks: read', but is only allowed 'checks: none'." A reusable workflow's job can never request more permissions than its caller grants, and the `checks: read` added to the "prepare" job's permissions (needed for the annotations call) wasn't granted by any of the 17 workflows that call slack-notifications.yml. Add checks: read alongside the existing actions: read / contents: read at each of those 17 call sites, matching the existing pattern exactly. See #65845.
The annotation-based timeout detection and checks: read permission grant were validated live on #639 (see run https://github.com/desrosj/wordpress-develop/actions/runs/33181690594): prepare completed successfully, correctly identified the forced timeout via its check-run annotation ("The job has exceeded the maximum execution time of 1m0s"), and the notify-prepare-failure job correctly stayed skipped since prepare didn't fail. Revert the 1-minute forced job timeout, the always()-gated ifs used to run on this fork's PR, and the console.log debug output. See #65845.
Neither approach considered for speeding up timeout detection touched
correctness the same way. Filtering to jobs whose runtime falls within some
window of the configured timeout-minutes was ruled out: that value isn't
exposed anywhere in the Jobs API, and in this repo it isn't even static
(`timeout-minutes: ${{ inputs.coverage-report && 120 || ... }}`), so
deriving it would mean reimplementing GitHub Actions expression evaluation
just to get a fuzzy matching window -- with real risk of a false negative
if that window is wrong for a given job's configured timeout.
Sorting cancelled jobs by actual runtime (completed_at - started_at)
descending has none of that risk: it only changes check order, not which
jobs get checked, so a real timeout can never be skipped -- worst case is
identical to before, but the annotations loop's early return now typically
fires after checking the single most-likely candidate instead of walking
through jobs cancelled early by an unrelated fail-fast cascade first.
See #65845.
Only used in the one .sort() call, so a standalone function was unnecessary. See #65845.
Alternative to the REST-based approach on claude/slack-webhook-empty-data-3r39gl, for comparison. Everything else is identical (the paginate() normalization fix, the checks: read permission grants, the notify-prepare-failure safety net) -- only how cancelled jobs' annotations get checked differs. REST's `checks.listAnnotations` is scoped to one check run per request, so checking N cancelled jobs costs up to N requests (mitigated on the REST branch by checking longest-running jobs first, but still worst-case O(N)). GraphQL's `CheckRun.annotations` field, reached via the root `nodes( ids: [ID!]! )` field, batches up to 100 check runs' annotations into a single request using the `node_id` already present on each job object from the existing REST jobs call -- collapsing what was up to N requests into a small constant number regardless of how many jobs were cancelled. checks: read is still required either way: GraphQL reads from the same Actions GITHUB_TOKEN permissions as REST, there's no separate GraphQL-only permission model. See #65845.
Same live-test setup as used to validate #639 (1-minute forced job timeout, always()-gated ifs to run on this fork's PR), plus logging of each GraphQL batch's result, to validate this branch's approach end to end. This is temporary and will be reverted once validated.
Validated live on #640 (run https://github.com/desrosj/wordpress-develop/actions/runs/33198104148): prepare completed successfully, and the GraphQL batch correctly identified the forced timeout across all 7 cancelled jobs in a single request (versus 7 separate REST requests on the equivalent #639 run). Revert the 1-minute forced job timeout, the always()-gated ifs used to run on this fork's PR, and the console.log debug output. See #65845.
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
2 participants
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.
This branches from
claude/slack-webhook-empty-data-3r39gl(#639) and changes exactly one thing, for side-by-side comparison: how the "Determine whether the workflow timed out" step checks cancelled jobs' check-run annotations for the real timeout signal.checks.listAnnotationsonce per job (checks: read), checking the longest-running jobs first as a fast path. Worst case is still one request per cancelled job.nodes( ids: [ID!]! )field andCheckRun.annotations, via each job'snode_id(already present on the REST job objects, no extra lookup). Collapses what could be dozens of requests into a small constant number.Everything else — the
github.paginate()normalization fix forlistJobsForWorkflowRunAttempt, thechecks: readpermission grant onprepareand all 17 calling workflows, and thenotify-prepare-failuresafety net — is identical between the two branches.checks: readis required either way; GraphQL reads from the same ActionsGITHUB_TOKENpermissions as REST.Trac ticket:
Use of AI Tools
AI assistance: Yes
Tool(s): Claude Code
Used for: Full investigation, implementation, and live validation (via a temporary forced job timeout on a draft PR) of both this approach and the REST-based #639. Reviewed by @desrosj throughout.
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.
Generated by Claude Code