From 542fa13cb35b5db77f92370d4daba1c8c7954e06 Mon Sep 17 00:00:00 2001 From: Dmitriy Vasyura Date: Mon, 24 Aug 2026 17:39:43 -0700 Subject: [PATCH 1/3] Fix terminal tool progress listener leak Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../chatTerminalToolProgressPart.ts | 17 ++++++----- .../contrib/terminal/browser/terminal.ts | 2 ++ .../chat/browser/terminalChatService.ts | 5 ++++ .../test/browser/terminalChatService.test.ts | 30 ++++++++++++++++++- 4 files changed, 46 insertions(+), 8 deletions(-) diff --git a/src/vs/workbench/contrib/chat/browser/widget/chatContentParts/toolInvocationParts/chatTerminalToolProgressPart.ts b/src/vs/workbench/contrib/chat/browser/widget/chatContentParts/toolInvocationParts/chatTerminalToolProgressPart.ts index 7d49548bc5c0b..c87d3197bf067 100644 --- a/src/vs/workbench/contrib/chat/browser/widget/chatContentParts/toolInvocationParts/chatTerminalToolProgressPart.ts +++ b/src/vs/workbench/contrib/chat/browser/widget/chatContentParts/toolInvocationParts/chatTerminalToolProgressPart.ts @@ -330,6 +330,10 @@ export class ChatTerminalToolProgressPart extends BaseChatToolInvocationSubPart return this._contentIndex; } + public get terminalToolSessionId(): string | undefined { + return this._terminalData.terminalToolSessionId; + } + constructor( toolInvocation: IChatToolInvocation | IChatToolInvocationSerialized, terminalData: IChatTerminalToolInvocationData | ILegacyChatTerminalToolInvocationData, @@ -449,13 +453,6 @@ export class ChatTerminalToolProgressPart extends BaseChatToolInvocationSubPart } })); } - this._register(this._terminalChatService.onDidContinueInBackground(sessionId => { - if (sessionId === terminalToolSessionId) { - this._terminalData.didContinueInBackground = true; - this._toolbarCanContinueInBackground = false; - this._updateToolbarActions(); - } - })); } let pastTenseMessage: string | undefined; if (toolInvocation.pastTenseMessage) { @@ -1217,6 +1214,12 @@ export class ChatTerminalToolProgressPart extends BaseChatToolInvocationSubPart } } + public markContinuedInBackground(): void { + this._terminalData.didContinueInBackground = true; + this._toolbarCanContinueInBackground = false; + this._updateToolbarActions(); + } + public async toggleOutputFromAction(): Promise { this._userToggledOutput = true; diff --git a/src/vs/workbench/contrib/terminal/browser/terminal.ts b/src/vs/workbench/contrib/terminal/browser/terminal.ts index 593e875a60dab..0dcfa551ee8b9 100644 --- a/src/vs/workbench/contrib/terminal/browser/terminal.ts +++ b/src/vs/workbench/contrib/terminal/browser/terminal.ts @@ -123,10 +123,12 @@ export interface IAhpTerminalCommandSource extends IDisposable { export interface IChatTerminalToolProgressPart { readonly elementIndex: number; readonly contentIndex: number; + readonly terminalToolSessionId: string | undefined; focusTerminal(): Promise; toggleOutputFromKeyboard(): Promise; toggleOutputFromAction(): Promise; continueInBackground(): void; + markContinuedInBackground(): void; focusOutput(): void; getCommandAndOutputAsText(): string | undefined; } diff --git a/src/vs/workbench/contrib/terminalContrib/chat/browser/terminalChatService.ts b/src/vs/workbench/contrib/terminalContrib/chat/browser/terminalChatService.ts index 584d47815e780..33c5d43295d6e 100644 --- a/src/vs/workbench/contrib/terminalContrib/chat/browser/terminalChatService.ts +++ b/src/vs/workbench/contrib/terminalContrib/chat/browser/terminalChatService.ts @@ -467,6 +467,11 @@ export class TerminalChatService extends Disposable implements ITerminalChatServ continueInBackground(terminalToolSessionId: string): void { this._onDidContinueInBackground.fire(terminalToolSessionId); + for (const part of this._activeProgressParts) { + if (part.terminalToolSessionId === terminalToolSessionId) { + part.markContinuedInBackground(); + } + } } registerAhpCommandSource(terminalToolSessionId: string, source: IAhpTerminalCommandSource, promisedTerminal: Promise): IDisposable { diff --git a/src/vs/workbench/contrib/terminalContrib/chat/test/browser/terminalChatService.test.ts b/src/vs/workbench/contrib/terminalContrib/chat/test/browser/terminalChatService.test.ts index 7068955a9e67c..7ce2b83e61344 100644 --- a/src/vs/workbench/contrib/terminalContrib/chat/test/browser/terminalChatService.test.ts +++ b/src/vs/workbench/contrib/terminalContrib/chat/test/browser/terminalChatService.test.ts @@ -17,7 +17,7 @@ import { ILogService, NullLogService } from '../../../../../../platform/log/comm import { ITreeSitterLibraryService } from '../../../../../../editor/common/services/treeSitter/treeSitterLibraryService.js'; import { InMemoryStorageService, IStorageService } from '../../../../../../platform/storage/common/storage.js'; import { IChatService } from '../../../../chat/common/chatService/chatService.js'; -import { IAhpTerminalCommandSource, ITerminalInstance, ITerminalService } from '../../../../terminal/browser/terminal.js'; +import { IAhpTerminalCommandSource, IChatTerminalToolProgressPart, ITerminalInstance, ITerminalService } from '../../../../terminal/browser/terminal.js'; import { TerminalChatService } from '../../browser/terminalChatService.js'; /** @@ -106,6 +106,34 @@ suite('TerminalChatService', () => { assert.strictEqual(service.getToolSessionIdForInstance(instance), 'tool-session-a'); }); + test('continueInBackground notifies matching progress parts without per-part event listeners', () => { + const markedSessionIds: string[] = []; + for (let index = 0; index < 50; index++) { + const sessionId = `tool-session-${index}`; + store.add(service.registerProgressPart(new class extends mock() { + override readonly elementIndex = index; + override readonly contentIndex = 0; + override readonly terminalToolSessionId = sessionId; + + override markContinuedInBackground(): void { + markedSessionIds.push(sessionId); + } + }())); + } + const eventSessionIds: string[] = []; + store.add(service.onDidContinueInBackground(sessionId => eventSessionIds.push(sessionId))); + + service.continueInBackground('tool-session-25'); + + assert.deepStrictEqual({ + markedSessionIds, + eventSessionIds, + }, { + markedSessionIds: ['tool-session-25'], + eventSessionIds: ['tool-session-25'], + }); + }); + test('getTerminalInstanceByToolSessionId waits for pending AHP terminal creation', async () => { const pendingTerminal = new DeferredPromise(); const instance = { instanceId: 3 } as ITerminalInstance; From fc94d1dd7ef2ab54e46949142649686228c17c13 Mon Sep 17 00:00:00 2001 From: Dmitriy Vasyura Date: Mon, 24 Aug 2026 18:11:03 -0700 Subject: [PATCH 2/3] Add rendered terminal listener regression coverage Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../chatTerminalToolProgressPart.ts | 1 - .../chatTerminalToolProgressPart.test.ts | 199 +++++++++++++++++- .../test/browser/terminalChatService.test.ts | 17 +- 3 files changed, 206 insertions(+), 11 deletions(-) diff --git a/src/vs/workbench/contrib/chat/browser/widget/chatContentParts/toolInvocationParts/chatTerminalToolProgressPart.ts b/src/vs/workbench/contrib/chat/browser/widget/chatContentParts/toolInvocationParts/chatTerminalToolProgressPart.ts index c87d3197bf067..25310ee3d4cc9 100644 --- a/src/vs/workbench/contrib/chat/browser/widget/chatContentParts/toolInvocationParts/chatTerminalToolProgressPart.ts +++ b/src/vs/workbench/contrib/chat/browser/widget/chatContentParts/toolInvocationParts/chatTerminalToolProgressPart.ts @@ -442,7 +442,6 @@ export class ChatTerminalToolProgressPart extends BaseChatToolInvocationSubPart initializeTerminalActionsOnce(); }); - // Listen for continue in background — updates toolbar to auto-hide the action const terminalToolSessionId = this._terminalData.terminalToolSessionId; if (terminalToolSessionId) { if (this._terminalData.isPty === false) { diff --git a/src/vs/workbench/contrib/chat/test/browser/widget/chatContentParts/chatTerminalToolProgressPart.test.ts b/src/vs/workbench/contrib/chat/test/browser/widget/chatContentParts/chatTerminalToolProgressPart.test.ts index 03fedbf67d120..a9e0343a18bda 100644 --- a/src/vs/workbench/contrib/chat/test/browser/widget/chatContentParts/chatTerminalToolProgressPart.test.ts +++ b/src/vs/workbench/contrib/chat/test/browser/widget/chatContentParts/chatTerminalToolProgressPart.test.ts @@ -6,26 +6,221 @@ import assert from 'assert'; import type { Terminal } from '@xterm/xterm'; import { importAMDNodeModule } from '../../../../../../../amdX.js'; +import { renderAsPlaintext } from '../../../../../../../base/browser/markdownRenderer.js'; import { mainWindow } from '../../../../../../../base/browser/window.js'; import { Emitter, Event } from '../../../../../../../base/common/event.js'; import { observableValue } from '../../../../../../../base/common/observable.js'; import { URI } from '../../../../../../../base/common/uri.js'; import { toDisposable } from '../../../../../../../base/common/lifecycle.js'; +import { mock } from '../../../../../../../base/test/common/mock.js'; import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../../../../base/test/common/utils.js'; import { runWithFakedTimers } from '../../../../../../../base/test/common/timeTravelScheduler.js'; import { timeout } from '../../../../../../../base/common/async.js'; import { TestInstantiationService } from '../../../../../../../platform/instantiation/test/common/instantiationServiceMock.js'; import { IAccessibleViewService } from '../../../../../../../platform/accessibility/browser/accessibleView.js'; +import { IMarkdownRenderer } from '../../../../../../../platform/markdown/browser/markdownRenderer.js'; import { workbenchInstantiationService } from '../../../../../../test/browser/workbenchTestServices.js'; +import { IAiEditTelemetryService } from '../../../../../editTelemetry/browser/telemetry/aiEditTelemetry/aiEditTelemetryService.js'; +import { IChatOutputRendererService } from '../../../../browser/chatOutputItemRenderer.js'; +import { IChatMarkdownAnchorService } from '../../../../browser/widget/chatContentParts/chatMarkdownAnchorService.js'; import { IChatContentPartRenderContext, InlineTextModelCollection } from '../../../../browser/widget/chatContentParts/chatContentParts.js'; import { DiffEditorPool, EditorPool } from '../../../../browser/widget/chatContentParts/chatContentCodePools.js'; -import { ChatTerminalThinkingCollapsibleWrapper, ChatTerminalToolOutputSection } from '../../../../browser/widget/chatContentParts/toolInvocationParts/chatTerminalToolProgressPart.js'; +import { ChatTerminalThinkingCollapsibleWrapper, ChatTerminalToolOutputSection, ChatTerminalToolProgressPart } from '../../../../browser/widget/chatContentParts/toolInvocationParts/chatTerminalToolProgressPart.js'; +import { IChatSessionsService } from '../../../../common/chatSessionsService.js'; +import { IChatTerminalToolInvocationData, IChatToolInvocationSerialized, ToolConfirmKind } from '../../../../common/chatService/chatService.js'; import { IChatResponseViewModel } from '../../../../common/model/chatViewModel.js'; import { TerminalToolAutoExpand, TerminalToolAutoExpandTimeout } from '../../../../browser/widget/chatContentParts/toolInvocationParts/terminalToolAutoExpand.js'; -import { ITerminalConfigurationService, ITerminalService, type IDetachedXTermOptions } from '../../../../../terminal/browser/terminal.js'; +import { IChatTerminalToolProgressPart, ITerminalChatService, ITerminalConfigurationService, ITerminalInstance, ITerminalService, type IDetachedXTermOptions } from '../../../../../terminal/browser/terminal.js'; import type { ITerminalFont } from '../../../../../terminal/common/terminal.js'; import { createFakeDetachedTerminal } from '../../../../../terminal/test/browser/chatTerminalMirrorTestUtils.js'; +function listenerCount(emitter: Emitter): number { + return (emitter as unknown as { _size: number })._size ?? 0; +} + +class TestTerminalChatService extends mock() { + override readonly onDidRegisterTerminalInstanceWithToolSession = Event.None; + override readonly onDidRegisterOutputSource = Event.None; + override readonly onDidContinueInBackground: Event; + + private readonly progressParts = new Set(); + + constructor( + private readonly continueInBackgroundEmitter: Emitter, + private readonly terminalInstance: ITerminalInstance, + ) { + super(); + this.onDidContinueInBackground = continueInBackgroundEmitter.event; + } + + override async getTerminalInstanceByToolSessionId(_terminalToolSessionId: string): Promise { + return this.terminalInstance; + } + + override registerProgressPart(part: IChatTerminalToolProgressPart) { + this.progressParts.add(part); + return toDisposable(() => this.progressParts.delete(part)); + } + + override continueInBackground(terminalToolSessionId: string): void { + this.continueInBackgroundEmitter.fire(terminalToolSessionId); + for (const part of this.progressParts) { + if (part.terminalToolSessionId === terminalToolSessionId) { + part.markContinuedInBackground(); + } + } + } + + override isBackgroundTerminal(): boolean { + return false; + } + + override getOutputSource() { + return undefined; + } + + override getAhpCommandSource() { + return undefined; + } + + override setFocusedProgressPart(): void { } + override clearFocusedProgressPart(): void { } +} + +suite('ChatTerminalToolProgressPart listener ownership', () => { + const store = ensureNoDisposablesAreLeakedInTestSuite(); + + test('rendered parts do not accumulate continue listeners and duplicate rows update', async () => { + const instantiationService = workbenchInstantiationService(undefined, store); + const continueInBackgroundEmitter = store.add(new Emitter()); + const terminalInstance = new class extends mock() { + override readonly isDisposed = false; + override readonly onDisposed = Event.None; + override readonly onWillData = Event.None; + override readonly capabilities = { + get: () => undefined, + onDidAddCommandDetectionCapability: Event.None, + } as ITerminalInstance['capabilities']; + }(); + const terminalChatService = new TestTerminalChatService(continueInBackgroundEmitter, terminalInstance); + instantiationService.stub(ITerminalChatService, terminalChatService); + instantiationService.stub(ITerminalService, new class extends mock() { + override readonly whenConnected = Promise.resolve(); + }()); + instantiationService.stub(IAccessibleViewService, new class extends mock() { }()); + instantiationService.stub(IChatMarkdownAnchorService, { + _serviceBrand: undefined, + register: () => toDisposable(() => { }), + lastFocusedAnchor: undefined, + }); + instantiationService.stub(IAiEditTelemetryService, new class extends mock() { }()); + instantiationService.stub(IChatOutputRendererService, new class extends mock() { + override hasCodeBlockRenderer(): boolean { + return false; + } + }()); + instantiationService.stub(IChatSessionsService, new class extends mock() { }()); + + const markdownRenderer: IMarkdownRenderer = { + render: (markdown, _options, outElement) => { + const element = outElement ?? mainWindow.document.createElement('div'); + element.textContent = renderAsPlaintext(markdown); + return { element, dispose() { } }; + } + }; + const editorPool = Object.create(EditorPool.prototype) as EditorPool; + const host = mainWindow.document.createElement('div'); + mainWindow.document.body.appendChild(host); + store.add(toDisposable(() => host.remove())); + const eventSessionIds: string[] = []; + store.add(continueInBackgroundEmitter.event(sessionId => eventSessionIds.push(sessionId))); + const listenerCountBeforeRender = listenerCount(continueInBackgroundEmitter); + + const targetSessionId = 'terminal-session-target'; + const terminalData: IChatTerminalToolInvocationData[] = []; + const parts: ChatTerminalToolProgressPart[] = []; + for (let index = 0; index < 50; index++) { + const data: IChatTerminalToolInvocationData = { + kind: 'terminal', + commandLine: { original: `echo ${index}` }, + language: 'shellscript', + terminalToolSessionId: index === 24 || index === 25 ? targetSessionId : `terminal-session-${index}`, + }; + const invocation: IChatToolInvocationSerialized = { + presentation: undefined, + toolSpecificData: data, + invocationMessage: 'Running command', + originMessage: undefined, + pastTenseMessage: 'Ran command', + isConfirmed: { type: ToolConfirmKind.ConfirmationNotNeeded }, + isComplete: true, + toolCallId: `tool-call-${index}`, + toolId: 'run_in_terminal', + source: undefined, + kind: 'toolInvocationSerialized', + }; + const element = Object.assign(Object.create(null) as IChatResponseViewModel, { + id: `response-${index}`, + isComplete: true, + sessionResource: URI.parse('chat-session://test/session'), + setVote() { }, + get model() { return {} as IChatResponseViewModel['model']; }, + }); + const context: IChatContentPartRenderContext = { + element, + elementIndex: index, + container: host, + content: [invocation], + contentIndex: 0, + inlineTextModels: Object.create(InlineTextModelCollection.prototype) as InlineTextModelCollection, + editorPool, + codeBlockStartIndex: 0, + treeStartIndex: 0, + diffEditorPool: Object.create(DiffEditorPool.prototype) as DiffEditorPool, + currentWidth: observableValue('testWidth', 500), + onDidChangeVisibility: Event.None, + }; + const part = store.add(instantiationService.createInstance( + ChatTerminalToolProgressPart, + invocation, + data, + context, + markdownRenderer, + editorPool, + () => 500, + 0, + )); + host.appendChild(part.domNode); + terminalData.push(data); + parts.push(part); + } + await timeout(0); + + const listenerCountAfterRender = listenerCount(continueInBackgroundEmitter); + const actionCountsBefore = parts.map(part => part.domNode.querySelectorAll('.action-item').length); + parts[24].continueInBackground(); + const actionCountsAfter = parts.map(part => part.domNode.querySelectorAll('.action-item').length); + + assert.deepStrictEqual({ + renderedRows: parts.filter(part => part.domNode.isConnected).length, + listenerCounts: [listenerCountBeforeRender, listenerCountAfterRender], + actionCountsBefore: [...new Set(actionCountsBefore)], + continuedRows: terminalData.flatMap((data, index) => data.didContinueInBackground ? [index] : []), + matchingActionCountsAfter: [actionCountsAfter[24], actionCountsAfter[25]], + unmatchedActionCountAfter: actionCountsAfter[0], + eventSessionIds, + }, { + renderedRows: 50, + listenerCounts: [1, 1], + actionCountsBefore: [2], + continuedRows: [24, 25], + matchingActionCountsAfter: [1, 1], + unmatchedActionCountAfter: 2, + eventSessionIds: [targetSessionId], + }); + }); +}); + suite('ChatTerminalToolProgressPart Auto-Expand Logic', () => { const store = ensureNoDisposablesAreLeakedInTestSuite(); diff --git a/src/vs/workbench/contrib/terminalContrib/chat/test/browser/terminalChatService.test.ts b/src/vs/workbench/contrib/terminalContrib/chat/test/browser/terminalChatService.test.ts index 7ce2b83e61344..5ed938b4586ad 100644 --- a/src/vs/workbench/contrib/terminalContrib/chat/test/browser/terminalChatService.test.ts +++ b/src/vs/workbench/contrib/terminalContrib/chat/test/browser/terminalChatService.test.ts @@ -106,31 +106,32 @@ suite('TerminalChatService', () => { assert.strictEqual(service.getToolSessionIdForInstance(instance), 'tool-session-a'); }); - test('continueInBackground notifies matching progress parts without per-part event listeners', () => { - const markedSessionIds: string[] = []; + test('continueInBackground notifies every matching progress part', () => { + const markedPartIndices: number[] = []; + const targetSessionId = 'tool-session-target'; for (let index = 0; index < 50; index++) { - const sessionId = `tool-session-${index}`; + const sessionId = index === 25 || index === 26 ? targetSessionId : `tool-session-${index}`; store.add(service.registerProgressPart(new class extends mock() { override readonly elementIndex = index; override readonly contentIndex = 0; override readonly terminalToolSessionId = sessionId; override markContinuedInBackground(): void { - markedSessionIds.push(sessionId); + markedPartIndices.push(index); } }())); } const eventSessionIds: string[] = []; store.add(service.onDidContinueInBackground(sessionId => eventSessionIds.push(sessionId))); - service.continueInBackground('tool-session-25'); + service.continueInBackground(targetSessionId); assert.deepStrictEqual({ - markedSessionIds, + markedPartIndices, eventSessionIds, }, { - markedSessionIds: ['tool-session-25'], - eventSessionIds: ['tool-session-25'], + markedPartIndices: [25, 26], + eventSessionIds: [targetSessionId], }); }); From 4d60573ef0f565ac0afcc2c97c66eac68ee72ef5 Mon Sep 17 00:00:00 2001 From: Dmitriy Vasyura Date: Mon, 24 Aug 2026 20:44:11 -0700 Subject: [PATCH 3/3] Use real terminal capabilities in listener test Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../chatContentParts/chatTerminalToolProgressPart.test.ts | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/vs/workbench/contrib/chat/test/browser/widget/chatContentParts/chatTerminalToolProgressPart.test.ts b/src/vs/workbench/contrib/chat/test/browser/widget/chatContentParts/chatTerminalToolProgressPart.test.ts index a9e0343a18bda..bee11808a7e7f 100644 --- a/src/vs/workbench/contrib/chat/test/browser/widget/chatContentParts/chatTerminalToolProgressPart.test.ts +++ b/src/vs/workbench/contrib/chat/test/browser/widget/chatContentParts/chatTerminalToolProgressPart.test.ts @@ -19,6 +19,7 @@ import { timeout } from '../../../../../../../base/common/async.js'; import { TestInstantiationService } from '../../../../../../../platform/instantiation/test/common/instantiationServiceMock.js'; import { IAccessibleViewService } from '../../../../../../../platform/accessibility/browser/accessibleView.js'; import { IMarkdownRenderer } from '../../../../../../../platform/markdown/browser/markdownRenderer.js'; +import { TerminalCapabilityStore } from '../../../../../../../platform/terminal/common/capabilities/terminalCapabilityStore.js'; import { workbenchInstantiationService } from '../../../../../../test/browser/workbenchTestServices.js'; import { IAiEditTelemetryService } from '../../../../../editTelemetry/browser/telemetry/aiEditTelemetry/aiEditTelemetryService.js'; import { IChatOutputRendererService } from '../../../../browser/chatOutputItemRenderer.js'; @@ -93,14 +94,12 @@ suite('ChatTerminalToolProgressPart listener ownership', () => { test('rendered parts do not accumulate continue listeners and duplicate rows update', async () => { const instantiationService = workbenchInstantiationService(undefined, store); const continueInBackgroundEmitter = store.add(new Emitter()); + const capabilities = store.add(new TerminalCapabilityStore()); const terminalInstance = new class extends mock() { override readonly isDisposed = false; override readonly onDisposed = Event.None; override readonly onWillData = Event.None; - override readonly capabilities = { - get: () => undefined, - onDidAddCommandDetectionCapability: Event.None, - } as ITerminalInstance['capabilities']; + override readonly capabilities = capabilities; }(); const terminalChatService = new TestTerminalChatService(continueInBackgroundEmitter, terminalInstance); instantiationService.stub(ITerminalChatService, terminalChatService);