Skip to content

Commit 1f1b7d5

Browse files
committed
fix(amplify-category-api-e2e-tests): guard getS3Buckets so one dead region can't abort cleanup
The scheduled amplify-category-api-cleanup-workflow job failed 5/5 consecutive runs, leaving every e2e account uncleaned. The per-region STS/CloudFormation/RDS listing calls already skip and continue on failure, but the S3 sweep did not. When getS3Buckets reached the hard-down me-south-1 region, the per-bucket catch fell through to `throw e` after logging "Irrecoverable error in getS3Buckets {code: ETIMEDOUT}". That rejection propagated through cleanupAccount's Promise.all, so a single unreachable region aborted the entire run for all ~13 e2e accounts. Stacks then accumulated until accounts hit the 2000-stack CloudFormation quota (L-0485CB21), which made the CreateApi canary hang and fire alarms. A bucket lives in exactly one region, so a region-level failure can only ever mean "this bucket is unprocessable right now"; it is never a reason to abandon the remaining accounts. This mirrors the existing skip-and-continue pattern (getOrphanRdsInstances, getAmplifyApps, listStacks) for the S3 sweep: - getS3Buckets: a per-bucket failure that is not NoSuchTagSet/NoSuchBucket/InvalidToken is now logged as an "(opt-in region failure) ... Skipping." and the sweep continues. - getOrphanS3TestBuckets: region resolution is guarded per bucket, so one bucket can no longer reject the surrounding Promise.all. - Both ListBuckets calls now go through a shared listStaleTestBuckets helper, keeping an account-level listing failure scoped to that account. The set of swept regions is deliberately unchanged. Adds unit tests for the regression, plus a jest config and `test` script so they run in CI without loading the AWS-backed e2e runner. The script entry point is now behind `require.main === module` so tests can import the module without deleting real resources. Refs: P492565382
1 parent caf03c8 commit 1f1b7d5

4 files changed

Lines changed: 265 additions & 23 deletions

File tree

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
// Unit tests for the maintenance scripts in this package. The package level jest config in package.json wires up the
2+
// AWS backed e2e runner, environment and global setup, none of which these tests should load, so they get their own
3+
// config and are excluded from the e2e test run.
4+
module.exports = {
5+
preset: 'ts-jest',
6+
testEnvironment: 'node',
7+
testMatch: ['<rootDir>/src/__unit_tests__/**/*.test.ts'],
8+
transform: {
9+
'^.+\\.tsx?$': [
10+
'ts-jest',
11+
{
12+
diagnostics: false,
13+
},
14+
],
15+
},
16+
};

packages/amplify-e2e-tests/package.json

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,8 @@
2121
"build-tests": "tsc --build tsconfig.tests.json",
2222
"setup-profile": "ts-node ./src/configure_tests.ts",
2323
"clean-stale-test-buckets": "ts-node ./src/cleanup-stale-test-buckets.ts",
24-
"clean-e2e-resources": "ts-node ./src/cleanup-e2e-resources.ts"
24+
"clean-e2e-resources": "ts-node ./src/cleanup-e2e-resources.ts",
25+
"test": "jest --config jest.unit.config.js"
2526
},
2627
"dependencies": {
2728
"@aws-amplify/amplify-app": "^5.0.35",
@@ -91,7 +92,8 @@
9192
"testRegex": "(src/__tests__/.*|\\.(test|spec))\\.(ts|tsx|js)$",
9293
"testPathIgnorePatterns": [
9394
"/node_modules/",
94-
"lib"
95+
"lib",
96+
"src/__unit_tests__"
9597
],
9698
"collectCoverage": false,
9799
"collectCoverageFrom": [
Lines changed: 183 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,183 @@
1+
/* eslint-disable spellcheck/spell-checker, @typescript-eslint/no-explicit-any, max-classes-per-file */
2+
import { getOrphanS3TestBuckets, getS3Buckets } from '../cleanup-e2e-resources';
3+
4+
type MockState = {
5+
listBuckets: () => any;
6+
getBucketLocation: (bucketName: string) => any;
7+
getBucketTagging: (bucketName: string, region?: string) => any;
8+
calls: { command: string; bucket?: string; region?: string }[];
9+
};
10+
11+
jest.mock('@aws-sdk/client-s3', () => {
12+
const state: MockState = {
13+
listBuckets: () => ({ Buckets: [] }),
14+
getBucketLocation: () => ({ LocationConstraint: 'us-east-1' }),
15+
getBucketTagging: () => ({ TagSet: [] }),
16+
calls: [],
17+
};
18+
19+
class ListBucketsCommand {
20+
readonly commandName = 'ListBuckets';
21+
constructor(readonly input: unknown) {}
22+
}
23+
class GetBucketLocationCommand {
24+
readonly commandName = 'GetBucketLocation';
25+
constructor(readonly input: { Bucket: string }) {}
26+
}
27+
class GetBucketTaggingCommand {
28+
readonly commandName = 'GetBucketTagging';
29+
constructor(readonly input: { Bucket: string }) {}
30+
}
31+
32+
class S3Client {
33+
constructor(readonly config: { region?: string }) {}
34+
35+
async send(command: any): Promise<any> {
36+
const bucket = command.input?.Bucket;
37+
state.calls.push({ command: command.commandName, bucket, region: this.config.region });
38+
switch (command.commandName) {
39+
case 'ListBuckets':
40+
return state.listBuckets();
41+
case 'GetBucketLocation':
42+
return state.getBucketLocation(bucket);
43+
case 'GetBucketTagging':
44+
return state.getBucketTagging(bucket, this.config.region);
45+
default:
46+
throw new Error(`Unexpected command ${command.commandName}`);
47+
}
48+
}
49+
}
50+
51+
return { S3Client, ListBucketsCommand, GetBucketLocationCommand, GetBucketTaggingCommand, mockState: state };
52+
});
53+
54+
const { mockState } = jest.requireMock('@aws-sdk/client-s3') as { mockState: MockState };
55+
56+
const account = { accountId: '123456789012', credentials: {} } as unknown as Parameters<typeof getS3Buckets>[0];
57+
58+
// `testBucketStalenessFilter` only considers buckets whose name matches /test/ and that are older than 6 hours.
59+
const staleCreationDate = new Date(Date.now() - 24 * 60 * 60 * 1000);
60+
const bucketRegions: Record<string, string> = {
61+
'amplify-test-bucket-alpha': 'us-east-2',
62+
'amplify-test-bucket-dead': 'me-south-1',
63+
'amplify-test-bucket-omega': 'eu-west-2',
64+
};
65+
const allBuckets = Object.keys(bucketRegions).map((Name) => ({ Name, CreationDate: staleCreationDate }));
66+
67+
/**
68+
* The failure from ticket P492565382: a hard-down region times out at the socket level, so the error carries
69+
* `code: ETIMEDOUT` and a `name` that matches none of the specifically handled S3 error names.
70+
*/
71+
const timeoutError = (): Error => Object.assign(new Error('connect ETIMEDOUT 52.95.128.1:443'), { code: 'ETIMEDOUT' });
72+
73+
let logSpy: jest.SpyInstance;
74+
let errorSpy: jest.SpyInstance;
75+
76+
beforeEach(() => {
77+
mockState.calls = [];
78+
mockState.listBuckets = () => ({ Buckets: allBuckets });
79+
mockState.getBucketLocation = (bucketName) => ({ LocationConstraint: bucketRegions[bucketName] });
80+
mockState.getBucketTagging = (bucketName) => ({ TagSet: [{ Key: 'codebuild:build_id', Value: `job-${bucketName}` }] });
81+
logSpy = jest.spyOn(console, 'log').mockImplementation(() => undefined);
82+
errorSpy = jest.spyOn(console, 'error').mockImplementation(() => undefined);
83+
});
84+
85+
afterEach(() => {
86+
jest.restoreAllMocks();
87+
});
88+
89+
describe('getS3Buckets', () => {
90+
it('skips a bucket whose region is unreachable and still returns the buckets of every other region', async () => {
91+
mockState.getBucketTagging = (bucketName, region) => {
92+
if (region === 'me-south-1') {
93+
throw timeoutError();
94+
}
95+
return { TagSet: [{ Key: 'codebuild:build_id', Value: `job-${bucketName}` }] };
96+
};
97+
98+
const buckets = await getS3Buckets(account);
99+
100+
expect(buckets.map((bucket) => bucket.name)).toEqual(['amplify-test-bucket-alpha', 'amplify-test-bucket-omega']);
101+
expect(buckets.map((bucket) => bucket.region)).toEqual(['us-east-2', 'eu-west-2']);
102+
// The dead region really was attempted, otherwise this test would pass without exercising the guard.
103+
expect(mockState.calls).toContainEqual({ command: 'GetBucketTagging', bucket: 'amplify-test-bucket-dead', region: 'me-south-1' });
104+
expect(logSpy).toHaveBeenCalledWith(
105+
expect.stringContaining('(opt-in region failure) Describing bucket amplify-test-bucket-dead for account 123456789012-me-south-1'),
106+
expect.any(String),
107+
);
108+
});
109+
110+
it('keeps sweeping the remaining regions when resolving a bucket region times out', async () => {
111+
mockState.getBucketLocation = (bucketName) => {
112+
if (bucketRegions[bucketName] === 'me-south-1') {
113+
throw timeoutError();
114+
}
115+
return { LocationConstraint: bucketRegions[bucketName] };
116+
};
117+
118+
const buckets = await getS3Buckets(account);
119+
120+
expect(buckets.map((bucket) => bucket.name)).toEqual(['amplify-test-bucket-alpha', 'amplify-test-bucket-omega']);
121+
});
122+
123+
it('returns no buckets instead of rejecting when the account cannot be listed at all', async () => {
124+
mockState.listBuckets = () => {
125+
throw timeoutError();
126+
};
127+
128+
await expect(getS3Buckets(account)).resolves.toEqual([]);
129+
expect(logSpy).toHaveBeenCalledWith(expect.stringContaining('(opt-in region failure) Listing S3 buckets for account 123456789012'));
130+
});
131+
132+
it('still records buckets that have no tag set, and still skips buckets with an InvalidToken failure', async () => {
133+
mockState.getBucketTagging = (bucketName) => {
134+
if (bucketName === 'amplify-test-bucket-alpha') {
135+
throw Object.assign(new Error('no tags'), { name: 'NoSuchTagSet' });
136+
}
137+
if (bucketName === 'amplify-test-bucket-dead') {
138+
throw Object.assign(new Error('invalid token'), { name: 'InvalidToken' });
139+
}
140+
return { TagSet: [{ Key: 'codebuild:build_id', Value: `job-${bucketName}` }] };
141+
};
142+
143+
const buckets = await getS3Buckets(account);
144+
145+
expect(buckets).toEqual([
146+
{ name: 'amplify-test-bucket-alpha', region: 'us-east-2' },
147+
{ name: 'amplify-test-bucket-omega', jobId: 'job-amplify-test-bucket-omega', region: 'eu-west-2' },
148+
]);
149+
expect(errorSpy).toHaveBeenCalledWith(
150+
expect.stringContaining('Skipping processing 123456789012, bucket amplify-test-bucket-dead'),
151+
expect.any(Error),
152+
);
153+
});
154+
});
155+
156+
describe('getOrphanS3TestBuckets', () => {
157+
it('skips the bucket in the unreachable region and still returns the others', async () => {
158+
mockState.getBucketLocation = (bucketName) => {
159+
if (bucketRegions[bucketName] === 'me-south-1') {
160+
throw timeoutError();
161+
}
162+
return { LocationConstraint: bucketRegions[bucketName] };
163+
};
164+
165+
const orphanBuckets = await getOrphanS3TestBuckets(account);
166+
167+
expect(orphanBuckets).toEqual([
168+
{ name: 'amplify-test-bucket-alpha', region: 'us-east-2' },
169+
{ name: 'amplify-test-bucket-omega', region: 'eu-west-2' },
170+
]);
171+
expect(logSpy).toHaveBeenCalledWith(
172+
expect.stringContaining('(opt-in region failure) Resolving the region of bucket amplify-test-bucket-dead for account 123456789012'),
173+
);
174+
});
175+
176+
it('returns no buckets instead of rejecting when the account cannot be listed at all', async () => {
177+
mockState.listBuckets = () => {
178+
throw timeoutError();
179+
};
180+
181+
await expect(getOrphanS3TestBuckets(account)).resolves.toEqual([]);
182+
});
183+
});

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

Lines changed: 62 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -188,24 +188,55 @@ const testInstanceStalenessFilter = (resource: DBInstance): boolean => {
188188
return !!isTestResource && !!isStaleResource;
189189
};
190190

191+
/**
192+
* List the stale test buckets of an account.
193+
*
194+
* Cleanup sweeps every e2e account in a single process, so a listing failure must stay scoped to the
195+
* account it happened in: we log it as a skip and return no buckets rather than rejecting and taking
196+
* the whole run down with us.
197+
*/
198+
const listStaleTestBuckets = async (account: AWSAccountInfo): Promise<Bucket[]> => {
199+
try {
200+
const s3Client = new S3Client({ credentials: account.credentials });
201+
const listBucketResponse = await s3Client.send(new ListBucketsCommand({}));
202+
return (listBucketResponse.Buckets ?? []).filter(testBucketStalenessFilter);
203+
} catch (e) {
204+
console.log(
205+
`(opt-in region failure) Listing S3 buckets for account ${account.accountId} failed with error with code ${
206+
e?.name ?? e?.code
207+
}. Skipping.`,
208+
);
209+
return [];
210+
}
211+
};
212+
191213
/**
192214
* Get all S3 buckets in the account, and filter down to the ones we consider stale.
193215
*/
194-
const getOrphanS3TestBuckets = async (account: AWSAccountInfo): Promise<S3BucketInfo[]> => {
195-
const s3Client = new S3Client({ credentials: account.credentials });
196-
const listBucketResponse = await s3Client.send(new ListBucketsCommand({}));
197-
const staleBuckets = listBucketResponse.Buckets.filter(testBucketStalenessFilter);
216+
export const getOrphanS3TestBuckets = async (account: AWSAccountInfo): Promise<S3BucketInfo[]> => {
217+
const staleBuckets = await listStaleTestBuckets(account);
198218

199219
const bucketInfos = await Promise.all(
200-
staleBuckets.map(async (staleBucket): Promise<S3BucketInfo> => {
201-
const region = await getBucketRegion(account, staleBucket.Name);
202-
return {
203-
name: staleBucket.Name,
204-
region,
205-
};
220+
staleBuckets.map(async (staleBucket): Promise<S3BucketInfo | undefined> => {
221+
try {
222+
const region = await getBucketRegion(account, staleBucket.Name);
223+
return {
224+
name: staleBucket.Name,
225+
region,
226+
};
227+
} catch (e) {
228+
// Resolving the region talks to the bucket's own region, so an unreachable region fails only this
229+
// bucket. Skip it instead of rejecting the Promise.all and aborting cleanup for every account.
230+
console.log(
231+
`(opt-in region failure) Resolving the region of bucket ${staleBucket.Name} for account ${
232+
account.accountId
233+
} failed with error with code ${e?.name ?? e?.code}. Skipping.`,
234+
);
235+
return undefined;
236+
}
206237
}),
207238
);
208-
return bucketInfos;
239+
return bucketInfos.filter((bucketInfo): bucketInfo is S3BucketInfo => !!bucketInfo);
209240
};
210241

211242
/**
@@ -482,11 +513,9 @@ const getBucketRegion = async (account: AWSAccountInfo, bucketName: string): Pro
482513
return region;
483514
};
484515

485-
const getS3Buckets = async (account: AWSAccountInfo): Promise<S3BucketInfo[]> => {
486-
const s3Client = new S3Client({ credentials: account.credentials });
487-
const buckets = await s3Client.send(new ListBucketsCommand({}));
516+
export const getS3Buckets = async (account: AWSAccountInfo): Promise<S3BucketInfo[]> => {
488517
const result: S3BucketInfo[] = [];
489-
for (const bucket of buckets.Buckets.filter(testBucketStalenessFilter)) {
518+
for (const bucket of await listStaleTestBuckets(account)) {
490519
let region: string | undefined;
491520
try {
492521
region = await getBucketRegion(account, bucket.Name);
@@ -518,8 +547,16 @@ const getS3Buckets = async (account: AWSAccountInfo): Promise<S3BucketInfo[]> =>
518547
// processing the rest of the buckets.
519548
console.error(`Skipping processing ${account.accountId}, bucket ${bucket.Name}`, e);
520549
} else {
521-
console.log('Irrecoverable error in getS3Buckets', JSON.stringify(e));
522-
throw e;
550+
// Every remaining failure is scoped to this one bucket, and a bucket lives in exactly one region, so a
551+
// region being unreachable (e.g. ETIMEDOUT) can only ever mean "this bucket is unprocessable right now".
552+
// Rethrowing here used to reject cleanupAccount's Promise.all and abort the run for every account, which
553+
// let stacks pile up to the CFN quota, so skip the bucket and keep sweeping the remaining regions.
554+
console.log(
555+
`(opt-in region failure) Describing bucket ${bucket.Name} for account ${account.accountId}-${
556+
region ?? 'unknown region'
557+
} failed with error with code ${e?.name ?? e?.code}. Skipping.`,
558+
JSON.stringify(e),
559+
);
523560
}
524561
}
525562
}
@@ -1026,7 +1063,11 @@ function chunk<A>(n: number, xs: A[]): A[][] {
10261063
return ret;
10271064
}
10281065

1029-
cleanup().catch((e) => {
1030-
console.error(e);
1031-
process.exitCode = 1;
1032-
});
1066+
// Only sweep when invoked as a script (`yarn clean-e2e-resources`); importing this module from a unit test must not
1067+
// start deleting real resources.
1068+
if (require.main === module) {
1069+
cleanup().catch((e) => {
1070+
console.error(e);
1071+
process.exitCode = 1;
1072+
});
1073+
}

0 commit comments

Comments
 (0)