Skip to content

fix(lambda): add post-deregister busy check to prevent terminating active runners #5201

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
npwolf wants to merge 1 commit into from
Closed

fix(lambda): add post-deregister busy check to prevent terminating active runners #5201

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 55 additions & 0 deletions lambdas/functions/control-plane/src/scale-runners/scale-down.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -360,6 +360,61 @@ describe('Scale down runners', () => {
checkNonTerminated(runners);
});

it(`Should not terminate a runner that became busy between deregister and post-deregister check.`, async () => {
// setup: runner appears idle on the pre-deregister check, deregister succeeds,
// but the post-deregister re-check finds it busy (a job was assigned in between)
const runners = [
createRunnerTestData('race-condition-1', type, MINIMUM_TIME_RUNNING_IN_MINUTES + 1, true, false, false),
];

mockGitHubRunners(runners);
mockAwsRunners(runners);

// First call (pre-deregister) returns not-busy, second call (post-deregister) returns busy
const busyCheckMock =
mockOctokit.actions[type === 'Repo' ? 'getSelfHostedRunnerForRepo' : 'getSelfHostedRunnerForOrg'];
busyCheckMock
.mockImplementationOnce(() => ({ data: { busy: false } }))
.mockImplementationOnce(() => ({ data: { busy: true } }));

// act

@npwolf npwolf Jul 6, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 [HIGH] This test asserts a response the real GitHub API can't return, masking the issue above

This "became busy between deregister and re-check" test mocks the second (post-deregister) busy check to return { busy: true }. A runner deleted with a 204 returns 404 on a subsequent GET — as the sibling 404 test elsewhere in this file correctly assumes. So this test exercises an impossible production state; it passes green while the production code actually takes the opposite branch (terminate). It gives false confidence that the guard works.

Fix: mock the post-deregister check to return 404 (matching real GitHub behavior after a successful delete) and assert what the code actually does in that case, rather than asserting the desired-but-unreachable behavior.

await expect(scaleDown()).resolves.not.toThrow();

// assert: runner should NOT be terminated
checkTerminated(runners);
checkNonTerminated(runners);
});

it(`Should terminate a runner when the post-deregister busy check returns 404.`, async () => {
// setup: after deregistration, GitHub API returns 404 (runner fully removed from GitHub)
const runners = [
createRunnerTestData('deregistered-404', type, MINIMUM_TIME_RUNNING_IN_MINUTES + 1, true, false, true),
];

mockGitHubRunners(runners);
mockAwsRunners(runners);

const error404 = new RequestError('Runner not found', 404, {
request: {
method: 'GET',
url: 'https://api.github.com/test',
headers: {},
},
});

// First call (pre-deregister) returns not-busy, second call (post-deregister) throws 404
const busyCheckMock =
mockOctokit.actions[type === 'Repo' ? 'getSelfHostedRunnerForRepo' : 'getSelfHostedRunnerForOrg'];
busyCheckMock.mockImplementationOnce(() => ({ data: { busy: false } })).mockRejectedValueOnce(error404);

// act
await expect(scaleDown()).resolves.not.toThrow();

// assert: runner should be terminated (404 post-deregister is treated as not busy)
checkTerminated(runners);
checkNonTerminated(runners);
});

it(`Should terminate orphan (Non JIT)`, async () => {
// setup
const orphanRunner = createRunnerTestData('orphan-1', type, MINIMUM_BOOT_TIME + 1, false, false, false);
Expand Down
61 changes: 43 additions & 18 deletions lambdas/functions/control-plane/src/scale-runners/scale-down.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,35 +172,60 @@ async function removeRunner(ec2runner: RunnerInfo, ghRunnerIds: number[]): Promi
return;
}

// Step 1: Check busy state as a fast-path to skip runners that are obviously busy.
const states = await Promise.all(
ghRunnerIds.map(async (ghRunnerId) => {
// Get busy state instead of using the output of listGitHubRunners(...) to minimize to race condition.
return await getGitHubRunnerBusyState(githubInstallationClient, ec2runner, ghRunnerId);
}),
);

if (states.every((busy) => busy === false)) {
const results = await Promise.all(
ghRunnerIds.map((ghRunnerId) => deleteGitHubRunner(githubInstallationClient, ec2runner, ghRunnerId)),
if (!states.every((busy) => busy === false)) {
logger.info(`Runner '${ec2runner.instanceId}' cannot be de-registered, because it is still busy.`);
return;
}

// Step 2: De-register the runner from GitHub. This prevents GitHub from assigning new jobs
// to this runner, closing the race window where a job could be assigned between the busy
// check above and the termination below.
const results = await Promise.all(
ghRunnerIds.map((ghRunnerId) => deleteGitHubRunner(githubInstallationClient, ec2runner, ghRunnerId)),
);

const allSucceeded = results.every((r) => r.success);
const failedRunners = results.filter((r) => !r.success);

if (!allSucceeded) {
// Only terminate EC2 if we successfully de-registered from GitHub
// Otherwise, leave the instance running so the next scale-down cycle can retry
logger.error(
`Failed to de-register ${failedRunners.length} GitHub runner(s) for instance '${ec2runner.instanceId}'. ` +
`Instance will NOT be terminated to allow retry on next scale-down cycle. ` +
`Failed runner IDs: ${failedRunners.map((r) => r.ghRunnerId).join(', ')}`,
);
return;
}

const allSucceeded = results.every((r) => r.success);
const failedRunners = results.filter((r) => !r.success);
// Step 3: Re-check busy state after de-registration. A job may have been assigned between
// step 1 and step 2. After de-registration no new jobs can be assigned, so this check is
// now stable. If the runner is busy, the in-flight job will complete using its job-scoped
// OAuth token (the runner worker uses credentials from the job message, not the runner
// registration). We leave the instance running and it will be cleaned up as an orphan.
const postDeregisterStates = await Promise.all(

@npwolf npwolf Jul 6, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 [HIGH] The post-deregister busy re-check can never see "busy" in production — the race this PR targets stays open

Step 2 deletes the runner from GitHub (only proceeds on 204). Step 3 then GETs that same runner ID, which now returns 404. getGitHubRunnerBusyState maps 404 → null → false ("treating as not busy", lines 84–91). So postDeregisterStates.every(busy => busy === false) is essentially always true in production → the instance is always terminated. If a job was assigned during the real race window (between the Step 1 check at :176 and the delete at :191), it is still running on the box and still gets killed. The re-check reduces to if (allDeregisterSucceeded) terminate plus a wasted API round-trip — the only real guard remains the pre-deregister Step 1 check, which is what already existed before this PR.

Fix: a busy signal that survives deregistration is required. Either re-check busy before deleting, or don't treat a post-delete 404 as authoritative "idle" — after your own successful DELETE, 404 is guaranteed and tells you nothing about job state. This needs a rethink of the mechanism, not a tweak.

(Independently confirmed by both a backend-correctness review and a security review of this diff.)

ghRunnerIds.map(async (ghRunnerId) => {
return await getGitHubRunnerBusyState(githubInstallationClient, ec2runner, ghRunnerId);
}),
);

if (allSucceeded) {
await terminateRunner(ec2runner.instanceId);
logger.info(`AWS runner instance '${ec2runner.instanceId}' is terminated and GitHub runner is de-registered.`);
} else {
// Only terminate EC2 if we successfully de-registered from GitHub
// Otherwise, leave the instance running so the next scale-down cycle can retry
logger.error(
`Failed to de-register ${failedRunners.length} GitHub runner(s) for instance '${ec2runner.instanceId}'. ` +
`Instance will NOT be terminated to allow retry on next scale-down cycle. ` +
`Failed runner IDs: ${failedRunners.map((r) => r.ghRunnerId).join(', ')}`,
);
}
if (postDeregisterStates.every((busy) => busy === false)) {
await terminateRunner(ec2runner.instanceId);
logger.info(`AWS runner instance '${ec2runner.instanceId}' is terminated and GitHub runner is de-registered.`);
} else {

@npwolf npwolf Jul 6, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 [MEDIUM] Even if this branch were reachable, orphan cleanup terminates the still-busy runner within ~2 cron cycles anyway

In the (production-unreachable) branch where Step 3 sees busy and leaves the instance running: next cycle, listGitHubRunners no longer includes the deregistered runner → markOrphan tags it as an orphan. Following cycle, terminateOrphan → lastChanceCheckOrphanRunner GETs it → 404 → state === null → isOrphan = true → terminateRunner. The safety check that spares offline && busy runners can never fire for a deregistered runner, since it's 404, not offline+busy. So the "leave it as an orphan, the job will complete" promise in the comments above is false — the job dies regardless of which branch is taken.

Fix: if you intend to spare busy runners, they need to stay registered (so busy state remains queryable) or be tagged with a marker the orphan-cleanup path explicitly honors as "keep until GitHub reports it idle/gone," rather than relying on 404 as a safe-to-kill signal.

logger.info(`Runner '${ec2runner.instanceId}' cannot be de-registered, because it is still busy.`);
logger.warn(
`Runner '${ec2runner.instanceId}' became busy between idle check and de-registration. ` +
`Skipping termination to allow the in-flight job to complete. ` +
`The instance will be cleaned up as an orphan on a subsequent cycle.`,
);
}
} catch (e) {
logger.error(
Expand Down