Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(cli): old setInterval remains and is not cleared in garbage collection #32985

Merged
merged 10 commits into from
Feb 10, 2025
Original file line number Diff line number Diff line change
Expand Up @@ -251,9 +251,10 @@ export class GarbageCollector {

debug(`Parsing through ${numImages} images in batches`);

printer.start();

for await (const batch of this.readRepoInBatches(ecr, repo, batchSize, currentTime)) {
await backgroundStackRefresh.noOlderThan(600_000); // 10 mins
printer.start();

const { included: isolated, excluded: notIsolated } = partition(batch, asset => !asset.tags.some(t => activeAssets.contains(t)));

Expand Down Expand Up @@ -323,12 +324,13 @@ export class GarbageCollector {

debug(`Parsing through ${numObjects} objects in batches`);

printer.start();

// Process objects in batches of 1000
// This is the batch limit of s3.DeleteObject and we intend to optimize for the "worst case" scenario
// where gc is run for the first time on a long-standing bucket where ~100% of objects are isolated.
for await (const batch of this.readBucketInBatches(s3, bucket, batchSize, currentTime)) {
await backgroundStackRefresh.noOlderThan(600_000); // 10 mins
printer.start();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like the actual fix!


const { included: isolated, excluded: notIsolated } = partition(batch, asset => !activeAssets.contains(asset.fileName()));

Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import * as chalk from 'chalk';
import { GcAsset as GCAsset } from './garbage-collector';
import { info } from '../../logging';
import { ToolkitError } from '../../toolkit/error';

export class ProgressPrinter {
private totalAssets: number;
Expand Down Expand Up @@ -41,6 +42,13 @@ export class ProgressPrinter {
}

public start() {
// If there is already a running setInterval, throw an error.
// This is because if this.setInterval is reassigned to another setInterval,
// the original setInterval remains and can no longer be cleared.
if (this.setInterval) {
throw new ToolkitError('ProgressPrinter is already running. Stop it first using the stop() method before starting it again.');
}

this.setInterval = setInterval(() => {
if (!this.isPaused) {
this.print();
Expand Down
28 changes: 28 additions & 0 deletions packages/aws-cdk/test/api/garbage-collection.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import {
BackgroundStackRefresh,
BackgroundStackRefreshProps,
} from '../../lib/api/garbage-collection/stack-refresh';
import { ProgressPrinter } from '../../lib/api/garbage-collection/progress-printer';
import {
BatchDeleteImageCommand,
BatchGetImageCommand,
Expand Down Expand Up @@ -1002,6 +1003,33 @@ describe('BackgroundStackRefresh', () => {
});
});

describe('ProgressPrinter', () => {
let progressPrinter: ProgressPrinter;
let setInterval: jest.SpyInstance;
let clearInterval: jest.SpyInstance;

beforeEach(() => {
jest.useFakeTimers();
setInterval = jest.spyOn(global, 'setInterval');
clearInterval = jest.spyOn(global, 'clearInterval');

progressPrinter = new ProgressPrinter(0, 1000);
});

afterEach(() => {
jest.clearAllTimers();
setInterval.mockRestore();
clearInterval.mockRestore();
});

test('throws if start is called twice without stop', () => {
progressPrinter.start();

expect(setInterval).toHaveBeenCalledTimes(1);
expect(() => progressPrinter.start()).toThrow('ProgressPrinter is already running. Stop it first using the stop() method before starting it again.');
});
});

function daysInThePast(days: number): Date {
const d = new Date();
d.setDate(d.getDate() - days);
Expand Down