Fix crash from nulled _session in ChatModel.dispose()#309570
Merged
roblourens merged 3 commits intomainfrom Apr 13, 2026
Merged
Fix crash from nulled _session in ChatModel.dispose()#309570roblourens merged 3 commits intomainfrom
roblourens merged 3 commits intomainfrom
Conversation
Remove the _session = undefined line that breaks the back-reference from requests to the ChatModel during dispose. This causes a crash in chatListRenderer when the _onDidUpdateViewModel event fires and accesses template.currentElement.sessionResource on a stale item whose session has been nulled out. The _requests array is already emptied right after, so the back-references are broken when the requests are GC'd with the model. Fixes #309568 (Written by Copilot)
Contributor
Contributor
|
This PR will be automatically cherry-picked to |
Contributor
There was a problem hiding this comment.
Pull request overview
Adjusts ChatModel.dispose() to avoid a crash during chat session/model switching by no longer mutating request objects in a way that can break stale view-model references.
Changes:
- Simplifies
dispose()to only dispose per-request responses and fire_onDidDispose. - Removes the explicit
(r as any)._session = undefinedmutation onChatRequestModelinstances during disposal. - Removes post-
super.dispose()cleanup that previously cleared_requestsand nulled other heavy fields.
Show a summary per file
| File | Description |
|---|---|
| src/vs/workbench/contrib/chat/common/model/chatModel.ts | Modifies ChatModel.dispose() behavior to prevent crashes from stale request/view-model references during session switching. |
Copilot's findings
Comments suppressed due to low confidence (2)
src/vs/workbench/contrib/chat/common/model/chatModel.ts:2825
- The PR description/issue text says the
_requestsarray is still cleared immediately after breaking back-references, but this change removes that cleanup entirely. If the intent is only to stop nullingChatRequestModel._session, the rest of the disposal cleanup should likely remain; otherwise this is a larger behavioral change than described (and increases the risk of memory retention for disposed models).
override dispose() {
this._requests.forEach(r => r.response?.dispose());
this._onDidDispose.fire();
super.dispose();
src/vs/workbench/contrib/chat/common/model/chatModel.ts:2825
- This change fixes a crash that occurs when stale
ChatRequestModelinstances are still referenced after the owningChatModelis disposed. There doesn’t appear to be a unit test exercising “dispose model, then access request/sessionResource via an old view-model element” to prevent regressions; consider adding a regression test that disposes a model and then readsrequest.session.sessionResource(or the view-model getter) to ensure it never throws during session switching.
override dispose() {
this._requests.forEach(r => r.response?.dispose());
this._onDidDispose.fire();
super.dispose();
- Files reviewed: 1/1 changed files
- Comments generated: 1
Restore the _requests.length, dataSerializer, and _editingSession cleanup — these null out fields on the ChatModel itself and are safe. Only the (r as any)._session = undefined line needs to be removed since it breaks getters accessed by stale event listeners. (Written by Copilot)
benvillalobos
approved these changes
Apr 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ChatModel.dispose()sets_session = undefinedon each request to break back-references for GC. However, the_onDidUpdateViewModelevent listener inchatListRenderer.tsaccessestemplate.currentElement.sessionResource, which resolves throughthis._model.session.sessionResource. WhenupdateViewModelfires during a model switch,template.currentElementmay still reference an item from the old (now-disposed) view model, causing:Fix: Remove the
_session = undefinedline. The_requestsarray is already emptied immediately after (this._requests.length = 0), so the back-references are broken when the requests are GC'd along with the model.Fixes #309568
(Written by Copilot)