From 6a6c2f72e4722771a6efa700f33fee149c8d51cc Mon Sep 17 00:00:00 2001 From: Christian Jensen Date: Mon, 27 Jul 2026 22:29:17 -0700 Subject: [PATCH] feat(scale-down): idle confirmation window before terminating not-busy 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 #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 #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. --- .../src/scale-runners/scale-down.test.ts | 90 +++++++++++++++++++ .../src/scale-runners/scale-down.ts | 59 ++++++++++++ .../aws/ec2/src/control-plane/runners.ts | 1 + .../aws/ec2/src/control-plane/scale-down.ts | 17 ++++ lambdas/libs/compute-providers/core/index.ts | 17 ++++ .../templates/provider/control-plane.ts | 5 ++ main.tf | 1 + modules/multi-runner/runners.tf | 1 + modules/multi-runner/variables.tf | 2 + modules/runners/scale-down.tf | 1 + modules/runners/variables.tf | 6 ++ variables.tf | 6 ++ 12 files changed, 206 insertions(+) diff --git a/lambdas/functions/control-plane/src/scale-runners/scale-down.test.ts b/lambdas/functions/control-plane/src/scale-runners/scale-down.test.ts index 90320be856..a5dbff04df 100644 --- a/lambdas/functions/control-plane/src/scale-runners/scale-down.test.ts +++ b/lambdas/functions/control-plane/src/scale-runners/scale-down.test.ts @@ -38,6 +38,8 @@ const mockComputeProvider = { bootTimeExceeded: vi.fn(), markOrphan: vi.fn(), unmarkOrphan: vi.fn(), + markIdle: vi.fn(), + unmarkIdle: vi.fn(), terminate: vi.fn(), } satisfies ScaleDownComputeProvider; @@ -49,6 +51,8 @@ const mockListRunners = vi.mocked(mockComputeProvider.list); const mockBootTimeExceeded = vi.mocked(mockComputeProvider.bootTimeExceeded); const mockMarkOrphan = vi.mocked(mockComputeProvider.markOrphan); const mockUnmarkOrphan = vi.mocked(mockComputeProvider.unmarkOrphan); +const mockMarkIdle = vi.mocked(mockComputeProvider.markIdle); +const mockUnmarkIdle = vi.mocked(mockComputeProvider.unmarkIdle); const mockTerminateRunners = vi.mocked(mockComputeProvider.terminate); const cleanEnv = process.env; @@ -693,6 +697,92 @@ describe('Scale down runners', () => { }); }); +describe('Scale down with the idle confirmation window', () => { + const CONFIRMATION_SECONDS = 300; + + beforeEach(() => { + process.env = { ...cleanEnv }; + process.env.GITHUB_APP_KEY_BASE64 = 'TEST_CERTIFICATE_DATA'; + process.env.GITHUB_APP_ID = '1337'; + process.env.GITHUB_APP_CLIENT_ID = 'TEST_CLIENT_ID'; + process.env.GITHUB_APP_CLIENT_SECRET = 'TEST_CLIENT_SECRET'; + process.env.RUNNERS_MAXIMUM_COUNT = '3'; + process.env.SCALE_DOWN_CONFIG = '[]'; + process.env.ENVIRONMENT = ENVIRONMENT; + process.env.MINIMUM_RUNNING_TIME_IN_MINUTES = MINIMUM_TIME_RUNNING_IN_MINUTES.toString(); + process.env.RUNNER_BOOT_TIME_IN_MINUTES = MINIMUM_BOOT_TIME.toString(); + process.env.COMPUTE_PROVIDER_TYPE = mockComputeProvider.type; + process.env.SCALE_DOWN_IDLE_CONFIRMATION_SECONDS = CONFIRMATION_SECONDS.toString(); + vi.clearAllMocks(); + vi.resetModules(); + mockedResolveCapability.mockReturnValue(() => mockComputeProvider); + mockBootTimeExceeded.mockImplementation((runner) => { + return moment(runner.launchTime).add(MINIMUM_BOOT_TIME, 'minutes') < moment(new Date()); + }); + }); + + it('starts the window instead of terminating on the first not-busy reading', async () => { + const runners = [createRunnerTestData('idle-1', 'Org', MINIMUM_TIME_RUNNING_IN_MINUTES + 1, true, false, false)]; + mockGitHubRunners(runners); + mockProviderRunners(runners); + + await scaleDown(); + + expect(mockMarkIdle).toHaveBeenCalledWith(runners[0].id, expect.any(String)); + expect(mockTerminateRunners).not.toHaveBeenCalled(); + }); + + it('defers termination while the window has not elapsed', async () => { + const runners = [createRunnerTestData('idle-1', 'Org', MINIMUM_TIME_RUNNING_IN_MINUTES + 1, true, false, false)]; + runners[0].idleDetectedAt = new Date(Date.now() - (CONFIRMATION_SECONDS - 240) * 1000).toISOString(); + mockGitHubRunners(runners); + mockProviderRunners(runners); + + await scaleDown(); + + expect(mockMarkIdle).not.toHaveBeenCalled(); + expect(mockTerminateRunners).not.toHaveBeenCalled(); + }); + + it('terminates once not-busy readings span the window', async () => { + const runners = [createRunnerTestData('idle-1', 'Org', MINIMUM_TIME_RUNNING_IN_MINUTES + 1, true, false, true)]; + runners[0].idleDetectedAt = new Date(Date.now() - (CONFIRMATION_SECONDS + 60) * 1000).toISOString(); + mockGitHubRunners(runners); + mockProviderRunners(runners); + + await scaleDown(); + + expect(mockTerminateRunners).toHaveBeenCalledWith(runners[0].id); + }); + + it('terminates on a single reading when the window is disabled (0)', async () => { + process.env.SCALE_DOWN_IDLE_CONFIRMATION_SECONDS = '0'; + const runners = [createRunnerTestData('idle-1', 'Org', MINIMUM_TIME_RUNNING_IN_MINUTES + 1, true, false, true)]; + mockGitHubRunners(runners); + mockProviderRunners(runners); + + await scaleDown(); + + expect(mockMarkIdle).not.toHaveBeenCalled(); + expect(mockTerminateRunners).toHaveBeenCalledWith(runners[0].id); + }); + + it('terminates on a single reading when the provider cannot persist idle state', async () => { + // A provider that implements neither markIdle nor unmarkIdle must keep the previous + // single-reading behaviour rather than deferring forever. + const { markIdle: _m, unmarkIdle: _u, ...withoutIdleSupport } = mockComputeProvider; + mockedResolveCapability.mockReturnValue(() => withoutIdleSupport as unknown as typeof mockComputeProvider); + const runners = [createRunnerTestData('idle-1', 'Org', MINIMUM_TIME_RUNNING_IN_MINUTES + 1, true, false, true)]; + mockGitHubRunners(runners); + mockProviderRunners(runners); + + await scaleDown(); + + expect(mockMarkIdle).not.toHaveBeenCalled(); + expect(mockTerminateRunners).toHaveBeenCalledWith(runners[0].id); + }); +}); + function mockProviderRunners(runners: RunnerTestItem[]) { mockListRunners.mockImplementation(async (_environment, orphan) => { return runners.filter((runner) => !orphan || orphan === runner.orphan); diff --git a/lambdas/functions/control-plane/src/scale-runners/scale-down.ts b/lambdas/functions/control-plane/src/scale-runners/scale-down.ts index 3e387bce06..fbb64edd53 100644 --- a/lambdas/functions/control-plane/src/scale-runners/scale-down.ts +++ b/lambdas/functions/control-plane/src/scale-runners/scale-down.ts @@ -170,6 +170,61 @@ async function deleteGitHubRunner( } } +function idleConfirmationSeconds(): number { + const raw = process.env.SCALE_DOWN_IDLE_CONFIRMATION_SECONDS; + const parsed = raw === undefined || raw === '' ? 0 : Number(raw); + return Number.isFinite(parsed) && parsed > 0 ? parsed : 0; +} + +// 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 #5085. A single busy=false reading is therefore not +// sufficient evidence that a runner is idle. When SCALE_DOWN_IDLE_CONFIRMATION_SECONDS > 0, +// require busy=false readings spanning at least that window before terminating; any +// busy=true reading in between resets the window (see clearIdleDetection). +// +// Providers that cannot persist per-runner state do not implement markIdle/unmarkIdle; +// for those the window is skipped entirely and behaviour is unchanged. +async function idleConfirmed(runner: RunnerInfo, computeProvider: ScaleDownComputeProvider): Promise { + const confirmationSeconds = idleConfirmationSeconds(); + if (confirmationSeconds === 0 || !computeProvider.markIdle) { + return true; + } + const idleDetectedAt = runner.idleDetectedAt; + const idleForSeconds = idleDetectedAt ? (Date.now() - Date.parse(idleDetectedAt)) / 1000 : NaN; + if (Number.isNaN(idleForSeconds)) { + // No marker yet, or an unparsable one: (re)start the confirmation window. + await computeProvider.markIdle(runner.id, new Date().toISOString()); + logger.info( + `Runner '${runner.id}' reads idle; deferring termination for at least ` + + `${confirmationSeconds}s to confirm the busy state is not stale.`, + ); + return false; + } + if (idleForSeconds < confirmationSeconds) { + logger.info( + `Runner '${runner.id}' reads idle since '${idleDetectedAt}' ` + + `(${Math.round(idleForSeconds)}s < ${confirmationSeconds}s); deferring termination.`, + ); + return false; + } + logger.info( + `Runner '${runner.id}' confirmed idle since '${idleDetectedAt}' ` + + `(${Math.round(idleForSeconds)}s >= ${confirmationSeconds}s).`, + ); + return true; +} + +async function clearIdleDetection(runner: RunnerInfo, computeProvider: ScaleDownComputeProvider): Promise { + if (idleConfirmationSeconds() === 0 || !computeProvider.unmarkIdle) { + return; + } + if (runner.idleDetectedAt) { + await computeProvider.unmarkIdle(runner.id); + logger.info(`Runner '${runner.id}' is busy again; idle-detection window reset.`); + } +} + async function removeRunner( runner: RunnerInfo, ghRunnerIds: number[], @@ -192,6 +247,9 @@ async function removeRunner( ); if (states.every((busy) => busy === false)) { + if (!(await idleConfirmed(runner, computeProvider))) { + return; + } const results = await Promise.all( ghRunnerIds.map((ghRunnerId) => deleteGitHubRunner(githubInstallationClient, runner, ghRunnerId)), ); @@ -213,6 +271,7 @@ async function removeRunner( ); } } else { + await clearIdleDetection(runner, computeProvider); logger.info(`Runner '${runner.id}' cannot be de-registered, because it is still busy.`); } } catch (e) { diff --git a/lambdas/libs/compute-providers/aws/ec2/src/control-plane/runners.ts b/lambdas/libs/compute-providers/aws/ec2/src/control-plane/runners.ts index 050804cce1..bb7c394421 100644 --- a/lambdas/libs/compute-providers/aws/ec2/src/control-plane/runners.ts +++ b/lambdas/libs/compute-providers/aws/ec2/src/control-plane/runners.ts @@ -100,6 +100,7 @@ function getRunnerInfo(runningInstances: DescribeInstancesResult) { orphan: i.Tags?.find((e) => e.Key === 'ghr:orphan')?.Value === 'true', githubRunnerId: i.Tags?.find((e) => e.Key === 'ghr:github_runner_id')?.Value as string, bypassRemoval: i.Tags?.find((e) => e.Key === 'ghr:bypass-removal')?.Value === 'true', + idleDetectedAt: i.Tags?.find((e) => e.Key === 'ghr:idle_detected_at')?.Value, }); } } diff --git a/lambdas/libs/compute-providers/aws/ec2/src/control-plane/scale-down.ts b/lambdas/libs/compute-providers/aws/ec2/src/control-plane/scale-down.ts index 55d52ac298..aeb7891507 100644 --- a/lambdas/libs/compute-providers/aws/ec2/src/control-plane/scale-down.ts +++ b/lambdas/libs/compute-providers/aws/ec2/src/control-plane/scale-down.ts @@ -13,12 +13,29 @@ async function unmarkEc2RunnerOrphan(id: string): Promise { await untag(id, [{ Key: 'ghr:orphan', Value: 'true' }]); } +/** + * Idle-confirmation window (see ScaleDownComputeProvider.markIdle). EC2 persists the + * observation as an instance tag, so it survives between scale-down invocations without + * any extra state store — the same mechanism `ghr:orphan` uses above. + */ +export const IDLE_DETECTED_TAG = 'ghr:idle_detected_at'; + +async function markEc2RunnerIdle(id: string, at: string): Promise { + await tag(id, [{ Key: IDLE_DETECTED_TAG, Value: at }]); +} + +async function unmarkEc2RunnerIdle(id: string): Promise { + await untag(id, [{ Key: IDLE_DETECTED_TAG }]); +} + export function createEc2ScaleDownProvider(): Omit { return { list: listEc2ScaleDownRunners, bootTimeExceeded, markOrphan: markEc2RunnerOrphan, unmarkOrphan: unmarkEc2RunnerOrphan, + markIdle: markEc2RunnerIdle, + unmarkIdle: unmarkEc2RunnerIdle, terminate: terminateRunner, }; } diff --git a/lambdas/libs/compute-providers/core/index.ts b/lambdas/libs/compute-providers/core/index.ts index 2b5f937f36..a809d033b8 100644 --- a/lambdas/libs/compute-providers/core/index.ts +++ b/lambdas/libs/compute-providers/core/index.ts @@ -82,6 +82,12 @@ export interface RunnerInfo { orphan?: boolean; githubRunnerId?: string; bypassRemoval?: boolean; + /** + * When the provider first observed this runner reporting idle, as an ISO-8601 string. + * Set and cleared via `markIdle` / `unmarkIdle`; absent when the provider does not + * implement the idle-confirmation window. + */ + idleDetectedAt?: string; } export interface ListRunnerFilters { @@ -97,6 +103,17 @@ export interface ScaleDownComputeProvider extends ComputeProvider { markOrphan(id: string): Promise; unmarkOrphan(id: string): Promise; terminate(id: string): Promise; + /** + * Record that the runner was observed idle at `at` (ISO-8601), so a later cycle can tell + * how long it has read idle. Surfaces back on `RunnerInfo.idleDetectedAt`. + * + * OPTIONAL on purpose: a provider with nowhere to persist per-runner state stays valid + * against this interface, and scale-down simply skips the confirmation window for it + * rather than failing. Only providers implementing BOTH halves get the behaviour. + */ + markIdle?(id: string, at: string): Promise; + /** Clear the idle marker — the runner was seen busy again, so the window restarts. */ + unmarkIdle?(id: string): Promise; } export interface RunnerStatus { diff --git a/lambdas/libs/compute-providers/templates/provider/control-plane.ts b/lambdas/libs/compute-providers/templates/provider/control-plane.ts index 418ac0dbf9..ea0f3fef60 100644 --- a/lambdas/libs/compute-providers/templates/provider/control-plane.ts +++ b/lambdas/libs/compute-providers/templates/provider/control-plane.ts @@ -67,6 +67,11 @@ export function createTemplateScaleDownProvider(): Omit notImplemented(`scaleDown.markOrphan(${id})`), unmarkOrphan: async (id) => notImplemented(`scaleDown.unmarkOrphan(${id})`), + // Optional. Implement BOTH to opt into the scale-down idle-confirmation window + // (SCALE_DOWN_IDLE_CONFIRMATION_SECONDS); omit both if the provider has nowhere to + // persist per-runner state, and scale-down keeps its single-reading behaviour. + markIdle: async (id, at) => notImplemented(`scaleDown.markIdle(${id}, ${at})`), + unmarkIdle: async (id) => notImplemented(`scaleDown.unmarkIdle(${id})`), terminate: async (id) => notImplemented(`scaleDown.terminate(${id})`), }; } diff --git a/main.tf b/main.tf index cad9b66c58..2d1ec622e7 100644 --- a/main.tf +++ b/main.tf @@ -215,6 +215,7 @@ module "runners" { scale_down_schedule_expression = var.scale_down_schedule_expression minimum_running_time_in_minutes = var.minimum_running_time_in_minutes runner_boot_time_in_minutes = var.runner_boot_time_in_minutes + scale_down_idle_confirmation_seconds = var.scale_down_idle_confirmation_seconds runner_disable_default_labels = var.runner_disable_default_labels runner_labels = local.runner_labels runner_as_root = var.runner_as_root diff --git a/modules/multi-runner/runners.tf b/modules/multi-runner/runners.tf index 892113dcc7..92b4fd8326 100644 --- a/modules/multi-runner/runners.tf +++ b/modules/multi-runner/runners.tf @@ -44,6 +44,7 @@ module "runners" { scale_down_schedule_expression = each.value.runner_config.scale_down_schedule_expression minimum_running_time_in_minutes = each.value.runner_config.minimum_running_time_in_minutes runner_boot_time_in_minutes = each.value.runner_config.runner_boot_time_in_minutes + scale_down_idle_confirmation_seconds = each.value.runner_config.scale_down_idle_confirmation_seconds runner_disable_default_labels = each.value.runner_config.runner_disable_default_labels runner_labels = each.value.runner_config.runner_disable_default_labels ? sort(distinct(each.value.runner_config.runner_extra_labels)) : sort(distinct(concat(["self-hosted", each.value.runner_config.runner_os, each.value.runner_config.runner_architecture], each.value.runner_config.runner_extra_labels))) runner_as_root = each.value.runner_config.runner_as_root diff --git a/modules/multi-runner/variables.tf b/modules/multi-runner/variables.tf index a47cd2a83c..905de2a626 100644 --- a/modules/multi-runner/variables.tf +++ b/modules/multi-runner/variables.tf @@ -136,6 +136,7 @@ variable "multi_runner_config" { pool_runner_owner = optional(string, null) runner_as_root = optional(bool, false) runner_boot_time_in_minutes = optional(number, 5) + scale_down_idle_confirmation_seconds = optional(number, 0) runner_disable_default_labels = optional(bool, false) runner_extra_labels = optional(list(string), []) runner_group_name = optional(string, "Default") @@ -281,6 +282,7 @@ variable "multi_runner_config" { runner_additional_security_group_ids: "List of additional security groups IDs to apply to the runner. If added outside the multi_runner_config block, the additional security group(s) will be applied to all runner configs. If added inside the multi_runner_config, the additional security group(s) will be applied to the individual runner." runner_as_root: "Run the action runner under the root user. Variable `runner_run_as` will be ignored." runner_boot_time_in_minutes: "The minimum time for an EC2 runner to boot and register as a runner." + scale_down_idle_confirmation_seconds: "Number of seconds a runner must consistently report not-busy before scale-down terminates it. GitHub's busy flag can be stale, so a single not-busy reading is not sufficient evidence a runner is idle. 0 keeps the previous single-reading behaviour." runner_disable_default_labels: "Disable default labels for the runners (os, architecture and `self-hosted`). If enabled, the runner will only have the extra labels provided in `runner_extra_labels`. In case you on own start script is used, this configuration parameter needs to be parsed via SSM." runner_extra_labels: "Extra (custom) labels for the runners (GitHub). Separate each label by a comma. Labels checks on the webhook can be enforced by setting `multi_runner_config.matcherConfig.exactMatch`. GitHub read-only labels should not be provided." runner_group_name: "Name of the runner group." diff --git a/modules/runners/scale-down.tf b/modules/runners/scale-down.tf index f2879a0b2a..e34f711b0f 100644 --- a/modules/runners/scale-down.tf +++ b/modules/runners/scale-down.tf @@ -38,6 +38,7 @@ resource "aws_lambda_function" "scale_down" { POWERTOOLS_LOGGER_LOG_EVENT = var.log_level == "debug" ? "true" : "false" RUNNER_BOOT_TIME_IN_MINUTES = var.runner_boot_time_in_minutes SCALE_DOWN_CONFIG = jsonencode(var.idle_config) + SCALE_DOWN_IDLE_CONFIRMATION_SECONDS = var.scale_down_idle_confirmation_seconds POWERTOOLS_SERVICE_NAME = "${var.prefix}-scale-down" POWERTOOLS_METRICS_NAMESPACE = var.metrics.namespace POWERTOOLS_TRACE_ENABLED = var.tracing_config.mode != null ? true : false diff --git a/modules/runners/variables.tf b/modules/runners/variables.tf index 946f9abf30..7812e7e1e6 100644 --- a/modules/runners/variables.tf +++ b/modules/runners/variables.tf @@ -270,6 +270,12 @@ variable "runner_boot_time_in_minutes" { default = 5 } +variable "scale_down_idle_confirmation_seconds" { + description = "Number of seconds a runner must consistently report not-busy before scale-down terminates it. GitHub's busy flag can be stale (it can read false for a runner that is actively executing a job), so a single not-busy reading is not sufficient evidence a runner is idle. Set to at least one scale-down schedule interval to require two consecutive not-busy evaluations; a busy reading resets the window. 0 keeps the previous single-reading behaviour." + type = number + default = 0 +} + variable "runner_disable_default_labels" { description = "Disable default labels for the runners (os, architecture and `self-hosted`). If enabled, the runner will only have the extra labels provided in `runner_extra_labels`." type = bool diff --git a/variables.tf b/variables.tf index c4e1e9b5cf..f384921c09 100644 --- a/variables.tf +++ b/variables.tf @@ -100,6 +100,12 @@ variable "minimum_running_time_in_minutes" { default = null } +variable "scale_down_idle_confirmation_seconds" { + description = "Number of seconds a runner must consistently report not-busy before scale-down terminates it. GitHub's busy flag can be stale (it can read false for a runner that is actively executing a job), so a single not-busy reading is not sufficient evidence a runner is idle. Set to at least one scale-down schedule interval to require two consecutive not-busy evaluations; a busy reading resets the window. 0 keeps the previous single-reading behaviour." + type = number + default = 0 +} + variable "runner_boot_time_in_minutes" { description = "The minimum time for an EC2 runner to boot and register as a runner." type = number