Alert when CI jobs wait in the runner queue - #408

Review of PR #408 (CI runner queue alert)

Verdict: Changes requested. Two inline findings; both are required before merge.

Workflow hygiene is good: pinned checkout, least-privilege token (Actions read, Contents read, Issues write), concurrency group, two-minute job timeout, a documented playbook section, and the dedup logic correctly ignores PRs via the pull_request key and recreates the label when missing.

Inline findings:

1. Coverage hole against the incident this fixes: only status=queued runs are scanned. A workflow run turns in_progress as soon as one job starts, while its remaining matrix jobs can stay queued for hours - exactly the multi-platform starvation of #181, where the Windows job of run 39648960935 waited about 5h40m. In that state the watcher never fires. Scan status=in_progress runs as well; the existing per-job queued filter keeps that safe.

2. New committed Python tooling. The project removed its Python toolchain deliberately (issue #178, PR #182; the repo has been Python-free since 2026-10-22), and new Python tooling needs an explicit owner decision, which neither this PR nor issue #181 provides - the issue sketches the watcher via gh api and mandates no language. Reimplement as a Rust helper in the tools crate pattern or as a gh/bash step, unless the owner explicitly approves Python.

Notes (no action required): under a real burst, up to five pages with a 20 s timeout per request across many queued runs can exceed the two-minute job timeout precisely when the alert matters most; a smaller per-run page budget or a higher timeout is worth a thought. The 2 MB response cap and bounded pagination are sensible.

Review of PR #408 (CI runner queue alert), round 2

Verdict: Changes requested. One of the two round-1 findings is resolved; the Python tooling finding is still open and remains required before merge.

Resolved: the coverage hole is fixed correctly. find_stale_jobs now scans both status=queued and status=in_progress runs with the same per-job queued filter, the threshold boundary is inclusive, malformed API data fails loudly instead of being skipped, and the new unit tests cover both run statuses, the exact-threshold job, an in_progress job in the same run and the malformed-timestamp path. The workflow wiring (test job with reduced contents-read permissions, detect skipped on pull_request events, path-filtered trigger) is sound.

Still open: the watcher remains committed Python tooling, and this round adds a second Python file. The project removed its Python toolchain deliberately (issue #178, PR #182; the repo has been Python-free since 2026-10-22), and new Python tooling requires an explicit owner decision. Neither this PR, issue #181 (which sketches the watcher via gh api with no language mandate), nor the PR discussion contains such a decision. Reimplement as a Rust helper in the existing tools crate pattern (or a gh/bash step) and keep the equivalent unit tests in Rust, or obtain and cite the owner approval for Python.

Notes (no action required): raising the pagination cap from 5 to 100 pages removes the round-1 truncation concern; under an extreme burst the two-minute detect timeout can still cut the scan short, but that now fails visibly rather than silently under-alerting. Running unittest against a namespace package from the repository root works on the ubuntu runner.

CI runner queue alert

Verdict: changes requested — one policy-level item; the implementation itself is solid.

Reviewed the full diff against master at 164248a: the scheduled workflow, the detector script, the unit tests and the development-docs section.

What checked out:

• API discipline. Pagination with a 100-page safety limit that fails loudly instead of reporting a healthy queue, a 2 MB response cap, explicit validation of run id, job id and created_at types, timezone-aware parsing with a loud failure on naive or malformed stamps.
• Dedup and delivery. One open ci-queue-labelled issue suppresses duplicates; the label is created before the issue is posted (a nonexistent label on issue creation would 422); the concurrency group prevents overlapping schedule runs; the title is truncated to the 256-byte limit and every interpolated string is length-capped.
• Permissions. Least-privilege split per job (test job gets contents read only; detect adds actions read and issues write), pinned action SHA with version comment, per-job timeouts.
• Tests. The queued/in-progress scan, the exact-threshold boundary, dependency-waiting jobs, and the malformed-timestamp refusal are covered with a clean injected-pages seam.

Non-blocking notes: jobs waiting behind a still-running dependency are counted as queue-wait, which widens the alert beyond runner starvation — the docstring and the docs section state this deliberately, so I treat it as intended; pagination behaviour of the injected API is untested; the message about a non-list API response says the opposite of what it checks. None of these need to change.

The requested change is in the inline finding: the repository deliberately removed its Python tooling (#178, #182) with new Python tools requiring an explicit owner decision, and this PR re-introduces Python under tools/ without citing that decision. Either record the approval in the PR description or bring the watcher in line with the no-Python policy.

The watcher currently cannot reach queue detection against the real GitHub REST API.

In tools/ci_queue_alert.py, pages() requires every response to be a top-level JSON array. However, GET /repos/{owner}/{repo}/actions/runs returns an object containing workflow_runs, and GET /repos/{owner}/{repo}/actions/runs/{run_id}/jobs returns an object containing jobs. The first Actions request therefore exits with GitHub API returned an unexpected list response, even when the queue is empty, and no alert can be created.

Please add the following before merging:

1. Endpoint-aware collection extraction for the workflow_runs and jobs envelopes, while retaining top-level array handling for issues and labels. Apply pagination termination and the safety limit to the extracted collection; reject missing or malformed collection fields explicitly.
2. Regression tests that exercise the actual HTTP-response decoding and pagination path using realistic JSON envelopes. The existing tests replace pages() with a stub returning lists, so they bypass the failing code. Cover both an empty Actions response and a stale queued job, plus multi-page collection handling.
3. A mocked end-to-end watcher test proving that realistic runs/jobs responses lead to exactly one issue creation, and that an existing open ci-queue issue suppresses another creation. This can run without a real queue incident or external writes.

The blocking issue is the API response-shape mismatch; green tests that bypass this adapter do not establish that the scheduled watcher works.

CI runner queue alert (re-review at 83bb60a)

Verdict: approved. The requested change is resolved; no new findings.

Re-reviewed since my changes-requested review of the Python version. The watcher and its tests are rewritten from Python to bash (tools/ci-queue-alert.sh and tools/test-ci-queue-alert.sh; both .py files deleted), which removes the policy conflict with the repository decision that removed Python tooling (#178, #182) — no Python remains in the PR.

What checked out on the full current head:

• Workflow. The jobs now call the shell scripts directly, the pull_request paths filter names the new files, and the earlier properties are intact: pinned action SHA, least-privilege per-job permissions, per-job timeouts, the schedule/dispatch/PR triggers with the concurrency group.
• Watcher. Same semantics as before through gh and jq: manual pagination with the 100-page loud failure, shape-aware envelope validation per collection, epoch-based threshold comparison, loud failures on malformed ids and timestamps, length-capped interpolations, the 256-byte title cap, label-before-issue ordering, and the open-ci-queue-issue dedup that ignores pull requests. The temporary directory is trapped and cleaned; no token appears in URLs or logs.
• Tests. The fake-gh harness drives the real script end to end and now covers more than the Python suite did: empty responses alert nothing, one stale job opens exactly one issue and creates the label, an open ci-queue issue suppresses, a labelled pull request does not suppress, multi-page collections are walked, and a malformed envelope fails loudly.

Non-blocking notes, unchanged in substance from the first round: jobs waiting behind a running dependency still count as queue-wait (stated deliberately in the docs section), and refs are interpolated into the markdown table without escaping, so a branch name containing a backtick or pipe could garble the table — same exposure as the previous version, only cosmetic.

Tests, shellcheck and CI were not run in this review; the workflow covers them.

Changes requested: CI test jobs are failing for the current head 83bb60a:

• test (ubuntu-latest, shard 5/8) — FAILURE.
• test (windows-latest, shard 5/8) — FAILURE.

Please investigate and resolve the failing checks, then rerun CI and obtain a green result before requesting review again. This review records the reported CI failures; their root cause has not been diagnosed here.

Review of PR #408 (CI runner queue alert)

Reviewed 1615de7 against base master 112be7b (4 files, +430/−0, four commits).

Verdict: approved. No blocking findings; no inline change requests.

What I checked and found sound:

• The Python-free policy is honored: the watcher is pure bash over preinstalled gh and jq (tools/fork-sync.sh precedent), with the recorded owner decision cited in the PR body; no Python remains in the tree.
• Input handling is defensive where it matters: GITHUB_REPOSITORY must contain exactly one slash before it reaches an API URL (this also blocks path games), the threshold must be an integer ≥ 1 before it reaches --argjson, and the collection decoders are shape-aware — workflow_runs/jobs envelope objects for the Actions endpoints, top-level arrays for issues and labels, with an explicit failure on anything else. The round-2 finding (a single run never being seen) is genuinely fixed, and the fake-gh test drives the real decoding path end to end through realistic envelopes.
• Pagination fails loudly at the 100-page safety limit instead of silently reporting a healthy queue; per-page short counts stop the loop, so a full 100-item last page costs one extra empty request, not a false negative.
• Deduplication works as documented: only non-PR open issues with the ci-queue label suppress the alert, the label is created on first use, and the issue body carries ref/job/run-link rows with bounded field lengths (200/500 chars) and a 256-char title cap.
• The workflow is least-privilege (Actions read, Contents read, Issues write), splits test from detect so PRs never post anything, pins checkout by SHA, uses a concurrency group without cancel-in-progress, and docs/development.md documents the lifecycle (close the issue after the drain).
• The test harness is a self-contained fake gh with fixture files and request logs, asserting the observable API call sequence and issue payloads without any external writes; mktemp+trap cleanup in the watcher.

Minor, non-blocking (no change requested):

• A head_branch containing markup or a newline would render an odd markdown table row; GitHub branch names make this exotic, and the field is length-bounded, so it is cosmetic at worst.