Don't ask to report an issue if the server process was killed externally - #9721
Open
dibarbet wants to merge 4 commits into
Open
Don't ask to report an issue if the server process was killed externally#9721dibarbet wants to merge 4 commits into
dibarbet wants to merge 4 commits into
Conversation
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>
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>
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>
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
External-termination detection is documented as unreliable on Windows, but the current implementation can still classify Windows exits as externally terminated and suppress “Report Issue” incorrectly.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Lite
Findings: 1
New issues introduced by this change (3)
| Severity | Finding |
|---|---|
 |
src/lsptoolshost/server/roslynLanguageClient.ts — waitForExternalTermination currently treats SIGTERM/SIGKILL (and exit code 143) as external… |
 |
src/lsptoolshost/server/roslynLanguageClient.ts — Comment has a missing word: "there's nothing we can" should be "there's nothing we can do". |
|
test/lsptoolshost/unitTests/roslynLanguageClient.test.ts — The first test only asserts the telemetry call count now, which allows regressions where the wrong… |
What changed in this PR
This PR refines the Roslyn language server crash UX by distinguishing externally terminated server processes (SIGKILL/SIGTERM) from actual crashes (where determinable), so users aren’t prompted to file issues when the extension can’t act on the information. It also updates the vscode-languageclient dependency and aligns TypeScript path mappings accordingly.
Changes:
- Track server-process exit/signal and suppress “Report Issue” when the server was externally terminated, while adding telemetry metadata for termination classification.
- Add/expand unit tests covering signal/exit-code scenarios and timing behavior around connection close vs. process exit.
- Upgrade
vscode-languageclientto 10.1.1 and adjusttsconfigpath mappings to use top-level protocol packages.
| File | Description |
|---|---|
| tsconfig.json | Updates TS path mapping for vscode-languageserver-protocol to the top-level dependency layout. |
| src/lsptoolshost/server/roslynLanguageClient.ts | Implements external-termination detection, updates crash notification flow, and adds telemetry dimension. |
| test/lsptoolshost/unitTests/roslynLanguageClient.test.ts | Adds unit tests for SIGKILL/SIGTERM and exit-code-based external termination plus timeout fallback behavior. |
| package.json | Bumps vscode-languageclient dependency version. |
| package-lock.json | Locks updated dependency graph for vscode-languageclient and related protocol/jsonrpc packages. |
| l10n/bundle.l10n.json | Adds localized string for the externally-terminated notification message. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+260
to
+281
| /** | ||
| * 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. | ||
| */ | ||
| function waitForExternalTermination(serverProcess: ChildProcess | undefined): Promise<boolean> | undefined { | ||
| if (serverProcess === undefined) { | ||
| return undefined; | ||
| } | ||
|
|
||
| return new Promise<boolean>((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' | ||
| ); | ||
| }); | ||
| }); | ||
| } |
Comment on lines
+231
to
+232
| // Show a notification without a report issue command - there's nothing we can if the server | ||
| // was terminated by some external process. |
Comment on lines
80
to
81
| expect(sendTelemetryEvent).toHaveBeenCalledTimes(1); | ||
| expect(sendTelemetryEvent).toHaveBeenCalledWith(TelemetryEventNames.ServerCrash); | ||
| expect(showCrashNotificationCore).toHaveBeenCalledTimes(1); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
have seen a few issue reports where the server process was externally killed (e.g. sigkill). There's nothing we can do with that information, so we don't need to ask for a issue report.
Note that this only works on linux/mac. Windows doesn't have a way to differentiate.