Skip to content

fix(terminal-chat): dedupe terminal tool-session registrations to prevent listener leak (#309906)#310740

Merged
meganrogge merged 2 commits intomicrosoft:mainfrom
maruthang:fix/issue-309906-terminal-tool-session-listener-leak
Apr 17, 2026
Merged

fix(terminal-chat): dedupe terminal tool-session registrations to prevent listener leak (#309906)#310740
meganrogge merged 2 commits intomicrosoft:mainfrom
maruthang:fix/issue-309906-terminal-tool-session-listener-leak

Conversation

@maruthang
Copy link
Copy Markdown
Contributor

What
Dedupe terminal tool-session registrations in TerminalChatService.registerTerminalInstanceWithToolSession so repeated registrations for the same terminal instance don't stack up event listeners.

Why
RunInTerminalTool.invoke() generates a fresh terminalToolSessionId per tool call and re-registers the same terminal instance each time. The existing implementation subscribed to chatService.onDidDisposeSession and instance.onDisposed on every call without disposing the prior listeners, so listeners accumulated on shared emitters until the leak detector tripped. This is the root cause behind the telemetry bucket [Unhandled Error] potential listener LEAK detected, dominated reported in #309906 (error-telemetry, recent-regression).

How
Added a dedupe guard at the top of registerTerminalInstanceWithToolSession:

  • If the instance is already registered with the same terminalToolSessionId, early-return (no-op).
  • Otherwise, deleteAndDispose the stale listener and drop the stale mapping before registering the new one.

This mirrors the pattern already used by the sibling registerTerminalInstanceWithChatSession in the same file, keeping the two registration paths consistent.

Test plan
Added src/vs/workbench/contrib/terminalContrib/chat/test/browser/terminalChatService.test.ts with two regression tests that inspect Emitter._size (the same counter the leak detector uses):

  • Asserts the listener count on onDidDisposeSession stays ≤ 1 after 50 re-registrations with fresh tool-session ids.
  • Asserts re-registering with the same id is a no-op and doesn't add a listener.

npm run compile-check-ts-native passes cleanly for this change.

Fixes #309906

@meganrogge meganrogge added this to the 1.117.0 milestone Apr 16, 2026
@meganrogge meganrogge enabled auto-merge (squash) April 16, 2026 17:12
Copy link
Copy Markdown
Collaborator

@meganrogge meganrogge left a comment

Choose a reason for hiding this comment

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

Thanks!

@meganrogge meganrogge merged commit 8dac599 into microsoft:main Apr 17, 2026
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Unhandled Error] potential listener LEAK detected, dominated

3 participants