fix: preserve chat session index during workspace transition#305109
fix: preserve chat session index during workspace transition#305109w1nsid wants to merge 1 commit intomicrosoft:mainfrom
Conversation
|
@microsoft-github-policy-service agree |
There was a problem hiding this comment.
Pull request overview
Fixes a Copilot Chat workspace-transition regression where chat session metadata (index: titles/timestamps/session IDs) can be lost when the workspace identity changes (e.g., Save Workspace As...), causing Show Chats to appear empty after reload even though session files exist on disk.
Changes:
- Preserve the in-memory chat session index during workspace migration so
flushIndex()writes the previous entries into the new workspace scope. - Add a regression test intended to validate session index preservation across a workspace transition.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/vs/workbench/contrib/chat/common/model/chatSessionStore.ts | Adjusts workspace migration to retain and flush the prior session index instead of regenerating an empty index in the new workspace scope. |
| src/vs/workbench/contrib/chat/test/common/model/chatSessionStore.test.ts | Adds a regression test for index preservation across a simulated workspace transition. |
51d9360 to
d01815d
Compare
|
@roblourens wanna take a look? |
roblourens
left a comment
There was a problem hiding this comment.
Good catch, this is reasonable. Could you
- Scope it down for just the case when the window is empty, reading from app storage, because the workspace case seems to work and is what we fixed earlier
- Open an issue to track this
- Check Copilot's comments
d76ed81 to
73b2d13
Compare
Thanks for taking a look @roblourens!
So my actual issue relates to a workspace→workspace transition : I was on Remote SSH with a single folder open, added another folder, then saved the workspace. That goes through The empty window case (APPLICATION scope) would survive the switch since it's global, but the workspace→workspace path is the one I actually hit and it wouldn't be covered by just reading from app storage. I switched the approach to hooking into
There's an existing one: #301793. Also linked #285242 which describes the same symptom.
The two new ones from the second review were about Disclosure: I used GitHub Copilot (chat + code review) throughout this PR — for tracing the root cause through the storage/workspace layers, drafting the fix, writing the test, and iterating on review feedback. |
When saving an untitled workspace (Save Workspace As...), the workspace identity changes and ChatSessionStore.migrateSessionsToNewWorkspace is called via onDidEnterWorkspace. The method copies .jsonl session files to the new storage location, but then clears indexCache and calls flushIndex(). At this point, storageService.switch() has already moved StorageScope.WORKSPACE to the new (empty) database. internalGetIndex() reads from the new scope, finds nothing, creates an empty index, and flushes that — losing all session metadata. After window reload (which always happens for remote connections), the session files exist on disk but the index is empty, so Show Chats returns nothing. The user sees all their chat history wiped out. Fix: capture the old index before clearing the cache and set it as the new indexCache so flushIndex() writes the preserved entries to the new workspace storage scope. Adds a regression test verifying the session index survives a workspace transition.
73b2d13 to
4aca4bf
Compare
| // Eagerly populate the index cache before the storage service | ||
| // switches workspace storage. storageService.switch() fires | ||
| // onWillSaveState before closing the old storage, so this | ||
| // ensures indexCache holds the old index when migration runs. | ||
| this._register(this.storageService.onWillSaveState(() => { | ||
| this.internalGetIndex(); | ||
| })); |
There was a problem hiding this comment.
onWillSaveState currently calls internalGetIndex(), but internalGetIndex() reads from getIndexStorageScope() when indexCache is undefined. In the real enterWorkspace() flow, the workspace context is updated during configurationService.initialize(...) (WorkspaceService.initialize updates this.workspace) before storageService.switch() emits onWillSaveState, so getIndexStorageScope() may already be WORKSPACE even when the old index lived in APPLICATION (empty-window → workspace). If indexCache wasn’t already populated, this will cache an empty WORKSPACE index and migration can still drop the old APPLICATION-scoped entries. Consider capturing the old index from an explicit scope (e.g. read+parse from StorageScope.APPLICATION when transitioning from an empty window, and from pre-switch workspace storage for workspace→workspace), instead of relying on getIndexStorageScope() inside the will-save handler.
| // Simulate the real storageService.switch() sequence: | ||
| // 1. onWillSaveState fires (populates indexCache in the store) | ||
| // 2. Storage scope changes from APPLICATION to WORKSPACE | ||
| // 3. onDidEnterWorkspace fires (triggers migration) | ||
| storageService.testEmitWillSaveState(WillSaveStateReason.NONE); | ||
|
|
||
| const contextService = instantiationService.get(IWorkspaceContextService) as TestContextService; | ||
| contextService.setWorkspace(TestWorkspace); | ||
|
|
There was a problem hiding this comment.
This test simulates onWillSaveState firing before the workspace context switches from empty-window → workspace, but in the actual enterWorkspace() sequence the context is updated during configurationService.initialize(...) before storageService.switch() emits onWillSaveState. With the real ordering, getIndexStorageScope() can already be WORKSPACE when the old index is in APPLICATION, so this test may miss regressions unless it reorders setWorkspace(...) before testEmitWillSaveState(...) and also covers the case where indexCache is undefined (e.g. clear it after storeSessions() to ensure the will-save path actually loads the index).
| // Simulate the real storageService.switch() sequence: | |
| // 1. onWillSaveState fires (populates indexCache in the store) | |
| // 2. Storage scope changes from APPLICATION to WORKSPACE | |
| // 3. onDidEnterWorkspace fires (triggers migration) | |
| storageService.testEmitWillSaveState(WillSaveStateReason.NONE); | |
| const contextService = instantiationService.get(IWorkspaceContextService) as TestContextService; | |
| contextService.setWorkspace(TestWorkspace); | |
| // Simulate the real enterWorkspace()/storageService.switch() sequence: | |
| // 1. Workspace context switches from empty-window to workspace (configurationService.initialize) | |
| // 2. storageService.switch() emits onWillSaveState while the index is still in APPLICATION scope | |
| // 3. onDidEnterWorkspace fires (triggers migration) | |
| (store as any).indexCache = undefined; | |
| const contextService = instantiationService.get(IWorkspaceContextService) as TestContextService; | |
| contextService.setWorkspace(TestWorkspace); | |
| storageService.testEmitWillSaveState(WillSaveStateReason.NONE); |
Bug
When the workspace identity changes (e.g. adding a folder to a single-folder Remote SSH session and saving as a
.code-workspacefile), all Copilot Chat history disappears from Show Chats. The.jsonlsession files are correctly copied to the new workspace storage folder, but the index that maps session IDs to titles/timestamps is lost — so after the window reloads (which always happens on Remote SSH), Show Chats returns nothing.Root Cause
In
ChatSessionStore.migrateSessionsToNewWorkspace():.jsonlfiles are copied from the old storage root to the new onethis.indexCache = undefinedclears the in-memory indexflushIndex()is called, which internally callsinternalGetIndex()storageService.switch()has already run (inenterWorkspace()beforefireDidEnterWorkspace), which closes the old workspace storage DB and creates a new empty oneStorageScope.WORKSPACEnow points to the new empty DB —internalGetIndex()reads nothing, creates an empty index, and flushes itThis affects both workspace→workspace transitions (the primary repro) and empty window→workspace transitions.
Fix
Register an
onWillSaveStatelistener that eagerly populatesindexCacheviainternalGetIndex(). The storage service firesonWillSaveStatebefore closing the old workspace storage inswitch(), so the cache captures the old index while the old DB is still open. WhenmigrateSessionsToNewWorkspaceruns afterward, it uses the cached index as the source of truth, then merges old entries into the new scope's index (preserving any entries that may already exist in the target).This approach works for all transition types because the cache is populated before the storage swap — regardless of whether the old scope was
APPLICATION(empty window) orWORKSPACE.Test
Added
session index is preserved after workspace transitionregression test that:onWillSaveStateto simulatestorageService.switch()populating the cacheTestContextServiceto a non-empty workspace (simulating the scope change)onDidEnterWorkspaceto trigger migrationChatSessionStoreto verify the index was persisted to the correctWORKSPACEscope (not just cached in memory)Repro steps (before fix)
.jsonlsession files exist in the newworkspaceStorage/folder but the index is emptyFixes #301793
Related: #285242