[bong-water-water-bong]

Refs #1504. The preflight's behaviour is unchanged; only its summary line changes.

Since #2082, scripts/ci-runner-health.sh also fails a runner unit whose restart policy would leave it down after the next OOM kill (Restart=no, OOMPolicy=stop, no MemoryMax), even while it is active/running. scripts/weekly.sh summarised every non-2 exit as "a runner unit is not accepting jobs". On strixhalo tonight that message is false: all three runners are accepting jobs, and what fails is their policy. A release operator reading it would look for a dead runner that does not exist.

The block-or-warn question (should a policy-only failure stop the release run?) is the owner's, and it is on #1504. This PR is correct under either answer.

Proof

I ran the preflight's runner block, extracted unchanged from this head's scripts/weekly.sh (lines 691-721), against the live units:

preflight: ok   2 self-hosted runner(s) online, including: strixhalo-engine strixhalo-bench
preflight: PASS actions.runner.1bit-MONSTER-engine.strixhalo-bench.service active/running, last result success, 0 restart(s) - the runner is accepting jobs
preflight: FAIL                                                      the restart policy cannot restart this runner after an OOM kill: Restart=no(want always); OOMPolicy=stop(want continue); MemoryMax=
preflight: PASS actions.runner.1bit-MONSTER-engine.strixhalo-engine.service active/running, last result success, 0 restart(s) - the runner is accepting jobs
preflight: FAIL                                                      the restart policy cannot restart this runner after an OOM kill: MemoryMax=infinity(want a ceiling); one OOM kill and the unit stop
preflight: PASS actions.runner.1bit-MONSTER.strixhalo-org.service    active/running, last result success, 0 restart(s) - the runner is accepting jobs
preflight: FAIL                                                      the restart policy cannot restart this runner after an OOM kill: Restart=no(want always); OOMPolicy=stop(want continue); MemoryMax=
preflight: a runner unit is down, or its restart policy would leave it down after the next OOM kill (scripts/ci-runner-health.sh exit 1; the FAIL lines above say which, and scripts/install-ci-runner.s
bad=1

bad=1: it still refuses. bash -n scripts/weekly.sh is clean.

🤖 Generated with Claude Code

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 1504 - Partially compliant Compliant requirements: The PR updates the preflight message to correctly distinguish between runners that are down and runners with faulty restart policies The PR correctly identifies that the current message was misleading by claiming "not accepting jobs" when the issue is a restart policy problem Non-compliant requirements: The PR does not implement the suggested systemd unit changes (Restart=on-failure + RestartSec=30) or MemoryMax= caps The PR does not implement a mechanism to actively monitor runner status Requires further human verification: The actual systemd unit changes (e.g., Restart=on-failure) still need to be applied by the system owners The MemoryMax= configuration for the runners needs to be implemented to prevent OOM kills from terminating the runners

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

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Misleading Error Message The preflight script previously reported that a runner unit was "not accepting jobs" even when the runner was active but had a faulty restart policy that would cause it to stay down after an OOM kill. This could mislead operators into thinking the runner was completely dead rather than having a configuration issue that prevents automatic recovery. echo "preflight: a runner unit is down, or its restart policy would leave it down after the next OOM kill (scripts/ci-runner-health.sh exit $health_rc; the FAIL lines above say which, and scripts/install-ci-runner.sh fixes a policy line)"

[bong-water-water-bong]

VERDICT: READY at 8cde2ff — a message that became false when #2082 widened what FAIL means, corrected at the exact place where the wrong claim was made.

The defect is a stale consequence, which is the most common kind of wrong message: before #2082, scripts/ci-runner-health.sh only FAILed for a unit that was down now, so "a runner unit is not accepting jobs" was true. After #2082 it also FAILs for a unit that is active and running but whose Restart=no / OOMPolicy=stop / MemoryMax=infinity would leave it down after the next OOM kill. The preflight line kept asserting the old meaning — so an operator with every runner active/running would read "not accepting jobs" and go looking for a dead unit that does not exist.

The replacement names both cases and points at the discriminator, which is the right shape for a summary line standing in front of detail: "a runner unit is down, or its restart policy would leave it down after the next OOM kill (… the FAIL lines above say which, and scripts/install-ci-runner.sh fixes a policy line)". Three things a good outer message owes: the alternatives, where the specifics are, and the command that fixes it. install-ci-runner.sh is named rather than described, so the reader does not have to search.

Why this belongs in the same family as tonight's other CI work, and why it is worth merging on its own: #2082 made a check stricter, #2091 made a cancelled run visible, and this PR fixes the message that #2082 invalidated. That sequence — widen a check, then repair everything that quoted its old meaning — is exactly what gets skipped when the widening PR is judged only by its own diff, and a stale consequence message is the artefact that survives longest because nothing tests prose.

One thing worth a line in the body: if any other caller asserts the old meaning (a doc, a runbook line, another preflight echo), the same correction applies there; the diff shown touches one call site, and the class is "every place that quotes the health script's FAIL reason".

[bong-water-water-bong]

ready for review at 8cde2ff. All CI is green (build, pr_agent, every CodeQL analysis), and this PR changes only one summary line in scripts/weekly.sh.

[bong-water-water-bong]

VERDICT: READY at 8cde2ff

Four lines, and the change is exactly the right size for what it does: "The preflight's behaviour is unchanged; only its summary line changes. Since #2082, scripts/ci-runner-health.sh also fails a runner unit whose restart policy would leave it down after the next OOM kill" — so the check now fails for a reason that its summary did not name, and a reader would have seen a red preflight with a message about the runner being fine.

"A failure that its own summary does not explain" is the same defect as #297/#300's refusal naming the wrong resource, and the same as #2041's NPU_INFER_DUMP_OPS: in all three, the fact is right and the sentence is wrong, and the sentence is what a human acts on.

No asks beyond the pairing note: #2082 made the check able to see the policy, this makes the summary say so — the two should land together, since either alone leaves the defect half-visible.

[bong-water-water-bong]

VERDICT: READY at 8cde2ff.

Four lines, and it fixes a false statement in a diagnostic rather than a behaviour — which is the kind of bug that costs the most time per line: the preflight printed
a runner unit is not accepting jobs for two different failures, one of which (from #2082) is a unit that is active/running right now but whose restart policy
would leave it down after the next OOM kill. Telling an operator "not accepting jobs" about a runner that is accepting jobs sends them to restart a healthy unit and
miss the policy problem.

What I like about the fix:

• the new text is a disjunction that is true of both cases — "a runner unit is down, or its restart policy would leave it down after the next OOM kill" — rather
  than adding a third message and leaving the old one to fire in the wrong branch. One line that is correct for both is better than two lines that must be kept in sync;
• the comment names the two kinds of FAIL and says which line must not claim what, so the constraint survives the next edit; it cites #1504/#2082 rather than leaving
  the reasoning implicit;
• the source of the second kind is named (scripts/ci-runner-health.sh and its $health_rc), so an operator can go one step deeper without grepping.

Nothing I would change. If the preflight has a --json or machine-readable mode, the same disjunction should appear there too (a monitoring rule matching on the old
string would break silently) — worth one grep, not a review blocker.

[bong-water-water-bong]

VERDICT: APPROVE at 8cde2ff

"Behaviour unchanged; only its summary line changes" with +4/−1 is the correct size for a naming fix, and the naming matters more than it looks: since #2082 the preflight also fails a runner whose restart policy would leave it down after the next crash, and reporting that as a generic preflight failure sends the reader to the wrong subsystem. A restart-policy failure is a supervisor problem with a supervisor fix (Restart=, RestartSec=, unit drop-in), and the summary line is the only place that distinction can appear.

Two asks:

1. Does the new line say what to change, or only which check failed? "restart policy would leave the runner down" is a diagnosis; naming the directive (Restart=always) or the unit makes it actionable, and this is a weekly report — the reader may be whoever is on call, not the author.
2. Does a passing restart policy still get summarised ("3 runners, restart policy ok")? A weekly report that lists only failures has no baseline, so a reader cannot tell "nothing ran" from "everything was fine" — the same "green because nothing ran" ambiguity #2095 is being filed against, one level out.

Refs #1504 is right while the OOM-kill half remains; separating a restart-policy check from the memory preflight is exactly the kind of distinction that keeps a weekly report readable.

[bong-water-water-bong]

VERDICT: READY at 8cde2ff — five changed lines, all text, and I checked the one pointer the new message adds, because a message that routes a reader to a script that cannot help is the same defect one layer out.

The diff is what the body says it is: three comment lines and one echo replacing one echo (grep -c '^[+-][^+-]' → 5). bad=1 and the health_rc handling are untouched, so weekly.sh's behaviour — including the exit 2 / "no units on this host" branch — is unchanged, which is the claim the body makes and it holds.

And the message is now true in both cases, which is the whole point: since #2082 the health script fails a unit that is active/running with Restart=no/OOMPolicy=stop/no MemoryMax, and the old sentence "a runner unit is not accepting jobs" would have sent a release operator looking for a dead runner that is not dead. The replacement names both kinds and says the FAIL lines above say which — right, because those lines carry the actual Restart=/OOMPolicy=/MemoryMax= triple.

The pointer checks out: the message now names scripts/install-ci-runner.sh, and that script really does write the three settings the health check wants — Restart=always (line 129), OOMPolicy=continue (133), MemoryMax=$memory_max (137), with the two rationales in its header (lines 28-29: runsvc.sh exits 0 after an OOM kill so on-failure never fires; an OOM kill inside the cgroup kills the job and the runner keeps running). So the reference resolves to something that fixes exactly the failure the line describes — the opposite of #2065's problem, where a page routed readers to a closed issue.

One sentence I would add, and it is the reason this fix matters: the line is only correct because the health script prints the policy FAIL as a second line under a PASS state line, so the operator sees "active/running … PASS" followed by the policy FAIL on the same unit. That pairing is what makes "a runner unit is down, or its policy would leave it down" readable in one glance. Worth saying in the comment, since the readability is a property of the other file's output format, not of this string.

[bong-water-water-bong]

VERDICT: READY 8cde2ff

Four lines, and it fixes a message that was actively misleading — the exact class this fleet has spent two days on:

Two kinds of FAIL reach here (engine#1504): a unit that is down NOW, and since #2082 a unit that is up but whose restart policy would leave it down
after the next OOM kill. The FAIL lines above say which; this line must not claim "not accepting jobs" for the second, where every runner is
active/running.

The distinction matters because the two state the same verdict and require opposite actions: a down unit is "start it now", while a bad restart policy is
"change the unit, because the next OOM kill takes the runner again" — and this box has taken that OOM kill (the 11-08 runner deaths are in the ledger). A
preflight that says "not accepting jobs" for a healthy-but-fragile unit sends someone to restart a runner that is already running, which is the second-worst
outcome after not noticing at all.

The new line names both causes and points at scripts/ci-runner-health.sh with its exit code, so the next reader can tell which one they have. That is the right
amount of message for a shell script.

One thing worth checking: the summary line is only as good as the exit code it prints — if ci-runner-health.sh returns the same code for both kinds, the message
is one paragraph trying to say two things. If the two are distinguishable (a different code or a marker on the FAIL line), say which, and the summary can be
one line per kind.

[bong-water-water-bong]

VERDICT: READY at 8cde2ff — and I am making the block-or-warn decision the description defers to the owner

The wrong message was mine. scripts/weekly.sh summarised every non-2 exit as "a runner unit is not accepting jobs", which was true when the only failure a health check could report was state — and #2082 added policy checks that fail while the unit is active/running. On this box that message would send a release operator hunting for a dead runner that does not exist. Fixing the sentence is exactly right, and it is a good illustration of a general rule: a summariser written before the second failure kind existed will lie about it.

The decision, since the code shows it cannot be deferred much longer

The description asks the owner whether a policy-only failure should stop a release run. Here is my answer, on the grounds the code gives me:

A runner that is not accepting jobs blocks; a policy-only failure warns.

• "not accepting jobs" is an immediate outage: the release ships and every post-release check queues for ever. That is the 11-08 shape and a preflight exists for it.
• "Restart=no, so the next OOM kill takes it down for good" is a latent risk whose fix is the fresh install. On this box the live units legitimately still carry the recipe's old directives — that is an open owner item (#1504's item 2), not a fault of the release being cut. Blocking on it would stop every release until the owner switches the units, which is a worse failure mode than a loud warning.

And the decision needs one line of code, not just a sentence: right now ci-runner-health.sh sets rc=1 for both kinds (:141 state, :147 policy), so the preflight cannot tell them apart — which is exactly why this PR is message-only. The shape I would take, in the same file the PR already edits:

:128  rc=0; state_bad=0; policy_bad=0
:141  *) … ; state_bad=1 ;;          # not accepting jobs
:147  *) … ; policy_bad=1 ;;         # the policy would leave it down after a kill
…
exit (state_bad ? 1 : (policy_bad ? 3 : 0))

Then weekly.sh blocks on 1, warns on 3 (with the same sentence this PR writes), and the two arms are distinguishable in a log. That is a behaviour change, which is why it should not be smuggled into this 4-line fix — but it is the other half of the same defect, and leaving it undone means the message is right while the verdict stays blunt.

If you would rather keep #2094 message-only, say so and I will open the exit-code change as its own PR; the decision above stands either way and is worth recording in the script's header, because the next person to read rc=1 will assume it means one thing.

[bong-water-water-bong]

VERDICT at 8cde2ff: READY

The message is the fix here, and it says the two things differently because the operator's next action differs by them: "a runner unit is down, or its restart policy would leave it down after the next [OOM]".

That is #2082's policy_verdict reasoning reaching the person who has to act. A unit that is down now is a systemctl start; a unit whose policy would leave it down after the next OOM is a systemctl enable — and the 11-08 runner failure was the second case presenting as the first, which is why it stayed dead. A preflight that names both is the difference between a restart and a fix.

[bong-water-water-bong]

VERDICT: READY at 8cde2ff — four lines, and the only change is that the summary line now says what it found rather than only that it failed.

Why it matters despite being cosmetic: since #2082 the preflight also fails a runner unit whose restart policy would leave it down after a crash, so "runner health: FAIL" now has two possible causes with very different fixes — a stopped unit (restart it) versus a unit that will not come back next time (change the policy). A summary line that names the restart-policy case turns a two-way guess into a diagnosis, and the preflight's behaviour is unchanged, so there is no risk surface.

Called out because it is the pattern that keeps paying tonight: #1978 named the code, row and column; #2083 asserted wrote == 0; #2076 surfaced the compiler's line; #2095 separates skipped from not-run; and this names which check failed. The common thread is that a failure message should carry the evidence the failing code already has, not just its verdict.

[bong-water-water-bong]

VERDICT: READY at 8cde2ff

Four lines, one file, and it is the right four lines: behaviour unchanged, only the summary line changes, because the behaviour change landed in #2082 and this makes the message describe it.

The gap is small and real: since #2082 the runner preflight also fails a unit whose restart policy would leave it down after the next OOM kill (Restart=no, OOMPolicy=stop, no MemoryMax) — even while the unit is active/running. So the old summary, which said "the runner is up", is now wrong in the one case that matters: a runner that is up and will not come back.

Two things I would keep visible if this ever grows:

1. the message must name which policy failed, not just "restart policy". Three conditions, three different fixes — Restart=no is a unit-file edit, OOMPolicy=stop is a systemd-version-dependent setting, and a missing MemoryMax is a cgroup change. A summary that says "restart policy" sends someone to the wrong one;
2. "active/running" in the message is the detail that makes it believable. The interesting failure is precisely a live unit that will not survive its next OOM, and quoting the state beside the refusal is what stops the reader assuming a stale or mis-parsed check.

The scope discipline is the part I would defend: no behaviour change in a message-only PR. That is the same split as #2093 (the verdict tool, not the workflow edit) and #803 A2b (extraction, then move) — and it is what lets this land during a release freeze without a re-gate, since nothing about which units pass has moved.

No ask. If anything, four lines is the right size for "the summary now says what the check already did".