client: reject start() if server process exits before connecting on pipe/socket transport by pujitha24 · Pull Request #1862 · microsoft/vscode-languageserver-node

Pujitha Paladugu (pujitha24) · 2026-09-22T17:25:54Z

Motivation:
When using TransportKind.pipe or TransportKind.socket, the client creates
a pipe/socket server and waits for the spawned server process to connect
to it via transport.onConnected(). That promise (created in
createClientPipeTransport / createClientSocketTransport in
jsonrpc/src/node/main.ts) is only ever resolved from inside the
connection callback - it has no reject path. If the server process exits
before connecting (for example because it fails to start, or exits with
a nonzero code), the promise never settles and LanguageClient#start()
hangs forever instead of failing.

Approach:
Add a small waitForConnection() helper in client/src/node/main.ts that
races transport.onConnected() against the spawned child process's 'exit'
event, rejecting with a descriptive error if the process exits first.
Use it at all six pipe/socket call sites in createMessageTransports
(module+runtime, forked module, and Executable/command branches), in
place of the bare transport.onConnected().then(...) calls.

Validation:

npm run compile (tsc -b) is clean in jsonrpc, protocol, types,
textDocument, server, client and client-node-tests after npm run symlink.
npm run lint is clean in client and client-node-tests (only a
pre-existing, unrelated eslint-plugin deprecation warning).
Added two integration tests in client-node-tests/src/integration.test.ts
('Socket transport' and new 'Pipe transport' suites) using a new
minimal server script (servers/exitBeforeConnectServer.ts) that exits
immediately without ever connecting, mirroring the reporter's repro.
Ran via npm test in client-node-tests (real VS Code Insiders +
Electron extension host, @vscode/test-electron): before the fix the
new socket test times out after 5s (client.start() never settles,
reproducing the reported hang exactly); after the fix both new tests
pass, and the full existing suite (207 tests) passes with no
regressions.
Additionally reproduced the root cause directly with a standalone
script using the repo's own built jsonrpc createClientSocketTransport
plus a real spawned process that exits non-zero without connecting,
confirming transport.onConnected() never settles without the fix and
rejects quickly with the fix in place.

This only changes what happens when the server process exits before the
pipe/socket connection is established - normal startup and shutdown
behavior (including the process 'exit' logging in the finally() block)
is unaffected, since the added listener is a no-op once the connection
promise has already settled.

Report: #1586
Signed-off-by: Pujitha Paladugu 10557236+pujitha24@users.noreply.github.com
Assisted-by: claude-sonnet-5 (via Claude Code)

Fixes #1586

azure-pipelines · 2026-09-22T17:26:04Z

Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run.

Copilot

Copilot review overview

🟡 Changes recommended

The failure path leaves the underlying pipe/socket listener active and its resources occupied.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1

Open (1)

Close transport listeners when the child exits · New

What changed in this PR

Prevents pipe/socket client startup from hanging when the server exits before connecting.

Changes:

Races transport connection against child-process exit.
Adds pipe and socket integration tests using an immediate-exit server.

File	Description
`client/src/node/main.ts`	Adds connection/exit coordination.
`client-node-tests/src/servers/exitBeforeConnectServer.ts`	Adds failing test server.
`client-node-tests/src/integration.test.ts`	Tests socket and pipe rejection behavior.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Dirk Bäumer (dbaeumer) · 2026-09-25T07:43:14Z

Pujitha Paladugu (@pujitha24) see review comments.

…ipe/socket transport Motivation: When using TransportKind.pipe or TransportKind.socket, the client creates a pipe/socket server and waits for the spawned server process to connect to it via transport.onConnected(). That promise (created in createClientPipeTransport / createClientSocketTransport in jsonrpc/src/node/main.ts) is only ever resolved from inside the connection callback - it has no reject path. If the server process exits before connecting (for example because it fails to start, or exits with a nonzero code), the promise never settles and LanguageClient#start() hangs forever instead of failing. Approach: Add a small waitForConnection() helper in client/src/node/main.ts that races transport.onConnected() against the spawned child process's 'exit' event, rejecting with a descriptive error if the process exits first. Use it at all six pipe/socket call sites in createMessageTransports (module+runtime, forked module, and Executable/command branches), in place of the bare transport.onConnected().then(...) calls. Validation: - npm run compile (tsc -b) is clean in jsonrpc, protocol, types, textDocument, server, client and client-node-tests after `npm run symlink`. - npm run lint is clean in client and client-node-tests (only a pre-existing, unrelated eslint-plugin deprecation warning). - Added two integration tests in client-node-tests/src/integration.test.ts ('Socket transport' and new 'Pipe transport' suites) using a new minimal server script (servers/exitBeforeConnectServer.ts) that exits immediately without ever connecting, mirroring the reporter's repro. Ran via `npm test` in client-node-tests (real VS Code Insiders + Electron extension host, @vscode/test-electron): before the fix the new socket test times out after 5s (client.start() never settles, reproducing the reported hang exactly); after the fix both new tests pass, and the full existing suite (207 tests) passes with no regressions. - Additionally reproduced the root cause directly with a standalone script using the repo's own built jsonrpc createClientSocketTransport plus a real spawned process that exits non-zero without connecting, confirming transport.onConnected() never settles without the fix and rejects quickly with the fix in place. This only changes what happens when the server process exits before the pipe/socket connection is established - normal startup and shutdown behavior (including the process 'exit' logging in the finally() block) is unaffected, since the added listener is a no-op once the connection promise has already settled. Report: microsoft#1586 Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com> Assisted-by: claude-sonnet-5 (via Claude Code)

Address review feedback: add dispose() to the jsonrpc client PipeTransport/SocketTransport, call it from waitForConnection when the child exits before connecting and on the spawn-failure early returns, and add jsonrpc tests that the port/pipe path is released. Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>

Copilot

Copilot review overview

🟡 Changes recommended

Spawn errors remain unhandled, and the transport method rename breaks public API compatibility.

Review effort: Balanced
Findings: 1 · 2

Open (3)

Handle spawn errors so start() always settles · New
Preserve PipeTransport.close() API compatibility · New
Preserve SocketTransport.close() API compatibility · New

Resolved since last review (1)

Close transport listeners when the child exits

+	 * other side never connects (e.g. the server process exited early).
 	 */
-	close(): void;
+	dispose(): void;


+	 * other side never connects (e.g. the server process exited early).
 	 */
-	close(): void;
+	dispose(): void;


Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>

Renaming PipeTransport/SocketTransport close() to dispose() broke the API that main already exports (and that is re-exported through the protocol and client node entry points). Revert jsonrpc to main's close() and its tests, and call close() from waitForConnection instead. waitForConnection now also removes its 'exit' and 'error' listeners from the child process as soon as the connection is established or fails, so a server exiting normally later no longer runs them. Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com> Assisted-by: claude-opus-5-5 (via Claude Code)

Pujitha Paladugu (pujitha24) · 2026-10-01T06:16:14Z

I've addressed the Copilot review comments:

Listener left open when the server exits before connecting (first review): waitForConnection closes the transport on the child's exit, then rejects (3776a91).
Spawn errors (second review): waitForConnection also handles the child's error event, which a failed spawn emits instead of exit. It closes the transport there too and rejects (bdfb867).
close() → dispose() rename breaking the API (second review): reverted in 0a2015b. jsonrpc is back to main's close() and its tests, and the client calls close(), so this PR no longer touches jsonrpc.
0a2015b also removes the exit/error listeners once the connection succeeds or fails, so a server that exits normally later doesn't trigger them.

Checked locally: npm run compile and client lint are clean, jsonrpc tests pass (73), and client-node-tests pass (209), including the new socket and pipe exit-before-connect tests.

Copilot

Copilot review overview

🟡 Changes recommended

The socket regression test does not exercise the executable-command path reported in issue #1586.

Review effort: Balanced
Findings: 3

Open (3)

Test misses executable command and socket regression path · New
Preserve SocketTransport.close() API compatibility
Preserve PipeTransport.close() API compatibility

Resolved since last review (1)

Handle spawn errors so start() always settles

Dirk Bäumer (dbaeumer) · 2026-10-01T12:52:48Z

Pujitha Paladugu (@pujitha24) another review from Copilot on the lastest changes you made.

…before connecting Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>

Copilot

Copilot review overview

🟢 Approval recommended

The focused implementation covers all six affected paths and includes representative integration tests.

Review effort: Balanced
Findings: 2

Open (2)

Preserve SocketTransport.close() API compatibility
Preserve PipeTransport.close() API compatibility

Resolved since last review (1)

Test misses executable command and socket regression path

Pujitha Paladugu (pujitha24) · 2026-10-02T11:42:49Z

The last Copilot comment (test only hit the forked NodeModule branch) is covered by 31ad450: there's now a socket and a pipe test that use command: process.execPath plus the exitBeforeConnectServer.js script as an argument, with ELECTRON_RUN_AS_NODE set since the tests run under Electron. That goes through the Executable path from #1586. The two close() compatibility comments were handled in 0a2015b, which keeps close() on both transports. I haven't run the integration suite locally for these new cases, so I'm relying on CI for that.

Dirk Bäumer (dbaeumer) · 2026-10-02T11:57:41Z

/azp run

azure-pipelines · 2026-10-02T11:57:48Z

No pipelines are associated with this pull request.

Dirk Bäumer (dbaeumer) requested a balanced review from Copilot September 23, 2026 15:44

Copilot started reviewing on behalf of Dirk Bäumer (dbaeumer) September 23, 2026 15:45 View session

Copilot AI reviewed Sep 23, 2026

View reviewed changes

Comment thread client/src/node/main.ts Outdated

Pujitha Paladugu (pujitha24) added 2 commits September 29, 2026 06:49

Pujitha Paladugu (pujitha24) force-pushed the auto/issue-1586 branch from cdcff7e to 101ba96 Compare September 29, 2026 13:50

Dirk Bäumer (dbaeumer) requested a balanced review from Copilot September 30, 2026 09:37

Copilot started reviewing on behalf of Dirk Bäumer (dbaeumer) September 30, 2026 09:38 View session

Copilot AI reviewed Sep 30, 2026

View reviewed changes

client: settle start() when the server process fails to spawn

bdfb867

Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>

Pujitha Paladugu (pujitha24) force-pushed the auto/issue-1586 branch from 506c0c9 to bdfb867 Compare September 30, 2026 17:19

Merge branch 'main' into auto/issue-1586

249e4ab

Dirk Bäumer (dbaeumer) requested a balanced review from Copilot October 1, 2026 12:40

Copilot started reviewing on behalf of Dirk Bäumer (dbaeumer) October 1, 2026 12:41 View session

Copilot AI reviewed Oct 1, 2026

View reviewed changes

Comment thread client-node-tests/src/integration.test.ts

client-node-tests: cover the Executable branch when the server exits …

31ad450

…before connecting Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>

Dirk Bäumer (dbaeumer) requested a balanced review from Copilot October 2, 2026 11:37

Copilot started reviewing on behalf of Dirk Bäumer (dbaeumer) October 2, 2026 11:38 View session

Copilot AI reviewed Oct 2, 2026

View reviewed changes

Dirk Bäumer (dbaeumer) self-assigned this Oct 2, 2026

Dirk Bäumer (dbaeumer) approved these changes Oct 2, 2026

View reviewed changes

Dirk Bäumer (dbaeumer) enabled auto-merge (squash) October 2, 2026 11:57

Dmitriy Vasyura (dmitrivMS) approved these changes Oct 2, 2026

View reviewed changes

Christof Marti (chrmarti) approved these changes Oct 2, 2026

View reviewed changes

Dirk Bäumer (dbaeumer) merged commit 2d17738 into microsoft:main Oct 2, 2026
5 checks passed

Navigation Menu

Repository navigation

client: reject start() if server process exits before connecting on pipe/socket transport - #1862

client: reject start() if server process exits before connecting on pipe/socket transport#1862
Dirk Bäumer (dbaeumer) merged 6 commits into
microsoft:mainfrom
pujitha24:auto/issue-1586

Pujitha Paladugu (pujitha24) commented Sep 22, 2026

Uh oh!

azure-pipelines Bot commented Sep 22, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Dirk Bäumer (dbaeumer) commented Sep 25, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Pujitha Paladugu (pujitha24) commented Oct 1, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Dirk Bäumer (dbaeumer) commented Oct 1, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Pujitha Paladugu (pujitha24) commented Oct 2, 2026

Uh oh!

Dirk Bäumer (dbaeumer) commented Oct 2, 2026

Uh oh!

azure-pipelines Bot commented Oct 2, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

Pujitha Paladugu (pujitha24) commented Sep 22, 2026

Uh oh!

azure-pipelines Bot commented Sep 22, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Dirk Bäumer (dbaeumer) commented Sep 25, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Pujitha Paladugu (pujitha24) commented Oct 1, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Dirk Bäumer (dbaeumer) commented Oct 1, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟢 Approval recommended

Uh oh!

Pujitha Paladugu (pujitha24) commented Oct 2, 2026

Uh oh!

Dirk Bäumer (dbaeumer) commented Oct 2, 2026

Uh oh!

azure-pipelines Bot commented Oct 2, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants