runners: Revert things related to batch termination (#6868)

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.

Author: seemethere

terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/runners.test.ts

@@ -2,12 +2,12 @@ import {
   RunnerInputParameters,
   createRunner,
   findAmiID,
+  getParameterNameForRunner,
   listRunners,
   listSSMParameters,
   resetRunnersCaches,
   terminateRunner,
   tryReuseRunner,
-  terminateRunners,
 } from './runners';
 import { RunnerInfo } from './utils';
 import { ScaleUpMetrics } from './metrics';
@@ -326,11 +326,10 @@ describe('listSSMParameters', () => {
   });
 });
 
-describe('terminateRunners', () => {
+describe('terminateRunner', () => {
   beforeEach(() => {
     mockSSMdescribeParametersRet.mockClear();
     mockEC2.terminateInstances.mockClear();
-    mockSSM.deleteParameter.mockClear();
     const config = {
       environment: 'gi-ci',
       minimumRunningTimeInMinutes: 45,
@@ -340,143 +339,68 @@ describe('terminateRunners', () => {
     resetRunnersCaches();
   });
 
-  it('terminates multiple runners in same region successfully', async () => {
-    const runners: RunnerInfo[] = [
-      {
-        awsRegion: 'us-east-1',
-        instanceId: 'i-1234',
-        environment: 'gi-ci',
-      },
-      {
-        awsRegion: 'us-east-1',
-        instanceId: 'i-5678',
-        environment: 'gi-ci',
-      },
-    ];
-
-    await terminateRunners(runners, metrics);
-
-    expect(mockEC2.terminateInstances).toBeCalledTimes(1);
-    expect(mockEC2.terminateInstances).toBeCalledWith({
-      InstanceIds: ['i-1234', 'i-5678'],
+  it('calls terminateInstances', async () => {
+    const runner: RunnerInfo = {
+      awsRegion: Config.Instance.awsRegion,
+      instanceId: 'i-1234',
+      environment: 'gi-ci',
+    };
+    mockSSMdescribeParametersRet.mockResolvedValueOnce({
+      Parameters: [getParameterNameForRunner(runner.environment as string, runner.instanceId)].map((s) => {
+        return { Name: s };
+      }),
     });
-  });
-
-  it('terminates runners across multiple regions', async () => {
-    const runners: RunnerInfo[] = [
-      {
-        awsRegion: 'us-east-1',
-        instanceId: 'i-1234',
-        environment: 'gi-ci',
-      },
-      {
-        awsRegion: 'us-west-2',
-        instanceId: 'i-5678',
-        environment: 'gi-ci',
-      },
-    ];
-
-    await terminateRunners(runners, metrics);
+    await terminateRunner(runner, metrics);
 
-    expect(mockEC2.terminateInstances).toBeCalledTimes(2);
-    expect(mockEC2.terminateInstances).toHaveBeenNthCalledWith(1, {
-      InstanceIds: ['i-1234'],
+    expect(mockEC2.terminateInstances).toBeCalledWith({
+      InstanceIds: [runner.instanceId],
     });
-    expect(mockEC2.terminateInstances).toHaveBeenNthCalledWith(2, {
-      InstanceIds: ['i-5678'],
+    expect(mockSSM.describeParameters).toBeCalledTimes(1);
+    expect(mockSSM.deleteParameter).toBeCalledTimes(1);
+    expect(mockSSM.deleteParameter).toBeCalledWith({
+      Name: getParameterNameForRunner(runner.environment as string, runner.instanceId),
     });
   });
 
-  it('handles partial failure - terminates some runners but fails on others', async () => {
-    const runners: RunnerInfo[] = [
-      {
-        awsRegion: 'us-east-1',
-        instanceId: 'i-1234',
-        environment: 'gi-ci',
-      },
-      {
-        awsRegion: 'us-east-1',
-        instanceId: 'i-5678',
-        environment: 'gi-ci',
-      },
-      {
-        awsRegion: 'us-west-2',
-        instanceId: 'i-9999',
-        environment: 'gi-ci',
-      },
-    ];
-
-    // First region succeeds, second region fails
-    mockEC2.terminateInstances
-      .mockReturnValueOnce({
-        promise: jest.fn().mockResolvedValueOnce({}),
-      })
-      .mockReturnValueOnce({
-        promise: jest.fn().mockRejectedValueOnce(new Error('Region failure')),
-      });
-
-    await expect(terminateRunners(runners, metrics)).rejects.toThrow(
-      'Failed to terminate some runners: Instance i-9999: Region failure',
-    );
-
-    expect(mockEC2.terminateInstances).toBeCalledTimes(2);
+  it('fails to terminate', async () => {
+    const errMsg = 'Error message';
+    const runner: RunnerInfo = {
+      awsRegion: Config.Instance.awsRegion,
+      instanceId: '1234',
+    };
+    mockEC2.terminateInstances.mockClear().mockReturnValue({
+      promise: jest.fn().mockRejectedValueOnce(Error(errMsg)),
+    });
+    expect(terminateRunner(runner, metrics)).rejects.toThrowError(errMsg);
+    expect(mockSSM.describeParameters).not.toBeCalled();
+    expect(mockSSM.deleteParameter).not.toBeCalled();
   });
 
-  it('handles large batches by splitting into chunks', async () => {
-    // Create 150 runners to test batching (should split into 2 batches of 100 and 50)
-    const runners: RunnerInfo[] = Array.from({ length: 150 }, (_, i) => ({
-      awsRegion: 'us-east-1',
-      instanceId: `i-${i.toString().padStart(4, '0')}`,
-      environment: 'gi-ci',
-    }));
-
-    await terminateRunners(runners, metrics);
+  it('fails to list parameters on terminate, then force delete all next parameters', async () => {
+    const runner1: RunnerInfo = {
+      awsRegion: Config.Instance.awsRegion,
+      instanceId: '1234',
+      environment: 'environ',
+    };
+    const runner2: RunnerInfo = {
+      awsRegion: Config.Instance.awsRegion,
+      instanceId: '1235',
+      environment: 'environ',
+    };
+    mockSSMdescribeParametersRet.mockRejectedValueOnce('Some Error');
+    await terminateRunner(runner1, metrics);
+    await terminateRunner(runner2, metrics);
 
-    // Should make 2 terminate calls (batches of 100 and 50)
     expect(mockEC2.terminateInstances).toBeCalledTimes(2);
-    expect(mockEC2.terminateInstances).toHaveBeenNthCalledWith(1, {
-      InstanceIds: runners.slice(0, 100).map((r) => r.instanceId),
+    expect(mockSSM.describeParameters).toBeCalledTimes(1);
+    expect(mockSSM.deleteParameter).toBeCalledTimes(2);
+    expect(mockSSM.deleteParameter).toBeCalledWith({
+      Name: getParameterNameForRunner(runner1.environment as string, runner1.instanceId),
     });
-    expect(mockEC2.terminateInstances).toHaveBeenNthCalledWith(2, {
-      InstanceIds: runners.slice(100, 150).map((r) => r.instanceId),
+    expect(mockSSM.deleteParameter).toBeCalledWith({
+      Name: getParameterNameForRunner(runner2.environment as string, runner2.instanceId),
     });
   });
-
-  it('cleans up SSM parameters for successful batches even when later batch fails', async () => {
-    // Create runners that will be split into 2 batches
-    const runners: RunnerInfo[] = Array.from({ length: 150 }, (_, i) => ({
-      awsRegion: 'us-east-1',
-      instanceId: `i-${i.toString().padStart(4, '0')}`,
-      environment: 'gi-ci',
-    }));
-
-    // First batch succeeds, second batch fails
-    mockEC2.terminateInstances
-      .mockReturnValueOnce({
-        promise: jest.fn().mockResolvedValueOnce({}),
-      })
-      .mockReturnValueOnce({
-        promise: jest.fn().mockRejectedValueOnce(new Error('Batch 2 failed')),
-      });
-
-    await expect(terminateRunners(runners, metrics)).rejects.toThrow('Failed to terminate some runners');
-
-    expect(mockEC2.terminateInstances).toBeCalledTimes(2);
-  });
-
-  it('handles SSM parameter cleanup failure gracefully', async () => {
-    const runners: RunnerInfo[] = [
-      {
-        awsRegion: 'us-east-1',
-        instanceId: 'i-1234',
-        environment: 'gi-ci',
-      },
-    ];
-
-    await terminateRunners(runners, metrics);
-
-    expect(mockEC2.terminateInstances).toBeCalledTimes(1);
-  });
 });
 
 describe('findAmiID', () => {
@@ -1339,28 +1263,7 @@ describe('createRunner', () => {
         Name: 'wg113-i-1234',
         Value: 'us-east-1-BLAH',
         Type: 'SecureString',
-        Policies: expect.any(String),
       });
-
-      // Verify the Policies parameter contains the correct expiration policy structure
-      const putParameterCall = mockSSM.putParameter.mock.calls[0][0];
-      const policies = JSON.parse(putParameterCall.Policies);
-      expect(policies).toEqual([
-        {
-          Type: 'Expiration',
-          Version: '1.0',
-          Attributes: {
-            Timestamp: expect.any(String),
-          },
-        },
-      ]);
-
-      // Verify the timestamp is approximately 30 minutes in the future
-      const expirationTime = new Date(policies[0].Attributes.Timestamp);
-      const now = Date.now();
-      const timeDiff = expirationTime.getTime() - now;
-      expect(timeDiff).toBeGreaterThan(25 * 60 * 1000); // at least 25 minutes (allowing for test execution time)
-      expect(timeDiff).toBeLessThan(35 * 60 * 1000); // at most 35 minutes (allowing for clock differences)
     });
 
     it('creates ssm experiment parameters when joining experiment', async () => {
@@ -1404,7 +1307,6 @@ describe('createRunner', () => {
         Name: 'wg113-i-1234',
         Value: 'us-east-1-BLAH #ON_AMI_EXPERIMENT',
         Type: 'SecureString',
-        Policies: expect.any(String),
       });
       expect(mockEC2.runInstances).toBeCalledTimes(1);
       expect(mockEC2.runInstances).toBeCalledWith(
@@ -1723,38 +1625,3 @@ describe('createRunner', () => {
     });
   });
 });
-
-describe('terminateRunner', () => {
-  beforeEach(() => {
-    mockSSMdescribeParametersRet.mockClear();
-    mockEC2.terminateInstances.mockClear();
-    mockSSM.deleteParameter.mockClear();
-    const config = {
-      environment: 'gi-ci',
-      minimumRunningTimeInMinutes: 45,
-    };
-    jest.spyOn(Config, 'Instance', 'get').mockImplementation(() => config as unknown as Config);
-
-    resetRunnersCaches();
-  });
-
-  it('delegates to terminateRunners with single runner array', async () => {
-    const runner: RunnerInfo = {
-      awsRegion: 'us-east-1',
-      instanceId: 'i-1234',
-      environment: 'gi-ci',
-    };
-
-    // Mock terminateRunners by mocking the underlying calls
-    mockEC2.terminateInstances.mockReturnValueOnce({
-      promise: jest.fn().mockResolvedValueOnce({}),
-    });
-
-    await terminateRunner(runner, metrics);
-
-    // Verify the calls match what terminateRunners would do with a single runner
-    expect(mockEC2.terminateInstances).toBeCalledWith({
-      InstanceIds: ['i-1234'],
-    });
-  });
-});