From 0df40213fc2ab7ba98ee5b76c44e022c2d6366ce Mon Sep 17 00:00:00 2001 From: Stuart Pearson <1926002+stuartp44@users.noreply.github.com> Date: Fri, 22 Aug 2025 11:28:31 +0200 Subject: [PATCH 1/4] test: add handling for 404 errors in scale-down tests and improve error logging --- .../src/scale-runners/scale-down.test.ts | 100 ++++++++++++++++++ .../src/scale-runners/scale-down.ts | 58 ++++++---- 2 files changed, 138 insertions(+), 20 deletions(-) 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 8dd25323a6..af1272e5c3 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 @@ -403,6 +403,106 @@ describe('Scale down runners', () => { expect(mockTerminateRunners).toHaveBeenCalledWith(orphanRunner.instanceId); }); + it('Should handle 404 error when checking orphaned runner (JIT) - treat as orphaned', async () => { + // arrange + const orphanRunner = createRunnerTestData( + 'orphan-jit-404', + type, + MINIMUM_BOOT_TIME + 1, + false, + true, + true, // should be terminated when 404 + undefined, + 1234567890, + ); + const runners = [orphanRunner]; + + mockGitHubRunners([]); + mockAwsRunners(runners); + + // Mock 404 error response + const error404 = new Error('Runner not found'); + (error404 as any).status = 404; + + if (type === 'Repo') { + mockOctokit.actions.getSelfHostedRunnerForRepo.mockRejectedValueOnce(error404); + } else { + mockOctokit.actions.getSelfHostedRunnerForOrg.mockRejectedValueOnce(error404); + } + + // act + await scaleDown(); + + // assert - should terminate since 404 means runner doesn't exist on GitHub + expect(mockTerminateRunners).toHaveBeenCalledWith(orphanRunner.instanceId); + }); + + it('Should handle 404 error when checking runner busy state - treat as not busy', async () => { + // arrange + const runner = createRunnerTestData( + 'runner-404', + type, + MINIMUM_TIME_RUNNING_IN_MINUTES + 1, + true, + false, + true, // should be terminated since not busy due to 404 + ); + const runners = [runner]; + + mockGitHubRunners(runners); + mockAwsRunners(runners); + + // Mock 404 error response for busy state check + const error404 = new Error('Runner not found'); + (error404 as any).status = 404; + + if (type === 'Repo') { + mockOctokit.actions.getSelfHostedRunnerForRepo.mockRejectedValueOnce(error404); + } else { + mockOctokit.actions.getSelfHostedRunnerForOrg.mockRejectedValueOnce(error404); + } + + // act + await scaleDown(); + + // assert - should terminate since 404 means runner is not busy + checkTerminated(runners); + }); + + it('Should re-throw non-404 errors when checking runner state', async () => { + // arrange + const orphanRunner = createRunnerTestData( + 'orphan-error', + type, + MINIMUM_BOOT_TIME + 1, + false, + true, + false, + undefined, + 1234567890, + ); + const runners = [orphanRunner]; + + mockGitHubRunners([]); + mockAwsRunners(runners); + + // Mock non-404 error response + const error500 = new Error('Internal server error'); + (error500 as any).status = 500; + + if (type === 'Repo') { + mockOctokit.actions.getSelfHostedRunnerForRepo.mockRejectedValueOnce(error500); + } else { + mockOctokit.actions.getSelfHostedRunnerForOrg.mockRejectedValueOnce(error500); + } + + // act & assert - should not throw because error handling is in terminateOrphan + await expect(scaleDown()).resolves.not.toThrow(); + + // Should not terminate since the error was not a 404 + expect(mockTerminateRunners).not.toHaveBeenCalledWith(orphanRunner.instanceId); + }); + it(`Should ignore errors when termination orphan fails.`, async () => { // setup const orphanRunner = createRunnerTestData('orphan-1', type, MINIMUM_BOOT_TIME + 1, false, true, true); 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 8f5cbd42d4..c7a136d79b 100644 --- a/lambdas/functions/control-plane/src/scale-runners/scale-down.ts +++ b/lambdas/functions/control-plane/src/scale-runners/scale-down.ts @@ -55,25 +55,37 @@ async function getGitHubSelfHostedRunnerState( client: Octokit, ec2runner: RunnerInfo, runnerId: number, -): Promise { - const state = - ec2runner.type === 'Org' - ? await client.actions.getSelfHostedRunnerForOrg({ - runner_id: runnerId, - org: ec2runner.owner, - }) - : await client.actions.getSelfHostedRunnerForRepo({ - runner_id: runnerId, - owner: ec2runner.owner.split('/')[0], - repo: ec2runner.owner.split('/')[1], - }); - metricGitHubAppRateLimit(state.headers); - - return state.data; +): Promise { + try { + const state = + ec2runner.type === 'Org' + ? await client.actions.getSelfHostedRunnerForOrg({ + runner_id: runnerId, + org: ec2runner.owner, + }) + : await client.actions.getSelfHostedRunnerForRepo({ + runner_id: runnerId, + owner: ec2runner.owner.split('/')[0], + repo: ec2runner.owner.split('/')[1], + }); + metricGitHubAppRateLimit(state.headers); + + return state.data; + } catch (error: any) { + if (error.status === 404) { + logger.info(`Runner '${ec2runner.instanceId}' with GitHub Runner ID '${runnerId}' not found on GitHub (404)`); + return null; + } + throw error; + } } async function getGitHubRunnerBusyState(client: Octokit, ec2runner: RunnerInfo, runnerId: number): Promise { const state = await getGitHubSelfHostedRunnerState(client, ec2runner, runnerId); + if (state === null) { + logger.info(`Runner '${ec2runner.instanceId}' - GitHub Runner ID '${runnerId}' - Not found on GitHub, treating as not busy`); + return false; + } logger.info(`Runner '${ec2runner.instanceId}' - GitHub Runner ID '${runnerId}' - Busy: ${state.busy}`); return state.busy; } @@ -227,12 +239,18 @@ async function lastChanceCheckOrphanRunner(runner: RunnerList): Promise const ec2Instance = runner as RunnerInfo; const state = await getGitHubSelfHostedRunnerState(client, ec2Instance, runnerId); let isOrphan = false; - logger.debug( - `Runner '${runner.instanceId}' is '${state.status}' and is currently '${state.busy ? 'busy' : 'idle'}'.`, - ); - const isOfflineAndBusy = state.status === 'offline' && state.busy; - if (isOfflineAndBusy) { + + if (state === null) { + logger.debug(`Runner '${runner.instanceId}' not found on GitHub, treating as orphaned.`); isOrphan = true; + } else { + logger.debug( + `Runner '${runner.instanceId}' is '${state.status}' and is currently '${state.busy ? 'busy' : 'idle'}'.`, + ); + const isOfflineAndBusy = state.status === 'offline' && state.busy; + if (isOfflineAndBusy) { + isOrphan = true; + } } logger.info(`Runner '${runner.instanceId}' is judged to ${isOrphan ? 'be' : 'not be'} orphaned.`); return isOrphan; From d3356d686b54a2a7785eefe8d9e4fc4a792626a0 Mon Sep 17 00:00:00 2001 From: Stuart Pearson <1926002+stuartp44@users.noreply.github.com> Date: Fri, 22 Aug 2025 12:00:37 +0200 Subject: [PATCH 2/4] fmt --- .../functions/control-plane/src/scale-runners/scale-down.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) 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 c7a136d79b..00ade16568 100644 --- a/lambdas/functions/control-plane/src/scale-runners/scale-down.ts +++ b/lambdas/functions/control-plane/src/scale-runners/scale-down.ts @@ -83,7 +83,9 @@ async function getGitHubSelfHostedRunnerState( async function getGitHubRunnerBusyState(client: Octokit, ec2runner: RunnerInfo, runnerId: number): Promise { const state = await getGitHubSelfHostedRunnerState(client, ec2runner, runnerId); if (state === null) { - logger.info(`Runner '${ec2runner.instanceId}' - GitHub Runner ID '${runnerId}' - Not found on GitHub, treating as not busy`); + logger.info( + `Runner '${ec2runner.instanceId}' - GitHub Runner ID '${runnerId}' - Not found on GitHub, treating as not busy`, + ); return false; } logger.info(`Runner '${ec2runner.instanceId}' - GitHub Runner ID '${runnerId}' - Busy: ${state.busy}`); From 387eb99e6fa8a642aca8cfaa170d72704a2ce6f9 Mon Sep 17 00:00:00 2001 From: Stuart Pearson <1926002+stuartp44@users.noreply.github.com> Date: Fri, 22 Aug 2025 15:34:40 +0200 Subject: [PATCH 3/4] test: enhance error handling for GitHub API responses in scale-down logic --- .../src/scale-runners/scale-down.test.ts | 28 +++++++++++++++---- .../src/scale-runners/scale-down.ts | 5 ++-- 2 files changed, 25 insertions(+), 8 deletions(-) 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 af1272e5c3..3bbe2cdb8d 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 @@ -1,4 +1,5 @@ import { Octokit } from '@octokit/rest'; +import { RequestError } from '@octokit/request-error'; import moment from 'moment'; import nock from 'nock'; @@ -421,8 +422,13 @@ describe('Scale down runners', () => { mockAwsRunners(runners); // Mock 404 error response - const error404 = new Error('Runner not found'); - (error404 as any).status = 404; + const error404 = new RequestError('Runner not found', 404, { + request: { + method: 'GET', + url: 'https://api.github.com/test', + headers: {}, + }, + }); if (type === 'Repo') { mockOctokit.actions.getSelfHostedRunnerForRepo.mockRejectedValueOnce(error404); @@ -453,8 +459,13 @@ describe('Scale down runners', () => { mockAwsRunners(runners); // Mock 404 error response for busy state check - const error404 = new Error('Runner not found'); - (error404 as any).status = 404; + const error404 = new RequestError('Runner not found', 404, { + request: { + method: 'GET', + url: 'https://api.github.com/test', + headers: {}, + }, + }); if (type === 'Repo') { mockOctokit.actions.getSelfHostedRunnerForRepo.mockRejectedValueOnce(error404); @@ -487,8 +498,13 @@ describe('Scale down runners', () => { mockAwsRunners(runners); // Mock non-404 error response - const error500 = new Error('Internal server error'); - (error500 as any).status = 500; + const error500 = new RequestError('Internal server error', 500, { + request: { + method: 'GET', + url: 'https://api.github.com/test', + headers: {}, + }, + }); if (type === 'Repo') { mockOctokit.actions.getSelfHostedRunnerForRepo.mockRejectedValueOnce(error500); 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 00ade16568..1e5e712a24 100644 --- a/lambdas/functions/control-plane/src/scale-runners/scale-down.ts +++ b/lambdas/functions/control-plane/src/scale-runners/scale-down.ts @@ -1,5 +1,6 @@ import { Octokit } from '@octokit/rest'; import { Endpoints } from '@octokit/types'; +import { RequestError } from '@octokit/request-error'; import { createChildLogger } from '@aws-github-runner/aws-powertools-util'; import moment from 'moment'; @@ -71,8 +72,8 @@ async function getGitHubSelfHostedRunnerState( metricGitHubAppRateLimit(state.headers); return state.data; - } catch (error: any) { - if (error.status === 404) { + } catch (error) { + if (error instanceof RequestError && error.status === 404) { logger.info(`Runner '${ec2runner.instanceId}' with GitHub Runner ID '${runnerId}' not found on GitHub (404)`); return null; } From 3c1cfbd4ad1bddce94b3f3bddcbc99cad4c36309 Mon Sep 17 00:00:00 2001 From: Stuart Pearson <1926002+stuartp44@users.noreply.github.com> Date: Fri, 22 Aug 2025 16:29:29 +0200 Subject: [PATCH 4/4] Update lambdas/functions/control-plane/src/scale-runners/scale-down.test.ts Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- .../control-plane/src/scale-runners/scale-down.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 3bbe2cdb8d..87b719a4f1 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 @@ -516,7 +516,7 @@ describe('Scale down runners', () => { await expect(scaleDown()).resolves.not.toThrow(); // Should not terminate since the error was not a 404 - expect(mockTerminateRunners).not.toHaveBeenCalledWith(orphanRunner.instanceId); + expect(terminateRunner).not.toHaveBeenCalledWith(orphanRunner.instanceId); }); it(`Should ignore errors when termination orphan fails.`, async () => {