fix: prevent server process leak (#154)#155
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a client/server process leak in the VS Code extension by preventing multiple LanguageClient instances from being spawned concurrently for the same workspace folder during activation and rapid document opening.
Changes:
- Adds
pendingClientFoldersto coalesce concurrentdidOpenTextDocumentcalls whileWorkspace.findFiles(...)is in flight. - Cancels in-flight client creation when a workspace folder is removed to reduce the chance of spawning servers for closed folders.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| client.start(); | ||
| clients.set(folderKey, client); | ||
| pendingClientFolders.delete(folderKey); |
There was a problem hiding this comment.
Fixed in d0791c6. Reordered so clients.set(folderKey, client) and pendingClientFolders.delete(folderKey) now happen before client.start() — so an onDidChangeWorkspaceFolders removal racing with this code can find the client in clients and stop it instead of leaking the server process. Wrapped start() in a try/catch: on failure we clients.delete(folderKey) (pending was already cleared) so the folder can be retried later, and we log via reporter.sendTelemetryErrorEvent.
| }, | ||
| (err) => { | ||
| pendingClientFolders.delete(folderKey); | ||
| throw err; |
There was a problem hiding this comment.
Fixed in d0791c6. Replaced the rethrow in the findFiles rejection handler with reporter.sendTelemetryErrorEvent(err, { context: "client", method: "startClientForFolder" }) and an implicit return. Pending state is still cleared first, so the folder can be retried later. This avoids the unhandled promise rejection — the surrounding try/catch couldn't catch it since the .then isn't awaited.
Builds on top of #156's pendingClientFolders race guard with three small robustness improvements identified in the #155 review: - Register the LanguageClient in `clients` BEFORE calling start(). If a workspace folder is removed mid-start, onDidChangeWorkspaceFolders can now find and stop the client. - Wrap client.start() in try/catch; on failure remove from `clients` so the folder can be retried on the next document-open. - Wrap Workspace.findFiles() in try/catch and log via telemetry instead of letting the rejection bubble up as an unhandled promise (the outer call site swallows it). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
d0791c6 to
c86bf4f
Compare
Builds on top of #156's pendingClientFolders race guard with three small robustness improvements identified in the #155 review: - Register the LanguageClient in `clients` BEFORE calling start(). If a workspace folder is removed mid-start, onDidChangeWorkspaceFolders can now find and stop the client. - Wrap client.start() in try/catch; on failure remove from `clients` so the folder can be retried on the next document-open. - Wrap Workspace.findFiles() in try/catch and log via telemetry instead of letting the rejection bubble up as an unhandled promise (the outer call site swallows it). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
c86bf4f to
95dbdb0
Compare
Summary
electron-nodejs server.jsprocesses, ~40-67MB each).didOpenTextDocumentused!clients.has(folder.uri.toString())to avoid duplicate spawns, but inserted intoclientsonly inside the.then()of an asyncWorkspace.findFiles(...). Every document opened between thefindFilescall and its resolution passed the guard and started a newLanguageClientfor the same workspace folder. At activation,Workspace.textDocuments.forEach(didOpenTextDocument)invokes this synchronously per open document, producing oneserver.jsprocess per supported document — matching the reporter's screenshot.pendingClientFoldersso concurrent calls coalesce, and clear that set on workspace folder removal so a folder closed mid-findFilesdoesn't leave a stranded server.Test plan
yarn buildpassesyarn testpasses (24/24)peekFromLanguages) — confirm exactly oneserver.jsprocess is spawned per workspace folder, not one per document. Toggling workspace folders should add/remove processes 1:1.Manual verification of the leak requires running VS Code for an extended period with workspaces containing many supported documents; there is no autonomous repro in the test suite.
🤖 Generated with Claude Code