From a4170ac1a5252b5556a20fc80e2480cad33796bb Mon Sep 17 00:00:00 2001 From: Christian Jensen Date: Tue, 8 Sep 2026 23:34:15 +0100 Subject: [PATCH 1/2] 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. Built on 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. Ported from #5228 onto current main (#5308, #5312, #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 Co-Authored-By: Claude Fable 5.1 --- docs/configuration.md | 12 ++ .../functions/control-plane/src/modules.d.ts | 1 + .../src/scale-runners/scale-down.test.ts | 109 ++++++++++++++++++ .../src/scale-runners/scale-down.ts | 62 ++++++++++ .../ec2/src/control-plane/scale-down.test.ts | 15 ++- .../aws/ec2/src/control-plane/scale-down.ts | 9 ++ .../compute-providers/aws/ec2/src/runners.ts | 1 + lambdas/libs/compute-providers/core/index.ts | 17 +++ .../libs/compute-providers/registry.test.ts | 2 + .../templates/provider/control-plane.ts | 5 + .../templates/provider/provider.test.ts | 2 + main.tf | 1 + .../config.experimental.resolved.tf | 4 + .../config.experimental.translation.tf | 2 + modules/multi-runner/runners.tf | 1 + ...les.experimental.orchestration-provider.tf | 2 + modules/multi-runner/variables.tf | 3 + modules/runners/scale-down.tf | 1 + modules/runners/variables.tf | 11 ++ variables.tf | 6 + 20 files changed, 265 insertions(+), 1 deletion(-) diff --git a/docs/configuration.md b/docs/configuration.md index be943866e5..4948229c27 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -146,6 +146,18 @@ Cron expressions are parsed by [cron-parser](https://github.com/harrisiirak/cron For time zones please check [TZ database name column](https://en.wikipedia.org/wiki/List_of_tz_database_time_zones) for the supported values. +### Idle confirmation window + +Before terminating a runner, the scale-down lambda asks GitHub whether the runner is busy. That busy flag can be stale: it can read `false` for a runner that was assigned a job a few seconds earlier, and in rare cases for a runner that has been executing a job for several minutes. When that happens the lambda terminates an instance mid-job and the job fails with "The runner has received a shutdown signal". + +Set `scale_down_idle_confirmation_seconds` to require not-busy readings that span at least the given window before a runner is terminated. On the first not-busy reading the lambda tags the instance with `ghr:idle_detected_at` and defers termination. It terminates only when a later evaluation still reads not-busy and the window has elapsed. Any busy reading in between removes the tag and restarts the window. Use at least one scale-down schedule interval, for example `300` for the default five minute schedule, so that two consecutive evaluations must agree. The trade-off is that a genuinely idle runner lives one extra interval before it is removed. + +```hcl +scale_down_idle_confirmation_seconds = 300 +``` + +The default of `0` keeps the previous single-reading behaviour. The `multi_runner_config` equivalent is `runner_config.scale_down_idle_confirmation_seconds`. + ## Ephemeral runners You can configure runners to be ephemeral, in which case runners will be used only for one job. The feature should be used in conjunction with listening for the workflow job event. Please consider the following: diff --git a/lambdas/functions/control-plane/src/modules.d.ts b/lambdas/functions/control-plane/src/modules.d.ts index f389817bfd..bc04aff0cb 100644 --- a/lambdas/functions/control-plane/src/modules.d.ts +++ b/lambdas/functions/control-plane/src/modules.d.ts @@ -11,6 +11,7 @@ declare namespace NodeJS { LOG_LEVEL: 'silly' | 'trace' | 'debug' | 'info' | 'warn' | 'error' | 'fatal'; LOG_TYPE: 'json' | 'pretty' | 'hidden'; MINIMUM_RUNNING_TIME_IN_MINUTES: string; + SCALE_DOWN_IDLE_CONFIRMATION_SECONDS?: string; PARAMETER_GITHUB_APP_CLIENT_ID_NAME: string; PARAMETER_GITHUB_APP_CLIENT_SECRET_NAME: string; PARAMETER_GITHUB_APP_ID_NAME: string; 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 743068e00c..011d894ebc 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 Omit; @@ -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; @@ -691,6 +695,111 @@ describe('Scale down runners', () => { }); }); }); + + describe('Scale down with the idle confirmation window', () => { + const CONFIRMATION_SECONDS = 300; + + beforeEach(() => { + process.env.SCALE_DOWN_IDLE_CONFIRMATION_SECONDS = CONFIRMATION_SECONDS.toString(); + }); + + 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('clears the window when a runner reads busy again', async () => { + // A busy reading is the signal that the earlier not-busy reading was stale; the window + // must restart from scratch rather than keep counting from the first observation. + const runners = [createRunnerTestData('idle-1', 'Org', MINIMUM_TIME_RUNNING_IN_MINUTES + 1, true, false, false)]; + runners[0].idleDetectedAt = new Date(Date.now() - (CONFIRMATION_SECONDS + 60) * 1000).toISOString(); + mockGitHubRunners(runners); + mockProviderRunners(runners); + mockOctokit.actions.getSelfHostedRunnerForOrg.mockImplementation(() => ({ data: { busy: true } })); + mockOctokit.actions.getSelfHostedRunnerForRepo.mockImplementation(() => ({ data: { busy: true } })); + + await scaleDown(); + + expect(mockUnmarkIdle).toHaveBeenCalledWith(runners[0].id); + expect(mockMarkIdle).not.toHaveBeenCalled(); + expect(mockTerminateRunners).not.toHaveBeenCalled(); + }); + + it('clears the window for runners kept idle by the idle config', async () => { + // A kept-idle runner is never evaluated for removal, so a marker left on it would go stale + // and allow immediate termination once the idle count drops. The marker must be cleared. + process.env.SCALE_DOWN_CONFIG = JSON.stringify([ + { idleCount: 1, cron: '* * * * * *', timeZone: 'Europe/Amsterdam' }, + ]); + const runners = [createRunnerTestData('idle-1', 'Org', MINIMUM_TIME_RUNNING_IN_MINUTES + 1, true, false, false)]; + runners[0].idleDetectedAt = new Date(Date.now() - (CONFIRMATION_SECONDS + 60) * 1000).toISOString(); + mockGitHubRunners(runners); + mockProviderRunners(runners); + + await scaleDown(); + + expect(mockUnmarkIdle).toHaveBeenCalledWith(runners[0].id); + expect(mockTerminateRunners).not.toHaveBeenCalled(); + }); + + 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 withoutIdleSupport = Object.fromEntries( + Object.entries(mockComputeProvider).filter(([key]) => key !== 'markIdle' && key !== 'unmarkIdle'), + ); + 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[]) { 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 329ce694d9..343e207a0a 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) { @@ -251,6 +310,9 @@ async function evaluateAndRemoveRunners( if (runnerMinimumTimeExceeded(runner)) { if (idleCounter > 0) { idleCounter--; + // A runner kept idle is not evaluated for removal, so its idle marker cannot be + // refreshed by busy readings. Clear it so a later evaluation starts a fresh window. + await clearIdleDetection(runner, computeProvider); logger.info(`Runner '${runner.id}' will be kept idle.`); } else { logger.info(`Terminating all non busy runners.`); diff --git a/lambdas/libs/compute-providers/aws/ec2/src/control-plane/scale-down.test.ts b/lambdas/libs/compute-providers/aws/ec2/src/control-plane/scale-down.test.ts index 2977e22221..698521e291 100644 --- a/lambdas/libs/compute-providers/aws/ec2/src/control-plane/scale-down.test.ts +++ b/lambdas/libs/compute-providers/aws/ec2/src/control-plane/scale-down.test.ts @@ -1,7 +1,7 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; import type { RunnerInfo, RunnerType } from '../../../../core'; -import { createEc2ScaleDownCapability } from './scale-down'; +import { IDLE_DETECTED_TAG, createEc2ScaleDownCapability } from './scale-down'; import type { Ec2RunnerResourceOperations } from '../runners'; const mockListRunners = vi.fn(); @@ -61,6 +61,19 @@ describe('Scale down runners', () => { expect(mockUntagRunner).toHaveBeenCalledWith(runner.id, [{ Key: 'ghr:orphan', Value: 'true' }]); }); + it('Should persist and clear the idle-detection marker as an instance tag.', async () => { + mockTagRunner.mockResolvedValue(); + mockUntagRunner.mockResolvedValue(); + const detectedAt = '2026-08-05T10:05:00.000Z'; + + await capability.markIdle?.(runner.id, detectedAt); + await capability.unmarkIdle?.(runner.id); + + expect(mockTagRunner).toHaveBeenCalledWith(runner.id, [{ Key: IDLE_DETECTED_TAG, Value: detectedAt }]); + expect(mockUntagRunner).toHaveBeenCalledWith(runner.id, [{ Key: IDLE_DETECTED_TAG }]); + expect(mockTerminateRunner).not.toHaveBeenCalled(); + }); + it(`Should respect booting runner.`, async () => { const scaleDownRunner: RunnerInfo = { ...runner, 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 8171550f5c..f2f4eb303a 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 @@ -1,6 +1,13 @@ import type { ScaleDownComputeProvider } from '../../../../core'; import { bootTimeExceeded, type Ec2RunnerResourceOperations } from '../runners'; +/** + * 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. + */ +export const IDLE_DETECTED_TAG = 'ghr:idle_detected_at'; + export function createEc2ScaleDownCapability( ec2Operations: Ec2RunnerResourceOperations, ): Omit { @@ -9,6 +16,8 @@ export function createEc2ScaleDownCapability( bootTimeExceeded, markOrphan: (id) => ec2Operations.tag(id, [{ Key: 'ghr:orphan', Value: 'true' }]), unmarkOrphan: (id) => ec2Operations.untag(id, [{ Key: 'ghr:orphan', Value: 'true' }]), + markIdle: (id, at) => ec2Operations.tag(id, [{ Key: IDLE_DETECTED_TAG, Value: at }]), + unmarkIdle: (id) => ec2Operations.untag(id, [{ Key: IDLE_DETECTED_TAG }]), terminate: (id) => ec2Operations.terminate(id), }; } diff --git a/lambdas/libs/compute-providers/aws/ec2/src/runners.ts b/lambdas/libs/compute-providers/aws/ec2/src/runners.ts index ce7f3de80f..6269790adb 100644 --- a/lambdas/libs/compute-providers/aws/ec2/src/runners.ts +++ b/lambdas/libs/compute-providers/aws/ec2/src/runners.ts @@ -155,6 +155,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/core/index.ts b/lambdas/libs/compute-providers/core/index.ts index 64fc98e348..1fbf54f067 100644 --- a/lambdas/libs/compute-providers/core/index.ts +++ b/lambdas/libs/compute-providers/core/index.ts @@ -90,6 +90,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 { @@ -105,6 +111,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/registry.test.ts b/lambdas/libs/compute-providers/registry.test.ts index 93227831cd..77f8030257 100644 --- a/lambdas/libs/compute-providers/registry.test.ts +++ b/lambdas/libs/compute-providers/registry.test.ts @@ -31,6 +31,8 @@ it('exposes every configured provider through both capability registries', () => bootTimeExceeded: expect.any(Function), markOrphan: expect.any(Function), unmarkOrphan: expect.any(Function), + markIdle: expect.any(Function), + unmarkIdle: expect.any(Function), terminate: expect.any(Function), }); expect(webhookProviderRegistry.capability(type, 'dynamicLabels').getViolations).toEqual(expect.any(Function)); diff --git a/lambdas/libs/compute-providers/templates/provider/control-plane.ts b/lambdas/libs/compute-providers/templates/provider/control-plane.ts index 312bad0168..ae5feaf1e9 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 createTemplateScaleDownCapability(): 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/lambdas/libs/compute-providers/templates/provider/provider.test.ts b/lambdas/libs/compute-providers/templates/provider/provider.test.ts index 2644fc4f2a..f5e07a3c26 100644 --- a/lambdas/libs/compute-providers/templates/provider/provider.test.ts +++ b/lambdas/libs/compute-providers/templates/provider/provider.test.ts @@ -26,6 +26,8 @@ it('exposes every compute provider capability from its compute-provider entry po bootTimeExceeded: expect.any(Function), markOrphan: expect.any(Function), unmarkOrphan: expect.any(Function), + markIdle: expect.any(Function), + unmarkIdle: expect.any(Function), terminate: expect.any(Function), }); expect(webhookPlugin.type).toBe(webhookProvider.type); diff --git a/main.tf b/main.tf index 7cb7c1026c..26e7e472f9 100644 --- a/main.tf +++ b/main.tf @@ -214,6 +214,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/config.experimental.resolved.tf b/modules/multi-runner/config.experimental.resolved.tf index b7e2e26ab0..2b44bfe797 100644 --- a/modules/multi-runner/config.experimental.resolved.tf +++ b/modules/multi-runner/config.experimental.resolved.tf @@ -230,6 +230,10 @@ locals { v.orchestration_provider.webhook.lambda.scale.down.idle_config, local.normalized_config.orchestration_provider.webhook.lambda.scale.down.idle_config, ) + idle_confirmation_seconds = coalesce( + v.orchestration_provider.webhook.lambda.scale.down.idle_confirmation_seconds, + local.normalized_config.orchestration_provider.webhook.lambda.scale.down.idle_confirmation_seconds, + ) tags = merge(local.normalized_config.orchestration_provider.webhook.lambda.scale.down.tags, v.orchestration_provider.webhook.lambda.scale.down.tags) }) }) diff --git a/modules/multi-runner/config.experimental.translation.tf b/modules/multi-runner/config.experimental.translation.tf index 07df28e2e7..301d37ec4d 100644 --- a/modules/multi-runner/config.experimental.translation.tf +++ b/modules/multi-runner/config.experimental.translation.tf @@ -102,6 +102,7 @@ locals { schedule_expression = "cron(*/5 * * * ? *)" minimum_running_time_in_minutes = null idle_config = [] + idle_confirmation_seconds = 0 tags = {} } } @@ -390,6 +391,7 @@ locals { schedule_expression = v.runner_config.scale_down_schedule_expression minimum_running_time_in_minutes = v.runner_config.minimum_running_time_in_minutes idle_config = v.runner_config.idle_config + idle_confirmation_seconds = v.runner_config.scale_down_idle_confirmation_seconds tags = {} } } diff --git a/modules/multi-runner/runners.tf b/modules/multi-runner/runners.tf index 61c5f57583..566329e792 100644 --- a/modules/multi-runner/runners.tf +++ b/modules/multi-runner/runners.tf @@ -55,6 +55,7 @@ module "runners" { runner_run_as = each.value.runner.run_as runners_maximum_count = each.value.orchestration_provider.webhook.runner.maximum_count idle_config = each.value.orchestration_provider.webhook.lambda.scale.down.idle_config + scale_down_idle_confirmation_seconds = each.value.orchestration_provider.webhook.lambda.scale.down.idle_confirmation_seconds enable_ssm_on_runners = each.value.compute_provider.aws.ec2.ssm_enabled egress_rules = each.value.compute_provider.aws.ec2.egress_rules runner_additional_security_group_ids = each.value.compute_provider.aws.ec2.additional_security_group_ids diff --git a/modules/multi-runner/variables.experimental.orchestration-provider.tf b/modules/multi-runner/variables.experimental.orchestration-provider.tf index fd962f9632..9919d3645e 100644 --- a/modules/multi-runner/variables.experimental.orchestration-provider.tf +++ b/modules/multi-runner/variables.experimental.orchestration-provider.tf @@ -28,6 +28,7 @@ variable "global_config_orchestration_provider" { lambda.scale.down.timeout: "Timeout in seconds for the scale-down Lambda." lambda.scale.down.schedule_expression: "Schedule expression for scale-down processing." lambda.scale.down.minimum_running_time_in_minutes: "Minimum runner lifetime before scale-down." + lambda.scale.down.idle_confirmation_seconds: "Seconds a runner must consistently report not-busy before scale-down terminates it; 0 disables the confirmation window." lambda.scale.down.idle_config: "Scheduled minimum idle-runner pool settings." lambda.scale.down.idle_config.cron: "Cron expression defining when the idle-runner count applies." lambda.scale.down.idle_config.timeZone: "Time zone used to evaluate the idle-runner schedule." @@ -109,6 +110,7 @@ variable "global_config_orchestration_provider" { timeout = optional(number, 60) schedule_expression = optional(string, "cron(*/5 * * * ? *)") minimum_running_time_in_minutes = optional(number, null) + idle_confirmation_seconds = optional(number, 0) idle_config = optional(list(object({ cron = string timeZone = string diff --git a/modules/multi-runner/variables.tf b/modules/multi-runner/variables.tf index bddc0873b4..23ecfcc024 100644 --- a/modules/multi-runner/variables.tf +++ b/modules/multi-runner/variables.tf @@ -153,6 +153,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") @@ -362,6 +363,7 @@ variable "multi_runner_config" { timeout = optional(number, null) schedule_expression = optional(string, null) minimum_running_time_in_minutes = optional(number, null) + idle_confirmation_seconds = optional(number, null) idle_config = optional(list(object({ cron = string timeZone = string @@ -618,6 +620,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 1983cfea84..ff7c91dff8 100644 --- a/modules/runners/scale-down.tf +++ b/modules/runners/scale-down.tf @@ -40,6 +40,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 807a79bc93..1d2f9b35a9 100644 --- a/modules/runners/variables.tf +++ b/modules/runners/variables.tf @@ -274,6 +274,17 @@ 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 + + validation { + condition = var.scale_down_idle_confirmation_seconds >= 0 + error_message = "The idle confirmation window must be 0 (disabled) or a positive number of seconds." + } +} + 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 From 0758f9066c33d95dd5c11259013ee835578a1269 Mon Sep 17 00:00:00 2001 From: Lochlan Bennett-Odlum Date: Wed, 9 Sep 2026 10:28:21 +0100 Subject: [PATCH 2/2] refactor(scale-down): make idle marker methods required on the provider Per maintainer feedback on #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 --- .../scale-runners/scale-down-contract.test.ts | 2 ++ .../src/scale-runners/scale-down.test.ts | 17 ----------------- .../src/scale-runners/scale-down.ts | 7 ++----- .../compute-provider-contracts/scale-down.ts | 2 ++ .../ec2/src/control-plane/scale-down.test.ts | 4 ++-- lambdas/libs/compute-providers/core/index.ts | 16 ++++++---------- .../templates/provider/control-plane.ts | 5 ++--- 7 files changed, 16 insertions(+), 37 deletions(-) diff --git a/lambdas/functions/control-plane/src/scale-runners/scale-down-contract.test.ts b/lambdas/functions/control-plane/src/scale-runners/scale-down-contract.test.ts index afd25211da..a4d52acc40 100644 --- a/lambdas/functions/control-plane/src/scale-runners/scale-down-contract.test.ts +++ b/lambdas/functions/control-plane/src/scale-runners/scale-down-contract.test.ts @@ -18,6 +18,8 @@ const computeProviders = providerTypes.map((type) => ({ bootTimeExceeded: vi.fn(), markOrphan: vi.fn(), unmarkOrphan: vi.fn(), + markIdle: vi.fn(), + unmarkIdle: vi.fn(), terminate: vi.fn(), } satisfies ScaleDownComputeProvider, })); 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 011d894ebc..3583247f8d 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 @@ -782,23 +782,6 @@ describe('Scale down runners', () => { 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 withoutIdleSupport = Object.fromEntries( - Object.entries(mockComputeProvider).filter(([key]) => key !== 'markIdle' && key !== 'unmarkIdle'), - ); - 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); - }); }); }); 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 343e207a0a..1e3e838aed 100644 --- a/lambdas/functions/control-plane/src/scale-runners/scale-down.ts +++ b/lambdas/functions/control-plane/src/scale-runners/scale-down.ts @@ -182,12 +182,9 @@ function idleConfirmationSeconds(): number { // 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) { + if (confirmationSeconds === 0) { return true; } const idleDetectedAt = runner.idleDetectedAt; @@ -216,7 +213,7 @@ async function idleConfirmed(runner: RunnerInfo, computeProvider: ScaleDownCompu } async function clearIdleDetection(runner: RunnerInfo, computeProvider: ScaleDownComputeProvider): Promise { - if (idleConfirmationSeconds() === 0 || !computeProvider.unmarkIdle) { + if (idleConfirmationSeconds() === 0) { return; } if (runner.idleDetectedAt) { diff --git a/lambdas/functions/control-plane/src/test/compute-provider-contracts/scale-down.ts b/lambdas/functions/control-plane/src/test/compute-provider-contracts/scale-down.ts index 97f6674c9a..58fd4b4426 100644 --- a/lambdas/functions/control-plane/src/test/compute-provider-contracts/scale-down.ts +++ b/lambdas/functions/control-plane/src/test/compute-provider-contracts/scale-down.ts @@ -34,6 +34,8 @@ export function defineScaleDownContractTests({ vi.mocked(provider.bootTimeExceeded).mockReturnValue(false); vi.mocked(provider.markOrphan).mockResolvedValue(); vi.mocked(provider.unmarkOrphan).mockResolvedValue(); + vi.mocked(provider.markIdle).mockResolvedValue(); + vi.mocked(provider.unmarkIdle).mockResolvedValue(); vi.mocked(provider.terminate).mockResolvedValue(); }); diff --git a/lambdas/libs/compute-providers/aws/ec2/src/control-plane/scale-down.test.ts b/lambdas/libs/compute-providers/aws/ec2/src/control-plane/scale-down.test.ts index 698521e291..fc3cb34c53 100644 --- a/lambdas/libs/compute-providers/aws/ec2/src/control-plane/scale-down.test.ts +++ b/lambdas/libs/compute-providers/aws/ec2/src/control-plane/scale-down.test.ts @@ -66,8 +66,8 @@ describe('Scale down runners', () => { mockUntagRunner.mockResolvedValue(); const detectedAt = '2026-08-05T10:05:00.000Z'; - await capability.markIdle?.(runner.id, detectedAt); - await capability.unmarkIdle?.(runner.id); + await capability.markIdle(runner.id, detectedAt); + await capability.unmarkIdle(runner.id); expect(mockTagRunner).toHaveBeenCalledWith(runner.id, [{ Key: IDLE_DETECTED_TAG, Value: detectedAt }]); expect(mockUntagRunner).toHaveBeenCalledWith(runner.id, [{ Key: IDLE_DETECTED_TAG }]); diff --git a/lambdas/libs/compute-providers/core/index.ts b/lambdas/libs/compute-providers/core/index.ts index 1fbf54f067..2457719a42 100644 --- a/lambdas/libs/compute-providers/core/index.ts +++ b/lambdas/libs/compute-providers/core/index.ts @@ -91,9 +91,8 @@ export interface RunnerInfo { 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. + * When scale-down first observed this runner reporting idle, as an ISO-8601 string. + * Set and cleared via `markIdle` / `unmarkIdle`; absent when no marker is recorded. */ idleDetectedAt?: string; } @@ -113,15 +112,12 @@ export interface ScaleDownComputeProvider extends ComputeProvider { 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. + * how long it has read idle. Surfaces back on `RunnerInfo.idleDetectedAt`. Only called + * when the idle-confirmation window (SCALE_DOWN_IDLE_CONFIRMATION_SECONDS) is enabled. */ - markIdle?(id: string, at: string): Promise; + markIdle(id: string, at: string): Promise; /** Clear the idle marker — the runner was seen busy again, so the window restarts. */ - unmarkIdle?(id: string): Promise; + 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 ae5feaf1e9..b05877d17f 100644 --- a/lambdas/libs/compute-providers/templates/provider/control-plane.ts +++ b/lambdas/libs/compute-providers/templates/provider/control-plane.ts @@ -67,9 +67,8 @@ export function createTemplateScaleDownCapability(): 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. + // Persist and clear the idle-detection marker used by the scale-down idle-confirmation + // window (SCALE_DOWN_IDLE_CONFIRMATION_SECONDS). Only called when the window is enabled. markIdle: async (id, at) => notImplemented(`scaleDown.markIdle(${id}, ${at})`), unmarkIdle: async (id) => notImplemented(`scaleDown.unmarkIdle(${id})`), terminate: async (id) => notImplemented(`scaleDown.terminate(${id})`),