Skip to content

fix: memory leak in extension host telemetry - #334096

Merged
Dmitriy Vasyura (dmitrivMS) merged 5 commits into
microsoft:mainfrom
SimonSiefke:fix/memory-leak-extHostTelemetry
Sep 28, 2026
Merged

Dmitriy Vasyura (dmitrivMS) merged 5 commits into
microsoft:mainfrom
SimonSiefke:fix/memory-leak-extHostTelemetry

Conversation

@SimonSiefke

Copy link
Copy Markdown
Contributor

Details

When an extension creates a telemetry logger, the extension host adds it to the extension's logger list. Disposing the logger clears its sender, but does not remove it from that list.

Change

Remove disposed telemetry loggers from the extension host logger list.

Before

When a telemetry logger is created and disposed 37 times, ExtHostTelemetryLogger and its API getters grow by 37:

before

After

No more extension host telemetry logger leak is detected.

Test Video

test-video.webm

Copilot AI balanced review requested due to automatic review settings September 2, 2026 19:35

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

A throwing sender flush() can still prevent bookkeeping cleanup and retain the logger.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Fixes a memory leak by removing disposed telemetry loggers from extension-host bookkeeping.

Changes:

  • Removes loggers from their extension map upon disposal.
  • Adds regression coverage for logger cleanup.
  • Cleanup must use finally so a throwing flush() cannot prevent removal.
File summaries
File Description
src/vs/workbench/api/common/extHostTelemetry.ts Adds disposal-driven removal; the synchronous flush() exception path remains unresolved.
src/vs/workbench/api/test/browser/extHostTelemetry.test.ts Tests logger and map-entry cleanup.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 1
  • Review effort level: Balanced

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/vs/workbench/api/common/extHostTelemetry.ts Outdated
Comment thread src/vs/workbench/api/common/extHostTelemetry.ts

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please take a look at feedback.

@dmitrivMS Dmitriy Vasyura (dmitrivMS) added freeze-slow-crash-leak VS Code crashing, performance, freeze and memory leak issues telemetry Telemetry system issues labels Sep 24, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟢 Approval recommended

The cleanup is correctly guarded by finally and covered by focused regression tests.

Review effort: Balanced
Findings: None

Resolved since last review (1)

@dmitrivMS

Copy link
Copy Markdown
Collaborator

Simon Siefke (@SimonSiefke) Thank you!

@dmitrivMS
Dmitriy Vasyura (dmitrivMS) merged commit 0c86455 into microsoft:main Sep 28, 2026
35 checks passed
@vs-code-engineering vs-code-engineering Bot added this to the 1.141.0 milestone Sep 28, 2026
@SimonSiefke
Simon Siefke (SimonSiefke) deleted the fix/memory-leak-extHostTelemetry branch September 29, 2026 11:15
Abdon Morales (abdonmorales) pushed a commit to abdonmorales/vscode-utcs that referenced this pull request Oct 1, 2026
* fix: memory leak in extHostTelemetry

* fix: ensure telemetry cleanup when flush throws

* Fix telemetry logger replacement during enablement notifications

---------

Co-authored-by: Aaron Munger <2019016+amunger@users.noreply.github.com>
Co-authored-by: Dmitriy Vasyura <dmitriv@microsoft.com>
(cherry picked from commit 0c86455)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

freeze-slow-crash-leak VS Code crashing, performance, freeze and memory leak issues telemetry Telemetry system issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants