Skip to content

fix: memory leak in terminal process startup - #340582

Open
Simon Siefke (SimonSiefke) wants to merge 4 commits into
microsoft:mainfrom
SimonSiefke:fix/memory-leak-terminalProcessStartupDisposal
Open

Simon Siefke (SimonSiefke) wants to merge 4 commits into
microsoft:mainfrom
SimonSiefke:fix/memory-leak-terminalProcessStartupDisposal

Conversation

@SimonSiefke

Copy link
Copy Markdown
Contributor

Details

A terminal can be shut down while executable validation or spawn throttling is still pending. Startup then creates a PTY and title-polling timer after disposal, so that timer is never cleared.

Change

Check whether the terminal has been disposed after the final await and return before spawning the PTY.

Before

When disposing terminals during startup 37 times, disposed terminal instances and title-polling callbacks grow by 37:

before

After

No more leak detected.

Test Video

test-video.webm

AI disclosure: Model: GPT-6.1-Sol. Worktime: 26 min

Copilot AI balanced review requested due to automatic review settings October 8, 2026 21:02
@vs-code-engineering

Copy link
Copy Markdown
Contributor

📬 CODENOTIFY

The following users are being notified based on files changed in this PR:

Anthony Kim (@anthonykim1)

Matched files:

  • src/vs/platform/terminal/node/terminalProcess.ts

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

The Windows-only spawn-throttling disposal path lacks deterministic test coverage.

1 open finding
What changed in this PR

Prevents PTY and title-timer creation after terminal disposal during asynchronous startup.

Changes:

  • Adds a post-throttle disposal guard.
  • Adds startup disposal and launch validation tests.
File Description
terminalProcess.ts Stops startup after disposal.
terminalProcess.test.ts Tests startup and disposal behavior.

🧠 Review effort: Balanced


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

Comment thread src/vs/platform/terminal/test/node/terminalProcess.test.ts Outdated

This branch has not been deployed

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants