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

[vs-code-engineering]

🔧 Error Fix

Summary

Error: Cannot read properties of undefined (reading 'then') in terminalProcessManager.ts:608

Root cause: When the terminal process manager is disposed, ptyProcessReady is set to undefined! (line 583). However, setDimensions can still be called after disposal due to a race condition: a configuration change event fires during the terminal disposal sequence, triggering setVisible → _resize → resize → _resizeBothCallback → _updatePtyDimensions → setDimensions. The isDisposed guard in _updatePtyDimensions passes because the terminal instance isn't fully disposed yet, but the process manager's ptyProcessReady has already been nulled by its own disposal handler.

Fixes #315282
Recommended reviewer: @Tyriar

Culprit Commit

Field	Value
Commit	363a5254
PR	#314795
Author	@bryanchen-d
Message	Guard terminal resize/dispose race against xterm.js dimension getters

This PR added guards to terminalInstance.ts and terminalResizeDebouncer.ts for resize/dispose races, but missed guarding the downstream setDimensions call in terminalProcessManager.ts where ptyProcessReady can be undefined after disposal.

Code Flow

graph TD
    A["Configuration change fires during disposal"] --> B["setVisible()"]
    B --> C["_resize()"]
    C --> D["resizeDebouncer.resize()"]
    D --> E["_resizeBothCallback()"]
    E --> F["_updatePtyDimensions()"]
    F --> G["processManager.setDimensions(cols, rows, false)"]
    G --> H["this.ptyProcessReady.then() — CRASH: ptyProcessReady is undefined"]

Loading

Affected Files

• src/vs/workbench/contrib/terminal/browser/terminalProcessManager.ts

Repro Steps

1. Open a terminal in VS Code
2. Trigger a command that disposes the terminal (e.g., kill terminal action)
3. If a configuration change event arrives simultaneously (race condition), setDimensions is called on the already-disposed process manager
4. ptyProcessReady is undefined, calling .then() on it throws TypeError

How the Fix Works

Chosen approach (terminalProcessManager.ts:608): Added a guard clause if (!this.ptyProcessReady) return Promise.resolve() before calling .then() on ptyProcessReady. This follows the principle of adding a guard clause upstream of the crash site so the error path is never reached. The process manager is already disposed at this point, so skipping the resize is correct — there is no PTY to resize.

Alternatives considered: Adding a try/catch around the .then() call was rejected because it would hide the error from telemetry rather than preventing it at the source. Checking this._isDisposed would also work but is less direct — the undefined ptyProcessReady is the actual invariant being violated.

Recommended Owner

@Tyriar — primary terminal area owner, responsible for terminalProcessManager.ts

errors-fix-driver — cycle 1

Trigger: cron_changes_requested · Head: 6601c31c6b5abe060be207907bd3bdcc6f13c418 (6601c31)

Item	Action
Review on terminalProcessManager.ts:608 by @anthonykim1 (CHANGES_REQUESTED)	Replied with technical analysis — thread requires human judgment on whether guard vs. disposal ordering fix is preferred. Not resolving; deferring to assignee @bryanchen-d.
CI (29 checks)	All passing ✅
Merge state	Clean

Push: no — no code changes this cycle · Copilot rerequested: skipped (no push)

Ready gate: unresolved review thread with CHANGES_REQUESTED → not marking ready this cycle

Generated by errors-fix · ● 6.5M · ◷

Generated by errors-fix-driver · ● 3.8M · ◷

[Copilot]

Pull request overview

Fixes a terminal resize/dispose race in the workbench terminal process manager by preventing setDimensions from calling .then() on a disposed/cleared ptyProcessReady promise reference.

Changes:

• Add an early-return guard in TerminalProcessManager.setDimensions when ptyProcessReady has been cleared during disposal, avoiding a Cannot read properties of undefined (reading 'then') crash.

Show a summary per file

File	Description
src/vs/workbench/contrib/terminal/browser/terminalProcessManager.ts	Add guard in setDimensions to safely no-op when ptyProcessReady is unset during disposal.

Copilot's findings

• Files reviewed: 1/1 changed files
• Comments generated: 0

[anthonykim1]

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.

[vs-code-engineering]

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