Skip to content

fix: guard setDimensions against disposed ptyProcessReady (fixes #315282) #315284

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
vs-code-engineering wants to merge 1 commit into
base: main
Choose a base branch
from
+3 −0
Open

fix: guard setDimensions against disposed ptyProcessReady (fixes #315282) #315284

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/vs/workbench/contrib/terminal/browser/terminalProcessManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -605,6 +605,9 @@ export class TerminalProcessManager extends Disposable implements ITerminalProce
return;
}

if (!this.ptyProcessReady) {
Copy link
Copy Markdown
Contributor

@anthonykim1 anthonykim1 May 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this new error from your recently merged PR?
Please, it would be good to have repro step and have you confirm that this resolves it.

This reads like startup state and silently resolving resize could hide real lifecycle bug.
Maybe there is something going on in debouncer with updatePtyDimension, and we should make sure that doesn't get invoked after disposable.

Copy link
Copy Markdown
Contributor Author

@vs-code-engineering vs-code-engineering Bot May 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review. To clarify the race condition:

  1. The TerminalResizeDebouncer already guards against disposal (this._store.isDisposed check at line 36 of terminalResizeDebouncer.ts), and _updatePtyDimensions in terminalInstance.ts guards with this.isDisposed (line 2059).
  2. However, these guards check the terminal instance's disposal state, not the process manager's. The race occurs because the process manager's ptyProcessReady is nulled (line 582–584) during its own disposal handler, while the terminal instance hasn't fully disposed yet — so both the debouncer and _updatePtyDimensions guards pass.
  3. Fixing this in the debouncer wouldn't help because the debouncer belongs to the terminal instance, not the process manager — it can't know the process manager's internal state.

That said, the reviewer's concern about whether this masks a deeper lifecycle ordering issue is valid and warrants input from @bryanchen-d (the assignee and author of the culprit PR #314795). Deferring to humans on whether a broader fix to the disposal ordering is preferred over this defensive guard.

Generated by errors-fix-driver · ● 3.8M

Copy link
Copy Markdown
Contributor

@bryanchen-d bryanchen-d May 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an error from a different path, similar to the xterm issue but stack is different.
Also the race condition is hard to repro from what I am trying. In terms of my understanding of the sequence, in a brief mental model, it is like:

this._processManager.dispose();   // nulls ptyProcessReady
this._onProcessExit(undefined);
this._onDisposed.fire(this);      // listeners may call setVisible → _resize → setDimensions -> ERROR thrown
super.dispose();                   // only NOW does isDisposed flip

Based on that, if we want to fix the sequencing here, we may want to dispose the debouncher before calling processManager.dispose() so it won't lead to the error.

return Promise.resolve();
}
return this.ptyProcessReady.then(() => this._resize(cols, rows, pixelWidth, pixelHeight));
}

Expand Down
Loading