Don't rely on Promise.prototype.finally when starting or stopping the client

[Yashvardhan Thanvi (Thanatos9404)]

Fixes #1653

Why finally is missing

Dirk Bäumer (@dbaeumer) asked why this happens before changing the code, so here is what I found.

The value that has no finally is the result of .then(...) in createMessageTransports. With an unmodified native promise that can't happen. Promise.prototype.then always builds its result through the native Promise species, so nothing the callback returns can change it. So either _getServerWorkingDir returns something that is not a native promise, or something in the extension host patched the native prototype (deleting Promise.prototype.finally, or wrapping .then the way zone.js does). The second would also break .finally in VS Code's own code and at node/main.ts:253, which makes the first much more likely.

_getServerWorkingDir creates its promise with new Promise(...) or Promise.resolve(undefined). Those use the global Promise and look it up when the code runs. Every extension in an extension host shares one global object. So if any code in that process sets globalThis.Promise to an older implementation (a pre ES2018 polyfill, or a library that installs its own Promise globally), the client's promise comes from that implementation. Its then returns an object without finally, and .finally(...) throws.

Running that case in plain Node gives exactly the message from the telemetry:

const Native = Promise;
class ThenOnly {
	constructor(executor) { this.p = new Native(executor); }
	then(a, b) { const p = this.p.then(a, b); return new ThenOnly((r, j) => p.then(r, j)); }
	static resolve(v) { return new ThenOnly(r => r(v)); }
}
class C {
	_getServerWorkingDir() { return new Promise(r => r(undefined)); }
	run() { return this._getServerWorkingDir().then(() => 1).finally(() => {}); }
}
globalThis.Promise = ThenOnly;
new C().run();
// TypeError: this._getServerWorkingDir(...).then(...).finally is not a function

This also fits the rest of the report:

• The error is thrown synchronously while .finally is being called, which is why the stack in the issue only has createMessageTransports, createConnection and start and no async frames.
• It depends on the environment, not on the server options. The Docker extension sees it on all three platforms, but only for some users. The StackOverflow post hits it with the JSON language server. Two people in this thread fixed it by deleting their VS Code data folders, including ~/.vscode where extensions live.

What I can't tell you is which extension or library swaps the global Promise for the affected users. The telemetry doesn't say. Everything above points at another extension in the same extension host, but I haven't pinned it to one.

There is a side effect on the current code too. The .then callback is already registered when .finally throws, so the server process still gets spawned after start() has failed. That process has no connection and no exit listener.

What changes

• client/src/node/main.ts: createMessageTransports is now async. The old .then callback is a local startServer function, kept line for line. The .finally is now try { return await startServer(await this._getServerWorkingDir(...)); } finally { ... }. The return await matters: the exit listener is still attached only after the transports are ready (for example after waitForConnection on pipe and socket), just as before.
• client/src/common/client.ts: shutdown has the same problem. Promise.race([tp, shutdown]).then(...).finally(...) goes through the global Promise, so stop() fails with Promise.race(...).then(...).finally is not a function and the client is left in the stopping state. With the start fix alone, affected clients would now get far enough to hit this on stop, so I changed it the same way: an async IIFE with try/catch around the race and the same cleanup in finally. As before, the "Stopping server failed" log only covers a rejected race, and the timeout error is thrown without that log.

await accepts any thenable, and an async function always returns a native promise, so neither path depends on the global Promise any more. Error propagation, cleanup order and log messages stay the same. One visible difference: because createMessageTransports is now async, a synchronous throw inside it (for example from a throwing server() callback) becomes a rejected promise. start() is unaffected since createConnection awaits it.

I looked at the other .finally calls in client/src. node/main.ts shutdown calls it on super.shutdown(), common/client.ts calls it on this._p2c.asDiagnostics(), and common/diagnostic.ts calls it on pullAsync(). All three are async functions, so they always return native promises and are not affected.

Tests

I added two tests to the Server tests suite in client-node-tests/src/integration.test.ts. One starts a client and the other stops one, each while the global Promise is replaced with a small then-only implementation (PromiseWithoutFinally). The replacement only lasts for the synchronous part of the start() / stop() call, and the native Promise is put back right after, so nothing else in the test host is affected.

• Before the fix, both fail. The start test fails with this._getServerWorkingDir(...).then(...).finally is not a function and the stop test fails with Promise.race(...).then(...).finally is not a function.
• After the fix, the full client suite passes (216 passing) on Windows with Node 22.17.1 and the current VS Code Insiders.

Also ran: npm run compile, npm run compile:client-node-tests, and npm --prefix client run lint / npm --prefix client-node-tests run lint, all clean. Lint prints the existing @stylistic deprecation warning and nothing else.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.