From 5b52b1dc4970fff45b60ee90884abb34b289807b Mon Sep 17 00:00:00 2001 From: David Barbet Date: Tue, 1 Sep 2026 16:21:42 -0700 Subject: [PATCH 1/4] Distinguish external SIGKILL termination from language server crashes The crash toast told every user that the language server "has crashed" and offered Report Issue, even when the process was killed by something outside it. On macOS and in containers this is common under memory pressure (see #9708), and no crash dump can ever be produced for it, so the message sent users down the wrong path. SIGKILL cannot be caught, blocked, or ignored, and the .NET runtime never raises it on itself - fatal CLR errors go through abort() and surface as SIGABRT. So a SIGKILL is always an external actor: an OOM killer, macOS Jetsam, a container memory limit, or kill -9. Report that case with its likely causes instead. The base client clears its own process reference before invoking the close handler, so hold onto it in handleConnectionClosed. The connection close is also observed a few milliseconds before the process is reaped, so read signalCode when it has already exited and otherwise wait briefly for the exit event. Upgrades vscode-languageclient from 10.0.0-next.20 to 10.1.1 for the public serverProcess accessor. npm hoists vscode-languageserver-protocol out of the nested folder that tsconfig paths pointed at, so retarget those two mappings. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- l10n/bundle.l10n.json | 1 + package-lock.json | 181 ++++++------------ package.json | 4 +- .../server/roslynLanguageClient.ts | 91 ++++++++- .../unitTests/roslynLanguageClient.test.ts | 127 +++++++++++- tsconfig.json | 4 +- 6 files changed, 271 insertions(+), 137 deletions(-) diff --git a/l10n/bundle.l10n.json b/l10n/bundle.l10n.json index 27e7d2c6ae..24e9399e4b 100644 --- a/l10n/bundle.l10n.json +++ b/l10n/bundle.l10n.json @@ -108,6 +108,7 @@ "Detected change in telemetry settings. These will not take effect until the language server is restarted, would you like to restart?": "Detected change in telemetry settings. These will not take effect until the language server is restarted, would you like to restart?", "Go to output": "Go to output", "Report Issue": "Report Issue", + "The C# language server was terminated by the operating system or another program rather than crashing. This is usually caused by running out of memory, a container memory limit, or another process stopping it. Restart extensions to re-enable C# functionality.": "The C# language server was terminated by the operating system or another program rather than crashing. This is usually caused by running out of memory, a container memory limit, or another process stopping it. Restart extensions to re-enable C# functionality.", "The C# language server has crashed. Restart extensions to re-enable C# functionality.": "The C# language server has crashed. Restart extensions to re-enable C# functionality.", "Restart extensions": "Restart extensions", "Restore {0}": "Restore {0}", diff --git a/package-lock.json b/package-lock.json index 09e0d6762d..3ac059de38 100644 --- a/package-lock.json +++ b/package-lock.json @@ -23,7 +23,7 @@ "rxjs": "6.6.7", "semver": "7.5.4", "vscode-html-languageservice": "^5.3.1", - "vscode-languageclient": "10.0.0-next.20", + "vscode-languageclient": "10.1.1", "yauzl": "3.2.1" }, "devDependencies": { @@ -1482,31 +1482,6 @@ "copilot-language-server": "dist/language-server.js" } }, - "node_modules/@github/copilot-language-server/node_modules/vscode-jsonrpc": { - "version": "8.2.0", - "resolved": "https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-public-npm/npm/registry/vscode-jsonrpc/-/vscode-jsonrpc-8.2.0.tgz", - "integrity": "sha1-9D36NftR52PRfNlNzKDJRY81q/k=", - "license": "MIT", - "engines": { - "node": ">=14.0.0" - } - }, - "node_modules/@github/copilot-language-server/node_modules/vscode-languageserver-protocol": { - "version": "3.17.5", - "resolved": "https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-public-npm/npm/registry/vscode-languageserver-protocol/-/vscode-languageserver-protocol-3.17.5.tgz", - "integrity": "sha1-hkqLjzkINVcvThO9n4MT0OOsS+o=", - "license": "MIT", - "dependencies": { - "vscode-jsonrpc": "8.2.0", - "vscode-languageserver-types": "3.17.5" - } - }, - "node_modules/@github/copilot-language-server/node_modules/vscode-languageserver-types": { - "version": "3.17.5", - "resolved": "https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-public-npm/npm/registry/vscode-languageserver-types/-/vscode-languageserver-types-3.17.5.tgz", - "integrity": "sha1-MnNnbwzy6rQLP0TQhay7fwijnYo=", - "license": "MIT" - }, "node_modules/@humanfs/core": { "version": "0.19.2", "resolved": "https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-public-npm/npm/registry/@humanfs/core/-/core-0.19.2.tgz", @@ -11379,30 +11354,25 @@ "vscode-uri": "^3.0.8" } }, - "node_modules/vscode-html-languageservice/node_modules/vscode-languageserver-types": { - "version": "3.17.5", - "resolved": "https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-public-npm/npm/registry/vscode-languageserver-types/-/vscode-languageserver-types-3.17.5.tgz", - "integrity": "sha1-MnNnbwzy6rQLP0TQhay7fwijnYo=", - "license": "MIT" - }, "node_modules/vscode-jsonrpc": { - "version": "9.0.0-next.11", - "resolved": "https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-public-npm/npm/registry/vscode-jsonrpc/-/vscode-jsonrpc-9.0.0-next.11.tgz", - "integrity": "sha1-6Sxurk6NCVDTD6hy9RX76cr18Pg=", + "version": "9.0.2", + "resolved": "https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-public-npm/npm/registry/vscode-jsonrpc/-/vscode-jsonrpc-9.0.2.tgz", + "integrity": "sha1-CGt2xPlPGwdD3urE4LV8nEpV6B0=", "license": "MIT", "engines": { "node": ">=14.0.0" } }, "node_modules/vscode-languageclient": { - "version": "10.0.0-next.20", - "resolved": "https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-public-npm/npm/registry/vscode-languageclient/-/vscode-languageclient-10.0.0-next.20.tgz", - "integrity": "sha1-6Mm7cctE4+gImdRLPv/C1T+und0=", + "version": "10.1.1", + "resolved": "https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-public-npm/npm/registry/vscode-languageclient/-/vscode-languageclient-10.1.1.tgz", + "integrity": "sha1-zVBsX5AQ8D5GAEiH8WR8lEVMtiI=", "license": "MIT", "dependencies": { - "minimatch": "^10.1.2", - "semver": "^7.7.1", - "vscode-languageserver-protocol": "3.17.6-next.16" + "minimatch": "^10.2.6", + "semver": "^7.8.1", + "vscode-languageserver-protocol": "3.18.3", + "vscode-languageserver-textdocument": "1.0.14" }, "engines": { "vscode": "^1.91.0" @@ -11430,12 +11400,12 @@ } }, "node_modules/vscode-languageclient/node_modules/minimatch": { - "version": "10.2.4", - "resolved": "https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-public-npm/npm/registry/minimatch/-/minimatch-10.2.4.tgz", - "integrity": "sha1-Rls6zL0CGLgoH1MB4nztxpf5b94=", + "version": "10.2.6", + "resolved": "https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-public-npm/npm/registry/minimatch/-/minimatch-10.2.6.tgz", + "integrity": "sha1-/ZVrvgt3JB6fFaxdzLHGOAYJaO8=", "license": "BlueOak-1.0.0", "dependencies": { - "brace-expansion": "^5.0.2" + "brace-expansion": "^5.0.8" }, "engines": { "node": "18 || 20 || >=22" @@ -11445,9 +11415,9 @@ } }, "node_modules/vscode-languageclient/node_modules/semver": { - "version": "7.7.1", - "resolved": "https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-public-npm/npm/registry/semver/-/semver-7.7.1.tgz", - "integrity": "sha1-q9UJjYKxjGyB9gdP8mR/0+ciDJ8=", + "version": "7.8.5", + "resolved": "https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-public-npm/npm/registry/semver/-/semver-7.8.5.tgz", + "integrity": "sha1-ObZGA33VDBT7RR5+TKxY7YuGP2k=", "license": "ISC", "bin": { "semver": "bin/semver.js" @@ -11456,26 +11426,26 @@ "node": ">=10" } }, - "node_modules/vscode-languageclient/node_modules/vscode-languageserver-protocol": { - "version": "3.17.6-next.16", - "resolved": "https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-public-npm/npm/registry/vscode-languageserver-protocol/-/vscode-languageserver-protocol-3.17.6-next.16.tgz", - "integrity": "sha1-Sr22BlL3wUkWQgdieXxKtp460+Y=", + "node_modules/vscode-languageserver-protocol": { + "version": "3.18.3", + "resolved": "https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-public-npm/npm/registry/vscode-languageserver-protocol/-/vscode-languageserver-protocol-3.18.3.tgz", + "integrity": "sha1-Y5OggGD7lPVSkkP6O+BWxnY5ghE=", "license": "MIT", "dependencies": { - "vscode-jsonrpc": "9.0.0-next.11", - "vscode-languageserver-types": "3.17.6-next.6" + "vscode-jsonrpc": "9.0.2", + "vscode-languageserver-types": "3.18.3" } }, "node_modules/vscode-languageserver-textdocument": { - "version": "1.0.12", - "resolved": "https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-public-npm/npm/registry/vscode-languageserver-textdocument/-/vscode-languageserver-textdocument-1.0.12.tgz", - "integrity": "sha1-RX7gQnGrOJmKCTxowjQvU/bkpjE=", + "version": "1.0.14", + "resolved": "https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-public-npm/npm/registry/vscode-languageserver-textdocument/-/vscode-languageserver-textdocument-1.0.14.tgz", + "integrity": "sha1-dzG4CLoakH41YNXkMXvNFLr4Gc4=", "license": "MIT" }, "node_modules/vscode-languageserver-types": { - "version": "3.17.6-next.6", - "resolved": "https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-public-npm/npm/registry/vscode-languageserver-types/-/vscode-languageserver-types-3.17.6-next.6.tgz", - "integrity": "sha1-ZD7DWlJ+qbMmxnB1UfZtj+qz/q8=", + "version": "3.18.3", + "resolved": "https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-public-npm/npm/registry/vscode-languageserver-types/-/vscode-languageserver-types-3.18.3.tgz", + "integrity": "sha1-6ylsQv1lk3WoKVG5zDPnZyup7+c=", "license": "MIT" }, "node_modules/vscode-oniguruma": { @@ -12648,27 +12618,6 @@ "integrity": "sha1-TK953sM6hRbXD7IR2JioKvPMlgI=", "requires": { "vscode-languageserver-protocol": "^3.17.5" - }, - "dependencies": { - "vscode-jsonrpc": { - "version": "8.2.0", - "resolved": "https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-public-npm/npm/registry/vscode-jsonrpc/-/vscode-jsonrpc-8.2.0.tgz", - "integrity": "sha1-9D36NftR52PRfNlNzKDJRY81q/k=" - }, - "vscode-languageserver-protocol": { - "version": "3.17.5", - "resolved": "https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-public-npm/npm/registry/vscode-languageserver-protocol/-/vscode-languageserver-protocol-3.17.5.tgz", - "integrity": "sha1-hkqLjzkINVcvThO9n4MT0OOsS+o=", - "requires": { - "vscode-jsonrpc": "8.2.0", - "vscode-languageserver-types": "3.17.5" - } - }, - "vscode-languageserver-types": { - "version": "3.17.5", - "resolved": "https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-public-npm/npm/registry/vscode-languageserver-types/-/vscode-languageserver-types-3.17.5.tgz", - "integrity": "sha1-MnNnbwzy6rQLP0TQhay7fwijnYo=" - } } }, "@humanfs/core": { @@ -19676,28 +19625,22 @@ "vscode-languageserver-textdocument": "^1.0.12", "vscode-languageserver-types": "^3.17.5", "vscode-uri": "^3.0.8" - }, - "dependencies": { - "vscode-languageserver-types": { - "version": "3.17.5", - "resolved": "https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-public-npm/npm/registry/vscode-languageserver-types/-/vscode-languageserver-types-3.17.5.tgz", - "integrity": "sha1-MnNnbwzy6rQLP0TQhay7fwijnYo=" - } } }, "vscode-jsonrpc": { - "version": "9.0.0-next.11", - "resolved": "https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-public-npm/npm/registry/vscode-jsonrpc/-/vscode-jsonrpc-9.0.0-next.11.tgz", - "integrity": "sha1-6Sxurk6NCVDTD6hy9RX76cr18Pg=" + "version": "9.0.2", + "resolved": "https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-public-npm/npm/registry/vscode-jsonrpc/-/vscode-jsonrpc-9.0.2.tgz", + "integrity": "sha1-CGt2xPlPGwdD3urE4LV8nEpV6B0=" }, "vscode-languageclient": { - "version": "10.0.0-next.20", - "resolved": "https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-public-npm/npm/registry/vscode-languageclient/-/vscode-languageclient-10.0.0-next.20.tgz", - "integrity": "sha1-6Mm7cctE4+gImdRLPv/C1T+und0=", + "version": "10.1.1", + "resolved": "https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-public-npm/npm/registry/vscode-languageclient/-/vscode-languageclient-10.1.1.tgz", + "integrity": "sha1-zVBsX5AQ8D5GAEiH8WR8lEVMtiI=", "requires": { - "minimatch": "^10.1.2", - "semver": "^7.7.1", - "vscode-languageserver-protocol": "3.17.6-next.16" + "minimatch": "^10.2.6", + "semver": "^7.8.1", + "vscode-languageserver-protocol": "3.18.3", + "vscode-languageserver-textdocument": "1.0.14" }, "dependencies": { "balanced-match": { @@ -19714,38 +19657,38 @@ } }, "minimatch": { - "version": "10.2.4", - "resolved": "https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-public-npm/npm/registry/minimatch/-/minimatch-10.2.4.tgz", - "integrity": "sha1-Rls6zL0CGLgoH1MB4nztxpf5b94=", + "version": "10.2.6", + "resolved": "https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-public-npm/npm/registry/minimatch/-/minimatch-10.2.6.tgz", + "integrity": "sha1-/ZVrvgt3JB6fFaxdzLHGOAYJaO8=", "requires": { - "brace-expansion": "^5.0.2" + "brace-expansion": "^5.0.8" } }, "semver": { - "version": "7.7.1", - "resolved": "https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-public-npm/npm/registry/semver/-/semver-7.7.1.tgz", - "integrity": "sha1-q9UJjYKxjGyB9gdP8mR/0+ciDJ8=" - }, - "vscode-languageserver-protocol": { - "version": "3.17.6-next.16", - "resolved": "https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-public-npm/npm/registry/vscode-languageserver-protocol/-/vscode-languageserver-protocol-3.17.6-next.16.tgz", - "integrity": "sha1-Sr22BlL3wUkWQgdieXxKtp460+Y=", - "requires": { - "vscode-jsonrpc": "9.0.0-next.11", - "vscode-languageserver-types": "3.17.6-next.6" - } + "version": "7.8.5", + "resolved": "https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-public-npm/npm/registry/semver/-/semver-7.8.5.tgz", + "integrity": "sha1-ObZGA33VDBT7RR5+TKxY7YuGP2k=" } } }, + "vscode-languageserver-protocol": { + "version": "3.18.3", + "resolved": "https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-public-npm/npm/registry/vscode-languageserver-protocol/-/vscode-languageserver-protocol-3.18.3.tgz", + "integrity": "sha1-Y5OggGD7lPVSkkP6O+BWxnY5ghE=", + "requires": { + "vscode-jsonrpc": "9.0.2", + "vscode-languageserver-types": "3.18.3" + } + }, "vscode-languageserver-textdocument": { - "version": "1.0.12", - "resolved": "https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-public-npm/npm/registry/vscode-languageserver-textdocument/-/vscode-languageserver-textdocument-1.0.12.tgz", - "integrity": "sha1-RX7gQnGrOJmKCTxowjQvU/bkpjE=" + "version": "1.0.14", + "resolved": "https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-public-npm/npm/registry/vscode-languageserver-textdocument/-/vscode-languageserver-textdocument-1.0.14.tgz", + "integrity": "sha1-dzG4CLoakH41YNXkMXvNFLr4Gc4=" }, "vscode-languageserver-types": { - "version": "3.17.6-next.6", - "resolved": "https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-public-npm/npm/registry/vscode-languageserver-types/-/vscode-languageserver-types-3.17.6-next.6.tgz", - "integrity": "sha1-ZD7DWlJ+qbMmxnB1UfZtj+qz/q8=" + "version": "3.18.3", + "resolved": "https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-public-npm/npm/registry/vscode-languageserver-types/-/vscode-languageserver-types-3.18.3.tgz", + "integrity": "sha1-6ylsQv1lk3WoKVG5zDPnZyup7+c=" }, "vscode-oniguruma": { "version": "1.7.0", diff --git a/package.json b/package.json index 1ad14b6476..fd0754c387 100644 --- a/package.json +++ b/package.json @@ -123,7 +123,7 @@ "rxjs": "6.6.7", "semver": "7.5.4", "vscode-html-languageservice": "^5.3.1", - "vscode-languageclient": "10.0.0-next.20", + "vscode-languageclient": "10.1.1", "yauzl": "3.2.1" }, "devDependencies": { @@ -5780,4 +5780,4 @@ } } } -} \ No newline at end of file +} diff --git a/src/lsptoolshost/server/roslynLanguageClient.ts b/src/lsptoolshost/server/roslynLanguageClient.ts index d063636fea..5a4aaa8e65 100644 --- a/src/lsptoolshost/server/roslynLanguageClient.ts +++ b/src/lsptoolshost/server/roslynLanguageClient.ts @@ -4,6 +4,7 @@ *--------------------------------------------------------------------------------------------*/ import * as vscode from 'vscode'; +import { ChildProcess } from 'child_process'; import { LanguageClient, ServerOptions } from 'vscode-languageclient/node'; import { CancellationToken, ErrorHandler, LanguageClientOptions, MessageSignature, State } from 'vscode-languageclient'; import CompositeDisposable from '../../compositeDisposable'; @@ -14,6 +15,26 @@ import { showErrorMessageWithOptions } from '../../shared/observers/utils/showMe import { ITelemetryReporter } from '../../shared/telemetryReporter'; import { TelemetryEventNames } from '../../shared/telemetryEventNames'; +/** + * How long to wait for the server process to be reaped after the connection closes. + * The connection closing and the process exiting are separate events, and the exit normally follows + * within a few milliseconds. This only elapses if the process is still alive (for example a protocol + * error), in which case we report the generic crash message. + */ +const serverExitTimeoutMs = 1000; + +/** + * SIGKILL cannot be caught, blocked, or ignored, and the .NET runtime never raises it on itself - + * fatal CLR errors (unhandled exceptions, stack overflow, Environment.FailFast) go through abort() + * and surface as SIGABRT instead. So SIGKILL means the OS or another program terminated the server: + * a Linux OOM killer, macOS Jetsam, a container memory limit, or `kill -9`. + * + * Other externally-originated signals such as SIGTERM are deliberately not treated this way. VS Code + * and our own teardown can send them during an intentional shutdown, so reporting them as an external + * kill risks blaming the user's environment for a normal stop. + */ +const externalTerminationSignal: NodeJS.Signals = 'SIGKILL'; + /** * Implementation of the base LanguageClient type that allows for additional items to be disposed of * when the base LanguageClient instance is disposed. @@ -30,6 +51,13 @@ export class RoslynLanguageClient extends LanguageClient { */ private _hasShownConnectionClose = false; + /** + * The server process from the current session, retained so we can report how it ended. + * The base client clears its own reference before invoking the close handler, so reading + * `serverProcess` at crash time is too late. + */ + private _closedServerProcess: ChildProcess | undefined; + constructor( id: string, name: string, @@ -61,6 +89,13 @@ export class RoslynLanguageClient extends LanguageClient { return super.dispose(timeout); } + protected override async handleConnectionClosed(): Promise { + // The base implementation drops its reference to the process before the close handler runs, + // so grab it here while it is still available. + this._closedServerProcess = this.serverProcess; + return super.handleConnectionClosed(); + } + override handleFailedRequest( type: MessageSignature, token: CancellationToken | undefined, @@ -161,15 +196,36 @@ export class RoslynLanguageClient extends LanguageClient { return; } + // Set the guard before awaiting so the error and closed handlers cannot both get past it. this._hasShownConnectionClose = true; - this._telemetryReporter.sendTelemetryEvent(TelemetryEventNames.ServerCrash); - this.showCrashNotificationCore(); + // The close handler has already cleared serverProcess, so fall back to the process we kept. + void this.showCrashNotificationAsync(this.serverProcess ?? this._closedServerProcess); } - private showCrashNotificationCore() { + private async showCrashNotificationAsync(serverProcess: ChildProcess | undefined): Promise { + const signal = await getExitSignal(serverProcess); + const externallyTerminated = signal === externalTerminationSignal; + + this._telemetryReporter.sendTelemetryEvent(TelemetryEventNames.ServerCrash, { + signal: signal ?? '', + externallyTerminated: externallyTerminated.toString(), + }); + + this.showCrashNotificationCore(externallyTerminated); + } + + private showCrashNotificationCore(externallyTerminated: boolean) { showErrorMessageWithOptions( vscode, - vscode.l10n.t('The C# language server has crashed. Restart extensions to re-enable C# functionality.'), + externallyTerminated + ? // The server did not fail on its own, so leading with a crash report or a dump would + // send the user down the wrong path. Name the likely external cause instead. + vscode.l10n.t( + 'The C# language server was terminated by the operating system or another program rather than crashing. This is usually caused by running out of memory, a container memory limit, or another process stopping it. Restart extensions to re-enable C# functionality.' + ) + : vscode.l10n.t( + 'The C# language server has crashed. Restart extensions to re-enable C# functionality.' + ), { modal: false }, { title: vscode.l10n.t('Restart extensions'), @@ -180,9 +236,34 @@ export class RoslynLanguageClient extends LanguageClient { action: async () => { vscode.commands.executeCommand('csharp.reportIssue'); // Re-show the notification so the user can still restart extensions after reporting. - this.showCrashNotificationCore(); + this.showCrashNotificationCore(externallyTerminated); }, } ); } } + +/** + * Reports the signal that killed the server process, or null if it was not killed by one. + * The connection close is observed a few milliseconds before the process is reaped, so wait briefly + * for the exit instead of concluding that no signal was involved. + */ +async function getExitSignal(serverProcess: ChildProcess | undefined): Promise { + if (serverProcess === undefined) { + return null; + } + + // Node populates these once the process is reaped. If it already exited then 'exit' has fired and + // will not fire again for a listener added now, so read the result directly. + if (serverProcess.exitCode !== null || serverProcess.signalCode !== null) { + return serverProcess.signalCode; + } + + return new Promise((resolve) => { + const timeout = setTimeout(() => resolve(null), serverExitTimeoutMs); + serverProcess.once('exit', (_code, signal) => { + clearTimeout(timeout); + resolve(signal); + }); + }); +} diff --git a/test/lsptoolshost/unitTests/roslynLanguageClient.test.ts b/test/lsptoolshost/unitTests/roslynLanguageClient.test.ts index 1dd4262ba9..5b2328a6f5 100644 --- a/test/lsptoolshost/unitTests/roslynLanguageClient.test.ts +++ b/test/lsptoolshost/unitTests/roslynLanguageClient.test.ts @@ -4,6 +4,8 @@ *--------------------------------------------------------------------------------------------*/ import { describe, expect, jest, test } from '@jest/globals'; +import { ChildProcess } from 'child_process'; +import { EventEmitter } from 'events'; jest.mock('vscode-languageclient/node', () => ({ LanguageClient: class {}, @@ -23,21 +25,128 @@ jest.mock('vscode-languageclient', () => ({ import { RoslynLanguageClient } from '../../../src/lsptoolshost/server/roslynLanguageClient'; import { TelemetryEventNames } from '../../../src/shared/telemetryEventNames'; -describe('RoslynLanguageClient', () => { - test('shows one crash notification and emits one telemetry event per crash', () => { - const sendTelemetryEvent = jest.fn(); - const showCrashNotificationCore = jest.fn(); - const client = Object.create(RoslynLanguageClient.prototype) as any; +/** A stand-in for an already reaped process, which is what we normally observe on a crash. */ +function exitedProcess(exitCode: number | null, signalCode: NodeJS.Signals | null): ChildProcess { + return { exitCode, signalCode } as ChildProcess; +} + +/** A stand-in for a process that has not been reaped yet, so the signal arrives via 'exit'. */ +function runningProcess(): ChildProcess { + const serverProcess = new EventEmitter() as unknown as ChildProcess; + (serverProcess as { exitCode: number | null }).exitCode = null; + (serverProcess as { signalCode: NodeJS.Signals | null }).signalCode = null; + return serverProcess; +} + +function createClient(serverProcess?: ChildProcess) { + const sendTelemetryEvent = jest.fn(); + const showCrashNotificationCore = jest.fn(); + const client = Object.create(RoslynLanguageClient.prototype) as any; + + client._hasShownConnectionClose = false; + client._telemetryReporter = { sendTelemetryEvent }; + client._closedServerProcess = serverProcess; + client.showCrashNotificationCore = showCrashNotificationCore; + + return { client, sendTelemetryEvent, showCrashNotificationCore }; +} - client._hasShownConnectionClose = false; - client._telemetryReporter = { sendTelemetryEvent }; - client.showCrashNotificationCore = showCrashNotificationCore; +/** Lets the fire-and-forget notification promise chain settle. */ +async function flushPendingNotifications(): Promise { + for (let i = 0; i < 5; i++) { + await Promise.resolve(); + } +} +describe('RoslynLanguageClient', () => { + test('shows one crash notification and emits one telemetry event per crash', async () => { + const { client, sendTelemetryEvent, showCrashNotificationCore } = createClient(); + + // Both the error and closed handlers fire when the server goes down. client.showCrashNotification(); client.showCrashNotification(); + await flushPendingNotifications(); expect(sendTelemetryEvent).toHaveBeenCalledTimes(1); - expect(sendTelemetryEvent).toHaveBeenCalledWith(TelemetryEventNames.ServerCrash); expect(showCrashNotificationCore).toHaveBeenCalledTimes(1); }); + + test('reports an external termination when the process was killed with SIGKILL', async () => { + const { client, sendTelemetryEvent, showCrashNotificationCore } = createClient(exitedProcess(null, 'SIGKILL')); + + client.showCrashNotification(); + await flushPendingNotifications(); + + expect(sendTelemetryEvent).toHaveBeenCalledWith(TelemetryEventNames.ServerCrash, { + signal: 'SIGKILL', + externallyTerminated: 'true', + }); + expect(showCrashNotificationCore).toHaveBeenCalledWith(true); + }); + + // The .NET runtime ends fatal errors with abort(), so SIGABRT is the server genuinely failing. + test('reports a crash when the runtime aborted the process', async () => { + const { client, sendTelemetryEvent, showCrashNotificationCore } = createClient(exitedProcess(null, 'SIGABRT')); + + client.showCrashNotification(); + await flushPendingNotifications(); + + expect(sendTelemetryEvent).toHaveBeenCalledWith(TelemetryEventNames.ServerCrash, { + signal: 'SIGABRT', + externallyTerminated: 'false', + }); + expect(showCrashNotificationCore).toHaveBeenCalledWith(false); + }); + + test('reports a crash when the process exited with a code rather than a signal', async () => { + const { client, sendTelemetryEvent, showCrashNotificationCore } = createClient(exitedProcess(1, null)); + + client.showCrashNotification(); + await flushPendingNotifications(); + + expect(sendTelemetryEvent).toHaveBeenCalledWith(TelemetryEventNames.ServerCrash, { + signal: '', + externallyTerminated: 'false', + }); + expect(showCrashNotificationCore).toHaveBeenCalledWith(false); + }); + + test('reports a crash when there is no process to inspect', async () => { + const { client, showCrashNotificationCore } = createClient(); + + client.showCrashNotification(); + await flushPendingNotifications(); + + expect(showCrashNotificationCore).toHaveBeenCalledWith(false); + }); + + test('waits for the exit event when the process has not been reaped yet', async () => { + const serverProcess = runningProcess(); + const { client, showCrashNotificationCore } = createClient(serverProcess); + + client.showCrashNotification(); + await flushPendingNotifications(); + + // The connection closes slightly before the process is reaped, so nothing is reported yet. + expect(showCrashNotificationCore).not.toHaveBeenCalled(); + + serverProcess.emit('exit', null, 'SIGKILL'); + await flushPendingNotifications(); + + expect(showCrashNotificationCore).toHaveBeenCalledWith(true); + }); + + test('falls back to the generic crash message when the process outlives the connection', async () => { + jest.useFakeTimers(); + try { + const { client, showCrashNotificationCore } = createClient(runningProcess()); + + client.showCrashNotification(); + await jest.advanceTimersByTimeAsync(1000); + + expect(showCrashNotificationCore).toHaveBeenCalledWith(false); + } finally { + jest.useRealTimers(); + } + }); }); diff --git a/tsconfig.json b/tsconfig.json index 9cf7928aac..5f8670eb10 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -22,8 +22,8 @@ "vscode-languageclient/selectionRange": ["./node_modules/vscode-languageclient/lib/common/selectionRange"], "vscode-languageclient/protocolConverter": ["./node_modules/vscode-languageclient/lib/common/protocolConverter"], "vscode-languageclient/node": ["./node_modules/vscode-languageclient/lib/node/main"], - "vscode-languageserver-protocol": ["./node_modules/vscode-languageclient/node_modules/vscode-languageserver-protocol/lib/common/api"], - "vscode-languageserver-protocol/node": ["./node_modules/vscode-languageclient/node_modules/vscode-languageserver-protocol/lib/node/main"], + "vscode-languageserver-protocol": ["./node_modules/vscode-languageserver-protocol/lib/common/api"], + "vscode-languageserver-protocol/node": ["./node_modules/vscode-languageserver-protocol/lib/node/main"], "vscode-jsonrpc": ["./node_modules/vscode-jsonrpc/lib/common/api"], "vscode-jsonrpc/node": ["./node_modules/vscode-jsonrpc/lib/node/main"], }, From cebd40e70498c57d2214a616bad4f1826ae7bde5 Mon Sep 17 00:00:00 2001 From: David Barbet Date: Tue, 1 Sep 2026 16:28:36 -0700 Subject: [PATCH 2/4] Capture the exit signal instead of retaining the server process Storing the ChildProcess kept a dead process and its stdio buffers alive for the lifetime of the client, and it was never cleared on restart, so a previous session's SIGKILL could be reported for a later, unrelated failure. Attach the exit listener in handleConnectionClosed and keep only the resulting signal. Clearing a reference does not unregister listeners and node keeps the child alive until it is reaped, so the listener still fires after the base client drops its own reference. The captured signal is reset alongside _hasShownConnectionClose when the server reaches Running. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../server/roslynLanguageClient.ts | 26 +++++---- .../unitTests/roslynLanguageClient.test.ts | 58 +++++++++++++++---- 2 files changed, 61 insertions(+), 23 deletions(-) diff --git a/src/lsptoolshost/server/roslynLanguageClient.ts b/src/lsptoolshost/server/roslynLanguageClient.ts index 5a4aaa8e65..16e635c618 100644 --- a/src/lsptoolshost/server/roslynLanguageClient.ts +++ b/src/lsptoolshost/server/roslynLanguageClient.ts @@ -52,11 +52,12 @@ export class RoslynLanguageClient extends LanguageClient { private _hasShownConnectionClose = false; /** - * The server process from the current session, retained so we can report how it ended. - * The base client clears its own reference before invoking the close handler, so reading - * `serverProcess` at crash time is too late. + * The signal that killed the server process for the current session, captured when the connection + * closes. Holding the resolved answer rather than the process itself means we do not keep a dead + * process and its stdio buffers alive. Reset on restart so a previous session's exit is never + * attributed to a later failure. */ - private _closedServerProcess: ChildProcess | undefined; + private _serverExitSignal: Promise | undefined; constructor( id: string, @@ -80,6 +81,7 @@ export class RoslynLanguageClient extends LanguageClient { this.onDidChangeState((e) => { if (e.newState === State.Running) { this._hasShownConnectionClose = false; + this._serverExitSignal = undefined; } }); } @@ -90,9 +92,10 @@ export class RoslynLanguageClient extends LanguageClient { } protected override async handleConnectionClosed(): Promise { - // The base implementation drops its reference to the process before the close handler runs, - // so grab it here while it is still available. - this._closedServerProcess = this.serverProcess; + // The base implementation drops its reference to the process before the close handler runs, so + // start listening now. Clearing that reference does not unregister the listener, and the process + // is normally reaped a few milliseconds later. + this._serverExitSignal = getExitSignal(this.serverProcess); return super.handleConnectionClosed(); } @@ -198,12 +201,13 @@ export class RoslynLanguageClient extends LanguageClient { // Set the guard before awaiting so the error and closed handlers cannot both get past it. this._hasShownConnectionClose = true; - // The close handler has already cleared serverProcess, so fall back to the process we kept. - void this.showCrashNotificationAsync(this.serverProcess ?? this._closedServerProcess); + // The connection close captures the signal itself, since it clears the process reference first. + // Reaching here without it means the connection errored while the process is still ours to read. + void this.showCrashNotificationAsync(this._serverExitSignal ?? getExitSignal(this.serverProcess)); } - private async showCrashNotificationAsync(serverProcess: ChildProcess | undefined): Promise { - const signal = await getExitSignal(serverProcess); + private async showCrashNotificationAsync(exitSignal: Promise): Promise { + const signal = await exitSignal; const externallyTerminated = signal === externalTerminationSignal; this._telemetryReporter.sendTelemetryEvent(TelemetryEventNames.ServerCrash, { diff --git a/test/lsptoolshost/unitTests/roslynLanguageClient.test.ts b/test/lsptoolshost/unitTests/roslynLanguageClient.test.ts index 5b2328a6f5..e1e54a9a8f 100644 --- a/test/lsptoolshost/unitTests/roslynLanguageClient.test.ts +++ b/test/lsptoolshost/unitTests/roslynLanguageClient.test.ts @@ -8,7 +8,16 @@ import { ChildProcess } from 'child_process'; import { EventEmitter } from 'events'; jest.mock('vscode-languageclient/node', () => ({ - LanguageClient: class {}, + LanguageClient: class { + serverProcess: unknown; + /** + * Stands in for the base implementation, which drops its reference to the server process + * before invoking the close handler. + */ + async handleConnectionClosed(): Promise { + this.serverProcess = undefined; + } + }, State: { Running: 2 }, })); jest.mock('vscode-languageclient', () => ({ @@ -20,11 +29,14 @@ jest.mock('vscode-languageclient', () => ({ ProtocolRequestType: class {}, RequestType: class {}, RequestType0: class {}, + State: { Running: 2 }, })); import { RoslynLanguageClient } from '../../../src/lsptoolshost/server/roslynLanguageClient'; import { TelemetryEventNames } from '../../../src/shared/telemetryEventNames'; +const runningState = 2; + /** A stand-in for an already reaped process, which is what we normally observe on a crash. */ function exitedProcess(exitCode: number | null, signalCode: NodeJS.Signals | null): ChildProcess { return { exitCode, signalCode } as ChildProcess; @@ -45,7 +57,7 @@ function createClient(serverProcess?: ChildProcess) { client._hasShownConnectionClose = false; client._telemetryReporter = { sendTelemetryEvent }; - client._closedServerProcess = serverProcess; + client.serverProcess = serverProcess; client.showCrashNotificationCore = showCrashNotificationCore; return { client, sendTelemetryEvent, showCrashNotificationCore }; @@ -136,17 +148,39 @@ describe('RoslynLanguageClient', () => { expect(showCrashNotificationCore).toHaveBeenCalledWith(true); }); - test('falls back to the generic crash message when the process outlives the connection', async () => { - jest.useFakeTimers(); - try { - const { client, showCrashNotificationCore } = createClient(runningProcess()); + test('captures the signal before the base client clears the process reference', async () => { + const serverProcess = runningProcess(); + const { client, showCrashNotificationCore } = createClient(serverProcess); + + await client.handleConnectionClosed(); - client.showCrashNotification(); - await jest.advanceTimersByTimeAsync(1000); + // The close handler leaves nothing to read the signal from, so the capture has to have + // happened first. Clearing that reference does not unregister our exit listener. + expect(client.serverProcess).toBeUndefined(); - expect(showCrashNotificationCore).toHaveBeenCalledWith(false); - } finally { - jest.useRealTimers(); - } + client.showCrashNotification(); + serverProcess.emit('exit', null, 'SIGKILL'); + await flushPendingNotifications(); + + expect(showCrashNotificationCore).toHaveBeenCalledWith(true); + }); + + test('clears the captured exit signal when the server restarts', async () => { + const { client, showCrashNotificationCore } = createClient(); + let onStateChange: ((e: { newState: number }) => void) | undefined; + client.onDidChangeState = (handler: (e: { newState: number }) => void) => (onStateChange = handler); + client.registerStateChangeHandler(); + + // A previous session was killed externally and reported. + client._serverExitSignal = Promise.resolve('SIGKILL'); + client._hasShownConnectionClose = true; + + onStateChange!({ newState: runningState }); + + // A later failure in the new session must not inherit the old session's exit. + client.showCrashNotification(); + await flushPendingNotifications(); + + expect(showCrashNotificationCore).toHaveBeenCalledWith(false); }); }); From 3011b5b2ca6e929c71389e024cf5d6c6fbb583ce Mon Sep 17 00:00:00 2001 From: David Barbet Date: Tue, 1 Sep 2026 16:39:02 -0700 Subject: [PATCH 3/4] Capture the server exit signal once at launch Capturing the signal lazily meant reading it from two places - the process the client still held, or the one captured as the connection closed - which needed precedence rules between them and a reset so an old session was not reported for a later failure. It was also easy to get wrong: swapping the two operands of the ?? silently disabled the fallback, since a promise is never nullish. Attach the listener when the server launches instead. There is then a single source for the answer, it always describes the current process because each launch replaces it, and no reset is needed. The already-exited check goes away too, since the process is known to be running at that point. The bounded wait stays at the point of reporting: a timeout started at launch would elapse long before any crash and latch the wrong answer. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../server/roslynLanguageClient.ts | 84 +++++++++------ .../unitTests/roslynLanguageClient.test.ts | 101 ++++++++++-------- 2 files changed, 105 insertions(+), 80 deletions(-) diff --git a/src/lsptoolshost/server/roslynLanguageClient.ts b/src/lsptoolshost/server/roslynLanguageClient.ts index 16e635c618..d827aeb8bd 100644 --- a/src/lsptoolshost/server/roslynLanguageClient.ts +++ b/src/lsptoolshost/server/roslynLanguageClient.ts @@ -6,7 +6,14 @@ import * as vscode from 'vscode'; import { ChildProcess } from 'child_process'; import { LanguageClient, ServerOptions } from 'vscode-languageclient/node'; -import { CancellationToken, ErrorHandler, LanguageClientOptions, MessageSignature, State } from 'vscode-languageclient'; +import { + CancellationToken, + ErrorHandler, + LanguageClientOptions, + MessageSignature, + MessageTransports, + State, +} from 'vscode-languageclient'; import CompositeDisposable from '../../compositeDisposable'; import { IDisposable } from '../../disposable'; import { languageServerOptions } from '../../shared/options'; @@ -52,10 +59,11 @@ export class RoslynLanguageClient extends LanguageClient { private _hasShownConnectionClose = false; /** - * The signal that killed the server process for the current session, captured when the connection - * closes. Holding the resolved answer rather than the process itself means we do not keep a dead - * process and its stdio buffers alive. Reset on restart so a previous session's exit is never - * attributed to a later failure. + * Resolves with the signal that killed the server process, or null if it ended some other way. + * Established when the server launches, both because a listener attached after the process has + * been reaped never fires and because the base client drops its own reference to the process + * before the close handler runs. Replaced on every launch, so it always describes the process + * for the current session. */ private _serverExitSignal: Promise | undefined; @@ -81,7 +89,6 @@ export class RoslynLanguageClient extends LanguageClient { this.onDidChangeState((e) => { if (e.newState === State.Running) { this._hasShownConnectionClose = false; - this._serverExitSignal = undefined; } }); } @@ -91,12 +98,12 @@ export class RoslynLanguageClient extends LanguageClient { return super.dispose(timeout); } - protected override async handleConnectionClosed(): Promise { - // The base implementation drops its reference to the process before the close handler runs, so - // start listening now. Clearing that reference does not unregister the listener, and the process - // is normally reaped a few milliseconds later. - this._serverExitSignal = getExitSignal(this.serverProcess); - return super.handleConnectionClosed(); + protected override async createMessageTransports(encoding: string): Promise { + const transports = await super.createMessageTransports(encoding); + // Start listening as soon as the process exists - see _serverExitSignal for why waiting until + // the crash is reported would be too late. + this._serverExitSignal = waitForExitSignal(this.serverProcess); + return transports; } override handleFailedRequest( @@ -201,13 +208,34 @@ export class RoslynLanguageClient extends LanguageClient { // Set the guard before awaiting so the error and closed handlers cannot both get past it. this._hasShownConnectionClose = true; - // The connection close captures the signal itself, since it clears the process reference first. - // Reaching here without it means the connection errored while the process is still ours to read. - void this.showCrashNotificationAsync(this._serverExitSignal ?? getExitSignal(this.serverProcess)); + void this.showCrashNotificationAsync(); } - private async showCrashNotificationAsync(exitSignal: Promise): Promise { - const signal = await exitSignal; + /** + * Waits briefly for the server process to be reaped. The connection closes a few milliseconds + * before the exit is reported, so give it a moment rather than concluding no signal was involved. + * If the process outlives the connection this reports no signal and we show the generic message. + */ + private async getExitSignal(): Promise { + if (this._serverExitSignal === undefined) { + return null; + } + + let timeout: NodeJS.Timeout | undefined; + try { + return await Promise.race([ + this._serverExitSignal, + new Promise((resolve) => { + timeout = setTimeout(() => resolve(null), serverExitTimeoutMs); + }), + ]); + } finally { + clearTimeout(timeout); + } + } + + private async showCrashNotificationAsync(): Promise { + const signal = await this.getExitSignal(); const externallyTerminated = signal === externalTerminationSignal; this._telemetryReporter.sendTelemetryEvent(TelemetryEventNames.ServerCrash, { @@ -248,26 +276,16 @@ export class RoslynLanguageClient extends LanguageClient { } /** - * Reports the signal that killed the server process, or null if it was not killed by one. - * The connection close is observed a few milliseconds before the process is reaped, so wait briefly - * for the exit instead of concluding that no signal was involved. + * Resolves when the server process exits, reporting the signal that killed it if there was one. + * Never times out: callers bound their own wait. Attached while the process is known to be running, + * so there is no risk of missing an exit that already happened. */ -async function getExitSignal(serverProcess: ChildProcess | undefined): Promise { +async function waitForExitSignal(serverProcess: ChildProcess | undefined): Promise { if (serverProcess === undefined) { - return null; - } - - // Node populates these once the process is reaped. If it already exited then 'exit' has fired and - // will not fire again for a listener added now, so read the result directly. - if (serverProcess.exitCode !== null || serverProcess.signalCode !== null) { - return serverProcess.signalCode; + return Promise.resolve(null); } return new Promise((resolve) => { - const timeout = setTimeout(() => resolve(null), serverExitTimeoutMs); - serverProcess.once('exit', (_code, signal) => { - clearTimeout(timeout); - resolve(signal); - }); + serverProcess.once('exit', (_code, signal) => resolve(signal)); }); } diff --git a/test/lsptoolshost/unitTests/roslynLanguageClient.test.ts b/test/lsptoolshost/unitTests/roslynLanguageClient.test.ts index e1e54a9a8f..29accbe393 100644 --- a/test/lsptoolshost/unitTests/roslynLanguageClient.test.ts +++ b/test/lsptoolshost/unitTests/roslynLanguageClient.test.ts @@ -10,6 +10,9 @@ import { EventEmitter } from 'events'; jest.mock('vscode-languageclient/node', () => ({ LanguageClient: class { serverProcess: unknown; + async createMessageTransports(): Promise { + return { reader: {}, writer: {} }; + } /** * Stands in for the base implementation, which drops its reference to the server process * before invoking the close handler. @@ -35,34 +38,29 @@ jest.mock('vscode-languageclient', () => ({ import { RoslynLanguageClient } from '../../../src/lsptoolshost/server/roslynLanguageClient'; import { TelemetryEventNames } from '../../../src/shared/telemetryEventNames'; -const runningState = 2; - -/** A stand-in for an already reaped process, which is what we normally observe on a crash. */ -function exitedProcess(exitCode: number | null, signalCode: NodeJS.Signals | null): ChildProcess { - return { exitCode, signalCode } as ChildProcess; -} - -/** A stand-in for a process that has not been reaped yet, so the signal arrives via 'exit'. */ +/** A stand-in for a server process that has not exited yet. */ function runningProcess(): ChildProcess { - const serverProcess = new EventEmitter() as unknown as ChildProcess; - (serverProcess as { exitCode: number | null }).exitCode = null; - (serverProcess as { signalCode: NodeJS.Signals | null }).signalCode = null; - return serverProcess; + return new EventEmitter() as unknown as ChildProcess; } -function createClient(serverProcess?: ChildProcess) { +function createClient() { const sendTelemetryEvent = jest.fn(); const showCrashNotificationCore = jest.fn(); const client = Object.create(RoslynLanguageClient.prototype) as any; client._hasShownConnectionClose = false; client._telemetryReporter = { sendTelemetryEvent }; - client.serverProcess = serverProcess; client.showCrashNotificationCore = showCrashNotificationCore; return { client, sendTelemetryEvent, showCrashNotificationCore }; } +/** Launches a server process the way the base client does, so the exit listener gets attached. */ +async function launchServer(client: any, serverProcess: ChildProcess): Promise { + client.serverProcess = serverProcess; + await client.createMessageTransports('utf8'); +} + /** Lets the fire-and-forget notification promise chain settle. */ async function flushPendingNotifications(): Promise { for (let i = 0; i < 5; i++) { @@ -84,8 +82,11 @@ describe('RoslynLanguageClient', () => { }); test('reports an external termination when the process was killed with SIGKILL', async () => { - const { client, sendTelemetryEvent, showCrashNotificationCore } = createClient(exitedProcess(null, 'SIGKILL')); + const serverProcess = runningProcess(); + const { client, sendTelemetryEvent, showCrashNotificationCore } = createClient(); + await launchServer(client, serverProcess); + serverProcess.emit('exit', null, 'SIGKILL'); client.showCrashNotification(); await flushPendingNotifications(); @@ -98,8 +99,11 @@ describe('RoslynLanguageClient', () => { // The .NET runtime ends fatal errors with abort(), so SIGABRT is the server genuinely failing. test('reports a crash when the runtime aborted the process', async () => { - const { client, sendTelemetryEvent, showCrashNotificationCore } = createClient(exitedProcess(null, 'SIGABRT')); + const serverProcess = runningProcess(); + const { client, sendTelemetryEvent, showCrashNotificationCore } = createClient(); + await launchServer(client, serverProcess); + serverProcess.emit('exit', null, 'SIGABRT'); client.showCrashNotification(); await flushPendingNotifications(); @@ -111,8 +115,11 @@ describe('RoslynLanguageClient', () => { }); test('reports a crash when the process exited with a code rather than a signal', async () => { - const { client, sendTelemetryEvent, showCrashNotificationCore } = createClient(exitedProcess(1, null)); + const serverProcess = runningProcess(); + const { client, sendTelemetryEvent, showCrashNotificationCore } = createClient(); + await launchServer(client, serverProcess); + serverProcess.emit('exit', 1, null); client.showCrashNotification(); await flushPendingNotifications(); @@ -123,7 +130,7 @@ describe('RoslynLanguageClient', () => { expect(showCrashNotificationCore).toHaveBeenCalledWith(false); }); - test('reports a crash when there is no process to inspect', async () => { + test('reports a crash when the server never launched', async () => { const { client, showCrashNotificationCore } = createClient(); client.showCrashNotification(); @@ -132,14 +139,18 @@ describe('RoslynLanguageClient', () => { expect(showCrashNotificationCore).toHaveBeenCalledWith(false); }); - test('waits for the exit event when the process has not been reaped yet', async () => { + test('keeps listening after the base client drops its reference to the process', async () => { const serverProcess = runningProcess(); - const { client, showCrashNotificationCore } = createClient(serverProcess); + const { client, showCrashNotificationCore } = createClient(); + await launchServer(client, serverProcess); + + // The close handler runs before the process is reaped and leaves nothing to read the signal + // from, but clearing that reference does not unregister our listener. + await client.handleConnectionClosed(); + expect(client.serverProcess).toBeUndefined(); client.showCrashNotification(); await flushPendingNotifications(); - - // The connection closes slightly before the process is reaped, so nothing is reported yet. expect(showCrashNotificationCore).not.toHaveBeenCalled(); serverProcess.emit('exit', null, 'SIGKILL'); @@ -148,39 +159,35 @@ describe('RoslynLanguageClient', () => { expect(showCrashNotificationCore).toHaveBeenCalledWith(true); }); - test('captures the signal before the base client clears the process reference', async () => { - const serverProcess = runningProcess(); - const { client, showCrashNotificationCore } = createClient(serverProcess); - - await client.handleConnectionClosed(); + test('describes the current process rather than a previous session', async () => { + const killedProcess = runningProcess(); + const { client, showCrashNotificationCore } = createClient(); + await launchServer(client, killedProcess); + killedProcess.emit('exit', null, 'SIGKILL'); - // The close handler leaves nothing to read the signal from, so the capture has to have - // happened first. Clearing that reference does not unregister our exit listener. - expect(client.serverProcess).toBeUndefined(); + // Restarting replaces the captured exit, so the earlier kill is not reported again. + const restartedProcess = runningProcess(); + await launchServer(client, restartedProcess); + restartedProcess.emit('exit', 1, null); client.showCrashNotification(); - serverProcess.emit('exit', null, 'SIGKILL'); await flushPendingNotifications(); - expect(showCrashNotificationCore).toHaveBeenCalledWith(true); + expect(showCrashNotificationCore).toHaveBeenCalledWith(false); }); - test('clears the captured exit signal when the server restarts', async () => { - const { client, showCrashNotificationCore } = createClient(); - let onStateChange: ((e: { newState: number }) => void) | undefined; - client.onDidChangeState = (handler: (e: { newState: number }) => void) => (onStateChange = handler); - client.registerStateChangeHandler(); - - // A previous session was killed externally and reported. - client._serverExitSignal = Promise.resolve('SIGKILL'); - client._hasShownConnectionClose = true; - - onStateChange!({ newState: runningState }); + test('falls back to the generic crash message when the process outlives the connection', async () => { + jest.useFakeTimers(); + try { + const { client, showCrashNotificationCore } = createClient(); + await launchServer(client, runningProcess()); - // A later failure in the new session must not inherit the old session's exit. - client.showCrashNotification(); - await flushPendingNotifications(); + client.showCrashNotification(); + await jest.advanceTimersByTimeAsync(1000); - expect(showCrashNotificationCore).toHaveBeenCalledWith(false); + expect(showCrashNotificationCore).toHaveBeenCalledWith(false); + } finally { + jest.useRealTimers(); + } }); }); From 4b44cc79756eebd44a80a8041b8ea1824218d04b Mon Sep 17 00:00:00 2001 From: David Barbet Date: Tue, 1 Sep 2026 17:31:26 -0700 Subject: [PATCH 4/4] don't ask to report issue if the server process was externally killed --- l10n/bundle.l10n.json | 4 +- .../server/roslynLanguageClient.ts | 128 ++++++++---------- .../unitTests/roslynLanguageClient.test.ts | 44 ++++-- 3 files changed, 96 insertions(+), 80 deletions(-) diff --git a/l10n/bundle.l10n.json b/l10n/bundle.l10n.json index 24e9399e4b..e93a22bac5 100644 --- a/l10n/bundle.l10n.json +++ b/l10n/bundle.l10n.json @@ -108,9 +108,9 @@ "Detected change in telemetry settings. These will not take effect until the language server is restarted, would you like to restart?": "Detected change in telemetry settings. These will not take effect until the language server is restarted, would you like to restart?", "Go to output": "Go to output", "Report Issue": "Report Issue", - "The C# language server was terminated by the operating system or another program rather than crashing. This is usually caused by running out of memory, a container memory limit, or another process stopping it. Restart extensions to re-enable C# functionality.": "The C# language server was terminated by the operating system or another program rather than crashing. This is usually caused by running out of memory, a container memory limit, or another process stopping it. Restart extensions to re-enable C# functionality.", - "The C# language server has crashed. Restart extensions to re-enable C# functionality.": "The C# language server has crashed. Restart extensions to re-enable C# functionality.", "Restart extensions": "Restart extensions", + "The C# language server was terminated externally. Restart extensions to re-enable C# functionality.": "The C# language server was terminated externally. Restart extensions to re-enable C# functionality.", + "The C# language server has crashed. Restart extensions to re-enable C# functionality.": "The C# language server has crashed. Restart extensions to re-enable C# functionality.", "Restore {0}": "Restore {0}", "Restore already in progress": "Restore already in progress", "Select context": "Select context", diff --git a/src/lsptoolshost/server/roslynLanguageClient.ts b/src/lsptoolshost/server/roslynLanguageClient.ts index d827aeb8bd..c39cb2aa8d 100644 --- a/src/lsptoolshost/server/roslynLanguageClient.ts +++ b/src/lsptoolshost/server/roslynLanguageClient.ts @@ -23,25 +23,10 @@ import { ITelemetryReporter } from '../../shared/telemetryReporter'; import { TelemetryEventNames } from '../../shared/telemetryEventNames'; /** - * How long to wait for the server process to be reaped after the connection closes. - * The connection closing and the process exiting are separate events, and the exit normally follows - * within a few milliseconds. This only elapses if the process is still alive (for example a protocol - * error), in which case we report the generic crash message. + * How long to wait for the server process to be reaped after the connection closes (which can happen milliseconds before the process actually exits) */ const serverExitTimeoutMs = 1000; -/** - * SIGKILL cannot be caught, blocked, or ignored, and the .NET runtime never raises it on itself - - * fatal CLR errors (unhandled exceptions, stack overflow, Environment.FailFast) go through abort() - * and surface as SIGABRT instead. So SIGKILL means the OS or another program terminated the server: - * a Linux OOM killer, macOS Jetsam, a container memory limit, or `kill -9`. - * - * Other externally-originated signals such as SIGTERM are deliberately not treated this way. VS Code - * and our own teardown can send them during an intentional shutdown, so reporting them as an external - * kill risks blaming the user's environment for a normal stop. - */ -const externalTerminationSignal: NodeJS.Signals = 'SIGKILL'; - /** * Implementation of the base LanguageClient type that allows for additional items to be disposed of * when the base LanguageClient instance is disposed. @@ -59,13 +44,9 @@ export class RoslynLanguageClient extends LanguageClient { private _hasShownConnectionClose = false; /** - * Resolves with the signal that killed the server process, or null if it ended some other way. - * Established when the server launches, both because a listener attached after the process has - * been reaped never fires and because the base client drops its own reference to the process - * before the close handler runs. Replaced on every launch, so it always describes the process - * for the current session. + * Resolves when the process exits, true when the server process was terminated externally. */ - private _serverExitSignal: Promise | undefined; + private _serverExit: Promise | undefined; constructor( id: string, @@ -100,9 +81,7 @@ export class RoslynLanguageClient extends LanguageClient { protected override async createMessageTransports(encoding: string): Promise { const transports = await super.createMessageTransports(encoding); - // Start listening as soon as the process exists - see _serverExitSignal for why waiting until - // the crash is reported would be too late. - this._serverExitSignal = waitForExitSignal(this.serverProcess); + this._serverExit = waitForExternalTermination(this.serverProcess); return transports; } @@ -212,21 +191,20 @@ export class RoslynLanguageClient extends LanguageClient { } /** - * Waits briefly for the server process to be reaped. The connection closes a few milliseconds - * before the exit is reported, so give it a moment rather than concluding no signal was involved. - * If the process outlives the connection this reports no signal and we show the generic message. + * Waits for the server process to exit, racing with a timeout. + * Reports false if the process outlives the connection. */ - private async getExitSignal(): Promise { - if (this._serverExitSignal === undefined) { - return null; + private async waitForProcessExit(): Promise { + if (this._serverExit === undefined) { + return false; } let timeout: NodeJS.Timeout | undefined; try { return await Promise.race([ - this._serverExitSignal, - new Promise((resolve) => { - timeout = setTimeout(() => resolve(null), serverExitTimeoutMs); + this._serverExit, + new Promise((resolve) => { + timeout = setTimeout(() => resolve(false), serverExitTimeoutMs); }), ]); } finally { @@ -235,11 +213,9 @@ export class RoslynLanguageClient extends LanguageClient { } private async showCrashNotificationAsync(): Promise { - const signal = await this.getExitSignal(); - const externallyTerminated = signal === externalTerminationSignal; + const externallyTerminated = await this.waitForProcessExit(); this._telemetryReporter.sendTelemetryEvent(TelemetryEventNames.ServerCrash, { - signal: signal ?? '', externallyTerminated: externallyTerminated.toString(), }); @@ -247,45 +223,59 @@ export class RoslynLanguageClient extends LanguageClient { } private showCrashNotificationCore(externallyTerminated: boolean) { - showErrorMessageWithOptions( - vscode, - externallyTerminated - ? // The server did not fail on its own, so leading with a crash report or a dump would - // send the user down the wrong path. Name the likely external cause instead. - vscode.l10n.t( - 'The C# language server was terminated by the operating system or another program rather than crashing. This is usually caused by running out of memory, a container memory limit, or another process stopping it. Restart extensions to re-enable C# functionality.' - ) - : vscode.l10n.t( - 'The C# language server has crashed. Restart extensions to re-enable C# functionality.' - ), - { modal: false }, - { - title: vscode.l10n.t('Restart extensions'), - command: 'workbench.action.restartExtensionHost', - }, - { - title: vscode.l10n.t('Report Issue'), - action: async () => { - vscode.commands.executeCommand('csharp.reportIssue'); - // Re-show the notification so the user can still restart extensions after reporting. - this.showCrashNotificationCore(externallyTerminated); - }, - } - ); + const restartCommand = { + title: vscode.l10n.t('Restart extensions'), + command: 'workbench.action.restartExtensionHost', + }; + if (externallyTerminated) { + // Show a notification without a report issue command - there's nothing we can if the server + // was terminated by some external process. + showErrorMessageWithOptions( + vscode, + vscode.l10n.t( + 'The C# language server was terminated externally. Restart extensions to re-enable C# functionality.' + ), + { modal: false }, + restartCommand + ); + } else { + showErrorMessageWithOptions( + vscode, + vscode.l10n.t('The C# language server has crashed. Restart extensions to re-enable C# functionality.'), + { modal: false }, + restartCommand, + { + title: vscode.l10n.t('Report Issue'), + action: async () => { + vscode.commands.executeCommand('csharp.reportIssue'); + // Re-show the notification so the user can still restart extensions after reporting. + this.showCrashNotificationCore(externallyTerminated); + }, + } + ); + } } } /** - * Resolves when the server process exits, reporting the signal that killed it if there was one. - * Never times out: callers bound their own wait. Attached while the process is known to be running, - * so there is no risk of missing an exit that already happened. + * Resolves when the server process exits, reporting whether it was stopped externally or not. + * Note that this is only reliable on non-windows platforms - on windows a killed process has no signal and can have any exit code. */ -async function waitForExitSignal(serverProcess: ChildProcess | undefined): Promise { +function waitForExternalTermination(serverProcess: ChildProcess | undefined): Promise | undefined { if (serverProcess === undefined) { - return Promise.resolve(null); + return undefined; } - return new Promise((resolve) => { - serverProcess.once('exit', (_code, signal) => resolve(signal)); + return new Promise((resolve) => { + serverProcess.once('exit', (code, signal) => { + resolve( + // SIGKILL cannot be caught, blocked, or ignored, so it was sent by an external process. + signal === 'SIGKILL' || + // .NET normally handles SIGTERM and exits with 128 + SIGTERM instead of reporting the signal. + code === 143 || + // The PAL re-raises SIGTERM on some paths. + signal === 'SIGTERM' + ); + }); }); } diff --git a/test/lsptoolshost/unitTests/roslynLanguageClient.test.ts b/test/lsptoolshost/unitTests/roslynLanguageClient.test.ts index 29accbe393..9c042ff57f 100644 --- a/test/lsptoolshost/unitTests/roslynLanguageClient.test.ts +++ b/test/lsptoolshost/unitTests/roslynLanguageClient.test.ts @@ -91,12 +91,41 @@ describe('RoslynLanguageClient', () => { await flushPendingNotifications(); expect(sendTelemetryEvent).toHaveBeenCalledWith(TelemetryEventNames.ServerCrash, { - signal: 'SIGKILL', externallyTerminated: 'true', }); expect(showCrashNotificationCore).toHaveBeenCalledWith(true); }); + // .NET handles SIGTERM rather than dying from it, shutting down and exiting with 128 + SIGTERM. + // This is how an external termination usually reaches us, and it arrives as a code, not a signal. + test('reports an external termination for the SIGTERM exit code', async () => { + const serverProcess = runningProcess(); + const { client, sendTelemetryEvent, showCrashNotificationCore } = createClient(); + await launchServer(client, serverProcess); + + serverProcess.emit('exit', 143, null); + client.showCrashNotification(); + await flushPendingNotifications(); + + expect(sendTelemetryEvent).toHaveBeenCalledWith(TelemetryEventNames.ServerCrash, { + externallyTerminated: 'true', + }); + expect(showCrashNotificationCore).toHaveBeenCalledWith(true); + }); + + // Some runtime paths re-raise SIGTERM instead of exiting with 143. + test('reports an external termination when SIGTERM arrives as a signal', async () => { + const serverProcess = runningProcess(); + const { client, showCrashNotificationCore } = createClient(); + await launchServer(client, serverProcess); + + serverProcess.emit('exit', null, 'SIGTERM'); + client.showCrashNotification(); + await flushPendingNotifications(); + + expect(showCrashNotificationCore).toHaveBeenCalledWith(true); + }); + // The .NET runtime ends fatal errors with abort(), so SIGABRT is the server genuinely failing. test('reports a crash when the runtime aborted the process', async () => { const serverProcess = runningProcess(); @@ -108,25 +137,22 @@ describe('RoslynLanguageClient', () => { await flushPendingNotifications(); expect(sendTelemetryEvent).toHaveBeenCalledWith(TelemetryEventNames.ServerCrash, { - signal: 'SIGABRT', externallyTerminated: 'false', }); expect(showCrashNotificationCore).toHaveBeenCalledWith(false); }); - test('reports a crash when the process exited with a code rather than a signal', async () => { + // 0xE0434352 is an unhandled managed exception and 0xC0000005 an access violation, both of which + // are the server failing on its own rather than being stopped. + test.each([3762504530, 3221225477, 1])('reports a crash for exit code %s', async (exitCode) => { const serverProcess = runningProcess(); - const { client, sendTelemetryEvent, showCrashNotificationCore } = createClient(); + const { client, showCrashNotificationCore } = createClient(); await launchServer(client, serverProcess); - serverProcess.emit('exit', 1, null); + serverProcess.emit('exit', exitCode, null); client.showCrashNotification(); await flushPendingNotifications(); - expect(sendTelemetryEvent).toHaveBeenCalledWith(TelemetryEventNames.ServerCrash, { - signal: '', - externallyTerminated: 'false', - }); expect(showCrashNotificationCore).toHaveBeenCalledWith(false); });