Skip to content

fix(aws-cdk): remove SIGINT listener leak in TelemetrySession - #1863

Open
Adityaj0 wants to merge 1 commit into
aws:mainfrom
Adityaj0:fix/telemetry-sigint-listener-leak
Open

fix(aws-cdk): remove SIGINT listener leak in TelemetrySession#1863
Adityaj0 wants to merge 1 commit into
aws:mainfrom
Adityaj0:fix/telemetry-sigint-listener-leak

Conversation

@Adityaj0

Copy link
Copy Markdown
Contributor

fixes #1862

Reason for this change

TelemetrySession.begin() registers a listener on the process-global SIGINT event every time it's called:

process.on('SIGINT', async () => {
  try {
    await this.end({ name: USER_INTERRUPTED_CODE, message: ABORTED_ERROR_MESSAGE });
  } catch (e: any) {
    await this.ioHost.defaults.trace(`Ending Telemetry failed: ${e.message}`);
  }
  process.exit(1);
});

Nothing ever removes it. Each closure retains the full TelemetrySession -- ioHost, the telemetry client, sanitized session info -- so every begin() call in a process that runs more than once without exiting (e.g. driving the CLI's exec() programmatically, or this repo's own in-process test suite: cli.test.ts, cli-commands.test.ts, deploy-options.test.ts, import-options.test.ts, diff-options.test.ts, ...) permanently leaks another retained object graph. Past 10 accumulated listeners you'd see Node's MaxListenersExceededWarning. It's also a latent correctness bug: if SIGINT ever fires with multiple accumulated listeners, all of them race to end() and process.exit().

Description of changes

  • TelemetrySession now stores the SIGINT listener it registers in begin() on a private field.
  • end() (already called on both the normal-completion path in cli.ts's .finally() and the SIGINT path itself) now removes that listener before doing anything else, so a session's listener is cleaned up exactly once regardless of which path ended it.

Testing

Added two tests to test/cli/telemetry/session.test.ts:

  • begin() registers exactly one SIGINT listener, and end() removes it — asserts the delta is exactly -1.
  • A repeated begin()/end() cycle test (20 iterations, simulating 20 exec() calls in one long-running host process) asserts process.listenerCount('SIGINT') returns to baseline every time, i.e. no net growth.

Full test/cli/telemetry/session.test.ts (34 tests) and test/cli/io-host/ (91 tests) suites pass. (Note: several unrelated test/cli/* suites currently fail to run locally on a fresh checkout due to a pre-existing missing-build-artifact issue -- @aws-cdk/cdk-explorer not being built and build-info.json not being generated -- unrelated to this change; verified the failures are all in that category and none mention telemetry/session.)

Checklist

  • Unit tests added/updated
  • Integration tests added/updated (not applicable — no new AWS resource types or cross-service interactions)
  • No manual edits to generated files

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

fixes aws#1862

TelemetrySession.begin() registered a listener on the process-global
SIGINT event on every call, and nothing ever removed it -- not in
begin(), not in end(), not anywhere else. Each closure retains the full
TelemetrySession (ioHost, telemetry client, sanitized session info), so
this is a real leak, not just a stray function reference: every begin()
call in a long-running process (e.g. calling exec() more than once
without exiting, as this repo's own cli.test.ts / cli-commands.test.ts /
deploy-options.test.ts etc. do) permanently pins another retained object
graph, and past 10 accumulated listeners Node prints
MaxListenersExceededWarning.

Store the listener reference on the instance and remove it in end(),
which is already called on both the normal-completion path (cli.ts's
.finally()) and the SIGINT path itself.

Added a regression test that asserts process.listenerCount('SIGINT')
does not grow across 20 begin()/end() cycles.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 52.17391% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.98%. Comparing base (37bbd4d) to head (1430ae2).

Files with missing lines Patch % Lines
packages/aws-cdk/lib/cli/telemetry/session.ts 52.17% 11 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1863      +/-   ##
==========================================
+ Coverage   90.96%   90.98%   +0.02%     
==========================================
  Files          80       80              
  Lines       12257    12269      +12     
  Branches     1761     1764       +3     
==========================================
+ Hits        11149    11163      +14     
+ Misses       1071     1069       -2     
  Partials       37       37              
Flag Coverage Δ
suite.unit 90.98% <52.17%> (+0.02%) ⬆️

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

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TelemetrySession leaks a SIGINT listener (and its retained closure graph) on every begin()

2 participants