Skip to content

feat(scale-down): opt-in idle confirmation window before terminating not-busy runners - #5228

Closed
jensenbox wants to merge 1 commit into
github-aws-runners:mainfrom
closient:fix/scale-down-idle-confirmation
Closed

feat(scale-down): opt-in idle confirmation window before terminating not-busy runners#5228
jensenbox wants to merge 1 commit into
github-aws-runners:mainfrom
closient:fix/scale-down-idle-confirmation

Conversation

@jensenbox

Copy link
Copy Markdown
Contributor

Problem

Closes the failure mode reported in #5085, including the harder variant discussed in its comments: GitHub's runner busy flag 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 through removeRunner's normal path on a busy: false reading, in two distinct shapes:

Variant A — assignment lag. The flag reads false for 25–60+ seconds after a job is already running on the runner:

job started 23:44:18 ... Busy: false at 23:45:18  → terminated → job killed
job started 23:44:23 ... Busy: false at 23:45:15  → terminated → job killed

Variant B — stale flag mid-job. The flag flips to false on a runner that has been continuously executing one job for many minutes:

03:05:10  Runner 'i-0a014...' - Busy: true   → correctly skipped
03:10:12  Runner 'i-0a014...' - Busy: false  → terminated; job died at 98% complete, all tests passing

Another case read false 12m15s 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 past minimum_running_time_in_minutes is 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 = 0 keeps today's single-reading behaviour bit-for-bit):

  • On a not-busy reading, scale-down tags the instance ghr:idle_detected_at=<ISO8601> and defers termination.
  • It terminates only when not-busy readings span at least the configured window (set it to one scale-down schedule interval to require two consecutive not-busy evaluations).
  • Any busy reading clears the tag and resets the window — this is what saves the Variant B runners, which read true on the tick before their fatal stale reading.
  • An unparsable tag value restarts the window rather than blocking or terminating.
  • Each invocation logs a census (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/DeleteTags for ghr:orphan. Threaded through the root module and multi-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

  • Unit tests: 5 new cases (first-reading deferral, in-window deferral, window-elapsed termination, busy-reading reset, unparsable-tag restart); full control-plane suite green on main (542 tests), eslint + prettier clean.
  • Production: we have run this patch on our fleet (both pools, 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 a false reading reported busy: true five 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.

@edersonbrilhante

Copy link
Copy Markdown
Contributor

@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.
@jensenbox
jensenbox force-pushed the fix/scale-down-idle-confirmation branch from a1c52b3 to 6a6c2f7 Compare August 21, 2026 04:35
@jensenbox

Copy link
Copy Markdown
Contributor Author

Rebased onto current main and ported to the compute-provider plugin framework from #5234. Sorry for the delay — the refactor landed the day after your rebase request, so this needed a port rather than a rebase.

Not a breaking interface change

The 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.

ScaleDownComputeProvider now has markIdle / unmarkIdle, and RunnerInfo carries idleDetectedAt. Both methods are optional, deliberately:

  • a provider with nowhere to persist per-runner state stays type-valid against the interface — nothing existing has to change
  • scale-down checks for the capability and, when it is absent, skips the confirmation window entirely and keeps the previous single-reading behaviour rather than failing or deferring forever
  • only a provider implementing them opts into the feature

aws/ec2 implements both with instance tags (ghr:idle_detected_at) — the same mechanism ghr:orphan already uses, so no new state store. The scaffold in templates/provider documents both as optional so plugin authors see them.

If you would rather express this as a separate capability interface than two optional methods on ScaleDownComputeProvider, say the word — that is a small change and I would rather match your intent for the framework than guess.

Why the feature

GitHub's busy flag can be stale. It reads false for runners actively executing a job, both shortly after assignment (25-60s lag) and deep into a running job (12+ minutes) — see #5085. A single busy=false reading is not sufficient evidence a runner is idle, so scale-down can terminate mid-job.

SCALE_DOWN_IDLE_CONFIRMATION_SECONDS (default 0, i.e. current behaviour) requires busy=false readings spanning at least that window before terminating. Any busy=true reading clears the marker and restarts the window.

Verification

  • 265 passed across the full scale-runners suite, 7 files
  • control-plane build clean, 0 TypeScript errors
  • 5 new tests: window start, deferral mid-window, elapse-then-terminate, the disabled (0) path, and a provider implementing neither method
  • I checked the new tests actually bite rather than assuming: stubbing idleConfirmed to always confirm fails exactly the window-start and deferral cases

Terraform plumbing (scale_down_idle_confirmation_seconds) is unchanged from the original PR and carried through the rebase cleanly.

@lochlanbennettodlum-taktile

Copy link
Copy Markdown
Contributor

Opened #5397 with this change rebased onto current main and ported through the multi-runner translation layers, plus the three lint fixes. Kept you as commit author, all credit to you for the approach and the fleet data. Only real addition is clearing the idle tag for runners kept warm by idle_config, since those never hit the removal path. Happy to close mine if you'd rather push the rebase here instead.

edersonbrilhante pushed a commit that referenced this pull request Sep 9, 2026
…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>
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.

3 participants