feat(scale-down): opt-in idle confirmation window before terminating not-busy runners - #5228
Conversation
|
@jensenbox Can you rebase? Heads up, I am working to refactor the module to introduce plugin layout. Ref: #5234 |
…y runners GitHub's busy flag can be stale: it reads false for runners that are actively executing a job, both shortly after job assignment (observed 25-60s lag) and deep into a running job (observed 12+ minutes). See github-aws-runners#5085. A single busy=false reading is therefore not sufficient evidence that a runner is idle, and scale-down can terminate a runner mid-job. SCALE_DOWN_IDLE_CONFIRMATION_SECONDS (default 0, previous behaviour) requires busy=false readings spanning at least that window before terminating. Any busy=true reading in between clears the marker and restarts the window. Ported onto the compute-provider plugin framework introduced in github-aws-runners#5234: - core: RunnerInfo gains `idleDetectedAt`; ScaleDownComputeProvider gains `markIdle` / `unmarkIdle`. Both are OPTIONAL, so this is not a breaking change for provider plugins -- a provider with nowhere to persist per-runner state stays type-valid, and scale-down skips the window for it rather than failing. Only providers implementing them opt into the behaviour. - aws/ec2: implements both via instance tags (`ghr:idle_detected_at`), the same mechanism `ghr:orphan` already uses, so no new state store is needed. - templates/provider: the scaffold documents both as optional. - The orchestration in scale-runners/scale-down.ts is provider-agnostic and calls through the interface rather than tagging EC2 directly. Tests: 5 cases covering window start, deferral, elapse-then-terminate, the disabled (0) path, and a provider that implements neither method. Verified the tests bite by stubbing idleConfirmed to always confirm -- the window-start and deferral cases fail as expected. Full scale-runners suite: 265 passed.
a1c52b3 to
6a6c2f7
Compare
|
Rebased onto current Not a breaking interface changeThe feature needs to persist "when was this runner first seen idle" across scale-down invocations. Before #5234 it tagged EC2 directly from the orchestration code, which is no longer the right layer.
If you would rather express this as a separate capability interface than two optional methods on Why the featureGitHub's
Verification
Terraform plumbing ( |
|
Opened #5397 with this change rebased onto current |
…not-busy runners (#5397) ## Description Closes #5085. Supersedes #5228. This is @jensenbox's idle confirmation window from #5228, rebased onto the new provider layout and with the lint failures fixed. All credit for the approach and the production data is theirs, I just carried it forward since it stopped applying after the plugin refactor. Quick recap of why: the GitHub `busy` flag isn't trustworthy enough to terminate on a single reading. People on #5085 have CloudTrail evidence of it reading `false` seconds after a job started, and in some cases minutes into a running job. The deregister-then-recheck idea in #5086 can't help because a GET after DELETE returns 404, which we already treat as "not busy". So instead, opt in with `scale_down_idle_confirmation_seconds`. On a not-busy reading the runner gets tagged and termination is deferred. It's only terminated when a later evaluation still says not-busy and the window has elapsed. A busy reading in between clears the tag and starts over. Default is `0`, which is the current behaviour. A couple of things I changed while porting: - `ScaleDownComputeProvider` gains `markIdle` / `unmarkIdle`. They're required, per @edersonbrilhante's note below about DynamoDB becoming the state store. EC2 uses an instance tag for now, same as `ghr:orphan`, so no IAM changes. The template provider stubs both. - Runners kept warm by `idle_config` also get their tag cleared. They never go through the removal path, so an old tag on one would otherwise let it be terminated on the first not-busy reading once the idle count drops. Added a test for this. - The variable is threaded through the root module, `modules/runners` and `modules/multi-runner` (including the new translation layers). I left the experimental webhook orchestration module for a follow-up. ## Test Plan - `yarn format-check`, `yarn lint`, `yarn build`, `yarn test` all green - New tests for: window start, deferral, termination after the window, busy reset, kept-idle reset, disabled window, EC2 tag payloads - `terraform validate`, `fmt -check` and `terraform test` pass for `modules/runners` and `modules/multi-runner` - From the original PR: 14 hours on the author's fleet with a 300s window, zero mid-job terminations vs roughly one burst per hour before ## Related Issues #5085, #5228, #5086, #5201 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Christian Jensen <christian@closient.com> Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
Problem
Closes the failure mode reported in #5085, including the harder variant discussed in its comments: GitHub's runner
busyflag is not a reliable input for termination decisions. We traced 14 terminated-mid-job runners across one incident window (org-level, non-ephemeral, on-demand, module v7.10.0, scale-down on the default 5-minute schedule) and every one was killed throughremoveRunner's normal path on abusy: falsereading, in two distinct shapes:Variant A — assignment lag. The flag reads
falsefor 25–60+ seconds after a job is already running on the runner:Variant B — stale flag mid-job. The flag flips to
falseon a runner that has been continuously executing one job for many minutes:Another case read
false12m15s into a running job. Because the DELETE call also succeeds in these cases (GitHub's server-side view is consistent with the wrong flag), no re-check-after-deregister sequencing can prevent Variant B — the module needs evidence across time instead of a single reading. With scale-to-zero config (idle_config = []) every instance pastminimum_running_time_in_minutesis evaluated on every tick, so one stale-flag window kills several runners in the same invocation — we measured roughly one multi-runner burst per hour under CI load, each costing a full matrix rerun.Change
Adds an opt-in confirmation window, default off (
scale_down_idle_confirmation_seconds = 0keeps today's single-reading behaviour bit-for-bit):ghr:idle_detected_at=<ISO8601>and defers termination.trueon the tick before their fatal stale reading.evaluated N runner(s): busy=… idle-deferred=… terminated=…) so "ran and found nothing" is distinguishable from "never ran".No new IAM: the scale-down role already carries scoped
ec2:CreateTags/DeleteTagsforghr:orphan. Threaded through the root module andmulti-runner(runner_config.scale_down_idle_confirmation_seconds). Existing behaviours unchanged: bypass-removal, orphan handling, and the "only terminate EC2 after successful de-registration" guard.Cost trade-off: a genuinely idle runner lives one extra evaluation interval before reap (~5 minutes on the default schedule).
Validation
main(542 tests), eslint + prettier clean.300s window, 5-minute schedule) since 2026-07-28 05:32 UTC. Prior baseline: ~1 kill burst/hour. Since deploy: zero mid-job terminations in 14 hours of CI load, ~35 terminations all via the confirmed-idle path, zero de-registration failures. We directly observed the mechanism catching a would-have-been kill: a runner deferred on afalsereading reportedbusy: truefive minutes later and survived.Happy to adjust naming/defaults or split the census logging out if you'd prefer a narrower diff. cc @npwolf / @ben-smyth from #5085 — this covers Variant B from your reports, which the deregister-then-recheck proposal cannot.