feat(scale-down): opt-in idle confirmation window before terminating not-busy runners - #5397
Open
lochlanbennettodlum-taktile wants to merge 1 commit into
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>
Contributor
|
@lochlanbennettodlum-taktile we plan to add dynamodb as state db. We can make the new functions required. |
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
3 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:
markIdle/unmarkIdleare optional onScaleDownComputeProvider. Providers that can't persist per-runner state just don't get the window. EC2 uses an instance tag, same asghr:orphan, so no IAM changes. @edersonbrilhante happy to make this a separate capability interface if you'd rather.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