fix(amplify-category-api-e2e-tests): guard getS3Buckets so one dead region can't abort cleanup

packages/amplify-e2e-tests/jest.unit.config.js

@@ -0,0 +1,16 @@
+// Unit tests for the maintenance scripts in this package. The package level jest config in package.json wires up the
+// AWS backed e2e runner, environment and global setup, none of which these tests should load, so they get their own
+// config and are excluded from the e2e test run.
+module.exports = {
+  preset: 'ts-jest',
+  testEnvironment: 'node',
+  testMatch: ['<rootDir>/src/__unit_tests__/**/*.test.ts'],
+  transform: {
+    '^.+\\.tsx?$': [
+      'ts-jest',
+      {
+        diagnostics: false,
+      },
+    ],
+  },
+};

packages/amplify-e2e-tests/package.json

@@ -21,7 +21,8 @@
     "build-tests": "tsc --build tsconfig.tests.json",
     "setup-profile": "ts-node ./src/configure_tests.ts",
     "clean-stale-test-buckets": "ts-node ./src/cleanup-stale-test-buckets.ts",
-    "clean-e2e-resources": "ts-node ./src/cleanup-e2e-resources.ts"
+    "clean-e2e-resources": "ts-node ./src/cleanup-e2e-resources.ts",
+    "test": "jest --config jest.unit.config.js"
   },
   "dependencies": {
     "@aws-amplify/amplify-app": "^5.0.35",
@@ -91,7 +92,8 @@
     "testRegex": "(src/__tests__/.*|\\.(test|spec))\\.(ts|tsx|js)$",
     "testPathIgnorePatterns": [
       "/node_modules/",
-      "lib"
+      "lib",
+      "src/__unit_tests__"
     ],
     "collectCoverage": false,
     "collectCoverageFrom": [

packages/amplify-e2e-tests/src/__unit_tests__/cleanup-e2e-resources.test.ts

@@ -0,0 +1,211 @@
+/* eslint-disable spellcheck/spell-checker, @typescript-eslint/no-explicit-any, max-classes-per-file */
+import { getOrphanS3TestBuckets, getS3Buckets } from '../cleanup-e2e-resources';
+
+type MockState = {
+  listBuckets: () => any;
+  getBucketLocation: (bucketName: string) => any;
+  getBucketTagging: (bucketName: string, region?: string) => any;
+  calls: { command: string; bucket?: string; region?: string }[];
+};
+
+jest.mock('@aws-sdk/client-s3', () => {
+  const state: MockState = {
+    listBuckets: () => ({ Buckets: [] }),
+    getBucketLocation: () => ({ LocationConstraint: 'us-east-1' }),
+    getBucketTagging: () => ({ TagSet: [] }),
+    calls: [],
+  };
+
+  class ListBucketsCommand {
+    readonly commandName = 'ListBuckets';
+    constructor(readonly input: unknown) {}
+  }
+  class GetBucketLocationCommand {
+    readonly commandName = 'GetBucketLocation';
+    constructor(readonly input: { Bucket: string }) {}
+  }
+  class GetBucketTaggingCommand {
+    readonly commandName = 'GetBucketTagging';
+    constructor(readonly input: { Bucket: string }) {}
+  }
+
+  class S3Client {
+    constructor(readonly config: { region?: string }) {}
+
+    async send(command: any): Promise<any> {
+      const bucket = command.input?.Bucket;
+      state.calls.push({ command: command.commandName, bucket, region: this.config.region });
+      switch (command.commandName) {
+        case 'ListBuckets':
+          return state.listBuckets();
+        case 'GetBucketLocation':
+          return state.getBucketLocation(bucket);
+        case 'GetBucketTagging':
+          return state.getBucketTagging(bucket, this.config.region);
+        default:
+          throw new Error(`Unexpected command ${command.commandName}`);
+      }
+    }
+  }
+
+  return { S3Client, ListBucketsCommand, GetBucketLocationCommand, GetBucketTaggingCommand, mockState: state };
+});
+
+const { mockState } = jest.requireMock('@aws-sdk/client-s3') as { mockState: MockState };
+
+const account = { accountId: '123456789012', credentials: {} } as unknown as Parameters<typeof getS3Buckets>[0];
+
+// `testBucketStalenessFilter` only considers buckets whose name matches /test/ and that are older than 6 hours.
+const staleCreationDate = new Date(Date.now() - 24 * 60 * 60 * 1000);
+const bucketRegions: Record<string, string> = {
+  'amplify-test-bucket-alpha': 'us-east-2',
+  'amplify-test-bucket-dead': 'me-south-1',
+  'amplify-test-bucket-omega': 'eu-west-2',
+};
+const allBuckets = Object.keys(bucketRegions).map((Name) => ({ Name, CreationDate: staleCreationDate }));
+
+/**
+ * The failure from ticket P492565382: a hard-down region times out at the socket level, so the error carries
+ * `code: ETIMEDOUT` while `name` stays the generic 'Error' that matches none of the specifically handled S3 error
+ * names. Those two properties disagreeing is what lets these tests pin the `code ?? name` ordering in the skip logs.
+ */
+const timeoutError = (): Error => Object.assign(new Error('connect ETIMEDOUT 52.95.128.1:443'), { code: 'ETIMEDOUT' });
+
+let logSpy: jest.SpyInstance;
+let errorSpy: jest.SpyInstance;
+
+beforeEach(() => {
+  mockState.calls = [];
+  mockState.listBuckets = () => ({ Buckets: allBuckets });
+  mockState.getBucketLocation = (bucketName) => ({ LocationConstraint: bucketRegions[bucketName] });
+  mockState.getBucketTagging = (bucketName) => ({ TagSet: [{ Key: 'codebuild:build_id', Value: `job-${bucketName}` }] });
+  logSpy = jest.spyOn(console, 'log').mockImplementation(() => undefined);
+  errorSpy = jest.spyOn(console, 'error').mockImplementation(() => undefined);
+});
+
+afterEach(() => {
+  jest.restoreAllMocks();
+});
+
+/**
+ * Find the skip log a guard emitted for one resource, failing loudly rather than vacuously if none was emitted.
+ */
+const skipLogFor = (fragment: string): { message: string; loggedError: unknown } => {
+  const call = logSpy.mock.calls.find(([message]) => String(message).includes(fragment));
+  if (!call) {
+    const logged = JSON.stringify(logSpy.mock.calls.map(([message]) => String(message)));
+    throw new Error(`Expected a skip log containing "${fragment}", but only these were logged: ${logged}`);
+  }
+  return { message: String(call[0]), loggedError: call[1] };
+};
+
+describe('getS3Buckets', () => {
+  it('skips a bucket whose region is unreachable and still returns the buckets of every other region', async () => {
+    const thrown = timeoutError();
+    mockState.getBucketTagging = (bucketName, region) => {
+      if (region === 'me-south-1') {
+        throw thrown;
+      }
+      return { TagSet: [{ Key: 'codebuild:build_id', Value: `job-${bucketName}` }] };
+    };
+
+    const buckets = await getS3Buckets(account);
+
+    expect(buckets.map((bucket) => bucket.name)).toEqual(['amplify-test-bucket-alpha', 'amplify-test-bucket-omega']);
+    expect(buckets.map((bucket) => bucket.region)).toEqual(['us-east-2', 'eu-west-2']);
+    // The dead region really was attempted, otherwise this test would pass without exercising the guard.
+    expect(mockState.calls).toContainEqual({ command: 'GetBucketTagging', bucket: 'amplify-test-bucket-dead', region: 'me-south-1' });
+    const skipLog = skipLogFor('Describing bucket amplify-test-bucket-dead for account 123456789012-me-south-1');
+    // ETIMEDOUT lives on `code` while `name` is the generic 'Error', so resolving `name` first would log 'code Error'
+    // and hide the only detail that identifies the dead region.
+    expect(skipLog.message).toContain('failed with error with code ETIMEDOUT');
+    // The Error itself reaches the log, rather than a JSON.stringify copy that would drop its message and stack.
+    expect(skipLog.loggedError).toBe(thrown);
+  });
+
+  it('keeps sweeping the remaining regions when resolving a bucket region times out', async () => {
+    const thrown = timeoutError();
+    mockState.getBucketLocation = (bucketName) => {
+      if (bucketRegions[bucketName] === 'me-south-1') {
+        throw thrown;
+      }
+      return { LocationConstraint: bucketRegions[bucketName] };
+    };
+
+    const buckets = await getS3Buckets(account);
+
+    expect(buckets.map((bucket) => bucket.name)).toEqual(['amplify-test-bucket-alpha', 'amplify-test-bucket-omega']);
+    const skipLog = skipLogFor('Describing bucket amplify-test-bucket-dead');
+    expect(skipLog.message).toContain('failed with error with code ETIMEDOUT');
+    expect(skipLog.loggedError).toBe(thrown);
+  });
+
+  it('returns no buckets instead of rejecting when the account cannot be listed at all', async () => {
+    const thrown = timeoutError();
+    mockState.listBuckets = () => {
+      throw thrown;
+    };
+
+    await expect(getS3Buckets(account)).resolves.toEqual([]);
+    const skipLog = skipLogFor('Listing S3 buckets for account 123456789012');
+    expect(skipLog.message).toContain('failed with error with code ETIMEDOUT');
+    expect(skipLog.loggedError).toBe(thrown);
+  });
+
+  it('still records buckets that have no tag set, and still skips buckets with an InvalidToken failure', async () => {
+    mockState.getBucketTagging = (bucketName) => {
+      if (bucketName === 'amplify-test-bucket-alpha') {
+        throw Object.assign(new Error('no tags'), { name: 'NoSuchTagSet' });
+      }
+      if (bucketName === 'amplify-test-bucket-dead') {
+        throw Object.assign(new Error('invalid token'), { name: 'InvalidToken' });
+      }
+      return { TagSet: [{ Key: 'codebuild:build_id', Value: `job-${bucketName}` }] };
+    };
+
+    const buckets = await getS3Buckets(account);
+
+    expect(buckets).toEqual([
+      { name: 'amplify-test-bucket-alpha', region: 'us-east-2' },
+      { name: 'amplify-test-bucket-omega', jobId: 'job-amplify-test-bucket-omega', region: 'eu-west-2' },
+    ]);
+    expect(errorSpy).toHaveBeenCalledWith(
+      expect.stringContaining('Skipping processing 123456789012, bucket amplify-test-bucket-dead'),
+      expect.any(Error),
+    );
+  });
+});
+
+describe('getOrphanS3TestBuckets', () => {
+  it('skips the bucket in the unreachable region and still returns the others', async () => {
+    const thrown = timeoutError();
+    mockState.getBucketLocation = (bucketName) => {
+      if (bucketRegions[bucketName] === 'me-south-1') {
+        throw thrown;
+      }
+      return { LocationConstraint: bucketRegions[bucketName] };
+    };
+
+    const orphanBuckets = await getOrphanS3TestBuckets(account);
+
+    expect(orphanBuckets).toEqual([
+      { name: 'amplify-test-bucket-alpha', region: 'us-east-2' },
+      { name: 'amplify-test-bucket-omega', region: 'eu-west-2' },
+    ]);
+    const skipLog = skipLogFor('Resolving the region of bucket amplify-test-bucket-dead for account 123456789012');
+    expect(skipLog.message).toContain('failed with error with code ETIMEDOUT');
+    expect(skipLog.loggedError).toBe(thrown);
+  });
+
+  it('returns no buckets instead of rejecting when the account cannot be listed at all', async () => {
+    const thrown = timeoutError();
+    mockState.listBuckets = () => {
+      throw thrown;
+    };
+
+    await expect(getOrphanS3TestBuckets(account)).resolves.toEqual([]);
+    const skipLog = skipLogFor('Listing S3 buckets for account 123456789012');
+    expect(skipLog.message).toContain('failed with error with code ETIMEDOUT');
+    expect(skipLog.loggedError).toBe(thrown);
+  });
+});

packages/amplify-e2e-tests/src/cleanup-e2e-resources.ts

@@ -188,24 +188,61 @@ const testInstanceStalenessFilter = (resource: DBInstance): boolean => {
   return !!isTestResource && !!isStaleResource;
 };
 
+/**
+ * List the stale test buckets of an account.
+ *
+ * Cleanup sweeps every e2e account in a single process, so a listing failure must stay scoped to the
+ * account it happened in: we log it as a skip and return no buckets rather than rejecting and taking
+ * the whole run down with us.
+ */
+const listStaleTestBuckets = async (account: AWSAccountInfo): Promise<Bucket[]> => {
+  try {
+    const s3Client = new S3Client({ credentials: account.credentials });
+    const listBucketResponse = await s3Client.send(new ListBucketsCommand({}));
+    return (listBucketResponse.Buckets ?? []).filter(testBucketStalenessFilter);
+  } catch (e) {
+    // `code` is read before `name` on purpose: a socket level failure like the ETIMEDOUT that motivated this guard
+    // carries the useful identifier on `code` and leaves `name` as the generic 'Error', so reading `name` first would
+    // log 'Error' and hide the very detail this guard exists to surface. Log the error too, since JSON.stringify drops
+    // an Error's non enumerable message and stack.
+    console.log(
+      `(opt-in region failure) Listing S3 buckets for account ${account.accountId} failed with error with code ${
+        e?.code ?? e?.name
+      }. Skipping.`,
+      e,
+    );
+    return [];
+  }
+};
+
 /**
  * Get all S3 buckets in the account, and filter down to the ones we consider stale.
  */
-const getOrphanS3TestBuckets = async (account: AWSAccountInfo): Promise<S3BucketInfo[]> => {
-  const s3Client = new S3Client({ credentials: account.credentials });
-  const listBucketResponse = await s3Client.send(new ListBucketsCommand({}));
-  const staleBuckets = listBucketResponse.Buckets.filter(testBucketStalenessFilter);
+export const getOrphanS3TestBuckets = async (account: AWSAccountInfo): Promise<S3BucketInfo[]> => {
+  const staleBuckets = await listStaleTestBuckets(account);
 
   const bucketInfos = await Promise.all(
-    staleBuckets.map(async (staleBucket): Promise<S3BucketInfo> => {
-      const region = await getBucketRegion(account, staleBucket.Name);
-      return {
-        name: staleBucket.Name,
-        region,
-      };
+    staleBuckets.map(async (staleBucket): Promise<S3BucketInfo | undefined> => {
+      try {
+        const region = await getBucketRegion(account, staleBucket.Name);
+        return {
+          name: staleBucket.Name,
+          region,
+        };
+      } catch (e) {
+        // Resolving the region talks to the bucket's own region, so an unreachable region fails only this
+        // bucket. Skip it instead of rejecting the Promise.all and aborting cleanup for every account.
+        console.log(
+          `(opt-in region failure) Resolving the region of bucket ${staleBucket.Name} for account ${
+            account.accountId
+          } failed with error with code ${e?.code ?? e?.name}. Skipping.`,
+          e,
+        );
+        return undefined;
+      }
     }),
   );
-  return bucketInfos;
+  return bucketInfos.filter((bucketInfo): bucketInfo is S3BucketInfo => !!bucketInfo);
 };
 
 /**
@@ -482,11 +519,9 @@ const getBucketRegion = async (account: AWSAccountInfo, bucketName: string): Pro
   return region;
 };
 
-const getS3Buckets = async (account: AWSAccountInfo): Promise<S3BucketInfo[]> => {
-  const s3Client = new S3Client({ credentials: account.credentials });
-  const buckets = await s3Client.send(new ListBucketsCommand({}));
+export const getS3Buckets = async (account: AWSAccountInfo): Promise<S3BucketInfo[]> => {
   const result: S3BucketInfo[] = [];
-  for (const bucket of buckets.Buckets.filter(testBucketStalenessFilter)) {
+  for (const bucket of await listStaleTestBuckets(account)) {
     let region: string | undefined;
     try {
       region = await getBucketRegion(account, bucket.Name);
@@ -518,8 +553,16 @@ const getS3Buckets = async (account: AWSAccountInfo): Promise<S3BucketInfo[]> =>
         // processing the rest of the buckets.
         console.error(`Skipping processing ${account.accountId}, bucket ${bucket.Name}`, e);
       } else {
-        console.log('Irrecoverable error in getS3Buckets', JSON.stringify(e));
-        throw e;
+        // Every remaining failure is scoped to this one bucket, and a bucket lives in exactly one region, so a
+        // region being unreachable (e.g. ETIMEDOUT) can only ever mean "this bucket is unprocessable right now".
+        // Rethrowing here used to reject cleanupAccount's Promise.all and abort the run for every account, which
+        // let stacks pile up to the CFN quota, so skip the bucket and keep sweeping the remaining regions.
+        console.log(
+          `(opt-in region failure) Describing bucket ${bucket.Name} for account ${account.accountId}-${
+            region ?? 'unknown region'
+          } failed with error with code ${e?.code ?? e?.name}. Skipping.`,
+          e,
+        );
       }
     }
   }
@@ -1026,7 +1069,11 @@ function chunk<A>(n: number, xs: A[]): A[][] {
   return ret;
 }
 
-cleanup().catch((e) => {
-  console.error(e);
-  process.exitCode = 1;
-});
+// Only sweep when invoked as a script (`yarn clean-e2e-resources`); importing this module from a unit test must not
+// start deleting real resources.
+if (require.main === module) {
+  cleanup().catch((e) => {
+    console.error(e);
+    process.exitCode = 1;
+  });
+}