fix(cli): old setInterval remains and is not cleared

[sakurai-ryo]

Issue # (if applicable)

Related to #32742

Reason for this change

In the garbageCollectS3 and garbageCollectEcr functions, a new setInterval is created in the ProgressPrinter class each time the for-await loop runs.
As a result, the old setInterval can no longer be referenced via this.setInterval, and because it cannot be cleared, the messages continue to be displayed.

aws-cdk/packages/aws-cdk/lib/api/garbage-collection/progress-printer.ts

Line 44 in 899965d

this.setInterval = setInterval(() => {

This behavior is the same as what occurs in the simple sample code below.

const sleep = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms))

class Printer {
  private setInterval?: ReturnType<typeof setTimeout>

  public start() {
    this.setInterval = setInterval(() => {
      console.log("Hello, world!")
    }, 1000)
  }

  public stop() {
    clearInterval(this.setInterval)
  }
}

;(async () => {
  const printer = new Printer()

  // print one message per second for 5 seconds
  printer.start()
  await sleep(5000)

  // print tow messages per second for 5 seconds
  printer.start()
  await sleep(5000)

  // clear the newer setInterval
  // the older one will still be running
  printer.stop()
})()

Description of changes

It didn't seem necessary to initialize setInterval each time the for-await loop runs, so I modified the code to call .start() before the loop begins.

Describe any new or updated permissions being added

Nothing.

Description of how you validated changes

Checklist

• My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

[aws-cdk-automation]

The pull request linter fails with the following errors:

❌ CLI code has changed. A maintainer must run the code through the testing pipeline (git fetch origin pull/32985/head && git push -f origin FETCH_HEAD:test-main-pipeline), then add the 'pr-linter/cli-integ-tested' label when the pipeline succeeds.

If you believe this pull request should receive an exemption, please comment and provide a justification. A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed, add Clarification Request to a comment.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.85%. Comparing base (62b3b60) to head (06322de).
Report is 8 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #32985   +/-   ##
=======================================
  Coverage   80.84%   80.85%           
=======================================
  Files         232      232           
  Lines       14135    14138    +3     
  Branches     2460     2461    +1     
=======================================
+ Hits        11428    11431    +3     
  Misses       2427     2427           
  Partials      280      280           

Flag	Coverage Δ
suite.unit	80.85% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
packages/aws-cdk	79.64% <100.00%> (+<0.01%)	⬆️
packages/aws-cdk-lib/core	82.17% <ø> (ø)

[rix0rrr]

Good find, but I'd rather see this thrown an exception saying that stop() should have been called first.

The problem is not that start() doesn't do what it should, the problem is that the consumer is calling this class incorrectly, but it's not obvious that it is.

[sakurai-ryo]

Yes, that makes sense.
f7726d9

[rix0rrr]

This seems like the actual fix!

[aws-cdk-automation]

➡️ PR build request submitted to test-main-pipeline ⬅️

A maintainer must now check the pipeline and add the pr-linter/cli-integ-tested label once the pipeline succeeds.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

[mergify]

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

[kaizencc]

@Mergifyio update

[mergify]

update

❌ Mergify doesn't have permission to update

For security reasons, Mergify can't update this pull request. Try updating locally.
GitHub response: refusing to allow a GitHub App to create or update workflow .github/workflows/check-suite-test.yaml without workflows permission

Pull request has been modified.

[BwL1289]

Pinging for an update

[kaizencc]

Suggested change

	throw new Error('ProgressPrinter is already running. Stop it first using the stop() method before starting it again.');
	throw new ToolkitError('ProgressPrinter is already running. Stop it first using the stop() method before starting it again.');

[kaizencc]

import { ToolkitError } from '../../toolkit/error';

^ needs to be added at the top of this file @sakurai-ryo. I don't have push access to your fork.

[sakurai-ryo]

70c7b85

[kaizencc]

@sakurai-ryo sorry to bother but i don't have access to push to your fork so I can't make this final change on my own

Pull request has been modified.

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
• Commit ID: 06322de
• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[sakurai-ryo]

@kaizencc
Pushing to this branch by maintainers is allowed, so it's a bit puzzling why this is happening.

I committed this change on my end, please check again.