-
Notifications
You must be signed in to change notification settings - Fork 36.9k
Fix terminal listener leaks in task system #283926
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
- Add DisposableStore to ITerminalData to track terminal-specific disposables - Update _doCreateTerminal to remove leaked onDisposed listeners - Update _createTerminal to use terminal-specific disposable store - Update _reconnectToTerminals to properly track disposal listeners - Update terminate method to avoid leaking onDisposed listener - Update problem matcher listener to use terminal-specific store - Dispose terminal-specific disposables in _deleteTaskAndTerminal Co-authored-by: meganrogge <29464607+meganrogge@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR addresses memory leaks in the task system by adding per-terminal disposal tracking. Previously, terminal event listeners were registered globally via TerminalTaskSystem's disposal store, causing disposed terminal references to be retained indefinitely.
Key changes:
- Added
disposables: DisposableStoreproperty toITerminalDatainterface to track terminal-specific listeners - Created per-terminal disposal stores in
_createTerminaland_reconnectToTerminals - Updated listener registration to use terminal-specific stores with fallback to global store
- Modified
_deleteTaskAndTerminalto dispose terminal-specific listeners
| const terminalKey = terminal.instanceId.toString(); | ||
| const terminalData = { terminal: terminal, lastTask: taskKey, group, shellIntegrationNonce: terminal.shellLaunchConfig.shellIntegrationNonce }; | ||
| const onDisposedListener = this._register(terminal.onDisposed(() => { | ||
| const terminalDisposables = new DisposableStore(); | ||
| const terminalData = { terminal: terminal, lastTask: taskKey, group, shellIntegrationNonce: terminal.shellLaunchConfig.shellIntegrationNonce, disposables: terminalDisposables }; | ||
| terminalDisposables.add(terminal.onDisposed(() => { | ||
| this._deleteTaskAndTerminal(terminal, terminalData); | ||
| onDisposedListener.dispose(); | ||
| })); | ||
| this._terminals[terminalKey] = terminalData; |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Memory leak: If a reconnected terminal is returned by _doCreateTerminal (line 1426-1432) and that terminal was already added to _terminals by _reconnectToTerminals (line 1472), this code will create a new DisposableStore and overwrite the existing entry. The old DisposableStore from the reconnection will never be disposed, leaking its listeners. Before creating a new entry, check if the terminal already exists in _terminals and reuse its DisposableStore, or dispose the old one before replacing it.
See below for a potential fix:
const existingTerminalData = this._terminals[terminalKey];
let terminalDisposables: DisposableStore;
let terminalData: ITerminalData;
if (existingTerminalData) {
// Reuse the existing DisposableStore to avoid leaking listeners created during reconnection.
terminalDisposables = existingTerminalData.disposables;
existingTerminalData.terminal = terminal;
existingTerminalData.lastTask = taskKey;
existingTerminalData.group = group;
existingTerminalData.shellIntegrationNonce = terminal.shellLaunchConfig.shellIntegrationNonce;
terminalData = existingTerminalData;
} else {
terminalDisposables = new DisposableStore();
terminalData = { terminal: terminal, lastTask: taskKey, group, shellIntegrationNonce: terminal.shellLaunchConfig.shellIntegrationNonce, disposables: terminalDisposables };
terminalDisposables.add(terminal.onDisposed(() => {
this._deleteTaskAndTerminal(terminal, terminalData);
}));
this._terminals[terminalKey] = terminalData;
}
| const terminalDisposables = new DisposableStore(); | ||
| const terminalData = { lastTask: data.lastTask, group: data.group, terminal, shellIntegrationNonce: data.shellIntegrationNonce, disposables: terminalDisposables }; | ||
| terminalDisposables.add(terminal.onDisposed(() => { | ||
| this._deleteTaskAndTerminal(terminal, terminalData); |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Behavioral change: The removed onDisposed listeners from _doCreateTerminal used to fire TaskEvent.terminated when terminals were disposed. Now with the new implementation calling _deleteTaskAndTerminal, this event is no longer fired when terminals are disposed by users (outside of explicit task termination). Consider adding this._fireTaskEvent(TaskEvent.terminated(...)) to the onDisposed handler or within _deleteTaskAndTerminal to maintain the original behavior.
| this._deleteTaskAndTerminal(terminal, terminalData); | |
| this._deleteTaskAndTerminal(terminal, terminalData); | |
| this._fireTaskEvent(TaskEvent.terminated()); |
| const onDisposedListener = this._register(terminal.onDisposed(() => { | ||
| const terminalDisposables = new DisposableStore(); | ||
| const terminalData = { terminal: terminal, lastTask: taskKey, group, shellIntegrationNonce: terminal.shellLaunchConfig.shellIntegrationNonce, disposables: terminalDisposables }; | ||
| terminalDisposables.add(terminal.onDisposed(() => { |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Behavioral change: The removed onDisposed listeners from _doCreateTerminal used to fire TaskEvent.terminated when terminals were disposed. Now with the new implementation calling _deleteTaskAndTerminal, this event is no longer fired when terminals are disposed by users (outside of explicit task termination). Consider adding this._fireTaskEvent(TaskEvent.terminated(...)) to the onDisposed handler or within _deleteTaskAndTerminal to maintain the original behavior.
| terminalDisposables.add(terminal.onDisposed(() => { | |
| terminalDisposables.add(terminal.onDisposed(() => { | |
| const event: ITaskEvent = { | |
| kind: TaskEventKind.Terminated, | |
| task | |
| }; | |
| this._fireTaskEvent(event); |
The task system leaked memory by registering terminal event listeners to
TerminalTaskSystem's global disposal store viathis._register(). When terminals disposed, listeners remained in the global store, maintaining references to disposed terminals indefinitely.Changes
Added per-terminal disposal tracking via
DisposableStoretoITerminalData:Listener registration changes:
_doCreateTerminal: Removed global registration ofonDisposedlisteners (lines 1418, 1430, 1440)_createTerminal: CreatesDisposableStoreper terminal, registersonDisposedlistener to it_reconnectToTerminals: CreatesDisposableStorefor reconnected terminals, registers listenersterminate: Uses terminal-specific store when available, falls back to global storeDisposal:
_deleteTaskAndTerminal: CallsterminalData.disposables.dispose()to clean up all terminal-specific listenersThis ensures listeners are disposed when their terminal is disposed, preventing indefinite retention.
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.