Skip to content

Commit 66e0b2d

Browse files
authored
fix(cli): gc does not delete isolated assets when rollback-buffer-days is set (#502)
The `Date` class allows the following for values: `new Date(value: number | string | Date)`, but sometimes does not work as expected. That's essentially what we are doing because we are storing the string value of the date in the tags and the conversion back to a date doesn't work correctly. A deep dive into the issue with Dates: ```ts console.log(Date.now()) // 1747346265800 console.log(Date(1747346265800)) // Thu May 15 2025 18:06:11 GMT-0400 (Eastern Daylight Time) console.log(Date('1747346265800')) // Thu May 15 2025 18:05:57 GMT-0400 (Eastern Daylight Time) console.log(new Date(1747346265800)) // 2025-05-15T21:57:45.800Z console.log(new Date('1747346265800')) // Invalid Date ``` I seem to have been unlucky with how I tried to create the date from a string. Also, the resulting dates with a number of milliseconds versus a string of milliseconds is _slightly_ different, 18:06:11 versus 18:05:57... This _was_ attempted to be unit tested but in the unit test I mistakenly mocked the tag to be an ISO String which _is_ a string so it had no problem getting converted into a Date. This test has been updated. Because we pessimistically handle errors; we just treated this as a nondeletable asset. I have added an integ test that tests this scenario and confirmed on my own local set up that this fixes the issue. --- By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license
1 parent 7e23e64 commit 66e0b2d

File tree

3 files changed

+74
-3
lines changed

3 files changed

+74
-3
lines changed
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,69 @@
1+
import { ListObjectsV2Command, PutObjectTaggingCommand } from '@aws-sdk/client-s3';
2+
import { integTest, withoutBootstrap, randomString } from '../../lib';
3+
4+
jest.setTimeout(2 * 60 * 60_000); // Includes the time to acquire locks, worst-case single-threaded runtime
5+
6+
const DAY = 24 * 60 * 60 * 1000;
7+
const S3_ISOLATED_TAG = 'aws-cdk:isolated';
8+
9+
integTest(
10+
'Garbage Collection deletes unused s3 objects with rollback-buffer-days',
11+
withoutBootstrap(async (fixture) => {
12+
const toolkitStackName = fixture.bootstrapStackName;
13+
const bootstrapBucketName = `aws-cdk-garbage-collect-integ-test-bckt-${randomString()}`;
14+
fixture.rememberToDeleteBucket(bootstrapBucketName); // just in case
15+
16+
await fixture.cdkBootstrapModern({
17+
toolkitStackName,
18+
bootstrapBucketName,
19+
});
20+
21+
await fixture.cdkDeploy('lambda', {
22+
options: [
23+
'--context', `bootstrapBucket=${bootstrapBucketName}`,
24+
'--context', `@aws-cdk/core:bootstrapQualifier=${fixture.qualifier}`,
25+
'--toolkit-stack-name', toolkitStackName,
26+
'--force',
27+
],
28+
});
29+
fixture.log('Setup complete!');
30+
31+
await fixture.cdkDestroy('lambda', {
32+
options: [
33+
'--context', `bootstrapBucket=${bootstrapBucketName}`,
34+
'--context', `@aws-cdk/core:bootstrapQualifier=${fixture.qualifier}`,
35+
'--toolkit-stack-name', toolkitStackName,
36+
'--force',
37+
],
38+
});
39+
40+
// Pretend the assets were tagged with an old date > 1 day ago so that garbage collection
41+
// should pick up and delete asset even with rollbackBufferDays=1
42+
const res = await fixture.aws.s3.send(new ListObjectsV2Command({ Bucket: bootstrapBucketName }));
43+
for (const contents of res.Contents ?? []) {
44+
await fixture.aws.s3.send(new PutObjectTaggingCommand({
45+
Bucket: bootstrapBucketName,
46+
Key: contents.Key,
47+
Tagging: {
48+
TagSet: [{
49+
Key: S3_ISOLATED_TAG,
50+
Value: String(Date.now() - (30 * DAY)),
51+
}],
52+
},
53+
}));
54+
}
55+
56+
await fixture.cdkGarbageCollect({
57+
rollbackBufferDays: 1,
58+
type: 's3',
59+
bootstrapStackName: toolkitStackName,
60+
});
61+
fixture.log('Garbage collection complete!');
62+
63+
// assert that the bootstrap bucket is empty
64+
await fixture.aws.s3.send(new ListObjectsV2Command({ Bucket: bootstrapBucketName }))
65+
.then((result) => {
66+
expect(result.Contents).toBeUndefined();
67+
});
68+
}),
69+
);

packages/@aws-cdk/toolkit-lib/lib/api/garbage-collection/garbage-collector.ts

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,8 @@ export class ImageAsset {
5555
if (!dateIsolated || dateIsolated == '') {
5656
return false;
5757
}
58-
return new Date(dateIsolated) < date;
58+
59+
return new Date(Number(dateIsolated)) < date;
5960
}
6061

6162
public buildImageTag(inc: number) {
@@ -115,7 +116,8 @@ export class ObjectAsset {
115116
if (!tagValue || tagValue == '') {
116117
return false;
117118
}
118-
return new Date(tagValue) < date;
119+
120+
return new Date(Number(tagValue)) < date;
119121
}
120122
}
121123

packages/@aws-cdk/toolkit-lib/test/api/garbage-collection/garbage-collection.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -916,7 +916,7 @@ describe('Garbage Collection with large # of objects', () => {
916916
// of the 2000 in use assets, 1000 are tagged.
917917
s3Client.on(GetObjectTaggingCommand).callsFake((params) => ({
918918
TagSet: Number(params.Key[params.Key.length - 5]) % 2 === 0
919-
? [{ Key: S3_ISOLATED_TAG, Value: new Date(2000, 1, 1).toISOString() }]
919+
? [{ Key: S3_ISOLATED_TAG, Value: new Date(2000, 1, 1).getTime() }]
920920
: [],
921921
}));
922922
}

0 commit comments

Comments
 (0)