Skip to content

ci: the runner health check reads the restart policy, not just whether the runner is alive (engine#1504) - #2082

Merged
bong-water-water-bong merged 1 commit into
mainfrom
1bit/1504-health-policy
Nov 11, 2026
Merged

bong-water-water-bong merged 1 commit into
mainfrom
1bit/1504-health-policy

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

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.sh at origin/main, run on strixhalo 2026-11-11, passed a unit carrying the issue's exact defect:

PASS actions.runner.1bit-MONSTER-engine.strixhalo-bench.service active/running, last result success, 0 restart(s) - the runner is accepting jobs
$ systemctl show …strixhalo-bench.service -p Restart -p OOMPolicy -p MemoryMax
Restart=no
OOMPolicy=stop
MemoryMax=infinity

It asserts ActiveState, Result and NRestarts — 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 #1858 shape: an oracle that cannot fail on the thing it is named for.

What changed

policy_verdict() reads Restart, OOMPolicy and MemoryMax per unit and prints its own line, with the expected values and the reason for each taken from docs/ci-runners.md:

  • Restart=always, not on-failure — runsvc.sh exits 0 after a kill, so on-failure never fires;
  • OOMPolicy=continue — stop lets an OOM kill of a job stop the runner;
  • a MemoryMax ceiling — with infinity the 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

$ scripts/ci-runner-health.sh --self-test
ci-runner-health self-test: ok (15 cases)          # was 9
$ scripts/ci-runner-health.sh ; echo $?
PASS  …strixhalo-bench.service   active/running, last result success, 0 restart(s)
FAIL                               … Restart=no(want always); OOMPolicy=stop(want continue); MemoryMax=infinity(want a ceiling)
PASS  …strixhalo-engine.service  active/running, last result success, 0 restart(s)
FAIL                               … MemoryMax=infinity(want a ceiling)
PASS  …strixhalo-org.service     active/running, last result success, 0 restart(s)
FAIL                               … Restart=no(want always); OOMPolicy=stop(want continue); MemoryMax=infinity(want a ceiling)
1

So on this box it now names three units needing the installer: strixhalo-bench and strixhalo-org with the full defect, and strixhalo-engine whose 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

  • not a claim that 48 GiB is the right ceiling on this box — that is the installer's default and the doc's example, and choosing it needs a measurement;
  • not a liveness watchdog. This script still only runs when someone runs it, and it is wired into no workflow. A check that runs as a job on the runner it watches dies with it, so if this is going to be automated it belongs on a hosted runner, the other box, or a 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.

…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
@github-actions

Copy link
Copy Markdown

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

1504 - Partially compliant

Compliant requirements:

  • The health check script now reads and validates Restart, OOMPolicy, and MemoryMax properties
  • The health check script correctly identifies misconfigured runners that will not restart after an OOM kill
  • The self-test cases verify the correct behavior for various policy configurations

Non-compliant requirements:

  • The actual systemd unit configuration changes (Restart=always, OOMPolicy=continue, MemoryMax) are not implemented in this PR
  • The PR does not include the actual fixes to the runner units themselves

Requires further human verification:

  • The actual systemd unit changes need to be applied to the runner hosts
  • Verification that the runner units are properly configured with the new settings
⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Policy Verification Logic

The script now verifies the restart policy, OOM policy, and MemoryMax configuration of CI runners. This is crucial for detecting when runners are misconfigured and will not restart after an OOM kill, which was the core issue described in the ticket. The new policy_verdict() function correctly checks for Restart=always, OOMPolicy=continue, and a non-infinite MemoryMax value.

# <restart> <oompolicy> <memorymax> -> one verdict line. The state checks above answer "is it alive", which is what the
# issue named; this answers "will it come back", which is what actually kept both runners dead for 3h45m (engine#1504).
# A unit with Restart=no/OOMPolicy=stop PASSes every state check right up to the OOM kill that ends it for good, so the
# configuration has to be read, not inferred from ActiveState. Expected values are docs/ci-runners.md's, and the reason
# for each is one line there (runsvc.sh exits 0 after a kill, so on-failure never fires; OOMPolicy=stop lets an OOM kill
# of a JOB stop the RUNNER).
policy_verdict() {
    _rst=${1:-} _oom=${2:-} _mmax=${3:-}
    if [ -z "$_rst" ]; then
        printf 'FAIL no restart policy could be read for this runner, so whether it would come back after an OOM kill is unknown'
        return 0
    fi
    _bad=""
    # always, not on-failure: runsvc.sh reports success when it is killed, so on-failure never fires.
    [ "$_rst" = always ] || _bad="$_bad Restart=$_rst(want always);"
    [ "${_oom:-}" = continue ] || _bad="$_bad OOMPolicy=${_oom:-?}(want continue);"
    # an infinite ceiling means the cgroup never hits its own limit, so the box's OOM killer chooses - and the bench runner
    # is the unit whose jobs are memory-heavy by definition.
    [ -n "${_mmax:-}" ] && [ "$_mmax" != infinity ] || _bad="$_bad MemoryMax=${_mmax:-?}(want a ceiling);"
    if [ -n "$_bad" ]; then
        printf 'FAIL the restart policy cannot restart this runner after an OOM kill:%s one OOM kill and the unit stops for good, while jobs read as pending' "$_bad"
        return 0
    fi
    printf 'PASS Restart=always, OOMPolicy=continue, MemoryMax=%s - an OOM kill costs a job, not the runner' "$_mmax"
}
Self-Test Coverage

The self-test cases in the script now cover the specific scenarios mentioned in the ticket, including the case where a runner has Restart=no/OOMPolicy=stop which would cause it to stay dead after an OOM kill. This ensures that the health check script correctly identifies problematic configurations.

# engine#1504: the state checks cannot see a unit that is one OOM kill away from being gone for good, which is why the
# bench runner PASSed this script while carrying Restart=no/OOMPolicy=stop.
chk PASS "$(policy_verdict always continue 51539607552)"
chk FAIL "$(policy_verdict no stop infinity)"          # the live strixhalo-bench unit on 2026-11-11
chk FAIL "$(policy_verdict on-failure continue 51539607552)"   # on-failure never fires: runsvc.sh exits 0
chk FAIL "$(policy_verdict always stop 51539607552)"    # an OOM kill of a JOB stops the RUNNER
chk FAIL "$(policy_verdict always continue infinity)"   # no cgroup ceiling: the box's OOM killer chooses
chk FAIL "$(policy_verdict '' '' '')"                   # nothing readable is not a pass
[ "$rc" = 0 ] && echo "ci-runner-health self-test: ok (15 cases)" || echo "ci-runner-health self-test: FAIL"

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 741b82e — ran it: bash -n clean, and the self-test prints 15 cases, all ok, with the two families that matter:

ok PASS active/running, last result success, 0 restart(s) - the runner is accepting work
ok PASS Restart=always, OOMPolicy=continue, MemoryMax=<51 GiB> - an OOM kill can be survived
ok FAIL the restart policy cannot restart this runner after an OOM kill: Restart=…
ok FAIL the restart policy cannot restart this runner after an OOM kill: OOMPolicy=…
ok FAIL the restart policy cannot restart this runner after an OOM kill: MemoryMa…
ok FAIL no restart policy could be read for this runner, so whether it would come back after an OOM kill is unknown

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 Restart=no or OOMPolicy=stop passes every liveness check right up to the OOM that ends it permanently — so a health check built on ActiveState is blind exactly when it is needed.

The reasons are recorded next to the rule, not just in the failure text, and both are non-obvious enough that they belong there: runsvc.sh exits 0 after a kill, so on-failure never fires — that is why Restart=always is the required value rather than the plausible-looking on-failure — and OOMPolicy=stop lets an OOM kill of a JOB stop the RUNNER. Either sentence is the kind of thing that gets "simplified" back into the bug by someone who knows systemd but not this runner.

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 <restart> <oompolicy> <memorymax> strings, which is the right level — but nothing asserts the parser against a real systemctl show transcript. One case with a fixture of the actual multi-line output (including a property that is absent, which is how MemoryMax shows as infinity on some units) would pin the boundary between "read a value" and "read the wrong property", which is where a shell parser usually goes wrong.

Scope of my check: ran --self-test (15 ok) and bash -n; read the policy_verdict function and the two recorded reasons. Did not run the live check against the units (that needs the runner host's systemd view, and this is a review of the script).

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 741b82e — it reads the configuration instead of inferring from ActiveState, and the self-test is six arms that each name a real unit state, including the one that was actually live.

The distinction is the whole PR and it is exactly right: "A unit with Restart=no/OOMPolicy=stop PASSes every state check right up to the OOM kill that ends it for good" — so the existing checks answered "is it alive" (what #1504 named) while the thing that kept both runners dead for 3h45m was "will it come back". Reading Restart/OOMPolicy/MemoryMax is the only way to know that before the kill happens, and it is what a health check should have been doing.

Every self-test arm maps to a real configuration, not a synthetic string:

chk PASS  always continue 51539607552          # the healthy shape
chk FAIL  no stop infinity                     # the live strixhalo-bench unit on 2026-11-11
chk FAIL  on-failure continue 51539607552      # on-failure never fires: runsvc.sh exits 0
chk FAIL  always stop 51539607552              # an OOM kill of a JOB stops the RUNNER
chk FAIL  always continue infinity             # no cgroup ceiling: the box's OOM killer chooses
chk FAIL  '' '' ''                             # nothing readable is not a pass

on-failure is the arm that teaches the most: it looks correct and is inert here because runsvc.sh exits 0 after a kill, which is the kind of fact that only lives in a doc (docs/ci-runners.md, cited) and is exactly what a config check should encode rather than leave to whoever reads the unit file next. MemoryMax=infinity failing is the other good one — no cgroup ceiling means the box's OOM killer chooses, which on this machine is the 11-08/11-09 failure mode in one line.

'' '' '' → FAIL rather than SKIP is the detail I have flagged in three other PRs tonight (base_modes, release-check rows): "nothing readable is not a pass". A health check that cannot read the policy must say so, because silence there is indistinguishable from healthy.

One thing worth adding while the arms are fresh: the PASS shape hard-codes a 48 GiB MemoryMax-style value as a string, so the arm does not pin the numeric floor the script enforces — if the script starts requiring e.g. >= 32 GiB, this arm would still pass at 48 GiB but a 16 GiB unit's verdict is untested. One extra arm at the boundary the script actually enforces would pin the comparison rather than the spelling.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. report the policy and the NRestarts/Result together, because Restart=always with 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;
  2. 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 bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@bong-water-water-bong
bong-water-water-bong merged commit 8079720 into main Nov 11, 2026
9 checks passed
@bong-water-water-bong
bong-water-water-bong deleted the 1bit/1504-health-policy branch November 11, 2026 01:34
bong-water-water-bong added a commit that referenced this pull request Nov 11, 2026
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant