microsoft / microsoft/vscode

fix: guard output channel model load against dispose-during-await race (fixes #328426)

Open
#328,432 0 comments 0 reactions 1 assignee Claimed by @sandy081 View on GitHub
agentic-workflows errors-fix
Dominant language
TypeScript
Stars
193k
Forks
42.4k
PR merge metrics
PR metrics pending

Description

### Summary

`AbstractFileOutputChannelModel.loadModel()` throws `TypeError: Cannot read properties of undefined (reading 'add')` at `outputChannelModel.ts:500` when the output channel model is disposed while `loadModel()` is awaiting `outputContentProvider.getContent()`. The disposal clears the `_register`ed `MutableDisposable`, so `this.modelDisposable.value` becomes `undefined` and the subsequent `.add(...)` calls crash. The 1.131.0 stable spike (~65.5x, 2 to 131 users) is the Agents/sessions window exercising this output-loading path more frequently; the underlying race is pre-existing.

Fixes microsoft/vscode\#328426
Recommended reviewer: `@sandy081`

### Culprit Commit

| Field | Value |
|-------|-------|
| Commit | [`bb4308eb`](https://github.com/microsoft/vscode/commit/bb4308eb6a75) |
| Author | `@sandy081` |
| PR | #237830 |
| Message | Support compound log - initial impl (#237830) |
| Why | Introduced the `loadModel()` body (Jan 2025) that sets `this.modelDisposable.value = new DisposableStore()` before an `await`, then dereferences `this.modelDisposable.value.add(...)` after the await with no guard against the instance being disposed during the await. This is not a recent regression -- the 1.131.0 spike comes from the sessions window exercising the path far more, not a code change. Reported here as the code introducer, not a recent-regression culprit. |

### Code Flow

```mermaid
sequenceDiagram
participant Caller as Output consumer
participant Model as AbstractFileOutputChannelModel
participant Store as MutableDisposable
participant CP as outputContentProvider

Caller->>Model: loadModel()
Model->>Store: value = new DisposableStore()
Model->>CP: await getContent()
Note over Model,Store: Root cause: model disposed during await,
MutableDisposable cleared, .value is undefined
CP-->>Model: content resolved
Model->>Store: this.modelDisposable.value.add(...)
Note over Model: TypeError: Cannot read properties of undefined (reading 'add')
```

### Affected Files

| File | Role | Evidence |
|------|------|----------|
| `src/vs/workbench/contrib/output/common/outputChannelModel.ts` | crash site | L500 (from stack) |
| `src/vs/workbench/contrib/output/common/outputChannelModel.ts` | root cause | L495-L500: `this.modelDisposable.value = new DisposableStore();` ... `await ...getContent();` ... `this.modelDisposable.value.add(...)` -- no disposed-check after the await; `MutableDisposable.value` returns `undefined` once disposed |

### Repro Steps

This is a timing-dependent disposal race:

1. Open an output channel whose content provider's `getContent()` resolves asynchronously (any file-backed output channel).
2. Trigger `loadModel()` (open the channel in the Output view / Agents window).
3. Before `getContent()` resolves, dispose the channel model (close the channel / switch the active session / close the window).
4. When `getContent()` resolves, `this.modelDisposable.value` is now `undefined` and `.add(...)` throws. Frequency increases in the sessions/Agents window where channels open and close rapidly.

### How the Fix Works

**Chosen approach** -- `src/vs/workbench/contrib/output/common/outputChannelModel.ts` -> `loadModel()`: capture the `DisposableStore` into a local `modelDisposable` when it is created, and immediately after the `await outputContentProvider.getContent()` check `modelDisposable.isDisposed`. If the store was disposed during the await (which happens when the model instance was disposed, since the `MutableDisposable` is `_register`ed), throw a `CancellationError` to abort the load cleanly through the existing `catch`/reject path. The remaining `.add(...)` calls use the captured local instead of re-reading `this.modelDisposable.value`, so they can never dereference `undefined`. This fixes the bug at the data producer -- the `loadModel` async body that leaves a stale/cleared store reference -- rather than guarding the crash site, and it does not swallow or hide the telemetry-fed error path.

**Alternatives considered**:
- Optional-chaining each `this.modelDisposable.value?.add(...)`: rejected -- it silently no-ops half the initialization when disposed, leaving `this.model` partially wired and watchers leaked, masking the race instead of aborting cleanly.
- Wrapping the body in a broader try/catch that returns a default: rejected -- hides the disposal race from the reject path and provides no clean abort semantics.

### Recommended Owner

`@sandy081` -- author of the `loadModel()` code (PR #237830) and owner of the output channel / sessions feature area; active in `microsoft/vscode` within the last 90 days and has write access.

> Generated by [errors-fix](https://github.com/microsoft/vscode-engineering/actions/runs/30644614992) · opus48 · 397.1 AIC · ⌖ 11.7 AIC · ⊞ 18.1K · [◷](https://github.com/search?q=repo%3Amicrosoft%2Fvscode+%22gh-aw-workflow-id%3A+errors-fix%22&type=pullrequests)

---

> [!NOTE]
> This was originally intended as a pull request, but the git push operation failed.
>
> **Original error:** The process '/usr/bin/git' failed with exit code 128
>
> **Workflow Run:** [View run details and download bundle artifact](https://github.com/microsoft/vscode-engineering/actions/runs/30644614992)
>
> The bundle file is available in the `agent` artifact in the workflow run linked above.

To create a pull request with the changes:

```sh
# Download the artifact from the workflow run
gh run download 30644614992 -n agent -D /tmp/agent-30644614992

# Fetch the bundle into a temporary ref, then update the local branch
git fetch /tmp/agent-30644614992/aw-microsoft-vscode-fix-output-model-dispose-race-328426.bundle refs/heads/fix/output-model-dispose-race-328426:refs/bundles/create-pr-fix-output-model-dispose-race-328426-8124caff51d9b6ab-7295bcb9
git update-ref refs/heads/fix/output-model-dispose-race-328426-8124caff51d9b6ab refs/bundles/create-pr-fix-output-model-dispose-race-328426-8124caff51d9b6ab-7295bcb9
git checkout fix/output-model-dispose-race-328426-8124caff51d9b6ab
# Ensure the working tree matches the updated branch
git reset --hard
# Remove the temporary bundle ref
git update-ref -d refs/bundles/create-pr-fix-output-model-dispose-race-328426-8124caff51d9b6ab-7295bcb9

# Push the branch to origin
git push https://github.com/bryanchen-d/vscode.git fix/output-model-dispose-race-328426-8124caff51d9b6ab

# Create the pull request
gh pr create --title 'fix: guard output channel model load against dispose-during-await race (fixes #328426)' --base main --head bryanchen-d:fix/output-model-dispose-race-328426-8124caff51d9b6ab --repo microsoft/vscode
```

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.