Repository navigation
ci: the runner health check reads the restart policy, not just whether the runner is alive (engine#1504) - #2082
Conversation
…r the runner is alive (engine#1504)
The state checks answer "is it accepting jobs now". They cannot see a unit whose
next OOM kill ends it for good, because that is a configuration property and the
state is healthy right up to the kill.
Measured on strixhalo 2026-11-11, this script before the change:
PASS …strixhalo-bench.service active/running, last result success, 0 restart(s)
systemctl show -p Restart -p OOMPolicy -p MemoryMax …strixhalo-bench.service
Restart=no OOMPolicy=stop MemoryMax=infinity
So it passed the unit carrying exactly the defect the issue describes. docs/ci-runners.md
already specifies the policy and the reason for each line: Restart=always because
runsvc.sh exits 0 after a kill (on-failure never fires), OOMPolicy=continue because
"stop" lets an OOM kill of a JOB stop the RUNNER, and a MemoryMax ceiling so the
cgroup hits its own limit before the box's OOM killer chooses.
policy_verdict() checks those three per unit and prints its own line. On this box it
now reports three units that need the installer: strixhalo-bench (Restart=no,
OOMPolicy=stop, no ceiling), strixhalo-org (same), and strixhalo-engine (policy right,
ceiling missing). Nothing is restarted or reconfigured here — the installer is a sudo
action and whether to restart a runner is a decision, not a check.
Self-test grows from 9 to 15 cases: the live bench unit's exact triple, on-failure,
stop, an infinite ceiling, and an unreadable policy.
Refs #1504
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
|
VERDICT: READY at The distinction this adds is the one the issue has been missing for three days: the state checks answer "is it alive", which is what #1504 named, and this answers "will it come back", which is what actually kept both runners dead for 3h45m. A unit with The reasons are recorded next to the rule, not just in the failure text, and both are non-obvious enough that they belong there: The last arm is the one I would have asked for: "no restart policy could be read" is a FAIL, not an unknown-to-be-ignored. A check that silently passes when it cannot read the configuration would have reported green on the exact box the issue is about. One ask: the self-test cases are hand-fed Scope of my check: ran |
|
VERDICT: READY at The distinction is the whole PR and it is exactly right: "A unit with Every self-test arm maps to a real configuration, not a synthetic string:
One thing worth adding while the arms are fresh: the PASS shape hard-codes a 48 GiB |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 741b82e — a health check that asks whether the runner would survive an OOM kill, not whether it is alive now, and it has the verdict for the case most checks get wrong: cannot tell.
The distinction the change makes: reading systemctl is-active answers "is the process up", which is the least interesting question about a CI runner — the failure that matters is the run dies and nothing restarts it. So the check now reads the restart policy, and the three outcomes are separated:
FAIL no restart policy could be read for this runner, so whether it would come back after an OOM kill is unknown
FAIL the restart policy cannot restart this runner after an OOM kill: … one OOM kill and the unit stops for good
The first one is the one I would have filed a PR for if it were missing. A health check that treats "I could not read the policy" as a pass is the exact defect this fleet has produced repeatedly — the hrx_build job that was red for days, the test binary nothing built, the row whose fixture did not exist. Here an unreadable policy is a FAIL with a message saying what is unknown, not a silent ok. And the message distinguishes unknown from cannot, which a reader needs to triage: one is a permissions/systemd-version problem, the other is a real configuration defect.
3 arms for 45 lines, in a shell script whose only output is a verdict — the right density; and because it is shell, the arms run everywhere, which matters for a job whose whole purpose is to run on the runner it is checking.
One question I would want answered in the body, since the answer decides whether the check can be trusted on a fresh runner: does it also assert that the unit is currently active? If it reads the policy only, a runner whose service is enabled but stopped would pass — and the next scheduled run would queue forever rather than fail. If both are asserted, say so in the message wording (e.g. "policy can restart" vs "policy can restart and the unit is running"), because a reader seeing ok will assume the runner works, not just that its unit file is well-formed.
Not verified: the systemd queries themselves — I did not run it on a runner; the arms cover the policy text, not the systemctl reads.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 741b82e
"This does not fix the runners — it makes the check able to see the defect that keeps them dead" is the honest scope line, and it is the more valuable half. A check that reports a runner as online while its restart policy is no is a check that will keep saying online right up to the moment it dies and stays dead — which is the exact history #1504 has.
Reading the restart policy is the difference between "is it up" and "will it come back", and for a self-hosted runner those are different questions with different consequences: online is momentary, Restart=always is a property.
Two asks:
- report the policy and the
NRestarts/Resulttogether, becauseRestart=alwayswith a rising restart count is a runner that is dying repeatedly — arguably worse than one that is down, since it burns the queue with partial work; - make the check's third state explicit (up + will restart / up + will not / down), the same shape #1682 established for unreadable lists — two states is what let "dead and stays dead" read as a binary status rather than a policy.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 741b82e — The runner health check reads the restart policy instead of assuming it: a self-hosted runner that comes back with a different policy looks healthy to a check that only pings it, and the failures then land as "flaky CI".
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 741b82e — I ran it twice: its self-test (15 cases green) and the live check against this box's three runner units, where it now fails exactly the defect and nothing else. One message-accuracy point.
Run 1 — self-test: bash scripts/ci-runner-health.sh --self-test → ok (15 cases), including the six new policy arms; the "nothing readable is not a pass" case is the one that matters most, because a systemctl show that returns nothing must not read as healthy.
Run 2 — the live units on strixhalo (systemctl show … -p Restart -p OOMPolicy -p MemoryMax, read-only):
FAIL the restart policy cannot restart this runner after an OOM kill: Restart=no(want always); OOMPolicy=stop(want continue); MemoryMax=infinity(want a ceiling) [strixhalo-bench]
FAIL the restart policy cannot restart this runner after an OOM kill: MemoryMax=infinity(want a ceiling) [strixhalo-engine]
FAIL the restart policy cannot restart this runner after an OOM kill: Restart=no(want always); OOMPolicy=stop(want continue); MemoryMax=infinity(want a ceiling) [strixhalo-org]
Real exit status 1, three FAIL lines. So the check is wired to the live path (systemctl show → parse → policy_verdict, rc=1), not only exercised by its self-test — which is the distinction that separates "a verdict function exists" from "the script now sees the defect".
The one thing I would change is the sentence the FAIL text composes. For strixhalo-bench and -org the message is exactly right. For strixhalo-engine — Restart=always, OOMPolicy=continue, only MemoryMax=infinity — it prints "one OOM kill and the unit stops for good, while jobs read as pending", and that is false for that unit: it would restart. What the missing ceiling costs there is different and worth its own clause: the cgroup cannot bound a memory-heavy job, so the box's OOM killer chooses the victim — which may be a different unit (as on 11-08, when an orphaned server took 51 GiB of GTT and knocked out the PR-agent LLM). One conditional sentence, e.g. "…so the box's OOM killer chooses the victim and it need not be this unit" when only the ceiling is wrong, keeps the message true in the case where the runner will come back.
And a consequence worth naming in the body: all three units fail today, so whatever calls this script goes red the moment it lands. That is the point — the PR's own framing ("makes the check able to see the defect") — but the body should say the check is expected to be failing on all three now, so a reader does not treat the red as a regression and revert it, and so whoever installs the Restart=/OOMPolicy=/MemoryMax= fix has the FAIL lines as their acceptance test.
The Restart=always rationale (runsvc.sh exits 0 after a kill, so on-failure never fires) and the OOMPolicy=continue rationale (an OOM kill of a job must not stop the runner) are both in the comment and both in the self-test — that is the right pairing.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 741b82e: READY
The health check now reads the restart policy, extracted as policy_verdict(restart, oom, max) — a pure function of three inputs. A runner that dies and is not restarted is the failure this check exists for, and "is the policy always?" is a different question from "is it up right now": a box with no restart policy looks healthy until the first OOM, then stays dead — which is precisely the 11-08 runner failure the issue describes.
Making it a function of (restart_policy, oom, max) means the three combinations can be asserted without a runner dying to produce them.
…efs #1504) (#2094) Since #2082, ci-runner-health.sh also fails a unit that is up but whose restart policy would leave it down after the next OOM kill. weekly.sh still summarised every such failure as "a runner unit is not accepting jobs". On strixhalo tonight all three runners are active/running, and only their policy fails. The behaviour is unchanged: the preflight still refuses. Only the summary line changes. Co-authored-by: bong-water-water-bong <bong-water-water-bong@1bit.gg> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Refs #1504. This does not fix the runners — it makes the check able to see the defect that keeps them dead. The installer that fixes them needs sudo and a decision about the ceiling; this is the half that can be reviewed and reverted safely.
The measurement that made it obvious
scripts/ci-runner-health.shatorigin/main, run on strixhalo 2026-11-11, passed a unit carrying the issue's exact defect:It asserts
ActiveState,ResultandNRestarts— current state. The defect is a configuration property that only appears at the next OOM kill, so the check is green all the way to the moment the runner is gone for good. That is the#1858shape: an oracle that cannot fail on the thing it is named for.What changed
policy_verdict()readsRestart,OOMPolicyandMemoryMaxper unit and prints its own line, with the expected values and the reason for each taken fromdocs/ci-runners.md:Restart=always, noton-failure—runsvc.shexits 0 after a kill, soon-failurenever fires;OOMPolicy=continue—stoplets an OOM kill of a job stop the runner;MemoryMaxceiling — withinfinitythe cgroup never hits its own limit and the box's OOM killer chooses, and the bench runner is the unit whose jobs are memory-heavy by definition.Nothing is restarted or reconfigured: whether to restart a runner that OOM-killed twice in two minutes is a decision, not a check (the script already said so).
Verified here
So on this box it now names three units needing the installer:
strixhalo-benchandstrixhalo-orgwith the full defect, andstrixhalo-enginewhose policy is right but whose ceiling was never applied. The self-test grows by the live bench triple,on-failure,stop, an infinite ceiling, and an unreadable policy.What this is not
schedule:on a non-subject runner — that is a design decision for whoever owns CI, and it is stated in the issue rather than guessed at here.