feat(scale-down): opt-in idle confirmation window before terminating not-busy runners - #5397
Merged
edersonbrilhante merged 2 commits intoSep 9, 2026
Conversation
…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. Built on 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. Ported from github-aws-runners#5228 onto current main (github-aws-runners#5308, github-aws-runners#5312, github-aws-runners#5342 and the multi-runner effective-configuration layers). Additions over the original: - Runners kept idle by `idle_config` also have their marker cleared. They are never evaluated for removal, so a marker left on them would go stale and permit immediate termination once the idle count drops. - `modules/runners` validates the variable is >= 0; multi-runner threads it through the translation and resolved-config layers; user docs added to docs/configuration.md. - Lint fixes in the test file and capability-shape assertions updated. Co-authored-by: Lochlan Bennett-Odlum <lochlan.bennett-odlum@taktile.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This was referenced Sep 8, 2026
Contributor
|
@lochlanbennettodlum-taktile we plan to add dynamodb as state db. We can make the new functions required. |
Contributor
|
@edersonbrilhante can we merge all PRs related to this issue soon? We have problems on a daily basis, and I don't want to fork it just to patch it. |
Per maintainer feedback on github-aws-runners#5397: a DynamoDB state store is planned, so every compute provider will be able to persist per-runner state. Make `markIdle` and `unmarkIdle` required members of ScaleDownComputeProvider instead of optional, drop the "provider without idle support" fallback and its test, and update the contract-test mocks and the provider template accordingly. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Contributor
Author
|
Done in 0758f90: |
edersonbrilhante
merged commit Sep 9, 2026
595f3e5
into
github-aws-runners:main
56 of 57 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
busyflag isn't trustworthy enough to terminate on a single reading. People on #5085 have CloudTrail evidence of it readingfalseseconds 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 is0, which is the current behaviour.A couple of things I changed while porting:
ScaleDownComputeProvidergainsmarkIdle/unmarkIdle. They're required, per @edersonbrilhante's note below about DynamoDB becoming the state store. EC2 uses an instance tag for now, same asghr:orphan, so no IAM changes. The template provider stubs both.idle_configalso 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.modules/runnersandmodules/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 testall greenterraform validate,fmt -checkandterraform testpass formodules/runnersandmodules/multi-runnerRelated Issues
#5085, #5228, #5086, #5201
🤖 Generated with Claude Code