runners: Add batching for terminateRunner #6852
Conversation
This adds batching for terminateRunner calls in order to fix performance bottlenecks. This also adds batching to the ssm parameter deletion as well in order to also fix some performance bottlenecks we were observing there as well. Goal here was to implement the performance improvements with minimal changes. Signed-off-by: Eli Uriegas <[email protected]>
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
facebook-github-bot
added
the
CLA Signed
|
This was somewhat vibe-coded but heavily reviewed by a human in the loop: vibe-code transcriptUnderstanding ssm parameter cleanup reasonsExported on 6/26/2025 at 21:48:29 PDT from Cursor (1.1.6) User Why would we do ssm parameter cleanup here? Cursor Looking at the code in the Based on my analysis of the code, the SSM parameter cleanup in the Why SSM Parameter Cleanup is Necessary
How the Cleanup Logic WorksThe cleanup logic in
This cleanup ensures that the SSM parameter store remains clean and doesn't accumulate orphaned parameters from terminated runners, which is essential for maintaining system hygiene and avoiding potential issues with parameter limits or security. User What does this do? Cursor Looking at this line of code: const paramName = getParameterNameForRunner(runner.environment || Config.Instance.environment, runner.instanceId);This line constructs the name of the SSM parameter that corresponds to a specific GitHub Actions runner instance. Let me break it down: What it does:
The resulting parameter name:Based on the export function getParameterNameForRunner(environment: string, instanceId: string): string {
return `${environment}-${instanceId}`;
}The parameter name follows the pattern: Example:
Purpose:This parameter name is used to:
The parameter contains the runner configuration data (like GitHub tokens, labels, etc.) that the runner needs to register itself with GitHub Actions and start processing jobs. const paramName = getParameterNameForRunner(runner.environment || Config.Instance.environment, runner.instanceId);export function getParameterNameForRunner(environment: string, instanceId: string): string {
return `${environment}-${instanceId}`;
}User I need to take this and make it batchable, can we modify to have it take in an array of RunnerInfo and then terminate instances in batches of 100 and delete ssm parameters in batches of 10 Cursor I'll help you modify the export async function terminateRunner(runner: RunnerInfo, metrics: Metrics): Promise<void> {
return (await terminateRunners([runner], metrics))[0];
}
export async function terminateRunners(runners: RunnerInfo[], metrics: Metrics): Promise<void[]> {
const results: void[] = new Array(runners.length);
const errors: Array<{ index: number; error: unknown }> = [];
// Group runners by region for efficient AWS API calls
const runnersByRegion = new Map<string, RunnerInfo[]>();
runners.forEach((runner, index) => {
if (!runnersByRegion.has(runner.awsRegion)) {
runnersByRegion.set(runner.awsRegion, []);
}
runnersByRegion.get(runner.awsRegion)!.push({ ...runner, _originalIndex: index });
});
// Process each region
for (const [region, regionRunners] of runnersByRegion) {
try {
await terminateRunnersInRegion(regionRunners, metrics, region);
// Mark all runners in this region as successfully processed
regionRunners.forEach((runner) => {
results[runner._originalIndex] = undefined;
});
} catch (e) {
// Mark all runners in this region as failed
regionRunners.forEach((runner) => {
errors.push({ index: runner._originalIndex, error: e });
});
}
}
// Throw errors if any occurred
if (errors.length > 0) {
const errorMessage = errors.map(({ index, error }) =>
`Runner ${index}: ${error instanceof Error ? error.message : String(error)}`
).join('; ');
throw new Error(`Failed to terminate some runners: ${errorMessage}`);
}
return results;
}
async function terminateRunnersInRegion(
runners: (RunnerInfo & { _originalIndex: number })[],
metrics: Metrics,
region: string
): Promise<void> {
const ec2 = new EC2({ region });
const ssm = new SSM({ region });
// Terminate instances in batches of 100
const instanceBatches = chunkArray(runners.map(r => r.instanceId), 100);
for (const instanceBatch of instanceBatches) {
try {
await expBackOff(() => {
return metrics.trackRequestRegion(
region,
metrics.ec2TerminateInstancesAWSCallSuccess,
metrics.ec2TerminateInstancesAWSCallFailure,
() => {
return ec2.terminateInstances({ InstanceIds: instanceBatch }).promise();
},
);
});
console.info(`Terminated ${instanceBatch.length} instances in region ${region}: ${instanceBatch.join(', ')}`);
} catch (e) {
console.error(`[${region}] Failed to terminate instances: ${instanceBatch.join(', ')} - ${e}`);
throw e;
}
}
// Handle SSM parameter cleanup
await cleanupSSMParametersForRunners(runners, metrics, region, ssm);
}
async function cleanupSSMParametersForRunners(
runners: (RunnerInfo & { _originalIndex: number })[],
metrics: Metrics,
region: string,
ssm: SSM
): Promise<void> {
const paramNames = runners.map(runner =>
getParameterNameForRunner(runner.environment || Config.Instance.environment, runner.instanceId)
);
const cacheName = `${SHOULD_NOT_TRY_LIST_SSM}_${region}`;
if (ssmParametersCache.has(cacheName)) {
// If we've had recent failures listing parameters, just try to delete them directly
await deleteSSMParametersInBatches(paramNames, metrics, region, ssm);
} else {
try {
const existingParams = await listSSMParameters(metrics, region);
const paramsToDelete = paramNames.filter(paramName => existingParams.has(paramName));
if (paramsToDelete.length > 0) {
await deleteSSMParametersInBatches(paramsToDelete, metrics, region, ssm);
} else {
console.info(`[${region}] No SSM parameters found to delete for ${paramNames.length} runners`);
}
} catch (e) {
ssmParametersCache.set(cacheName, 1, 60 * 1000);
console.error(
`[terminateRunnersInRegion - listSSMParameters] [${region}] ` +
`Failed to list parameters, attempting direct deletion: ${e}`,
);
await deleteSSMParametersInBatches(paramNames, metrics, region, ssm);
}
}
}
async function deleteSSMParametersInBatches(
paramNames: string[],
metrics: Metrics,
region: string,
ssm: SSM
): Promise<void> {
const batches = chunkArray(paramNames, 10);
for (const batch of batches) {
try {
await Promise.all(
batch.map(paramName =>
expBackOff(() => {
return metrics.trackRequestRegion(
region,
metrics.ssmdeleteParameterAWSCallSuccess,
metrics.ssmdeleteParameterAWSCallFailure,
() => {
return ssm.deleteParameter({ Name: paramName }).promise();
},
);
})
)
);
console.debug(`[${region}] Deleted SSM parameters: ${batch.join(', ')}`);
} catch (e) {
console.error(`[${region}] Failed to delete SSM parameters batch: ${batch.join(', ')} - ${e}`);
// Continue with other batches even if one fails
}
}
}
function chunkArray<T>(array: T[], chunkSize: number): T[][] {
const chunks: T[][] = [];
for (let i = 0; i < array.length; i += chunkSize) {
chunks.push(array.slice(i, i + chunkSize));
}
return chunks;
}I need to fix the TypeScript errors by properly handling the type extension. Let me update the code: export async function terminateRunner(runner: RunnerInfo, metrics: Metrics): Promise<void> {
return (await terminateRunners([runner], metrics))[0];
}
interface RunnerInfoWithIndex extends RunnerInfo {
_originalIndex: number;
}
export async function terminateRunners(runners: RunnerInfo[], metrics: Metrics): Promise<void[]> {
const results: void[] = new Array(runners.length);
const errors: Array<{ index: number; error: unknown }> = [];
// Group runners by region for efficient AWS API calls
const runnersByRegion = new Map<string, RunnerInfoWithIndex[]>();
runners.forEach((runner, index) => {
if (!runnersByRegion.has(runner.awsRegion)) {
runnersByRegion.set(runner.awsRegion, []);
}
runnersByRegion.get(runner.awsRegion)!.push({ ...runner, _originalIndex: index });
});
// Process each region
for (const [region, regionRunners] of runnersByRegion) {
try {
await terminateRunnersInRegion(regionRunners, metrics, region);
// Mark all runners in this region as successfully processed
regionRunners.forEach((runner) => {
results[runner._originalIndex] = undefined;
});
} catch (e) {
// Mark all runners in this region as failed
regionRunners.forEach((runner) => {
errors.push({ index: runner._originalIndex, error: e });
});
}
}
// Throw errors if any occurred
if (errors.length > 0) {
const errorMessage = errors.map(({ index, error }) =>
`Runner ${index}: ${error instanceof Error ? error.message : String(error)}`
).join('; ');
throw new Error(`Failed to terminate some runners: ${errorMessage}`);
}
return results;
}
async function terminateRunnersInRegion(
runners: RunnerInfoWithIndex[],
metrics: Metrics,
region: string
): Promise<void> {
const ec2 = new EC2({ region });
const ssm = new SSM({ region });
// Terminate instances in batches of 100
const instanceBatches = chunkArray(runners.map(r => r.instanceId), 100);
for (const instanceBatch of instanceBatches) {
try {
await expBackOff(() => {
return metrics.trackRequestRegion(
region,
metrics.ec2TerminateInstancesAWSCallSuccess,
metrics.ec2TerminateInstancesAWSCallFailure,
() => {
return ec2.terminateInstances({ InstanceIds: instanceBatch }).promise();
},
);
});
console.info(`Terminated ${instanceBatch.length} instances in region ${region}: ${instanceBatch.join(', ')}`);
} catch (e) {
console.error(`[${region}] Failed to terminate instances: ${instanceBatch.join(', ')} - ${e}`);
throw e;
}
}
// Handle SSM parameter cleanup
await cleanupSSMParametersForRunners(runners, metrics, region, ssm);
}
async function cleanupSSMParametersForRunners(
runners: RunnerInfoWithIndex[],
metrics: Metrics,
region: string,
ssm: SSM
): Promise<void> {
const paramNames = runners.map(runner =>
getParameterNameForRunner(runner.environment || Config.Instance.environment, runner.instanceId)
);
const cacheName = `${SHOULD_NOT_TRY_LIST_SSM}_${region}`;
if (ssmParametersCache.has(cacheName)) {
// If we've had recent failures listing parameters, just try to delete them directly
await deleteSSMParametersInBatches(paramNames, metrics, region, ssm);
} else {
try {
const existingParams = await listSSMParameters(metrics, region);
const paramsToDelete = paramNames.filter(paramName => existingParams.has(paramName));
if (paramsToDelete.length > 0) {
await deleteSSMParametersInBatches(paramsToDelete, metrics, region, ssm);
} else {
console.info(`[${region}] No SSM parameters found to delete for ${paramNames.length} runners`);
}
} catch (e) {
ssmParametersCache.set(cacheName, 1, 60 * 1000);
console.error(
`[terminateRunnersInRegion - listSSMParameters] [${region}] ` +
`Failed to list parameters, attempting direct deletion: ${e}`,
);
await deleteSSMParametersInBatches(paramNames, metrics, region, ssm);
}
}
}Perfect! I've successfully modified the Key Features:
How to Use:// Single runner (existing usage still works)
await terminateRunner(runner, metrics);
// Multiple runners (new batch functionality)
await terminateRunners([runner1, runner2, runner3], metrics);Benefits:
The implementation maintains all the original functionality while adding the batch processing capabilities you requested. // Single runner (existing usage still works)
await terminateRunner(runner, metrics);
// Multiple runners (new batch functionality)
await terminateRunners([runner1, runner2, runner3], metrics);User why do we need this? Cursor The The ProblemWhen we group runners by region, we need to keep track of each runner's original position in the input array so we can:
The TypeScript IssueThe original // Original interface - no _originalIndex
interface RunnerInfo {
instanceId: string;
awsRegion: string;
// ... other properties
}But our batch processing logic needs to add this property: // We need to add _originalIndex to track position
runners.forEach((runner, index) => {
runnersByRegion.get(runner.awsRegion)!.push({
...runner,
_originalIndex: index // ← This property doesn't exist in RunnerInfo
});
});The SolutionThe interface RunnerInfoWithIndex extends RunnerInfo {
_originalIndex: number;
}This allows us to:
Alternative ApproachesWe could have used:
The interface extension is the cleanest, most type-safe approach that clearly communicates the intent of the code. // Original interface - no _originalIndex
interface RunnerInfo {
instanceId: string;
awsRegion: string;
// ... other properties
}// We need to add _originalIndex to track position
runners.forEach((runner, index) => {
runnersByRegion.get(runner.awsRegion)!.push({
...runner,
_originalIndex: index // ← This property doesn't exist in RunnerInfo
});
});interface RunnerInfoWithIndex extends RunnerInfo {
_originalIndex: number;
}User Can we add a comment to provide context then Cursor export async function terminateRunner(runner: RunnerInfo, metrics: Metrics): Promise<void> {
return (await terminateRunners([runner], metrics))[0];
}
/**
* Extended RunnerInfo interface that includes the original index position.
* This is needed for batch processing to track which result corresponds to which
* original runner in the input array, enabling accurate error reporting and
* result mapping.
*/
interface RunnerInfoWithIndex extends RunnerInfo {
_originalIndex: number;
}Perfect! I've added a clear comment that explains:
This comment will help future developers (including yourself) understand the purpose of this interface when they encounter it in the codebase. User why undefined? Cursor Looking at line 343, the The Looking at the code: export async function terminateRunners(runners: RunnerInfo[], metrics: Metrics): Promise<void[]> {
const results: void[] = new Array(runners.length);
// ...
// Mark all runners in this region as successfully processed
regionRunners.forEach((runner) => {
results[runner._originalIndex] = undefined; // ← This is void
});
return results;
}Why
|
Signed-off-by: Eli Uriegas <[email protected]>
|
Test fixing transcript: the vibeUpdate mocks for test casesExported on 6/26/2025 at 22:10:04 PDT from Cursor (1.1.6) User I think we need to update the mocks for these tests. Make minimal changes if possible. Cursor // ... existing code ...
listSSMParameters: jest.fn().mockResolvedValue(new Map()),
// Added default resolved value to prevent undefined errors in tests
terminateRunner: jest.fn(),
terminateRunners: jest.fn(), // Mock terminateRunners to avoid real AWS calls
// ... existing code ...Added mocked implementations for the previously un-stubbed functions in the tests. Key updates in
These minimal changes keep the test environment fully mocked and prevent credential-related errors without affecting the existing test expectations. User Still failing? Cursor // ... existing code ...
jest.mock('./runners', () => {
/* eslint-disable-next-line @typescript-eslint/no-explicit-any */
const actual = jest.requireActual('./runners') as any;
const terminateRunner = jest.fn();
return {
...actual,
doDeleteSSMParameter: jest.fn(),
listRunners: jest.fn(),
listSSMParameters: jest.fn().mockResolvedValue(new Map()),
resetRunnersCaches: jest.fn(),
terminateRunner,
terminateRunners: jest.fn().mockImplementation((runners: any[], metrics: any) => {
runners.forEach((r) => terminateRunner(r, metrics));
return Promise.resolve();
}),
};
});
// ... existing code ...Updated the
With these changes, the test’s expected call counts for User based on the changes in our latest commit how should we update @scale-down.test.ts to account for the new batching in @terminateRunners() Cursor // ... existing code ...
-import {
- doDeleteSSMParameter,
- listRunners,
- resetRunnersCaches,
- terminateRunner,
+import {
+ doDeleteSSMParameter,
+ listRunners,
+ resetRunnersCaches,
+ terminateRunner,
+ terminateRunners,
RunnerType,
listSSMParameters,
} from './runners';
// ... existing code ...
-jest.mock('./runners', () => {
- /* eslint-disable-next-line @typescript-eslint/no-explicit-any */
- const actual = jest.requireActual('./runners') as any;
- const terminateRunner = jest.fn();
- return {
- ...actual,
- doDeleteSSMParameter: jest.fn(),
- listRunners: jest.fn(),
- listSSMParameters: jest.fn().mockResolvedValue(new Map()),
- resetRunnersCaches: jest.fn(),
- terminateRunner,
- terminateRunners: jest.fn().mockImplementation((runners: any[], metrics: any) => {
- runners.forEach((r) => terminateRunner(r, metrics));
- return Promise.resolve();
- }),
- };
-});
+jest.mock('./runners', () => {
+ /* eslint-disable-next-line @typescript-eslint/no-explicit-any */
+ const actual = jest.requireActual('./runners') as any;
+ return {
+ ...actual,
+ doDeleteSSMParameter: jest.fn(),
+ listRunners: jest.fn(),
+ listSSMParameters: jest.fn().mockResolvedValue(new Map()),
+ resetRunnersCaches: jest.fn(),
+ terminateRunner: jest.fn(),
+ terminateRunners: jest.fn(),
+ };
+});
// ... existing code ...// ... in 'scaleDown' top tests ...
- const mockedTerminateRunner = mocked(terminateRunner);
+ const mockedTerminateRunners = mocked(terminateRunners);
...
- expect(mockedTerminateRunner).not.toBeCalled();
+ expect(mockedTerminateRunners).not.toBeCalled();
// ... in 'org' describe test ...
- const mockedTerminateRunner = mocked(terminateRunner);
+ const mockedTerminateRunners = mocked(terminateRunners);
...
- expect(mockedTerminateRunner).toBeCalledTimes(5);
+ expect(mockedTerminateRunners).toBeCalledTimes(1);
+ const terminated = mockedTerminateRunners.mock.calls[0][0] as RunnerInfo[];
+ expect(terminated.map((r) => r.instanceId).sort()).toEqual(
+ [
+ 'keep-lt-min-no-ghrunner-no-ghr-02',
+ 'keep-lt-min-no-ghrunner-no-ghr-01',
+ 'keep-min-runners-oldest-02',
+ 'keep-min-runners-oldest-01',
+ 'remove-ephemeral-02',
+ ].sort(),
+ );
...
- {
- const { awsR } = getRunnerPair('keep-lt-min-no-ghrunner-no-ghr-02');
- expect(mockedTerminateRunner).toBeCalledWith(awsR, metrics);
- }
- ... (repeat blocks) ...
+ // validate terminateRunners received correct RunnerInfo objects
// ... Similarly in 'repo' describe test ...
- const mockedTerminateRunner = mocked(terminateRunner);
+ const mockedTerminateRunners = mocked(terminateRunners);
...
- expect(mockedTerminateRunner).toBeCalledTimes(5);
+ expect(mockedTerminateRunners).toBeCalledTimes(1);
+ const terminatedRepo = mockedTerminateRunners.mock.calls[0][0] as RunnerInfo[];
+ expect(terminatedRepo.map((r) => r.instanceId).sort()).toEqual(
+ [
+ 'keep-lt-min-no-ghrunner-no-ghr-02',
+ 'keep-lt-min-no-ghrunner-no-ghr-01',
+ 'keep-min-runners-oldest-02',
+ 'keep-min-runners-oldest-01',
+ 'remove-ephemeral-02',
+ ].sort(),
+ );
+
- { blocks verifying individual calls }
+ // no individual terminateRunner expectations now
// ...// ... existing code ...
-import { mocked } from 'ts-jest/utils';
+import { mocked } from 'ts-jest/utils';
// ... ensure no further changesTests failed because they still asserted that Key updates made to
These changes align the test suite with the new batching behavior implemented in User Looks like we're failing? Cursor // ... inside 'ec2runner with repo = undefined && org = undefined' test...
- expect(mockedRemoveGithubRunnerOrg).not.toBeCalled();
- expect(mockedRemoveGithubRunnerRepo).not.toBeCalled();
- expect(mockedTerminateRunners).not.toBeCalled();
+ expect(mockedRemoveGithubRunnerOrg).not.toBeCalled();
+ expect(mockedRemoveGithubRunnerRepo).not.toBeCalled();
+
+ expect(mockedTerminateRunners).toBeCalledTimes(1);
+ expect(mockedTerminateRunners).toBeCalledWith([], metrics);
// ... existing code ...Updated the “ec2runner with repo = undefined && org = undefined” test case: • Now expects All other assertions remain unchanged. |
| } | ||
|
|
||
| // Handle SSM parameter cleanup | ||
| await cleanupSSMParametersForRunners(runners, metrics, region, ssm); |
There was a problem hiding this comment.
pottential SSM parameters leak. If a request succeed deleting 9 out of 10 runners, we never reach here and never try to perform this cleanup.
It is best to catch exceptions above, check the instances terminated in the request and bash cleanup ssm parameters for them before re-throwing the exception.
|
Ohh this is a nice improvement! |
|
What is our test plan to deploy this change ? Could you please update PR description |
|
NIT: Would be nice to ask claude to update tests for the changes in runners.ts :P |
Signed-off-by: Eli Uriegas <[email protected]>
There was a problem hiding this comment.
Any idea what the behavior is when a batch fails? Is always a "either everything is deleted or nothing is deleted" type of situation, or can only a subset of instances get deleted?
If the latter, that's another opportunity for SSM parameter leak and it would be good to document the plan for dealing with those. (Doesn't have to be implemented in this PR though)
Signed-off-by: Eli Uriegas <[email protected]>
When a batch fails it'll error out and go directly to SSM parameter cleanup for the instances that were successfully terminated |
Signed-off-by: Eli Uriegas <[email protected]>
Signed-off-by: Eli Uriegas <[email protected]>
This reverts commit 003bee0.
This reverts the following PRs: * #6859 * #6858 * #6855 * #6854 * #6852 These were causing issues where scale-down was too aggressively scaling down instances leading to runners not being refreshed by scale-up. I do think the SSM expiration stuff is worth a re-do though but there were merge conflicts so I have to revert the entire thing.
I had noticed that we were terminating instances 1 by 1 in the original code so this adds batching for terminateRunner calls in order to fix those performance bottlenecks.
As well during the termination we were deleting ssm parameters one by one so this also adds batching to the ssm parameter deletion as well.
Goal here was to implement the performance improvements with minimal changes.
This PR super-cedes #6725