Fix listener leak in terminal tool progress parts, execute strategies, and detached terminals#310157
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Fixes multiple “potential listener LEAK detected” reports by preventing per-instance subscriptions to shared singleton emitters across terminal tool progress UI, execute strategies, and detached terminals.
Changes:
- Register
execute()-scopedDisposableStores with execute strategy lifetimes soEvent.toPromiselisteners are cleaned up on early disposal. - Add a
detachedmode toXtermTerminalto skip global singleton listeners, and centralize theme/config propagation inTerminalService. - Replace
MenuWorkbenchToolBarinChatTerminalToolProgressPartwith anActionBarrebuilt from internal state instead of context keys/menus.
Show a summary per file
| File | Description |
|---|---|
| src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/executeStrategy/richExecuteStrategy.ts | Registers DisposableStore with strategy lifetime to avoid lingering listeners on early disposal. |
| src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/executeStrategy/basicExecuteStrategy.ts | Same as above for basic strategy execution. |
| src/vs/workbench/contrib/terminal/browser/xterm/xtermTerminal.ts | Introduces detached option to skip global listeners; adds updateTheme() for external updates. |
| src/vs/workbench/contrib/terminal/browser/terminalService.ts | Centralizes theme/config listeners for detached terminals and forwards updates to all instances. |
| src/vs/workbench/contrib/terminal/browser/terminal.ts | Extends detached xterm interface with updateConfig() / updateTheme(). |
| src/vs/workbench/contrib/chat/browser/widget/chatContentParts/toolInvocationParts/chatTerminalToolProgressPart.ts | Replaces menu/context-key-driven toolbar with rebuilt ActionBar actions to avoid listener buildup. |
Copilot's findings
Comments suppressed due to low confidence (1)
src/vs/workbench/contrib/chat/browser/widget/chatContentParts/toolInvocationParts/chatTerminalToolProgressPart.ts:1
_toolbarOutputExpandedis only updated in_toggleOutput(...), but_updateToolbarContextKeys(...)can change the output expansion state indirectly (eg via_maybeAutoExpandTerminalOutput(...)or initial restored state). That can leave the toolbar toggle icon/label out of sync withthis._outputView.isExpanded. Consider setting_toolbarOutputExpanded = this._outputView.isExpandedinside_updateToolbarContextKeys(...)before calling_updateToolbarActions(), and also explicitly resetting it when_usesCollapsibleWrapperis true to avoid stale state.
/*---------------------------------------------------------------------------------------------
- Files reviewed: 6/6 changed files
- Comments generated: 7
Contributor
Screenshot ChangesBase: Changed (1)
|
| Before | After |
|---|---|
 |
 |
roblourens
approved these changes
Apr 15, 2026
This was referenced Apr 29, 2026
meganrogge
added a commit
that referenced
this pull request
Apr 29, 2026
…13369) Each execute() call registered a per-call DisposableStore via this._register(store), but store.dispose() in the finally block does not remove the reference from the parent _toDispose Set. Stale, already-disposed stores accumulated across invocations on long-lived strategies (28 observed in the wild). Track in-flight stores in a dedicated _executionStores DisposableStore and delete(store) on completion. Preserves the mid-flight disposal cleanup from #310157 while clearing the reference on the happy path. Fixes #313368
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #309684
Fixes #309921
Fixes #309923
Fixes #310046
These four issues all report "potential listener LEAK detected, popular" errors from
accumulating too many listeners on shared singleton emitters.
MenuWorkbenchToolBarinChatTerminalToolProgressPartwith a lightweightActionBarthat is rebuilt on state changes, avoiding per-instanceIMenusubscriptionsto the global
IContextKeyService([Unhandled Error] potential listener LEAK detected, popular — chatTerminalToolProgressPart / toolbar → menuService #309684, [Unhandled Error] potential listener LEAK detected, popular — chatTerminalToolProgressPart / chatToolInvocationPart #309923)DisposableStoreused byEvent.toPromisecalls inRichExecuteStrategyand
BasicExecuteStrategywith the strategy's disposable chain so listeners are cleanedup if the strategy is disposed mid-execution ([Unhandled Error] potential listener LEAK detected, popular — runInTerminalTool / Event.toPromise #309921)
detachedoption toXtermTerminalthat skips registering listeners on globalsingletons (
IConfigurationService,IThemeService), and forward updates from a singlecentralized listener in
TerminalService.createDetachedTerminal([Unhandled Error] potential listener LEAK detected, popular — terminal/xtermTerminal / terminalService #310046)cc @anthonykim1