Skip to content

fix(scheduler): never destroy a runner that is executing a job - #151

Open
luthermonson wants to merge 1 commit into
mainfrom
fix/never-reap-busy-runner
Open

fix(scheduler): never destroy a runner that is executing a job#151
luthermonson wants to merge 1 commit into
mainfrom
fix/never-reap-busy-runner

Conversation

@luthermonson

@luthermonson luthermonson commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

The bug

The orphan sweep could destroy a runner that was actively executing a build.

Observed 2026-08-12 on linux-amd64 (coyotes): three dind-test jobs with
identical labels dispatched concurrently, and a live build torn down under it.

"Unbound" was inferred from webhooks and never verified. runnerBinding.bound
flips only when handleInProgress processes an in_progress delivery naming that
runner. Until then the runner looks idle in the ledger regardless of what it is
actually running.

That is not a dropped-delivery edge case. It is the normal shape of a same-label
burst, and rule 2 fires immediately, with no grace window:

  1. Three same-label jobs A, B, C queue. Three JIT runners rA, rB, rC are dispatched
    (intent keys A, B, C). All three intent keys land in served, so class demand is 0.
  2. GitHub permutes the assignments — say rA→B, rB→C, rC→A — and all three runners
    start executing.
  3. The three in_progress deliveries arrive over three separate HTTP requests and are
    processed in whatever order they land. The first one processed (job A, naming rC)
    sets started[A] and binds rC.
  4. started[A] is the discharge signal for rA (intent key A), not for the runner
    GitHub actually gave A to. rA is now unbound + discharged + zero class demand.
    That is rule 2 exactly, and it retires immediately.
  5. handleCompleted calls sweepOrphanRunners on every completion, so a sweep
    lands in that window routinely.

rA was executing job B. The window is just the inter-delivery skew between two
in_progress webhooks of the same burst
— hundreds of milliseconds is enough, and
nothing has to be dropped, delayed or reordered. Rule 1 kills the same runner more
slowly whenever a delivery genuinely is lost.

This confirms the original analysis and sharpens it: reproducing it needs no missing
webhook at all.

The fix: the rules nominate, a busy check vetoes

The nomination logic is unchanged — rule 1, rule 2, the demand counting, the
observed-assignment teardown keying, reapRunnerLocked's staleness bail. All of it
stays. What changes is that it no longer has authority.

Both rules now produce nominations. Nothing is unhooked until a check taken at the
moment of teardown
, from ground truth rather than event history, confirms no job is
executing. Hard invariant: never destroy a runner that is executing a job.

1. Local introspection (primary) — pkg/runnerbusy

The actions-runner forks a Runner.Worker child only while a job is executing. The
listener is alive for the runner's whole life, so "a runner process exists" is not the
signal; the worker is.

Path Probe Notes
Linux, containerd task.Pids() then argv[0] (fallback comm) from /proc runc leaves container PIDs visible in the host namespace
Windows, HCS / Hyper-V hcsshim ProcessList() then ImageName guest processes are invisible to the host process table; HCS proxies the listing through the GCS. Same handle pkg/metrics already opens per container
macOS VM pgrep -x Runner.Worker over the existing per-job SSH channel the host sees the guest as one opaque process
Native macOS pgrep -g <pgid> -x Runner.Worker the runner is a process-group leader; the worker inherits the group
Dispatched to the Linux sidecar VM explicitly unavailable that containerd is behind the dispatch gRPC boundary and the host has no view into the guest PID namespace. Degrades to layer 2 — it never silently answers "not busy"

Every failure mode resolves to Unknown: task query failed, empty process list, no listed
PID readable under /proc, no probe on this platform. State's zero value is Unknown,
so nothing can accidentally read as idle.

2. GitHub busy flag (secondary)

providers.RunnerBusyReporter, implemented by the GitHub provider over
GET .../actions/runners/{id} (org- or repo-scoped, matching how the JIT runner was
registered). Consulted only when the local probe cannot answer, and only for a
runner already nominated — one GET per nomination, off every hot path. A 404 reads as
not-busy (an ephemeral runner deregisters itself when its job ends); every other error
is an error, which fails safe.

3. Race handling

The probes do I/O, so they run with s.mu released. That opens a window for an
in_progress delivery to bind a nominated runner, so reapRunnerLocked now
re-validates — same *runnerBinding, still unbound — before unhooking anything.
A runner bound during the probe survives its own nomination. Covered by
TestSweepOrphanRunners_BindingRaceDuringProbe.

Escape hatches

A veto that could never be overridden trades one leak for another. Two bounds, both
logged at warn with an ESCAPE: prefix and counted in
ephemerd_orphan_reap_decisions_total{outcome="escaped"}:

  • Unknown, bounded by the grace window. If busy-ness cannot be determined for the
    whole grace window, teardown proceeds. This is exactly the pre-veto behaviour, so no
    platform is worse off than before, and rule 1 nominations (already past the window)
    keep their original timing where no probe exists.
  • Busy, bounded by the hard bound. A positive busy verdict is overridden only past
    job_timeout + 30m (or GitHub's 6h per-job ceiling + 30m when no job timeout is set).
    A job exceeding job_timeout has already had its context cancelled and its runner torn
    down by the normal path, so a runner still claiming busy out there is wedged, not working.

Why time and not consecutive failed probes. The sweep runs on every job completion as
well as on a timer. A probe-count escape would fire within milliseconds on a busy node —
during exactly the same-label burst this change exists to survive — and effectively never
on a quiet one. It is backwards: loosest precisely when the risk is highest. Wedged-ness
is a property of duration, so duration is what bounds it. The verdict asymmetry does the
rest: a definite busy answer is held to the strong bound, an undeterminable one only
to the weak one.

orphan_grace can now be relaxed (recommendation only — fleet config untouched)

orphan_grace existed to paper over exactly this uncertainty: too short killed live work,
too long squatted a concurrency slot (90 minutes of one on a max_concurrent = 1 host).
It is no longer load-bearing — it now only governs how long an undeterminable runner is
held.

For pools whose runners are locally probeable (every Linux and Windows pool, plus macOS-VM
jobs), the tuned values can be dropped and the 10m default left alone:

  • pools.linux-arm64.orphan_grace = "2m" — the stopgap for the mac double-claiming arm64.
    The veto answers that case directly ("is the loser actually busy?" then no, immediately),
    so this can be dropped.
  • pools.mac-arm64.orphan_grace = "15m" — same reasoning for macOS-VM jobs, which are
    SSH-probeable. Droppable.
  • The only pool where the value still does real work is one relying on the dispatched
    Linux-sidecar-VM path and unable to reach the GitHub API, where the fallback bound
    applies. Both are off in the current fleet ([pools.mac-arm64.vm.linux] enabled = false).

The fleet config is deliberately not changed here.

Tests

  • decideReap is a pure function (repo idiom: imagegc.PlanEviction,
    controlPlaneInputRules) with a full matrix: busy never reaped inside or past the
    grace window, verified-idle reaped immediately, unknown treated as busy, each escape
    firing only past its own bound.
  • TestReapPolicy pins the bound derivation.
  • TestSweepOrphanRunners_BusyVeto runs the same matrix end-to-end through the real sweep.
  • TestSweepOrphanRunners_BindingRaceDuringProbe pins the bind-during-probe race.
  • TestProbeLocalBusy_UnavailablePathsAreUnknown pins that no unavailable path can
    return idle.
  • TestProbeRunnerBusy_FallsBackToProvider pins the layering and its fail-safe.
  • pkg/runnerbusy: IsWorkerProcess across Linux argv0 / comm / Windows ImageName, with
    Runner.Listener explicitly not matching; the Linux probe end-to-end against a real
    process named Runner.Worker; the Windows probe against a faked HCS process list; all
    three Unknown failure modes on each.
  • pkg/github: org/repo scoping, 404-is-not-busy, error-is-not-an-answer.
  • Existing rule-1/rule-2 nomination tests are preserved. The rule-2 matrix now pins the
    busy probe to verified-idle, because it is testing nomination; without that pin every
    row would silently be testing the veto instead.

Conflict surface

Confined to pkg/scheduler plus the new pkg/runnerbusy, with small additive changes to
pkg/providers, pkg/github, pkg/native and pkg/metrics. No overlap with #150
(pkg/imagegc, pkg/buildkit, pkg/dind, cmd/ephemerd/main.go,
.github/workflows/dind-test.yml) — in particular cmd/ephemerd/main.go is untouched:
the prober is built from existing scheduler config.

Metal: the failure, reconstructed

Reproduced from mfl-linux-amd64-100 (coyotes)'s own journal — this is the incident, not a
model of it. dind-test run 31651684484, three same-label jobs, docker-build = job
94297174551:

23:39:33  queued 94297174556, 94297174551, 94297174567
23:39:34  JIT runner vivid_heisenberg  (intent 551)
23:39:35  JIT runner warm_ride         (intent 556)
23:39:35  JIT runner slim_mayer        (intent 567)

23:39:41.967  in_progress 556 -> vivid_heisenberg   # sets started[556]; binds vivid_heisenberg
23:39:41.967  in_progress 567 -> slim_mayer         # binds slim_mayer
23:39:45.368  vivid_heisenberg exits (556 done); 551 never observed -> re-provision
23:39:47.272  JIT runner slim_hopper   (intent 551)

23:39:48.403  completed 567 -> handleCompleted -> defer sweepOrphanRunners()
23:39:48.403  "retiring discharged runner" runner=warm_ride dispatched_for_job=556
23:39:48.403  "destroying runner environment" id=warm_ride

23:39:50.716  in_progress 551 -> warm_ride          # 2.3s TOO LATE
23:49:42      completed 551 -> failure

GitHub annotation on docker-build: "The self-hosted runner lost communication with the
server."

warm_ride was the runner GitHub had given docker-build to. At 23:39:48 it was unbound
(its in_progress was still in flight), discharged (started[556] was set 6.4s earlier by
a sibling's in_progress), and its class had zero demand — rule 2 exactly. It was
destroyed 2.3 seconds before the webhook that would have bound it. No delivery was
dropped, delayed or reordered.

Metal: the fix, proven

A test build of this branch (v0.1.9-151veto, built on the node itself from the public
branch tarball) was deployed to the same host, with orphan_grace temporarily set to 5s
so the sweep's rules would nominate aggressively, then driven with a sustained stream of
same-label dind-test jobs.

Three runners were nominated for teardown while executing a job, and every one was
vetoed by the local container probe:

01:19:58.039  orphan sweep nomination vetoed  runner=steady_hopper  busy_verdict=busy probe=container
01:19:58.371  in_progress 94313841886 -> steady_hopper      (+332ms)
01:20:03.558  completed  94313841886 -> success

01:20:26.213  orphan sweep nomination vetoed  runner=sure_darwin    busy_verdict=busy probe=container
01:20:26.976  in_progress 94313882496 -> sure_darwin        (+763ms)
01:20:33.276  completed  94313882496 -> success

01:20:41.179  orphan sweep nomination vetoed  runner=sure_sagan     busy_verdict=busy probe=container
01:20:41.786  in_progress 94313884220 -> sure_sagan         (+607ms)
01:20:47.683  completed  94313884220 -> success

Each is the identical race that killed warm_ride: nominated a fraction of a second before
the in_progress that would have protected it. All three survived; all three jobs finished
successfully.

The veto is not a blanket. In the same window a genuinely idle runner was still reaped,
because the probe positively observed no worker in it:

01:21:01.433  destroying orphaned runner: dispatched but never assigned a job within the
              grace window  runner=sharp_ada  busy_verdict=idle  grace=5s
ephemerd_orphan_reap_decisions_total{outcome="vetoed",verdict="busy"} 3
ephemerd_orphan_reap_decisions_total{outcome="reaped",verdict="idle"} 1
ephemerd_orphan_reap_decisions_total{outcome="escaped",...}          0

Every dind-test run in the exercise passed. The node was then restored to its previous
binary and config, and a further dind-test on main completed green.

One honest limit

sharp_ada was reaped as idle at 01:21:01.433 and GitHub's in_progress for it arrived
1.37s later — GitHub had assigned it a job, but the runner had not yet forked a worker, so
the probe correctly answered "idle". The self-heal handled it (abandoning dispatch: job was observed running while this dispatch waited for a concurrency slot) and no job was
lost, but it is worth stating plainly: the busy check closes the window to the sub-second
gap between "GitHub assigns" and "the worker spawns", not to zero.
Covering that
remainder is what the grace window is for, and it only came into play here because the test
had orphan_grace cranked down to 5 seconds — three orders of magnitude below the 10m
default.

The orphan sweep could retire a runner mid-build. Observed 2026-08-12:
three same-label `dind-test` jobs dispatched concurrently, and a live
build torn down under it.

"Unbound" was inferred from webhooks and never verified. A runner only
looks busy once `handleInProgress` processes an `in_progress` delivery
naming it; until then it looks idle no matter what it is running. That
is not a dropped-delivery edge case, it is the normal shape of a
same-label burst:

  1. Three same-label jobs queue; three JIT runners are dispatched, so
     every job is `served` and class demand is 0.
  2. GitHub permutes the assignments and all three runners start work.
  3. The first `in_progress` processed sets `started[A]` — which is the
     discharge signal for the runner dispatched FOR A, not for the
     runner GitHub gave A to.
  4. That runner is now unbound + discharged + in a zero-demand class:
     rule 2, which fires immediately with no grace window.
  5. `handleCompleted` sweeps on every completion, so a sweep lands in
     that window routinely.

The window is the inter-delivery skew between two `in_progress`
webhooks. Nothing has to be dropped or reordered. Rule 1 kills the same
runner more slowly whenever a delivery genuinely is lost.

Both rules now only NOMINATE. Teardown is gated on a busy check taken at
the moment of reaping, from ground truth rather than event history:

  1. Local introspection for the runner's worker process. The
     actions-runner forks `Runner.Worker` only while a job is executing
     (the listener is always alive, so process-exists is not the
     signal). Linux: containerd task PIDs read via /proc. Windows: HCS
     `ProcessList` — Hyper-V isolated guests are invisible to the host
     process table. macOS VM: pgrep over the existing per-job SSH
     channel. Native macOS: pgrep against the runner's process group.
     Dispatched into the Linux sidecar VM: explicitly unavailable.
  2. GitHub's per-runner `busy` flag, only for nominations the local
     probe could not answer. One GET per nomination, off every hot path.
  3. Unknown — which means possibly busy, never idle.

The probes run with the scheduler mutex released, so `reapRunnerLocked`
re-validates the ledger entry before unhooking: a runner bound by a
webhook that landed during the probe survives its nomination.

Two bounded escapes keep a wedged runner from squatting a slot forever,
both logged loudly and counted in
`ephemerd_orphan_reap_decisions_total{outcome="escaped"}`: an
undeterminable state escapes at the grace window (exactly the pre-veto
behaviour, so no platform is worse off than before), and a positive busy
verdict escapes at `job_timeout + 30m`. Both are measured in elapsed
time rather than consecutive failed probes — the sweep runs on every
completion, so a probe-count escape would fire in milliseconds during
the very burst this change exists to survive.

The decision is a pure function (`decideReap`) with a table test; the
probes, the provider fallback, the fail-safe unknown paths and the
bind-during-probe race have their own tests.

`orphan_grace` stops being load-bearing: it now only governs how long an
undeterminable runner is held, so the per-pool tuning it accumulated is
no longer needed on any pool whose runners are locally probeable.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant