Skip to content

Commit 6a6c2f7

Browse files
committed
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.
1 parent 4475534 commit 6a6c2f7

12 files changed

Lines changed: 206 additions & 0 deletions

File tree

lambdas/functions/control-plane/src/scale-runners/scale-down.test.ts

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,8 @@ const mockComputeProvider = {
3838
bootTimeExceeded: vi.fn(),
3939
markOrphan: vi.fn(),
4040
unmarkOrphan: vi.fn(),
41+
markIdle: vi.fn(),
42+
unmarkIdle: vi.fn(),
4143
terminate: vi.fn(),
4244
} satisfies ScaleDownComputeProvider;
4345

@@ -49,6 +51,8 @@ const mockListRunners = vi.mocked(mockComputeProvider.list);
4951
const mockBootTimeExceeded = vi.mocked(mockComputeProvider.bootTimeExceeded);
5052
const mockMarkOrphan = vi.mocked(mockComputeProvider.markOrphan);
5153
const mockUnmarkOrphan = vi.mocked(mockComputeProvider.unmarkOrphan);
54+
const mockMarkIdle = vi.mocked(mockComputeProvider.markIdle);
55+
const mockUnmarkIdle = vi.mocked(mockComputeProvider.unmarkIdle);
5256
const mockTerminateRunners = vi.mocked(mockComputeProvider.terminate);
5357

5458
const cleanEnv = process.env;
@@ -693,6 +697,92 @@ describe('Scale down runners', () => {
693697
});
694698
});
695699

700+
describe('Scale down with the idle confirmation window', () => {
701+
const CONFIRMATION_SECONDS = 300;
702+
703+
beforeEach(() => {
704+
process.env = { ...cleanEnv };
705+
process.env.GITHUB_APP_KEY_BASE64 = 'TEST_CERTIFICATE_DATA';
706+
process.env.GITHUB_APP_ID = '1337';
707+
process.env.GITHUB_APP_CLIENT_ID = 'TEST_CLIENT_ID';
708+
process.env.GITHUB_APP_CLIENT_SECRET = 'TEST_CLIENT_SECRET';
709+
process.env.RUNNERS_MAXIMUM_COUNT = '3';
710+
process.env.SCALE_DOWN_CONFIG = '[]';
711+
process.env.ENVIRONMENT = ENVIRONMENT;
712+
process.env.MINIMUM_RUNNING_TIME_IN_MINUTES = MINIMUM_TIME_RUNNING_IN_MINUTES.toString();
713+
process.env.RUNNER_BOOT_TIME_IN_MINUTES = MINIMUM_BOOT_TIME.toString();
714+
process.env.COMPUTE_PROVIDER_TYPE = mockComputeProvider.type;
715+
process.env.SCALE_DOWN_IDLE_CONFIRMATION_SECONDS = CONFIRMATION_SECONDS.toString();
716+
vi.clearAllMocks();
717+
vi.resetModules();
718+
mockedResolveCapability.mockReturnValue(() => mockComputeProvider);
719+
mockBootTimeExceeded.mockImplementation((runner) => {
720+
return moment(runner.launchTime).add(MINIMUM_BOOT_TIME, 'minutes') < moment(new Date());
721+
});
722+
});
723+
724+
it('starts the window instead of terminating on the first not-busy reading', async () => {
725+
const runners = [createRunnerTestData('idle-1', 'Org', MINIMUM_TIME_RUNNING_IN_MINUTES + 1, true, false, false)];
726+
mockGitHubRunners(runners);
727+
mockProviderRunners(runners);
728+
729+
await scaleDown();
730+
731+
expect(mockMarkIdle).toHaveBeenCalledWith(runners[0].id, expect.any(String));
732+
expect(mockTerminateRunners).not.toHaveBeenCalled();
733+
});
734+
735+
it('defers termination while the window has not elapsed', async () => {
736+
const runners = [createRunnerTestData('idle-1', 'Org', MINIMUM_TIME_RUNNING_IN_MINUTES + 1, true, false, false)];
737+
runners[0].idleDetectedAt = new Date(Date.now() - (CONFIRMATION_SECONDS - 240) * 1000).toISOString();
738+
mockGitHubRunners(runners);
739+
mockProviderRunners(runners);
740+
741+
await scaleDown();
742+
743+
expect(mockMarkIdle).not.toHaveBeenCalled();
744+
expect(mockTerminateRunners).not.toHaveBeenCalled();
745+
});
746+
747+
it('terminates once not-busy readings span the window', async () => {
748+
const runners = [createRunnerTestData('idle-1', 'Org', MINIMUM_TIME_RUNNING_IN_MINUTES + 1, true, false, true)];
749+
runners[0].idleDetectedAt = new Date(Date.now() - (CONFIRMATION_SECONDS + 60) * 1000).toISOString();
750+
mockGitHubRunners(runners);
751+
mockProviderRunners(runners);
752+
753+
await scaleDown();
754+
755+
expect(mockTerminateRunners).toHaveBeenCalledWith(runners[0].id);
756+
});
757+
758+
it('terminates on a single reading when the window is disabled (0)', async () => {
759+
process.env.SCALE_DOWN_IDLE_CONFIRMATION_SECONDS = '0';
760+
const runners = [createRunnerTestData('idle-1', 'Org', MINIMUM_TIME_RUNNING_IN_MINUTES + 1, true, false, true)];
761+
mockGitHubRunners(runners);
762+
mockProviderRunners(runners);
763+
764+
await scaleDown();
765+
766+
expect(mockMarkIdle).not.toHaveBeenCalled();
767+
expect(mockTerminateRunners).toHaveBeenCalledWith(runners[0].id);
768+
});
769+
770+
it('terminates on a single reading when the provider cannot persist idle state', async () => {
771+
// A provider that implements neither markIdle nor unmarkIdle must keep the previous
772+
// single-reading behaviour rather than deferring forever.
773+
const { markIdle: _m, unmarkIdle: _u, ...withoutIdleSupport } = mockComputeProvider;
774+
mockedResolveCapability.mockReturnValue(() => withoutIdleSupport as unknown as typeof mockComputeProvider);
775+
const runners = [createRunnerTestData('idle-1', 'Org', MINIMUM_TIME_RUNNING_IN_MINUTES + 1, true, false, true)];
776+
mockGitHubRunners(runners);
777+
mockProviderRunners(runners);
778+
779+
await scaleDown();
780+
781+
expect(mockMarkIdle).not.toHaveBeenCalled();
782+
expect(mockTerminateRunners).toHaveBeenCalledWith(runners[0].id);
783+
});
784+
});
785+
696786
function mockProviderRunners(runners: RunnerTestItem[]) {
697787
mockListRunners.mockImplementation(async (_environment, orphan) => {
698788
return runners.filter((runner) => !orphan || orphan === runner.orphan);

lambdas/functions/control-plane/src/scale-runners/scale-down.ts

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -170,6 +170,61 @@ async function deleteGitHubRunner(
170170
}
171171
}
172172

173+
function idleConfirmationSeconds(): number {
174+
const raw = process.env.SCALE_DOWN_IDLE_CONFIRMATION_SECONDS;
175+
const parsed = raw === undefined || raw === '' ? 0 : Number(raw);
176+
return Number.isFinite(parsed) && parsed > 0 ? parsed : 0;
177+
}
178+
179+
// GitHub's busy flag can be stale: it reads false for runners that are actively executing
180+
// a job, both shortly after job assignment (observed 25-60s lag) and deep into a running
181+
// job (observed 12+ minutes). See #5085. A single busy=false reading is therefore not
182+
// sufficient evidence that a runner is idle. When SCALE_DOWN_IDLE_CONFIRMATION_SECONDS > 0,
183+
// require busy=false readings spanning at least that window before terminating; any
184+
// busy=true reading in between resets the window (see clearIdleDetection).
185+
//
186+
// Providers that cannot persist per-runner state do not implement markIdle/unmarkIdle;
187+
// for those the window is skipped entirely and behaviour is unchanged.
188+
async function idleConfirmed(runner: RunnerInfo, computeProvider: ScaleDownComputeProvider): Promise<boolean> {
189+
const confirmationSeconds = idleConfirmationSeconds();
190+
if (confirmationSeconds === 0 || !computeProvider.markIdle) {
191+
return true;
192+
}
193+
const idleDetectedAt = runner.idleDetectedAt;
194+
const idleForSeconds = idleDetectedAt ? (Date.now() - Date.parse(idleDetectedAt)) / 1000 : NaN;
195+
if (Number.isNaN(idleForSeconds)) {
196+
// No marker yet, or an unparsable one: (re)start the confirmation window.
197+
await computeProvider.markIdle(runner.id, new Date().toISOString());
198+
logger.info(
199+
`Runner '${runner.id}' reads idle; deferring termination for at least ` +
200+
`${confirmationSeconds}s to confirm the busy state is not stale.`,
201+
);
202+
return false;
203+
}
204+
if (idleForSeconds < confirmationSeconds) {
205+
logger.info(
206+
`Runner '${runner.id}' reads idle since '${idleDetectedAt}' ` +
207+
`(${Math.round(idleForSeconds)}s < ${confirmationSeconds}s); deferring termination.`,
208+
);
209+
return false;
210+
}
211+
logger.info(
212+
`Runner '${runner.id}' confirmed idle since '${idleDetectedAt}' ` +
213+
`(${Math.round(idleForSeconds)}s >= ${confirmationSeconds}s).`,
214+
);
215+
return true;
216+
}
217+
218+
async function clearIdleDetection(runner: RunnerInfo, computeProvider: ScaleDownComputeProvider): Promise<void> {
219+
if (idleConfirmationSeconds() === 0 || !computeProvider.unmarkIdle) {
220+
return;
221+
}
222+
if (runner.idleDetectedAt) {
223+
await computeProvider.unmarkIdle(runner.id);
224+
logger.info(`Runner '${runner.id}' is busy again; idle-detection window reset.`);
225+
}
226+
}
227+
173228
async function removeRunner(
174229
runner: RunnerInfo,
175230
ghRunnerIds: number[],
@@ -192,6 +247,9 @@ async function removeRunner(
192247
);
193248

194249
if (states.every((busy) => busy === false)) {
250+
if (!(await idleConfirmed(runner, computeProvider))) {
251+
return;
252+
}
195253
const results = await Promise.all(
196254
ghRunnerIds.map((ghRunnerId) => deleteGitHubRunner(githubInstallationClient, runner, ghRunnerId)),
197255
);
@@ -213,6 +271,7 @@ async function removeRunner(
213271
);
214272
}
215273
} else {
274+
await clearIdleDetection(runner, computeProvider);
216275
logger.info(`Runner '${runner.id}' cannot be de-registered, because it is still busy.`);
217276
}
218277
} catch (e) {

lambdas/libs/compute-providers/aws/ec2/src/control-plane/runners.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,7 @@ function getRunnerInfo(runningInstances: DescribeInstancesResult) {
100100
orphan: i.Tags?.find((e) => e.Key === 'ghr:orphan')?.Value === 'true',
101101
githubRunnerId: i.Tags?.find((e) => e.Key === 'ghr:github_runner_id')?.Value as string,
102102
bypassRemoval: i.Tags?.find((e) => e.Key === 'ghr:bypass-removal')?.Value === 'true',
103+
idleDetectedAt: i.Tags?.find((e) => e.Key === 'ghr:idle_detected_at')?.Value,
103104
});
104105
}
105106
}

lambdas/libs/compute-providers/aws/ec2/src/control-plane/scale-down.ts

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,12 +13,29 @@ async function unmarkEc2RunnerOrphan(id: string): Promise<void> {
1313
await untag(id, [{ Key: 'ghr:orphan', Value: 'true' }]);
1414
}
1515

16+
/**
17+
* Idle-confirmation window (see ScaleDownComputeProvider.markIdle). EC2 persists the
18+
* observation as an instance tag, so it survives between scale-down invocations without
19+
* any extra state store — the same mechanism `ghr:orphan` uses above.
20+
*/
21+
export const IDLE_DETECTED_TAG = 'ghr:idle_detected_at';
22+
23+
async function markEc2RunnerIdle(id: string, at: string): Promise<void> {
24+
await tag(id, [{ Key: IDLE_DETECTED_TAG, Value: at }]);
25+
}
26+
27+
async function unmarkEc2RunnerIdle(id: string): Promise<void> {
28+
await untag(id, [{ Key: IDLE_DETECTED_TAG }]);
29+
}
30+
1631
export function createEc2ScaleDownProvider(): Omit<ScaleDownComputeProvider, 'type'> {
1732
return {
1833
list: listEc2ScaleDownRunners,
1934
bootTimeExceeded,
2035
markOrphan: markEc2RunnerOrphan,
2136
unmarkOrphan: unmarkEc2RunnerOrphan,
37+
markIdle: markEc2RunnerIdle,
38+
unmarkIdle: unmarkEc2RunnerIdle,
2239
terminate: terminateRunner,
2340
};
2441
}

lambdas/libs/compute-providers/core/index.ts

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,12 @@ export interface RunnerInfo {
8282
orphan?: boolean;
8383
githubRunnerId?: string;
8484
bypassRemoval?: boolean;
85+
/**
86+
* When the provider first observed this runner reporting idle, as an ISO-8601 string.
87+
* Set and cleared via `markIdle` / `unmarkIdle`; absent when the provider does not
88+
* implement the idle-confirmation window.
89+
*/
90+
idleDetectedAt?: string;
8591
}
8692

8793
export interface ListRunnerFilters {
@@ -97,6 +103,17 @@ export interface ScaleDownComputeProvider extends ComputeProvider {
97103
markOrphan(id: string): Promise<void>;
98104
unmarkOrphan(id: string): Promise<void>;
99105
terminate(id: string): Promise<void>;
106+
/**
107+
* Record that the runner was observed idle at `at` (ISO-8601), so a later cycle can tell
108+
* how long it has read idle. Surfaces back on `RunnerInfo.idleDetectedAt`.
109+
*
110+
* OPTIONAL on purpose: a provider with nowhere to persist per-runner state stays valid
111+
* against this interface, and scale-down simply skips the confirmation window for it
112+
* rather than failing. Only providers implementing BOTH halves get the behaviour.
113+
*/
114+
markIdle?(id: string, at: string): Promise<void>;
115+
/** Clear the idle marker — the runner was seen busy again, so the window restarts. */
116+
unmarkIdle?(id: string): Promise<void>;
100117
}
101118

102119
export interface RunnerStatus {

lambdas/libs/compute-providers/templates/provider/control-plane.ts

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,11 @@ export function createTemplateScaleDownProvider(): Omit<ScaleDownComputeProvider
6767
},
6868
markOrphan: async (id) => notImplemented(`scaleDown.markOrphan(${id})`),
6969
unmarkOrphan: async (id) => notImplemented(`scaleDown.unmarkOrphan(${id})`),
70+
// Optional. Implement BOTH to opt into the scale-down idle-confirmation window
71+
// (SCALE_DOWN_IDLE_CONFIRMATION_SECONDS); omit both if the provider has nowhere to
72+
// persist per-runner state, and scale-down keeps its single-reading behaviour.
73+
markIdle: async (id, at) => notImplemented(`scaleDown.markIdle(${id}, ${at})`),
74+
unmarkIdle: async (id) => notImplemented(`scaleDown.unmarkIdle(${id})`),
7075
terminate: async (id) => notImplemented(`scaleDown.terminate(${id})`),
7176
};
7277
}

main.tf

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -215,6 +215,7 @@ module "runners" {
215215
scale_down_schedule_expression = var.scale_down_schedule_expression
216216
minimum_running_time_in_minutes = var.minimum_running_time_in_minutes
217217
runner_boot_time_in_minutes = var.runner_boot_time_in_minutes
218+
scale_down_idle_confirmation_seconds = var.scale_down_idle_confirmation_seconds
218219
runner_disable_default_labels = var.runner_disable_default_labels
219220
runner_labels = local.runner_labels
220221
runner_as_root = var.runner_as_root

modules/multi-runner/runners.tf

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@ module "runners" {
4444
scale_down_schedule_expression = each.value.runner_config.scale_down_schedule_expression
4545
minimum_running_time_in_minutes = each.value.runner_config.minimum_running_time_in_minutes
4646
runner_boot_time_in_minutes = each.value.runner_config.runner_boot_time_in_minutes
47+
scale_down_idle_confirmation_seconds = each.value.runner_config.scale_down_idle_confirmation_seconds
4748
runner_disable_default_labels = each.value.runner_config.runner_disable_default_labels
4849
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)))
4950
runner_as_root = each.value.runner_config.runner_as_root

modules/multi-runner/variables.tf

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -136,6 +136,7 @@ variable "multi_runner_config" {
136136
pool_runner_owner = optional(string, null)
137137
runner_as_root = optional(bool, false)
138138
runner_boot_time_in_minutes = optional(number, 5)
139+
scale_down_idle_confirmation_seconds = optional(number, 0)
139140
runner_disable_default_labels = optional(bool, false)
140141
runner_extra_labels = optional(list(string), [])
141142
runner_group_name = optional(string, "Default")
@@ -281,6 +282,7 @@ variable "multi_runner_config" {
281282
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."
282283
runner_as_root: "Run the action runner under the root user. Variable `runner_run_as` will be ignored."
283284
runner_boot_time_in_minutes: "The minimum time for an EC2 runner to boot and register as a runner."
285+
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."
284286
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."
285287
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."
286288
runner_group_name: "Name of the runner group."

modules/runners/scale-down.tf

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@ resource "aws_lambda_function" "scale_down" {
3838
POWERTOOLS_LOGGER_LOG_EVENT = var.log_level == "debug" ? "true" : "false"
3939
RUNNER_BOOT_TIME_IN_MINUTES = var.runner_boot_time_in_minutes
4040
SCALE_DOWN_CONFIG = jsonencode(var.idle_config)
41+
SCALE_DOWN_IDLE_CONFIRMATION_SECONDS = var.scale_down_idle_confirmation_seconds
4142
POWERTOOLS_SERVICE_NAME = "${var.prefix}-scale-down"
4243
POWERTOOLS_METRICS_NAMESPACE = var.metrics.namespace
4344
POWERTOOLS_TRACE_ENABLED = var.tracing_config.mode != null ? true : false

0 commit comments

Comments
 (0)