From d5747b31c42016bea5aace06b6b0fabfc0cce3e5 Mon Sep 17 00:00:00 2001 From: Alex Dima Date: Mon, 18 May 2026 01:06:58 +0200 Subject: [PATCH] fix: delete local sessions immediately and prevent re-persistence Local sessions were not removed from the UI when deleted, and reappeared after reload. Three issues were at play: 1. removeHistoryEntry deleted the persisted file but left the live ChatModel alive and unmarked, so getLiveSessionItems kept returning it and willDisposeModel re-saved it on shutdown. Fixed by adding an isDeleted flag on ChatModel, checked by shouldBeInHistory and shouldStoreSession. 2. LocalAgentsSessionsController fired a removed event on onDidDisposeSession but never removed the session from _items, so subsequent refresh cycles via resolveProvider re-added it. Fixed by deleting from _items in the dispose handler. 3. _refreshSessionCache only detected removal of AgentSessionAdapter instances, but committed local sessions could remain as LocalNewSession in the cache. Changed the filter to skip only _currentNewSession. --- .../browser/copilotChatSessionsProvider.ts | 2 +- .../localAgentSessionsController.ts | 3 + .../common/chatService/chatServiceImpl.ts | 9 ++- .../contrib/chat/common/model/chatModel.ts | 8 +++ .../localAgentSessionsController.test.ts | 68 +++++++++++++++++++ .../common/chatService/chatService.test.ts | 63 +++++++++++++++++ 6 files changed, 151 insertions(+), 2 deletions(-) diff --git a/src/vs/sessions/contrib/providers/copilotChatSessions/browser/copilotChatSessionsProvider.ts b/src/vs/sessions/contrib/providers/copilotChatSessions/browser/copilotChatSessionsProvider.ts index 422bd2823a7..85001efd765 100644 --- a/src/vs/sessions/contrib/providers/copilotChatSessions/browser/copilotChatSessionsProvider.ts +++ b/src/vs/sessions/contrib/providers/copilotChatSessions/browser/copilotChatSessionsProvider.ts @@ -2867,7 +2867,7 @@ export class CopilotChatSessionsProvider extends Disposable implements ISessions const removedData: ICopilotChatSession[] = []; for (const [key, adapter] of this._sessionCache) { - if (!currentKeys.has(key) && adapter instanceof AgentSessionAdapter) { + if (!currentKeys.has(key) && adapter !== this._currentNewSession) { removedData.push(adapter); cacheChanged = true; } diff --git a/src/vs/workbench/contrib/chat/browser/agentSessions/localAgentSessionsController.ts b/src/vs/workbench/contrib/chat/browser/agentSessions/localAgentSessionsController.ts index ac27bd668db..175b684ff0a 100644 --- a/src/vs/workbench/contrib/chat/browser/agentSessions/localAgentSessionsController.ts +++ b/src/vs/workbench/contrib/chat/browser/agentSessions/localAgentSessionsController.ts @@ -112,6 +112,9 @@ export class LocalAgentsSessionsController extends Disposable implements IChatSe const removedSessionResources = e.sessionResources.filter(resource => getChatSessionType(resource) === this.chatSessionType); if (removedSessionResources.length) { + for (const resource of removedSessionResources) { + this._items.delete(resource); + } this._onDidChangeChatSessionItems.fire({ removed: removedSessionResources }); } })); diff --git a/src/vs/workbench/contrib/chat/common/chatService/chatServiceImpl.ts b/src/vs/workbench/contrib/chat/common/chatService/chatServiceImpl.ts index e79df2b779f..53ed3c7f1eb 100644 --- a/src/vs/workbench/contrib/chat/common/chatService/chatServiceImpl.ts +++ b/src/vs/workbench/contrib/chat/common/chatService/chatServiceImpl.ts @@ -289,6 +289,9 @@ export class ChatService extends Disposable implements IChatService { * Only persist local sessions from chat that are not imported. */ private shouldStoreSession(session: ChatModel): boolean { + if (session.isDeleted) { + return false; + } if (!LocalChatSessionUri.parseLocalSessionId(session.sessionResource)) { return false; } @@ -437,11 +440,15 @@ export class ChatService extends Disposable implements IChatService { } private shouldBeInHistory(entry: ChatModel): boolean { - return !entry.isImported && !!LocalChatSessionUri.parseLocalSessionId(entry.sessionResource) && entry.initialLocation === ChatAgentLocation.Chat; + return !entry.isImported && !entry.isDeleted && !!LocalChatSessionUri.parseLocalSessionId(entry.sessionResource) && entry.initialLocation === ChatAgentLocation.Chat; } async removeHistoryEntry(sessionResource: URI): Promise { await this._chatSessionStore.deleteSession(this.toLocalSessionId(sessionResource)); + const model = this._sessionModels.get(sessionResource); + if (model) { + model.markDeleted(); + } this._onDidDisposeSession.fire({ sessionResources: [sessionResource], reason: 'cleared' }); } diff --git a/src/vs/workbench/contrib/chat/common/model/chatModel.ts b/src/vs/workbench/contrib/chat/common/model/chatModel.ts index a17a6538658..e10e17c4bc2 100644 --- a/src/vs/workbench/contrib/chat/common/model/chatModel.ts +++ b/src/vs/workbench/contrib/chat/common/model/chatModel.ts @@ -2310,6 +2310,14 @@ export class ChatModel extends Disposable implements IChatModel { return this._isImported; } + private _isDeleted = false; + get isDeleted(): boolean { + return this._isDeleted; + } + markDeleted(): void { + this._isDeleted = true; + } + private _customTitle: string | undefined; get customTitle(): string | undefined { return this._customTitle; diff --git a/src/vs/workbench/contrib/chat/test/browser/agentSessions/localAgentSessionsController.test.ts b/src/vs/workbench/contrib/chat/test/browser/agentSessions/localAgentSessionsController.test.ts index 25b6f0cb4fb..84996d098c8 100644 --- a/src/vs/workbench/contrib/chat/test/browser/agentSessions/localAgentSessionsController.test.ts +++ b/src/vs/workbench/contrib/chat/test/browser/agentSessions/localAgentSessionsController.test.ts @@ -702,5 +702,73 @@ suite('LocalAgentsSessionsController', () => { assert.strictEqual(changeEventCount, 0, 'onDidChangeChatSessionItems should NOT fire after model is removed'); }); }); + + test('should remove session from items and fire removed event on onDidDisposeSession', async () => { + return runWithFakedTimers({}, async () => { + const controller = createController(); + + const sessionResource = LocalChatSessionUri.forSession('dispose-session'); + const mockModel = createMockChatModel({ + sessionResource, + hasRequests: true + }); + + // Add the session and populate items + mockChatService.addSession(mockModel); + mockChatService.setLiveSessionItems([await chatModelToChatDetail(mockModel)]); + await controller.refresh(CancellationToken.None); + assert.strictEqual(controller.items.length, 1); + + // Listen for the removed event + const removedResources: URI[] = []; + disposables.add(controller.onDidChangeChatSessionItems(delta => { + if (delta.removed) { + removedResources.push(...delta.removed); + } + })); + + // Fire onDidDisposeSession (simulates removeHistoryEntry) + mockChatService.fireDidDisposeSession([sessionResource]); + + // Session should be removed from items immediately + assert.strictEqual(controller.items.length, 0, 'items should be empty after dispose'); + assert.strictEqual(removedResources.length, 1, 'removed event should fire'); + assert.strictEqual(removedResources[0].toString(), sessionResource.toString()); + + // Even if refresh is called again, the session should not reappear + // (because getLiveSessionItems would still return it, but shouldBeInHistory + // would filter it in the real ChatService — here we simulate by keeping + // liveSessionItems unchanged, but _items was already cleared) + }); + }); + + test('should not re-add disposed session to items on refresh', async () => { + return runWithFakedTimers({}, async () => { + const controller = createController(); + + const sessionResource = LocalChatSessionUri.forSession('disposed-refresh-session'); + const mockModel = createMockChatModel({ + sessionResource, + hasRequests: true + }); + + // Add the session and populate items + mockChatService.addSession(mockModel); + mockChatService.setLiveSessionItems([await chatModelToChatDetail(mockModel)]); + await controller.refresh(CancellationToken.None); + assert.strictEqual(controller.items.length, 1); + + // Dispose the session + mockChatService.fireDidDisposeSession([sessionResource]); + assert.strictEqual(controller.items.length, 0); + + // Clear live items (simulates isDeleted filtering in real ChatService) + mockChatService.setLiveSessionItems([]); + + // Refresh should not bring it back + await controller.refresh(CancellationToken.None); + assert.strictEqual(controller.items.length, 0, 'disposed session should not reappear after refresh'); + }); + }); }); }); diff --git a/src/vs/workbench/contrib/chat/test/common/chatService/chatService.test.ts b/src/vs/workbench/contrib/chat/test/common/chatService/chatService.test.ts index 31b67e5d80a..1611c7e226b 100644 --- a/src/vs/workbench/contrib/chat/test/common/chatService/chatService.test.ts +++ b/src/vs/workbench/contrib/chat/test/common/chatService/chatService.test.ts @@ -1817,6 +1817,69 @@ suite('ChatService', () => { // Clean up ref.dispose(); }); + + test('removeHistoryEntry marks model as deleted and excludes from getLiveSessionItems', async () => { + testDisposables.add(chatAgentService.registerAgentImplementation(chatAgentWithMarkdownId, chatAgentWithMarkdown)); + + const testService = createChatService(); + + // Create a session and send a message so it has requests + const ref = testDisposables.add(startSessionModel(testService)); + const model = ref.object; + const response = await testService.sendRequest(model.sessionResource, `@${chatAgentWithMarkdownId} test request`); + ChatSendResult.assertSent(response); + await response.data.responseCompletePromise; + assert.strictEqual(model.getRequests().length, 1); + + // Verify the session appears in live session items + const liveItemsBefore = await testService.getLiveSessionItems(); + assert.ok( + liveItemsBefore.some(item => item.sessionResource.toString() === model.sessionResource.toString()), + 'Session should appear in getLiveSessionItems before deletion' + ); + + // Delete the session + await testService.removeHistoryEntry(model.sessionResource); + + // Verify the session no longer appears in live session items + const liveItemsAfter = await testService.getLiveSessionItems(); + assert.ok( + !liveItemsAfter.some(item => item.sessionResource.toString() === model.sessionResource.toString()), + 'Session should NOT appear in getLiveSessionItems after deletion' + ); + + // Verify onDidDisposeSession was fired + // (model is still alive because ref holds it, but it's marked deleted) + assert.strictEqual((model as ChatModel).isDeleted, true); + }); + + test('removeHistoryEntry prevents re-saving on model disposal', async () => { + testDisposables.add(chatAgentService.registerAgentImplementation(chatAgentWithMarkdownId, chatAgentWithMarkdown)); + + const testService = createChatService(); + + // Create a session with a request + const ref = testDisposables.add(startSessionModel(testService)); + const model = ref.object; + const response = await testService.sendRequest(model.sessionResource, `@${chatAgentWithMarkdownId} test request`); + ChatSendResult.assertSent(response); + await response.data.responseCompletePromise; + + // Delete the history entry + await testService.removeHistoryEntry(model.sessionResource); + + // Release the model reference — this triggers willDisposeModel + ref.dispose(); + await testService.waitForModelDisposals(); + + // Verify the session does NOT reappear in history after disposal + const testService2 = createChatService(); + const historyItems = await testService2.getHistorySessionItems(); + assert.ok( + !historyItems.some(item => item.sessionResource.toString() === model.sessionResource.toString()), + 'Deleted session should NOT reappear in history after model disposal' + ); + }); });