[bong-water-water-bong]

Refs #1504 (a release blocker, row "self-hosted runners die on OOM and stay dead"). This is the half a lane can land: the preflight can now tell an outage from a latent risk, and treats them differently.

The two failures that were sharing one exit code

scripts/ci-runner-health.sh set rc=1 for both, so scripts/weekly.sh's preflight had to treat them the same — and #2094 (merged) fixed the sentence while leaving the verdict blunt:

failure	what it means	right response
a unit is not accepting jobs	an outage now: every check that needs the box reads as pending for ever — the 11-08 shape	block the release
since #2082, a unit is active/running but its restart policy would leave it down after the next OOM kill	a latent risk; on this box the live units legitimately still carry those directives	warn, loudly and specifically

The change

ci-runner-health.sh:  0 clean · 1 down now · 3 policy only · 2 nothing to inspect (unchanged)
weekly.sh:            blocks on 1, warns on 3 (the FAIL lines say which directive is wrong)

Why warn and not block on 3 — the decision, made here rather than left open: switching the live units' directives is #1504's item 2, an owner action, not a fault of the release being cut. A release that will not start until the owner gets to it is a worse failure mode than a warning that names Restart=no(want always) in the log. If the owner would rather block, it is one word in weekly.sh and the exit code already distinguishes the case.

Verified with a stubbed systemctl (no root, no real units touched)

MODE=good    active/running, Restart=always,  OOMPolicy=continue, MemoryMax set     -> rc=0
MODE=policy  active/running, Restart=no,      OOMPolicy=stop,     MemoryMax=infinity -> rc=3
MODE=down    inactive/dead                                                          -> rc=1
ci-runner-health.sh --self-test                                                     -> ok (15 cases)

Not stacked on #2091, deliberately

#2091 also edits this file (--runs), and its base predates #2082 — so it has no policy block to split and I would have built on the stale-base trap that PR is currently sitting in (it is CONFLICTING for exactly that reason; comment on it). This branch is cut from main, and both PRs touch different regions of the file, so whichever lands second wants a rebase rather than a hand-merge.

[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 introduces a mechanism to distinguish between runners that are currently down and runners with problematic restart policies The preflight now differentiates between immediate outages (exit code 1) and latent risks (exit code 3) The preflight warns on latent risks instead of blocking, which aligns with the suggestion that changing restart policies is an owner action Non-compliant requirements: The PR does not implement the suggested systemd unit changes (Restart=on-failure, RestartSec=30, MemoryMax=) The PR does not implement a mechanism to actively monitor and notify about runner outages Requires further human verification: Verification that the new exit codes (1 and 3) are correctly interpreted by the release process Confirmation that the warning mechanism is sufficient to alert on latent risks without causing false positives

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

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Exit Code Ambiguity The change introduces exit code 3 to distinguish a latent risk from an immediate outage, but the documentation and logic do not clearly explain how the policy verdict is determined. Specifically, the policy_verdict function's behavior needs to be verified to ensure it correctly identifies when a restart policy would fail after an OOM kill. *) printf 'FAIL %-52s %s\n' "$u" "${v#FAIL }"; state_bad=1 ;; esac # engine#1504: alive now is not the same as after the next OOM kill, so the policy is its own line. p=$(policy_verdict "$rst" "$oom" "$mmax") case "$p" in PASS*) printf 'PASS %-52s %s\n' "" "${p#PASS }" ;; *) printf 'FAIL %-52s %s\n' "" "${p#FAIL }"; policy_bad=1 ;; esac done if [ "$state_bad" = 1 ]; then exit 1; fi if [ "$policy_bad" = 1 ]; then exit 3; fi Warning vs Blocking Logic The PR changes the preflight logic to warn on exit code 3 (policy only) instead of blocking, which is a deliberate design choice. However, this introduces a potential risk where a latent issue might not be caught until the next OOM event, especially if the warning is not properly monitored or escalated. elif [ "$health_rc" = 3 ]; then # engine#1504: policy only. Every runner is accepting jobs NOW; what fails is that the next OOM kill # would stop one for good. Switching the live units' directives is #1504's open owner item on this # box, so a release that will not start until then is a worse failure than a loud warning. The # split is ci-runner-health.sh's exit codes (1 = down now, 3 = policy only). printf '%s\n' "$health_out" | sed 's/^/preflight: /' echo "preflight: WARNING a runner would not come back after the next OOM kill (its restart policy) - not blocking this release, and #1504's owner item on this box"

[bong-water-water-bong]

Closing as superseded — the same design landed at 0f68f1ef1 ("weekly: the runner preflight warns on a short restart policy and blocks only on a dead runner"), 23:58 on 11-10, nearly four hours before I opened this at 03:50. Not a near-miss in content: it is the same feature, the same exit-code split, and the same reasoning.

Main already has all of it:

scripts/ci-runner-health.sh      ) printf 'WARN …'; policy=1 ;;
                                 [ "$rc" = 0 ] && [ "$policy" = 1 ] && rc=3
                                 # Exit: … 3 when every runner is accepting jobs but a restart policy would leave one
                                 # down after the next OOM kill. 1 and 3 are separate because they call for different
                                 # things (engine#1504, owner 2026-11-11): a dead runner blocks a release …
scripts/weekly.sh  3)  "preflight: WARNING, not blocking: every runner unit is accepting jobs, but a restart policy
                        would leave one down after the next OOM kill (… exit 3 …)"
                   *)  blocks

So the decision I "made" in the #2094 review — warn on policy-only, block on down-now, exit 3 — was made independently by the lane that owns the file, and merged. Two lanes reaching the same conclusion is reassuring about the conclusion and says nothing good about my process.

Why I missed it, precisely, because the reason is reusable. My checks were:

1. grep -c policy_verdict scripts/ci-runner-health.sh → 8 on main, 0 on ci-runner-health: --runs, so a cancelled check is a number and not a shrug (refs #2077) #2091's branch — a check about the policy block, which is what I was reasoning about;
2. a scan of open PRs for ones touching ci-runner-health.sh → only ci-runner-health: --runs, so a cancelled check is a number and not a shrug (refs #2077) #2091 — a check about work in flight;
3. nothing that asked whether the exit-code split already existed on main.

The third check is one grep — git show origin/main:scripts/ci-runner-health.sh | grep -n 'policy=1\|rc=3' — and it is the only one that would have caught this, because a merged feature is in neither the diff of an open PR nor the shape I was looking at. "Search PR bodies for the issue number" (my earlier lesson) finds work in flight; this needs the complement: grep main for the behaviour you are about to build, at the moment you are about to build it, not from what an earlier review said the state was.

Nothing else to salvage: my patch was the same two edits, so there is no residual to re-offer.

[bong-water-water-bong]

VERDICT: READY at 2116's head — and the distinction is in the exit code, which is where it has to be.

+# Exit: 0 when every runner is active/running with no failure result AND its restart policy would survive the nex[t]
+# kill; 1 when a runner is not accepting jobs (an outage now); 3 when ONLY a restart policy would leave it down a[fter the]
+# next OOM kill (a latent one -- engine#1504, and scripts/weekly.sh warns rather than blocks on it);

Why exit codes rather than messages

1 blocks (a runner not accepting jobs is an outage now — every check that needs the box reads as pending forever) and 3 warns (scripts/weekly.sh), and a caller can only apply two policies to one failure if the two failures are distinguishable without parsing prose. That is the same rule the test rows follow (77 means skipped, not passed) and the same one #2093 is building a verdict tool around: the vocabulary is the contract.

It is also the correct read of #1504's own history — the issue is "die on an OOM kill and stay dead", i.e. two facts (a kill happened; nothing restarted it), and only the second is a latent risk that a release can proceed past. A single "runner unhealthy" would have blocked the release for a condition that may never recur.

The one thing I would pin

Is "not accepting jobs" observed or configured? An outage is an observation (a probe that asks the runner, or a job that fails to be picked up) while a restart policy is a configuration (a unit property, Restart=/RestartSec=). The PR's exit-0 line already requires both and the failure result — so the check is doing both in one pass; one sentence in the body naming which of the three inputs is a probe and which is a file read would stop the next person from making the configuration check stand in for the observation (which is exactly how a dead runner stays "healthy").

Two small things: an arm for each exit code on fixture inputs (a static unit file plus a fabricated status), and the message for 3 naming the policy it read — "Restart=on-failure with RestartSec=0 would not survive an OOM kill" is the sentence that tells an operator what to change.