microsoft / microsoft/vscode

Multi-diff empty-side models are prematurely disposed by comment document references

Open
#336,548 0 comments 0 reactions 1 assignee Claimed by @hediet View on GitHub
Dominant language
TypeScript
Stars
193k
Forks
42.4k
PR merge metrics
PR metrics pending

Description

## Summary

The multi-diff editor can leave large blank regions because synthetic empty-side text models are disposed while their diff view models are still alive.

Comment-range prefetch resolves these models through the text-model resolver. When the resulting temporary document reference expires or is evicted, the resolver destroys the underlying model despite the multi-diff retaining its own independent ownership. Rendering then throws `Model is disposed!`; the virtualized item latches the failure while continuing to reserve its default 500-pixel height.

## Observed behavior

Investigated in the Agents window on Insiders build `b065ad9cd83dae266607baf77f10c1fbf0330fba`.

- In a session changes view containing 55 files, 51 original-side models were disposed; none of the modified-side models were disposed.
- The disposed originals were synthetic `inmemory://model/...` empty sides for added files.
- Their `DocumentDiffItemViewModel` owners and owning disposable stores were still alive.
- Re-rendering an affected item threw `Model is disposed!` from text-model attachment.
- Failed virtualized entries remained unbound but retained a 500-pixel size, producing the whitespace.

## Reproduction

1. Open session changes containing added files in the Agents multi-diff editor, with a comment-range provider enabled.
2. Allow comment-range prefetch to open the synthetic empty-side documents.
3. Let the temporary document references expire (three minutes), or load enough documents to trigger reference-cache eviction.
4. Scroll so affected entries need to acquire/rebind an editor template.
5. Observe missing diff contents and large blank regions; rendering fails with `Model is disposed!`.

The ownership conflict was also verified deterministically with an isolated diff-view-model probe: after its empty side was automatically opened through comment prefetch, releasing only that probe's temporary document-reference entry produced:

```text
text model disposed: true
multi-diff owner alive: true
multi-diff reference count: 1
multi-diff owning store disposed: false
```

The live reference-cache settings were `maxAge = 180000 ms` and `maxSize = 50`; count cleanup starts at 60 entries and evicts 10.

## Traced code path

1. [DocumentDiffItemViewModel](https://github.com/microsoft/vscode/blob/b065ad9cd83dae266607baf77f10c1fbf0330fba/src/vs/editor/browser/widget/multiDiffEditor/multiDiffEditorViewModel.ts#L195) creates a missing side directly with `IModelService.createModel('', null)` and puts the raw model in the diff view model's disposable store. It does not hold a resolver reference for that side.
2. [CommentService](https://github.com/microsoft/vscode/blob/b065ad9cd83dae266607baf77f10c1fbf0330fba/src/vs/workbench/contrib/comments/browser/commentService.ts#L239) prefetches comments on `onModelAdded`; it excludes `vscodeSourceControl`, but not `inmemory`.
3. [ExtHostComments.$provideCommentingRanges](https://github.com/microsoft/vscode/blob/b065ad9cd83dae266607baf77f10c1fbf0330fba/src/vs/workbench/api/common/extHostComments.ts#L200) calls `ensureDocumentData`, which can call `$tryOpenDocument`.
4. [MainThreadDocuments](https://github.com/microsoft/vscode/blob/b065ad9cd83dae266607baf77f10c1fbf0330fba/src/vs/workbench/api/browser/mainThreadDocuments.ts#L286) acquires a resolver reference and adds it to `BoundModelReferenceCollection`.
5. [BoundModelReferenceCollection](https://github.com/microsoft/vscode/blob/b065ad9cd83dae266607baf77f10c1fbf0330fba/src/vs/workbench/api/browser/mainThreadDocuments.ts#L28) releases references on timeout/count/size eviction.
6. [ResourceModelCollection.destroyReferencedObject](https://github.com/microsoft/vscode/blob/b065ad9cd83dae266607baf77f10c1fbf0330fba/src/vs/workbench/services/textmodelResolver/common/textModelResolverService.ts#L107) disposes the resource-model wrapper when its own reference count reaches zero. The multi-diff's independent count is not included.
7. [TextResourceEditorModel.dispose](https://github.com/microsoft/vscode/blob/b065ad9cd83dae266607baf77f10c1fbf0330fba/src/vs/workbench/common/editor/textResourceEditorModel.ts#L30) unconditionally calls `modelService.destroyModel(...)`, including for an existing model it did not create.
8. [ManagedVirtualizedItem.render](https://github.com/microsoft/vscode/blob/b065ad9cd83dae266607baf77f10c1fbf0330fba/src/vs/editor/browser/widget/multiDiffEditor/virtualizedItemManager.ts#L157) latches `_didRenderFail`, so subsequent renders are skipped while layout still reserves the item's height.

Captured runtime stacks confirmed both sides of this chain:

```text
Acquisition:
ExtHostComments.$provideCommentingRanges -> ExtHostDocuments.ensureDocumentData
-> RPC -> MainThreadDocuments.$tryOpenDocument -> _handleAsResourceInput
-> TextModelResolverService.createModelReference

Disposal:
ResourceModelCollection.destroyReferencedObject
-> TextResourceEditorModel.dispose -> ModelService.destroyModel
-> TextModel.dispose
```

## Expected behavior / fix direction

Synthetic diff sides should participate in the same shared lifetime as resolver clients. Releasing a comment/document reference must not destroy a model still needed by the multi-diff; conversely, closing the diff must not destroy a model while another shared reference remains.

A focused fix should give synthetic sides long-lived resolver references tied to the shared diff view model (not recycled editor templates), and avoid independently disposing their raw text models. Initialization/cancellation and standalone-editor behavior need to be accounted for. Missing-side resource identity should remain distinct from the synthetic backing model so additions/deletions retain the correct semantics.

Filtering comment prefetch would avoid this trigger but would not fix the ownership mismatch. Removing destruction from `TextResourceEditorModel` globally also needs care: existing resolver tests explicitly expect in-memory models to be disposed on last-reference release.

## Regression coverage

- Comment-reference timeout and count eviction while a diff remains alive.
- Scroll away/back and editor-template recycling after reference release.
- Both lifetime orders: comment reference released first; diff released first while another resolver reference remains.
- Disposal exactly once after the last shared owner releases.
- Cancellation/partial initialization without leaked references.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.