Skip to content

Subscribe to dispose session in local agents provider #281450

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.

Sign up for GitHub

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

Merged
osortega merged 1 commit into from
Dec 9, 2025
+47 −28
Merged

Subscribe to dispose session in local agents provider #281450

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/vs/workbench/api/browser/mainThreadChatAgents2.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,9 @@ export class MainThreadChatAgents2 extends Disposable implements MainThreadChatA
this._proxy = extHostContext.getProxy(ExtHostContext.ExtHostChatAgents2);

this._register(this._chatService.onDidDisposeSession(e => {
this._proxy.$releaseSession(e.sessionResource);
for (const resource of e.sessionResource) {
this._proxy.$releaseSession(resource);
}
}));
this._register(this._chatService.onDidPerformUserAction(e => {
if (typeof e.agentId === 'string') {
Expand Down
8 changes: 8 additions & 0 deletions src/vs/workbench/contrib/chat/browser/agentSessions/localAgentSessionsProvider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import { IWorkbenchContribution } from '../../../../common/contributions.js';
import { IChatModel } from '../../common/chatModel.js';
import { IChatDetail, IChatService } from '../../common/chatService.js';
import { ChatSessionStatus, IChatSessionItem, IChatSessionItemProvider, IChatSessionsService, localChatSessionType } from '../../common/chatSessionsService.js';
import { getChatSessionType } from '../../common/chatUri.js';
import { ChatSessionItemWithProvider } from '../chatSessions/common.js';

export class LocalAgentsSessionsProvider extends Disposable implements IChatSessionItemProvider, IWorkbenchContribution {
Expand Down Expand Up @@ -53,6 +54,13 @@ export class LocalAgentsSessionsProvider extends Disposable implements IChatSess
this._onDidChange.fire();
}
}));

this._register(this.chatService.onDidDisposeSession(e => {
const session = e.sessionResource.filter(resource => getChatSessionType(resource) === this.chatSessionType);
if (session.length > 0) {
this._onDidChangeChatSessionItems.fire();
}
}));
}


Expand Down
4 changes: 3 additions & 1 deletion src/vs/workbench/contrib/chat/browser/chatEditing/chatEditingServiceImpl.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,9 @@ export class ChatEditingService extends Disposable implements IChatEditingServic

this._register(this._chatService.onDidDisposeSession((e) => {
if (e.reason === 'cleared') {
this.getEditingSession(e.sessionResource)?.stop();
for (const resource of e.sessionResource) {
this.getEditingSession(resource)?.stop();
}
}
}));

Expand Down
2 changes: 1 addition & 1 deletion src/vs/workbench/contrib/chat/common/chatService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1058,7 +1058,7 @@ export interface IChatService {

readonly onDidPerformUserAction: Event<IChatUserActionEvent>;
notifyUserAction(event: IChatUserActionEvent): void;
readonly onDidDisposeSession: Event<{ readonly sessionResource: URI; readonly reason: 'cleared' }>;
readonly onDidDisposeSession: Event<{ readonly sessionResource: URI[]; readonly reason: 'cleared' }>;

transferChatSession(transferredSessionData: IChatTransferredSessionData, toWorkspace: URI): void;

Expand Down
5 changes: 3 additions & 2 deletions src/vs/workbench/contrib/chat/common/chatServiceImpl.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ export class ChatService extends Disposable implements IChatService {
private readonly _onDidPerformUserAction = this._register(new Emitter<IChatUserActionEvent>());
public readonly onDidPerformUserAction: Event<IChatUserActionEvent> = this._onDidPerformUserAction.event;

private readonly _onDidDisposeSession = this._register(new Emitter<{ readonly sessionResource: URI; reason: 'cleared' }>());
private readonly _onDidDisposeSession = this._register(new Emitter<{ readonly sessionResource: URI[]; reason: 'cleared' }>());
public readonly onDidDisposeSession = this._onDidDisposeSession.event;

private readonly _sessionFollowupCancelTokens = this._register(new DisposableResourceMap<CancellationTokenSource>());
Expand Down Expand Up @@ -159,7 +159,7 @@ export class ChatService extends Disposable implements IChatService {
}
}));
this._register(this._sessionModels.onDidDisposeModel(model => {
this._onDidDisposeSession.fire({ sessionResource: model.sessionResource, reason: 'cleared' });
this._onDidDisposeSession.fire({ sessionResource: [model.sessionResource], reason: 'cleared' });
}));

this._chatServiceTelemetry = this.instantiationService.createInstance(ChatServiceTelemetry);
Expand Down Expand Up @@ -443,6 +443,7 @@ export class ChatService extends Disposable implements IChatService {

async removeHistoryEntry(sessionResource: URI): Promise<void> {
await this._chatSessionStore.deleteSession(this.toLocalSessionId(sessionResource));
this._onDidDisposeSession.fire({ sessionResource: [sessionResource], reason: 'cleared' });
}

async clearAllHistoryEntries(): Promise<void> {
Expand Down
4 changes: 2 additions & 2 deletions src/vs/workbench/contrib/chat/test/browser/localAgentSessionsProvider.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,10 @@ class MockChatService implements IChatService {
private liveSessionItems: IChatDetail[] = [];
private historySessionItems: IChatDetail[] = [];

private readonly _onDidDisposeSession = new Emitter<{ sessionResource: URI; reason: 'cleared' }>();
private readonly _onDidDisposeSession = new Emitter<{ sessionResource: URI[]; reason: 'cleared' }>();
readonly onDidDisposeSession = this._onDidDisposeSession.event;

fireDidDisposeSession(sessionResource: URI): void {
fireDidDisposeSession(sessionResource: URI[]): void {
this._onDidDisposeSession.fire({ sessionResource, reason: 'cleared' });
}

Expand Down
6 changes: 4 additions & 2 deletions src/vs/workbench/contrib/chat/test/common/chatService.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -405,8 +405,10 @@ suite('ChatService', () => {

let disposed = false;
testDisposables.add(testService.onDidDisposeSession(e => {
if (e.sessionResource.toString() === model.sessionResource.toString()) {
disposed = true;
for (const resource of e.sessionResource) {
if (resource.toString() === model.sessionResource.toString()) {
disposed = true;
}
}
}));

Expand Down
2 changes: 1 addition & 1 deletion src/vs/workbench/contrib/chat/test/common/mockChatService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ export class MockChatService implements IChatService {
notifyUserAction(event: IChatUserActionEvent): void {
throw new Error('Method not implemented.');
}
readonly onDidDisposeSession: Event<{ sessionResource: URI; reason: 'cleared' }> = undefined!;
readonly onDidDisposeSession: Event<{ sessionResource: URI[]; reason: 'cleared' }> = undefined!;

transferChatSession(transferredSessionData: IChatTransferredSessionData, toWorkspace: URI): void {
throw new Error('Method not implemented.');
Expand Down
22 changes: 12 additions & 10 deletions src/vs/workbench/contrib/terminalContrib/chat/browser/terminalChatService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,17 +85,19 @@ export class TerminalChatService extends Disposable implements ITerminalChatServ
}));

this._register(this._chatService.onDidDisposeSession(e => {
if (LocalChatSessionUri.parseLocalSessionId(e.sessionResource) === terminalToolSessionId) {
this._terminalInstancesByToolSessionId.delete(terminalToolSessionId);
this._toolSessionIdByTerminalInstance.delete(instance);
this._terminalInstanceListenersByToolSessionId.deleteAndDispose(terminalToolSessionId);
// Clean up session auto approval state
const sessionId = LocalChatSessionUri.parseLocalSessionId(e.sessionResource);
if (sessionId) {
this._sessionAutoApprovalEnabled.delete(sessionId);
for (const resource of e.sessionResource) {
if (LocalChatSessionUri.parseLocalSessionId(resource) === terminalToolSessionId) {
this._terminalInstancesByToolSessionId.delete(terminalToolSessionId);
this._toolSessionIdByTerminalInstance.delete(instance);
this._terminalInstanceListenersByToolSessionId.deleteAndDispose(terminalToolSessionId);
// Clean up session auto approval state
const sessionId = LocalChatSessionUri.parseLocalSessionId(resource);
if (sessionId) {
this._sessionAutoApprovalEnabled.delete(sessionId);
}
this._persistToStorage();
this._updateHasToolTerminalContextKeys();
}
this._persistToStorage();
this._updateHasToolTerminalContextKeys();
}
}));
Comment on lines 87 to 102
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

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

This event handler registration should be moved outside of registerTerminalInstanceWithToolSession to avoid creating duplicate listeners. Each time this method is called, a new listener is registered on the service (via this._register), but they all remain active for the service's lifetime. This causes:

  1. Memory leak - handlers accumulate and are never disposed individually
  2. Duplicate cleanup - all handlers fire for every session disposal, causing redundant cleanup operations

The listener should be registered once in the constructor and check all relevant sessions/instances inside the handler. Alternatively, register the listener per-session and store it in _terminalInstanceListenersByToolSessionId so it can be properly disposed.

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@osortega osortega Dec 5, 2025

Choose a reason for hiding this comment

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

@copilot open a new pull request to apply changes based on this feedback


Expand Down
8 changes: 5 additions & 3 deletions src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/runInTerminalTool.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -335,9 +335,11 @@ export class RunInTerminalTool extends Disposable implements IToolImpl {

// Listen for chat session disposal to clean up associated terminals
this._register(this._chatService.onDidDisposeSession(e => {
const localSessionId = LocalChatSessionUri.parseLocalSessionId(e.sessionResource);
if (localSessionId) {
this._cleanupSessionTerminals(localSessionId);
for (const resource of e.sessionResource) {
const localSessionId = LocalChatSessionUri.parseLocalSessionId(resource);
if (localSessionId) {
this._cleanupSessionTerminals(localSessionId);
}
}
}));
}
Expand Down
10 changes: 5 additions & 5 deletions ...ch/contrib/terminalContrib/chatAgentTools/test/electron-browser/runInTerminalTool.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ suite('RunInTerminalTool', () => {
let storageService: IStorageService;
let workspaceContextService: TestContextService;
let terminalServiceDisposeEmitter: Emitter<ITerminalInstance>;
let chatServiceDisposeEmitter: Emitter<{ sessionResource: URI; reason: 'cleared' }>;
let chatServiceDisposeEmitter: Emitter<{ sessionResource: URI[]; reason: 'cleared' }>;

let runInTerminalTool: TestRunInTerminalTool;

Expand All @@ -75,7 +75,7 @@ suite('RunInTerminalTool', () => {

setConfig(TerminalChatAgentToolsSettingId.EnableAutoApprove, true);
terminalServiceDisposeEmitter = new Emitter<ITerminalInstance>();
chatServiceDisposeEmitter = new Emitter<{ sessionResource: URI; reason: 'cleared' }>();
chatServiceDisposeEmitter = new Emitter<{ sessionResource: URI[]; reason: 'cleared' }>();

instantiationService = workbenchInstantiationService({
configurationService: () => configurationService,
Expand Down Expand Up @@ -936,7 +936,7 @@ suite('RunInTerminalTool', () => {

ok(runInTerminalTool.sessionTerminalAssociations.has(sessionId), 'Terminal association should exist before disposal');

chatServiceDisposeEmitter.fire({ sessionResource: LocalChatSessionUri.forSession(sessionId), reason: 'cleared' });
chatServiceDisposeEmitter.fire({ sessionResource: [LocalChatSessionUri.forSession(sessionId)], reason: 'cleared' });

strictEqual(terminalDisposed, true, 'Terminal should have been disposed');
ok(!runInTerminalTool.sessionTerminalAssociations.has(sessionId), 'Terminal association should be removed after disposal');
Expand Down Expand Up @@ -973,7 +973,7 @@ suite('RunInTerminalTool', () => {
ok(runInTerminalTool.sessionTerminalAssociations.has(sessionId1), 'Session 1 terminal association should exist');
ok(runInTerminalTool.sessionTerminalAssociations.has(sessionId2), 'Session 2 terminal association should exist');

chatServiceDisposeEmitter.fire({ sessionResource: LocalChatSessionUri.forSession(sessionId1), reason: 'cleared' });
chatServiceDisposeEmitter.fire({ sessionResource: [LocalChatSessionUri.forSession(sessionId1)], reason: 'cleared' });

strictEqual(terminal1Disposed, true, 'Terminal 1 should have been disposed');
strictEqual(terminal2Disposed, false, 'Terminal 2 should NOT have been disposed');
Expand All @@ -983,7 +983,7 @@ suite('RunInTerminalTool', () => {

test('should handle disposal of non-existent session gracefully', () => {
strictEqual(runInTerminalTool.sessionTerminalAssociations.size, 0, 'No associations should exist initially');
chatServiceDisposeEmitter.fire({ sessionResource: LocalChatSessionUri.forSession('non-existent-session'), reason: 'cleared' });
chatServiceDisposeEmitter.fire({ sessionResource: [LocalChatSessionUri.forSession('non-existent-session')], reason: 'cleared' });
strictEqual(runInTerminalTool.sessionTerminalAssociations.size, 0, 'No associations should exist after handling non-existent session');
});
});
Expand Down
Loading