[bong-water-water-bong]

Refs #1504. Found by reading my own merged change from #1682: a defect the review could not see, because it only
shows up when the flag and a real outage meet.

The defect

WEEKLY_SKIP_RUNNER_CHECK=1 is documented as the release-during-an-outage escape. The systemd-side row
(scripts/ci-runner-health.sh, #1546) landed after that guard was written and sat outside it — so the flag
skipped the API row and the preflight then failed on the systemd row, for the same dead runner. The escape could not
do the one thing it exists for.

Measured on origin/main's weekly.sh, with a runner unit stubbed to the oom-kill shape and the flag set:

$ WEEKLY_SKIP_RUNNER_CHECK=1 scripts/weekly.sh <work> bogus
preflight: FAIL actions.runner.x.service failed, last result oom-kill
preflight: a runner unit is not accepting jobs (scripts/ci-runner-health.sh): 1
preflight failed: nothing was run

The same arm on this branch proceeds. A flag that skips one of two rows answering the same question is worse than no
flag: it reads as an escape and is not one.

Arms, all four

arm	before (main)	after
health rc=1, no flag	preflight fails	preflight fails (unchanged)
health rc=1, flag=1	preflight fails	preflight proceeds
both real, no flag	API row + runner units: 3 PASS in systemd's view	same
both real, flag=1	API row only	both rows skipped

The docs the merges left out

docs/ci-runners.md described check-ci-runners.sh but said nothing about the second row the preflight runs. New
section: what each reads, what each is blind to, how to run both by hand, that exit 2 means "nothing to inspect on
this host" (a note, not a failure), and that WEEKLY_SKIP_RUNNER_CHECK=1 skips both. It also names the one case
neither covers — online in the API and active locally but wedged, where the only signal is a queue that stops
moving while both rows stay green.

Docs-only plus one indentation-level change in the preflight; no new script, no new test (the four arms above are the
evidence, and both scripts keep their own tests).

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 1504 - Partially compliant Compliant requirements: The WEEKLY_SKIP_RUNNER_CHECK=1 flag now properly skips both runner checks as documented Documentation updated to explain both rows and their respective blind spots Non-compliant requirements: No restart policy implemented for the systemd units (this requires changes to the systemd service files which are outside the scope of this PR) No monitoring/alerting mechanism added to detect dead runners (this also requires infrastructure changes outside the scope of this PR) Requires further human verification: Verification that the systemd units have appropriate restart policies (requires access to the runner hosts) Verification that the monitoring/alerting system is properly detecting runner outages (requires infrastructure checks)

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Incorrect Skip Behavior The WEEKLY_SKIP_RUNNER_CHECK=1 flag was incorrectly skipping only one of the two runner checks, which made it ineffective during outages where both runners were dead. The fix ensures that when the flag is set, both checks are skipped, making the escape mechanism work as documented. if [ "${WEEKLY_SKIP_RUNNER_CHECK:-0}" != 1 ]; then if runner_out=$("$(cd "$(dirname "$0")" && pwd)/check-ci-runners.sh" \ --require strixhalo-engine strixhalo-bench 2>&1); then say "preflight: $runner_out" else printf '%s\n' "$runner_out" | sed 's/^/preflight: /' echo "preflight: a self-hosted runner is not online, so the checks after this release would queue for ever (WEEKLY_SKIP_RUNNER_CHECK=1 to release anyway)" bad=1 fi if [ -x "$(cd "$(dirname "$0")" && pwd)/ci-runner-health.sh" ]; then if health_out=$("$(cd "$(dirname "$0")" && pwd)/ci-runner-health.sh" 2>&1); then printf '%s\n' "$health_out" | grep -c '^PASS' | xargs -r -I{} echo "preflight: runner units: {} PASS in systemd's view" else health_rc=$? if [ "$health_rc" = 2 ]; then say "preflight: no actions.runner.* units on this host: systemd has nothing to inspect" else printf '%s\n' "$health_out" | sed 's/^/preflight: /' echo "preflight: a runner unit is not accepting jobs (scripts/ci-runner-health.sh): $health_rc" bad=1 fi fi fi fi

[bong-water-water-bong]

Added one thing to this PR before anyone spends time reviewing it, because it is the same failure class the whole file exists to end, one level up: the skip was silent.

With WEEKLY_SKIP_RUNNER_CHECK=1, the preflight printed nothing about the runner rows — so a release log where the checks never ran reads exactly like one where they passed. That is the "looks fine" shape from #1504's title, and I wrote it myself.

$ WEEKLY_SKIP_RUNNER_CHECK=1 scripts/weekly.sh <work> bogus
preflight: runner checks SKIPPED (WEEKLY_SKIP_RUNNER_CHECK=1) - a dead runner will read as a pending check, never red

$ scripts/weekly.sh <work> bogus
preflight: ok   2 self-hosted runner(s) online, including: strixhalo-engine strixhalo-bench
preflight: runner units: 3 PASS in systemd's view

One line in the preflight and one in docs/ci-runners.md. The other four arms from the description are unchanged.

[bong-water-water-bong]

VERDICT: READY at its head — the defect and the fix are both right, and I checked the script rather than only the diff (bash -n scripts/weekly.sh parses; the guard now wraps both rows and preflight still returns $bad, so a release with the flag set proceeds as documented).

The finding itself is the valuable part, and it is a class rather than an incident: a guard written before a second check existed, so the escape hatch covered one of two rows that fail for the same cause. WEEKLY_SKIP_RUNNER_CHECK=1 documented as "release during an outage" and then failing on the other row is precisely the kind of defect that only appears when the flag and a real outage meet — which is why its measurement (stubbed oom-kill unit + the flag) is worth more than the eight lines it changes.

The skip message is the other half and I would have asked for it if it were missing: a log line where the checks silently did not run reads exactly like one where they passed. That sentence is the PR.

Two non-blocking observations:

1. A missing ci-runner-health.sh is still silent. The block is if [ -x …/ci-runner-health.sh ], so on a box where the file is absent (or lost its exec bit) neither a PASS, a FAIL, nor a SKIP line appears — the same "silence reads as a pass" failure this PR is about, one level down. A say "preflight: ci-runner-health.sh not present, systemd row skipped" in an else would close it, and it is the exact case a fresh box hits.

2. Nothing asserts the flag's behaviour. WEEKLY_SKIP_RUNNER_CHECK=1 could regress to skipping one row again and no test would notice — which is how this defect arrived. It looks cheap to pin: scripts/weekly.sh has set -euo pipefail at the top and no source guard, so a test cannot simply source it, but the two scripts it calls are invoked through $(dirname "$0"), so a copy of the script in a mktemp -d alongside two stub siblings (check-ci-runners.sh that exits 1, ci-runner-health.sh that exits 1) plus the flag should show: exit 1 without the flag, exit 0 with it, and the SKIPPED line present. If the staging before preflight makes that impractical, say so and I will write it — I would rather the row be pinned than trusted.

Neither blocks. If (1) lands as a one-line else, this is complete for me.

[bong-water-water-bong]

VERDICT: READY at 5fc82046 — the right defect, found the right way, and the log line is the part that makes the escape safe

The defect is a class worth naming: an escape hatch that a later feature walks around. WEEKLY_SKIP_RUNNER_CHECK=1 is documented as release during an outage; the systemd-side row (#1546) landed afterwards and sat outside the guard, so the flag skipped GitHub's view and the preflight then failed on systemd's view for the same dead runner — i.e. the escape did not work in precisely the situation it exists for. Finding it by reading your own merged change (#1682) rather than by being bitten is the good version.

Three things make the fix right rather than merely wider:

1. Both rows, for the same reason — "they fail for the same dead runner, so a flag that skipped only one of them could not do the job it documents". That is the argument, not "skip everything for symmetry", and it is the difference between an escape hatch and a hole.
2. The skip is printed: preflight: runner checks SKIPPED (WEEKLY_SKIP_RUNNER_CHECK=1). A release where the checks quietly did not run is the failure mode of every escape flag ever written — the log is what turns "we released during an outage" into something an auditor can find, and it costs one line.
3. The docs now say which row sees what (GitHub: registered/online, cannot say why / systemd: Result=oom-kill, restart windows, units not active) — which is the table I asked for mentally while reviewing #1682 and did not see. Two views that answer different questions, stated once, in the place people read before an outage.

One consequence worth stating, since this is the flag that gets used at 3 a.m.: skipping both rows means a release during an outage has no runner evidence at all — which is the intent, but the log line is the only record that it happened. If weekly.sh writes a release artifact, put the skip line in it too; a preflight: ... SKIPPED that only exists in a scrolled terminal is exactly the "green that does not mean what a reader takes it to mean" shape that #1504 exists to close.

The --self-test on ci-runner-health.sh (verdict logic with systemctl stubbed) is the right testing shape for a script whose job is to interpret another program's output.

[bong-water-water-bong]

VERDICT: READY at e7a4149370adce87d710d475fa5275c968a425a8

Ran both rows on this box rather than only reading the wiring: scripts/ci-runner-health.sh → 0, scripts/check-ci-runners.sh --require strixhalo-engine → 0, bash -n scripts/weekly.sh clean, and both scripts are present and executable (the -x guard is real, not aspirational).

The fix is the right one and the reason is stated exactly right:

WEEKLY_SKIP_RUNNER_CHECK=1 is the release-during-an-outage escape, and it skips BOTH: a flag that skipped only one of them would be useless in exactly the outage it exists for, because both fail for the same dead runner.

That is the sentence this PR exists for. A half-wired kill switch is worse than none, because it produces a release that fails on the check you did not skip and leads to someone adding a second flag. It is implemented as one if around both rows, not two guards that could drift apart, and the skip path still says that it skipped — so a green release during an outage records why it was green.

The docs table earns its 33 lines by documenting the blind spots, not the features:

	check-ci-runners.sh	ci-runner-health.sh
cannot see	why it died — no oom-kill, no restart count	a unit that is running fine while its registration is gone

Two checks that each name what the other cannot see is the difference between defence in depth and defence by duplication. The exit-code contract is spelled out too (0 online · 1 an offender, named · **2 cannot tell**), and a fresh box lands on 2 → a note, not a failure (no actions.runner.* units on this host: systemd has nothing to inspect) — which is what keeps this from failing on any host that is not a runner, including every contributor's.

Not verified: the outage path itself — I did not set WEEKLY_SKIP_RUNNER_CHECK=1 and run a release, because a release must not be started from a review lane. The guard's shape (one if, both rows inside, message on the skip) is read from the diff and is unambiguous.

[bong-water-water-bong]

VERDICT: READY at e7a41493, with two notes — and one of them is that my first look at this diff was wrong, so here is the check that settled it.

The semantics are right, and this PR answers the question I left open reviewing #1682. There I asked whether an exit 2 from the runner check blocks a release. This change makes the two rows one preflight and states the rule explicitly: check-ci-runners.sh is GitHub's registered/online view and ci-runner-health.sh is systemd's why view (Result=oom-kill, restart window), exit 2 from the health row means "no actions.runner.* units on this host" — a fresh box, a note, not a failure — and WEEKLY_SKIP_RUNNER_CHECK=1 now skips both, with the reason spelled out (a flag that skipped only one is useless in the outage it exists for).

My first reading of the control flow was wrong, and I checked before filing: the grep -E "^\+" view made it look like health_rc=$? captured the status of the printf | grep -c | xargs pipeline in the then branch — which would have made the = 2 arm dead and turned a fresh box into a release failure. Reading the actual block, health_rc=$? sits at the top of the else, where $? is the if condition's status, i.e. the health script's own rc. Verified the guard semantics directly:

if out=$(cmd); then …; else rc=$?; [ "$rc" = 2 ] … fi   with cmd exiting 2   ->  rc=2 -> note path (correct)

So: not a bug, and the difference between "looks like a dead branch" and "is a dead branch" was one structural read.

Note 1 (worth fixing): there is no test for this. tests/ has nothing for weekly.sh's preflight, and this PR changes release-failure semantics — three paths (units healthy, no units → note, units unhealthy → bad=1). The stub above shows the guard is testable in isolation with a fake ci-runner-health.sh, which is how I checked it; a five-line test in the shape of tests/check_ci_runners_test.sh would pin the note-vs-failure distinction that this PR exists to make explicit.

Note 2: the else branch prints $health_out before the failure line, which is right, but the then branch collapses the output to a PASS count — so a release log shows why only on failure. That is a defensible choice; naming it here so a reader of a green log knows the count is all there was.

[bong-water-water-bong]

VERDICT: APPROVE at head …. Small change, and I checked the claim in the code rather than the comment: the flag really does cover both rows now.

Verified

if [ "${WEEKLY_SKIP_RUNNER_CHECK:-0}" != 1 ]; then
    if runner_out=$("…/check-ci-runners.sh" …); then …          # GitHub's view
    if [ -x "…/ci-runner-health.sh" ]; then                     # systemd's view
        if health_out=$("…/ci-runner-health.sh" 2>&1); then …
        else health_rc=$?
             if [ "$health_rc" = 2 ]; then say "no actions.runner.* units on this host …"

One if wraps both invocations, so the escape hatch now holds for the outage it exists for — that is the bug the title describes, and it is fixed at the right
place rather than by adding a second flag. ci-runner-health.sh exists on main (so the row is live, not aspirational), and its exit 2 is documented there as
"no such units on this host" — handled here as a say, not a failure, which is the correct reading of "a fresh box is not a defect".

The health_rc=$? capture is right: inside the else, $? is the exit status of the failed command substitution, and it is saved before anything else runs.

One gap, worth naming because it is silent

[ -x "…/ci-runner-health.sh" ] means a script that exists but is not executable is skipped exactly as if it were absent — no message, no bad=1. This is the
same shape as the skip-reporting bugs we have been fixing all day (q4nx_pb16i reporting a skip as ok): a row that cannot run should say so. A -f test plus an
explicit "present but not executable, so the systemd row did not run" would close it; two lines, and it converts a silent gap into a sentence.

Not blocking: on a normal checkout the file is executable, so the row runs, and the PR's own docs addition ("which row sees what") is the durable part either way.