Merge pull request microsoft#187096 from microsoft/tyriar/187076

Fix several pty host lifecycle issues on remotes

Author: alexr00

src/vs/platform/terminal/electron-main/electronPtyHostStarter.ts

@@ -23,8 +23,8 @@ export class ElectronPtyHostStarter extends Disposable implements IPtyHostStarte
 
 	private utilityProcess: UtilityProcess | undefined = undefined;
 
-	private readonly _onBeforeWindowConnection = new Emitter<void>();
-	readonly onBeforeWindowConnection = this._onBeforeWindowConnection.event;
+	private readonly _onRequestConnection = new Emitter<void>();
+	readonly onRequestConnection = this._onRequestConnection.event;
 	private readonly _onWillShutdown = new Emitter<void>();
 	readonly onWillShutdown = this._onWillShutdown.event;
 
@@ -104,7 +104,7 @@ export class ElectronPtyHostStarter extends Disposable implements IPtyHostStarte
 	}
 
 	private _onWindowConnection(e: IpcMainEvent, nonce: string) {
-		this._onBeforeWindowConnection.fire();
+		this._onRequestConnection.fire();
 
 		const port = this.utilityProcess!.connect();
 

src/vs/platform/terminal/node/ptyHost.ts

@@ -14,7 +14,7 @@ export interface IPtyHostConnection {
 }
 
 export interface IPtyHostStarter extends IDisposable {
-	onBeforeWindowConnection?: Event<void>;
+	onRequestConnection?: Event<void>;
 	onWillShutdown?: Event<void>;
 
 	/**

src/vs/platform/terminal/node/ptyHostMain.ts

@@ -88,6 +88,7 @@ async function startPtyHost() {
 
 	// Clean up
 	process.once('exit', () => {
+		logService.trace('Pty host exiting');
 		logService.dispose();
 		heartbeatService.dispose();
 		ptyService.dispose();

src/vs/platform/terminal/node/ptyHostService.ts

@@ -5,10 +5,10 @@
 
 import { Emitter, Event } from 'vs/base/common/event';
 import { Disposable, toDisposable } from 'vs/base/common/lifecycle';
-import { IProcessEnvironment, OperatingSystem, isWindows } from 'vs/base/common/platform';
+import { IProcessEnvironment, OS, OperatingSystem, isWindows } from 'vs/base/common/platform';
 import { ProxyChannel } from 'vs/base/parts/ipc/common/ipc';
 import { IConfigurationService } from 'vs/platform/configuration/common/configuration';
-import { ILogService, ILoggerService } from 'vs/platform/log/common/log';
+import { ILogService, ILoggerService, LogLevel } from 'vs/platform/log/common/log';
 import { RemoteLoggerChannelClient } from 'vs/platform/log/common/logIpc';
 import { getResolvedShellEnv } from 'vs/platform/shell/node/shellEnv';
 import { IPtyHostProcessReplayEvent } from 'vs/platform/terminal/common/capabilities/capabilities';
@@ -19,6 +19,7 @@ import { IGetTerminalLayoutInfoArgs, IProcessDetails, ISetTerminalLayoutInfoArgs
 import { IPtyHostConnection, IPtyHostStarter } from 'vs/platform/terminal/node/ptyHost';
 import { detectAvailableProfiles } from 'vs/platform/terminal/node/terminalProfiles';
 import * as performance from 'vs/base/common/performance';
+import { getSystemShell } from 'vs/base/node/shell';
 
 enum Constants {
 	MaxRestarts = 5
@@ -43,6 +44,13 @@ export class PtyHostService extends Disposable implements IPtyService {
 		this._ensurePtyHost();
 		return this.__proxy!;
 	}
+	/**
+	 * Get the proxy if it exists, otherwise undefined. This is used when calls are not needed to be
+	 * passed through to the pty host if it has not yet been spawned.
+	 */
+	private get _optionalProxy(): IPtyService | undefined {
+		return this.__proxy;
+	}
 
 	private _ensurePtyHost() {
 		if (!this.__connection) {
@@ -102,12 +110,9 @@ export class PtyHostService extends Disposable implements IPtyService {
 		this._resolveVariablesRequestStore = this._register(new RequestStore(undefined, this._logService));
 		this._resolveVariablesRequestStore.onCreateRequest(this._onPtyHostRequestResolveVariables.fire, this._onPtyHostRequestResolveVariables);
 
-		// Force the pty host to start as the first window is starting if the starter has that
-		// capability
-		if (this._ptyHostStarter.onBeforeWindowConnection) {
-			Event.once(this._ptyHostStarter.onBeforeWindowConnection)(() => this._ensurePtyHost());
-		} else {
-			this._ensurePtyHost();
+		// Start the pty host when a window requests a connection, if the starter has that capability.
+		if (this._ptyHostStarter.onRequestConnection) {
+			Event.once(this._ptyHostStarter.onRequestConnection)(() => this._ensurePtyHost());
 		}
 
 		this._ptyHostStarter.onWillShutdown?.(() => this._wasQuitRequested = true);
@@ -118,7 +123,7 @@ export class PtyHostService extends Disposable implements IPtyService {
 	}
 
 	private async _refreshIgnoreProcessNames(): Promise<void> {
-		return this._proxy.refreshIgnoreProcessNames?.(this._ignoreProcessNames);
+		return this._optionalProxy?.refreshIgnoreProcessNames?.(this._ignoreProcessNames);
 	}
 
 	private async _resolveShellEnv(): Promise<typeof process.env> {
@@ -139,6 +144,11 @@ export class PtyHostService extends Disposable implements IPtyService {
 		const connection = this._ptyHostStarter.start();
 		const client = connection.client;
 
+		// Log a full stack trace which will tell the exact reason the pty host is starting up
+		if (this._logService.getLevel() === LogLevel.Trace) {
+			this._logService.trace('PtyHostService#_startPtyHost', new Error().stack?.replace(/^Error/, ''));
+		}
+
 		// Setup heartbeat service and trigger a heartbeat immediately to reset the timeouts
 		const heartbeatService = ProxyChannel.toService<IHeartbeatService>(client.getChannel(TerminalIpcChannels.Heartbeat));
 		heartbeatService.onBeat(() => this._handleHeartbeat());
@@ -224,13 +234,10 @@ export class PtyHostService extends Disposable implements IPtyService {
 		return this._proxy.listProcesses();
 	}
 	async getPerformanceMarks(): Promise<performance.PerformanceMark[]> {
-		if (!this.__proxy) {
-			return [];
-		}
-		return this._proxy.getPerformanceMarks();
+		return this._optionalProxy?.getPerformanceMarks() ?? [];
 	}
-	reduceConnectionGraceTime(): Promise<void> {
-		return this._proxy.reduceConnectionGraceTime();
+	async reduceConnectionGraceTime(): Promise<void> {
+		return this._optionalProxy?.reduceConnectionGraceTime();
 	}
 	start(id: number): Promise<ITerminalLaunchError | { injectedArgs: string[] } | undefined> {
 		return this._proxy.start(id);
@@ -280,7 +287,7 @@ export class PtyHostService extends Disposable implements IPtyService {
 	}
 
 	getDefaultSystemShell(osOverride?: OperatingSystem): Promise<string> {
-		return this._proxy.getDefaultSystemShell(osOverride);
+		return this._optionalProxy?.getDefaultSystemShell(osOverride) ?? getSystemShell(osOverride ?? OS, process.env);
 	}
 	async getProfiles(workspaceId: string, profiles: unknown, defaultProfile: unknown, includeDetectedProfiles: boolean = false): Promise<ITerminalProfile[]> {
 		const shellEnv = await this._resolveShellEnv();

src/vs/platform/terminal/node/terminalProcess.ts

@@ -13,7 +13,7 @@ import { IProcessEnvironment, isLinux, isMacintosh, isWindows } from 'vs/base/co
 import { URI } from 'vs/base/common/uri';
 import { Promises } from 'vs/base/node/pfs';
 import { localize } from 'vs/nls';
-import { ILogService } from 'vs/platform/log/common/log';
+import { ILogService, LogLevel } from 'vs/platform/log/common/log';
 import { IProductService } from 'vs/platform/product/common/productService';
 import { FlowControlConstants, IShellLaunchConfig, ITerminalChildProcess, ITerminalLaunchError, IProcessProperty, IProcessPropertyMap as IProcessPropertyMap, ProcessPropertyType, TerminalShellType, IProcessReadyEvent, ITerminalProcessOptions, PosixShellType, IProcessReadyWindowsPty } from 'vs/platform/terminal/common/terminal';
 import { ChildProcessMonitor } from 'vs/platform/terminal/node/childProcessMonitor';
@@ -353,6 +353,9 @@ export class TerminalProcess extends Disposable implements ITerminalChildProcess
 	// Allow any trailing data events to be sent before the exit event is sent.
 	// See https://github.com/Tyriar/node-pty/issues/72
 	private _queueProcessExit() {
+		if (this._logService.getLevel() === LogLevel.Trace) {
+			this._logService.trace('TerminalProcess#_queueProcessExit', new Error().stack?.replace(/^Error/, ''));
+		}
 		if (this._closeTimeout) {
 			clearTimeout(this._closeTimeout);
 		}
@@ -417,6 +420,9 @@ export class TerminalProcess extends Disposable implements ITerminalChildProcess
 	}
 
 	shutdown(immediate: boolean): void {
+		if (this._logService.getLevel() === LogLevel.Trace) {
+			this._logService.trace('TerminalProcess#shutdown', new Error().stack?.replace(/^Error/, ''));
+		}
 		// don't force immediate disposal of the terminal processes on Windows as an additional
 		// mitigation for https://github.com/microsoft/vscode/issues/71966 which causes the pty host
 		// to become unresponsive, disconnecting all terminals across all windows.

src/vs/workbench/contrib/terminal/browser/remoteTerminalBackend.ts

@@ -6,7 +6,7 @@
 import { DeferredPromise } from 'vs/base/common/async';
 import { Emitter } from 'vs/base/common/event';
 import { revive } from 'vs/base/common/marshalling';
-import { PerformanceMark } from 'vs/base/common/performance';
+import { PerformanceMark, mark } from 'vs/base/common/performance';
 import { IProcessEnvironment, OperatingSystem } from 'vs/base/common/platform';
 import { ICommandService } from 'vs/platform/commands/common/commands';
 import { IConfigurationService } from 'vs/platform/configuration/common/configuration';
@@ -324,22 +324,28 @@ class RemoteTerminalBackend extends BaseTerminalBackend implements ITerminalBack
 		// Revive processes if needed
 		const serializedState = this._storageService.get(TerminalStorageKeys.TerminalBufferState, StorageScope.WORKSPACE);
 		const parsed = this._deserializeTerminalState(serializedState);
-		if (parsed) {
-			try {
-				// Note that remote terminals do not get their environment re-resolved unlike in local terminals
-
-				await this._remoteTerminalChannel.reviveTerminalProcesses(parsed, Intl.DateTimeFormat().resolvedOptions().locale);
-				this._storageService.remove(TerminalStorageKeys.TerminalBufferState, StorageScope.WORKSPACE);
-				// If reviving processes, send the terminal layout info back to the pty host as it
-				// will not have been persisted on application exit
-				const layoutInfo = this._storageService.get(TerminalStorageKeys.TerminalLayoutInfo, StorageScope.WORKSPACE);
-				if (layoutInfo) {
-					await this._remoteTerminalChannel.setTerminalLayoutInfo(JSON.parse(layoutInfo));
-					this._storageService.remove(TerminalStorageKeys.TerminalLayoutInfo, StorageScope.WORKSPACE);
-				}
-			} catch {
-				// no-op
+		if (!parsed) {
+			return undefined;
+		}
+
+		try {
+			// Note that remote terminals do not get their environment re-resolved unlike in local terminals
+
+			mark('code/terminal/willReviveTerminalProcessesRemote');
+			await this._remoteTerminalChannel.reviveTerminalProcesses(parsed, Intl.DateTimeFormat().resolvedOptions().locale);
+			mark('code/terminal/didReviveTerminalProcessesRemote');
+			this._storageService.remove(TerminalStorageKeys.TerminalBufferState, StorageScope.WORKSPACE);
+			// If reviving processes, send the terminal layout info back to the pty host as it
+			// will not have been persisted on application exit
+			const layoutInfo = this._storageService.get(TerminalStorageKeys.TerminalLayoutInfo, StorageScope.WORKSPACE);
+			if (layoutInfo) {
+				mark('code/terminal/willSetTerminalLayoutInfoRemote');
+				await this._remoteTerminalChannel.setTerminalLayoutInfo(JSON.parse(layoutInfo));
+				mark('code/terminal/didSetTerminalLayoutInfoRemote');
+				this._storageService.remove(TerminalStorageKeys.TerminalLayoutInfo, StorageScope.WORKSPACE);
 			}
+		} catch (e: unknown) {
+			this._logService.warn('RemoteTerminalBackend#getTerminalLayoutInfo Error', e && typeof e === 'object' && 'message' in e ? e.message : e);
 		}
 
 		return this._remoteTerminalChannel.getTerminalLayoutInfo();

src/vs/workbench/contrib/terminal/browser/terminalInstance.ts

@@ -2080,24 +2080,33 @@ export class TerminalInstance extends Disposable implements ITerminalInstance {
 		this._refreshEnvironmentVariableInfoWidgetState(info);
 	}
 
-	private _refreshEnvironmentVariableInfoWidgetState(info?: IEnvironmentVariableInfo): void {
+	private async _refreshEnvironmentVariableInfoWidgetState(info?: IEnvironmentVariableInfo): Promise<void> {
 		// Check if the status should exist
 		if (!info) {
 			this.statusList.remove(TerminalStatus.RelaunchNeeded);
 			this.statusList.remove(TerminalStatus.EnvironmentVariableInfoChangesActive);
 			return;
 		}
 
-		// Recreate the process if the terminal has not yet been interacted with and it's not a
-		// special terminal (eg. extension terminal)
+		// Recreate the process seamlessly without informing the use if the following conditions are
+		// met.
 		if (
+			// The change requires a relaunch
 			info.requiresAction &&
+			// The feature is enabled
 			this._configHelper.config.environmentChangesRelaunch &&
+			// Has not been interacted with
 			!this._processManager.hasWrittenData &&
+			// Not a feature terminal or is a reconnecting task terminal (TODO: Need to explain the latter case)
 			(!this._shellLaunchConfig.isFeatureTerminal || (this.reconnectionProperties && this._configurationService.getValue(TaskSettingId.Reconnection) === true)) &&
-			!this._shellLaunchConfig.customPtyImplementation
-			&& !this._shellLaunchConfig.isExtensionOwnedTerminal &&
-			!this._shellLaunchConfig.attachPersistentProcess
+			// Not a custom pty
+			!this._shellLaunchConfig.customPtyImplementation &&
+			// Not an extension owned terminal
+			!this._shellLaunchConfig.isExtensionOwnedTerminal &&
+			// Not a reconnected or revived terminal
+			!this._shellLaunchConfig.attachPersistentProcess &&
+			// Not a Windows remote using ConPTY (#187084)
+			!(this._processManager.remoteAuthority && this._configHelper.config.windowsEnableConpty && (await this._processManager.getBackendOS()) === OperatingSystem.Windows)
 		) {
 			this.relaunch();
 			return;

src/vs/workbench/contrib/terminal/browser/terminalService.ts

@@ -422,7 +422,7 @@ export class TerminalService implements ITerminalService {
 	private _setConnected() {
 		this._connectionState = TerminalConnectionState.Connected;
 		this._onDidChangeConnectionState.fire();
-		this._logService.trace('Reconnected to terminals');
+		this._logService.trace('Pty host ready');
 	}
 
 	private async _reconnectToRemoteTerminals(): Promise<void> {
@@ -444,6 +444,8 @@ export class TerminalService implements ITerminalService {
 		// now that terminals have been restored,
 		// attach listeners to update remote when terminals are changed
 		this._attachProcessLayoutListeners();
+
+		this._logService.trace('Reconnected to remote terminals');
 	}
 
 	private async _reconnectToLocalTerminals(): Promise<void> {
@@ -462,6 +464,8 @@ export class TerminalService implements ITerminalService {
 		// now that terminals have been restored,
 		// attach listeners to update local state when terminals are changed
 		this._attachProcessLayoutListeners();
+
+		this._logService.trace('Reconnected to local terminals');
 	}
 
 	private _recreateTerminalGroups(layoutInfo?: ITerminalsLayoutInfo): Promise<ITerminalGroup[]> {