From 94cad708867abf87eeddab00a87325403e9c36d7 Mon Sep 17 00:00:00 2001 From: Adegbite Ayoade Date: Sat, 12 Sep 2026 02:31:24 +0100 Subject: [PATCH 1/3] fix(vscode): stop the LSP restarting on unrelated configuration changes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The extension restarted the language server on every configuration change in the editor and allowed those restarts to overlap, which showed up as `Client got disposed and can't be restarted` plus a stream of `command '...' already exists` errors. Two changes: 1. Filter the configuration event. `extension.ts` subscribed to `workspace.onDidChangeConfiguration`, which fires for every setting in the editor including ones written by other extensions, and restarted unconditionally. It now restarts only when a section the server reads is affected — `sqlmesh` (`projectPaths`, `lspEntrypoint`) or `python.defaultInterpreterPath`. This is why running any python command in a VS Code terminal killed the extension: the Python extension touches its own settings in response, and that was enough to restart the server. It also explains why running a copy of the same interpreter from a different path did not reproduce it, and why a subshell, `su`, `uvx` or a notebook did not either — none of those make the Python extension write a setting. 2. Serialize restarts. `restart()` stops a client and starts another, so concurrent restarts leave the outgoing client's command registrations colliding with the incoming one's and leave requests aimed at a client that has already been disposed. Restarts now run one at a time, with triggers that arrive mid-restart collapsing into a single rerun rather than queueing one restart each. An explicit restart is never downgraded to an automatic one when the two are coalesced. Both helpers are kept free of `vscode` imports so they are covered by `vitest` in the `test-vscode` stage. Fixes #5920 Fixes #5642 Signed-off-by: Adegbite Ayoade --- vscode/extension/src/extension.ts | 33 ++++- .../src/utilities/coalesceAsync.test.ts | 115 ++++++++++++++++++ .../extension/src/utilities/coalesceAsync.ts | 49 ++++++++ .../common/configurationChange.test.ts | 58 +++++++++ .../utilities/common/configurationChange.ts | 33 +++++ 5 files changed, 286 insertions(+), 2 deletions(-) create mode 100644 vscode/extension/src/utilities/coalesceAsync.test.ts create mode 100644 vscode/extension/src/utilities/coalesceAsync.ts create mode 100644 vscode/extension/src/utilities/common/configurationChange.test.ts create mode 100644 vscode/extension/src/utilities/common/configurationChange.ts diff --git a/vscode/extension/src/extension.ts b/vscode/extension/src/extension.ts index cfea8c2228..a18807ac6e 100644 --- a/vscode/extension/src/extension.ts +++ b/vscode/extension/src/extension.ts @@ -24,6 +24,8 @@ import { traceError, } from './utilities/common/log' import { onDidChangePythonInterpreter } from './utilities/common/python' +import { requiresLspRestart } from './utilities/common/configurationChange' +import { coalesceAsync } from './utilities/coalesceAsync' import { sleep } from './utilities/sleep' import { handleError } from './utilities/errors' @@ -65,7 +67,16 @@ export async function activate(context: vscode.ExtensionContext) { ), ) - const restartLsp = async (invokedByUser = false): Promise => { + /** + * Set when a restart was asked for explicitly, so that a user-invoked restart + * coalesced together with an automatic one is still treated as user-invoked. + */ + let restartInvokedByUser = false + + const runRestart = async (): Promise => { + const invokedByUser = restartInvokedByUser + restartInvokedByUser = false + if (!lspClient) { lspClient = new LSPClient() } @@ -95,6 +106,18 @@ export async function activate(context: vscode.ExtensionContext) { context.subscriptions.push(testControllerDisposable) } + /** + * Restarts are serialized: a client disposing while the next one starts up + * leaves colliding command registrations and requests aimed at a disposed + * client, which is what surfaced as constant crashes. + */ + const restartLspSerialized = coalesceAsync(runRestart) + + const restartLsp = async (invokedByUser = false): Promise => { + restartInvokedByUser = restartInvokedByUser || invokedByUser + await restartLspSerialized() + } + // commands needing the restart helper context.subscriptions.push( vscode.commands.registerCommand( @@ -191,7 +214,13 @@ export async function activate(context: vscode.ExtensionContext) { context.subscriptions.push( onDidChangePythonInterpreter(() => restartLsp()), - onDidChangeConfiguration(() => restartLsp()), + // Only restart for settings the server actually reads. This event fires for + // every setting in the editor, including ones written by other extensions. + onDidChangeConfiguration(event => { + if (requiresLspRestart(event)) { + void restartLsp() + } + }), ) if (!lspClient.hasCompletionCapability()) { diff --git a/vscode/extension/src/utilities/coalesceAsync.test.ts b/vscode/extension/src/utilities/coalesceAsync.test.ts new file mode 100644 index 0000000000..c5c0c11347 --- /dev/null +++ b/vscode/extension/src/utilities/coalesceAsync.test.ts @@ -0,0 +1,115 @@ +import { describe, expect, it } from 'vitest' +import { coalesceAsync } from './coalesceAsync' + +/** + * Let pending microtasks run. A rerun is scheduled off the promise of the run + * that precedes it, so it starts a few hops after that run settles. + */ +const flush = () => new Promise(resolve => setTimeout(resolve, 0)) + +/** A task that only settles when the test releases it. */ +function deferredTask() { + let release: (() => void) | undefined + let rejectWith: ((error: Error) => void) | undefined + let calls = 0 + + const run = () => { + calls += 1 + return new Promise((resolve, reject) => { + release = resolve + rejectWith = reject + }) + } + + return { + run, + get calls() { + return calls + }, + release: () => release?.(), + reject: (error: Error) => rejectWith?.(error), + } +} + +describe('coalesceAsync', () => { + it('runs the task immediately when idle', async () => { + const task = deferredTask() + const run = coalesceAsync(task.run) + + const first = run() + expect(task.calls).toBe(1) + + task.release() + await first + }) + + // A burst of triggers used to start a restart per event, which left clients + // disposing and starting concurrently. See #5642. + it('collapses every call made while running into a single rerun', async () => { + const task = deferredTask() + const run = coalesceAsync(task.run) + + const first = run() + expect(task.calls).toBe(1) + + const queued = [run(), run(), run(), run()] + expect(task.calls).toBe(1) + + task.release() + await first + await flush() + + // Exactly one rerun is scheduled, no matter how many calls arrived. + expect(task.calls).toBe(2) + + task.release() + await Promise.all(queued) + await flush() + expect(task.calls).toBe(2) + }) + + it('runs again for calls made after the previous run finished', async () => { + const task = deferredTask() + const run = coalesceAsync(task.run) + + const first = run() + task.release() + await first + expect(task.calls).toBe(1) + + const second = run() + expect(task.calls).toBe(2) + task.release() + await second + }) + + it('resolves the callers that were coalesced together', async () => { + const task = deferredTask() + const run = coalesceAsync(task.run) + + const first = run() + const queued = [run(), run()] + + task.release() + await first + // The rerun has to be in flight before it can be released. + await flush() + task.release() + + await expect(Promise.all(queued)).resolves.toEqual([undefined, undefined]) + }) + + it('surfaces a failure without wedging later calls', async () => { + const task = deferredTask() + const run = coalesceAsync(task.run) + + const first = run() + task.reject(new Error('restart failed')) + await expect(first).rejects.toThrow('restart failed') + + const second = run() + expect(task.calls).toBe(2) + task.release() + await second + }) +}) diff --git a/vscode/extension/src/utilities/coalesceAsync.ts b/vscode/extension/src/utilities/coalesceAsync.ts new file mode 100644 index 0000000000..ff5c223c32 --- /dev/null +++ b/vscode/extension/src/utilities/coalesceAsync.ts @@ -0,0 +1,49 @@ +/** + * Serialize an async task so it never overlaps with itself, collapsing any + * calls that arrive while it is running into a single rerun. + * + * Restarting the language server means stopping a client and starting another. + * Letting two of those interleave leaves commands registered by the outgoing + * client colliding with the incoming one, and leaves requests addressed to a + * client that has already been disposed. Queueing one run per trigger would + * only spread the same problem out over time, so queued triggers collapse into + * one rerun: all the caller wants is for the task to have run after its + * request, not for it to run once per request. + * + * @param task The task to serialize. + * @returns A function that resolves once the task has run for that call. + */ +export function coalesceAsync(task: () => Promise): () => Promise { + let running: Promise | undefined + let queued: Promise | undefined + + const start = async (): Promise => { + try { + await task() + } finally { + running = undefined + } + } + + return (): Promise => { + if (!running) { + running = start() + return running + } + + // A rerun is already scheduled, so this call is satisfied by that one. + if (!queued) { + queued = running + // A failed run must not stop the rerun that was asked for; the caller + // waiting on the failed run is the one that sees the error. + .catch(() => undefined) + .then(() => { + queued = undefined + running = start() + return running + }) + } + + return queued + } +} diff --git a/vscode/extension/src/utilities/common/configurationChange.test.ts b/vscode/extension/src/utilities/common/configurationChange.test.ts new file mode 100644 index 0000000000..7259bf694f --- /dev/null +++ b/vscode/extension/src/utilities/common/configurationChange.test.ts @@ -0,0 +1,58 @@ +import { describe, expect, it } from 'vitest' +import { requiresLspRestart } from './configurationChange' + +/** + * Build a stand-in for `vscode.ConfigurationChangeEvent` from the settings that + * changed. VS Code reports a section as affected when the changed key is the + * section itself or sits underneath it. + */ +const changed = (...keys: string[]) => ({ + affectsConfiguration: (section: string) => + keys.some(key => key === section || key.startsWith(`${section}.`)), +}) + +describe('requiresLspRestart', () => { + it('restarts when a sqlmesh setting changes', () => { + expect(requiresLspRestart(changed('sqlmesh.projectPaths'))).toBe(true) + expect(requiresLspRestart(changed('sqlmesh.lspEntrypoint'))).toBe(true) + }) + + it('restarts when the python interpreter changes', () => { + expect(requiresLspRestart(changed('python.defaultInterpreterPath'))).toBe( + true, + ) + }) + + // The LSP used to restart on every configuration change in the editor, so + // anything that wrote a setting took the extension down with it. See #5920. + it('ignores settings the language server does not read', () => { + expect(requiresLspRestart(changed('editor.fontSize'))).toBe(false) + expect(requiresLspRestart(changed('workbench.colorTheme'))).toBe(false) + expect(requiresLspRestart(changed('files.autoSave'))).toBe(false) + }) + + // Running any python command in a VS Code terminal makes the Python + // extension touch its own terminal settings, which is what made the + // extension crash whenever sqlmesh was run in the terminal. See #5642. + it('ignores python settings unrelated to the interpreter', () => { + expect( + requiresLspRestart(changed('python.terminal.activateEnvironment')), + ).toBe(false) + expect( + requiresLspRestart(changed('python.analysis.typeCheckingMode')), + ).toBe(false) + expect(requiresLspRestart(changed('terminal.integrated.env.linux'))).toBe( + false, + ) + }) + + it('does not restart when nothing relevant changed', () => { + expect(requiresLspRestart(changed())).toBe(false) + }) + + it('restarts when a relevant setting changes alongside irrelevant ones', () => { + expect( + requiresLspRestart(changed('editor.fontSize', 'sqlmesh.projectPaths')), + ).toBe(true) + }) +}) diff --git a/vscode/extension/src/utilities/common/configurationChange.ts b/vscode/extension/src/utilities/common/configurationChange.ts new file mode 100644 index 0000000000..3676efd85f --- /dev/null +++ b/vscode/extension/src/utilities/common/configurationChange.ts @@ -0,0 +1,33 @@ +/** + * Configuration sections the language server reads, so a change to any of them + * needs the server restarted to take effect. + * + * `sqlmesh` covers `sqlmesh.projectPaths` and `sqlmesh.lspEntrypoint`, both of + * which decide how the server is launched. The interpreter path matters because + * the server runs inside that interpreter. + */ +export const RESTART_CONFIGURATION_SECTIONS = [ + 'sqlmesh', + 'python.defaultInterpreterPath', +] + +/** + * The part of `vscode.ConfigurationChangeEvent` this module needs. Declared + * structurally so the check stays unit testable without the VS Code runtime. + */ +export interface ConfigurationChange { + affectsConfiguration(section: string): boolean +} + +/** + * Whether a configuration change affects a setting the language server reads. + * + * `workspace.onDidChangeConfiguration` fires for every setting in the editor, + * including ones written by other extensions, so the event has to be filtered + * before it triggers a restart. + */ +export function requiresLspRestart(event: ConfigurationChange): boolean { + return RESTART_CONFIGURATION_SECTIONS.some(section => + event.affectsConfiguration(section), + ) +} From 4c999cb4a4a50803e638ef14c28eade13646c9ce Mon Sep 17 00:00:00 2001 From: Adegbite Ayoade Date: Thu, 24 Sep 2026 01:51:39 +0100 Subject: [PATCH 2/3] fix(vscode): handle a failed restart outside the serialized run Review feedback on #6057. The restart failure handler ran inside the serialized task, and the not_signed_in branch of it signs the user in and then restarts the client again. That restart waited on the run that was still waiting on the handler, so a successful Tobiko Cloud sign-in never completed and the server stayed stopped. The run now records the error and returns; the caller handles it once the run has finished, so the restart triggered by signing in starts a fresh run. The error is claimed by whichever caller reads it first, so callers coalesced into the same run do not each report it. Covered by a test that mirrors the shape of the real restart. That test only works because the simulated run awaits before it reports failure: without that await the re-entrant call lands in the synchronous prefix of the run, before it is recorded as in flight, and no deadlock is possible. Verified it times out against the previous arrangement. Signed-off-by: Adegbite Ayoade --- vscode/extension/src/extension.ts | 24 +++++++---- .../src/utilities/coalesceAsync.test.ts | 42 +++++++++++++++++++ 2 files changed, 59 insertions(+), 7 deletions(-) diff --git a/vscode/extension/src/extension.ts b/vscode/extension/src/extension.ts index a18807ac6e..db72414389 100644 --- a/vscode/extension/src/extension.ts +++ b/vscode/extension/src/extension.ts @@ -27,7 +27,7 @@ import { onDidChangePythonInterpreter } from './utilities/common/python' import { requiresLspRestart } from './utilities/common/configurationChange' import { coalesceAsync } from './utilities/coalesceAsync' import { sleep } from './utilities/sleep' -import { handleError } from './utilities/errors' +import { ErrorType, handleError } from './utilities/errors' import { selector, completionProvider } from './completion/completion' import { LineagePanel } from './webviews/lineagePanel' @@ -73,6 +73,14 @@ export async function activate(context: vscode.ExtensionContext) { */ let restartInvokedByUser = false + /** + * Set by a failed run and handled by the caller once the run has finished. + * Handling it inside the run would deadlock: the not_signed_in handler waits + * on a sign-in that restarts the client again, and that restart would wait on + * the run that is still waiting on the handler. + */ + let restartError: ErrorType | undefined + const runRestart = async (): Promise => { const invokedByUser = restartInvokedByUser restartInvokedByUser = false @@ -84,12 +92,7 @@ export async function activate(context: vscode.ExtensionContext) { traceVerbose('Restarting SQLMesh LSP client') const result = await lspClient.restart(invokedByUser) if (isErr(result)) { - await handleError( - authProvider, - restartLsp, - result.error, - 'LSP restart failed', - ) + restartError = result.error return } @@ -116,6 +119,13 @@ export async function activate(context: vscode.ExtensionContext) { const restartLsp = async (invokedByUser = false): Promise => { restartInvokedByUser = restartInvokedByUser || invokedByUser await restartLspSerialized() + + // Claimed so that callers coalesced into the same run don't each report it. + const error = restartError + restartError = undefined + if (error) { + await handleError(authProvider, restartLsp, error, 'LSP restart failed') + } } // commands needing the restart helper diff --git a/vscode/extension/src/utilities/coalesceAsync.test.ts b/vscode/extension/src/utilities/coalesceAsync.test.ts index c5c0c11347..5c87500298 100644 --- a/vscode/extension/src/utilities/coalesceAsync.test.ts +++ b/vscode/extension/src/utilities/coalesceAsync.test.ts @@ -112,4 +112,46 @@ describe('coalesceAsync', () => { task.release() await second }) + + // A failed restart is reported to the caller, which handles it after the run + // has finished. The not_signed_in handler signs the user in and restarts the + // client again, so that handler must not run inside the task: the restart it + // triggers would wait on the run that is still waiting on the handler, and + // neither would ever settle. See #5920. + it('lets a failure handler trigger another run without deadlocking', async () => { + let failNextRun = true + let reportedError: string | undefined + let runs = 0 + + const runRestart = async (): Promise => { + runs += 1 + // The real run awaits the client restart before it knows the outcome. + // Without that await the re-entrant call below lands in the synchronous + // prefix of the run, before it has been recorded as in flight, and the + // deadlock this covers cannot happen. + const failed = await Promise.resolve(failNextRun) + if (failed) { + failNextRun = false + reportedError = 'not_signed_in' + } + } + + const runRestartSerialized = coalesceAsync(runRestart) + + const restart = async (): Promise => { + await runRestartSerialized() + + const error = reportedError + reportedError = undefined + if (error === 'not_signed_in') { + // Stands in for the sign-in flow, which restarts once signed in. + await restart() + } + } + + await restart() + + expect(runs).toBe(2) + expect(reportedError).toBeUndefined() + }, 2_000) }) From 067d511a071ee74e4f4e099c21e07b8b8550b4e2 Mon Sep 17 00:00:00 2001 From: Adegbite Ayoade Date: Thu, 24 Sep 2026 02:38:31 +0100 Subject: [PATCH 3/3] chore(vscode): add SPDX license headers to new files Signed-off-by: Adegbite Ayoade --- vscode/extension/src/utilities/coalesceAsync.test.ts | 2 ++ vscode/extension/src/utilities/coalesceAsync.ts | 2 ++ .../extension/src/utilities/common/configurationChange.test.ts | 2 ++ vscode/extension/src/utilities/common/configurationChange.ts | 2 ++ 4 files changed, 8 insertions(+) diff --git a/vscode/extension/src/utilities/coalesceAsync.test.ts b/vscode/extension/src/utilities/coalesceAsync.test.ts index 5c87500298..f40aba0aaa 100644 --- a/vscode/extension/src/utilities/coalesceAsync.test.ts +++ b/vscode/extension/src/utilities/coalesceAsync.test.ts @@ -1,3 +1,5 @@ +// SPDX-License-Identifier: Apache-2.0 + import { describe, expect, it } from 'vitest' import { coalesceAsync } from './coalesceAsync' diff --git a/vscode/extension/src/utilities/coalesceAsync.ts b/vscode/extension/src/utilities/coalesceAsync.ts index ff5c223c32..46259f9d79 100644 --- a/vscode/extension/src/utilities/coalesceAsync.ts +++ b/vscode/extension/src/utilities/coalesceAsync.ts @@ -1,3 +1,5 @@ +// SPDX-License-Identifier: Apache-2.0 + /** * Serialize an async task so it never overlaps with itself, collapsing any * calls that arrive while it is running into a single rerun. diff --git a/vscode/extension/src/utilities/common/configurationChange.test.ts b/vscode/extension/src/utilities/common/configurationChange.test.ts index 7259bf694f..1a9c633ead 100644 --- a/vscode/extension/src/utilities/common/configurationChange.test.ts +++ b/vscode/extension/src/utilities/common/configurationChange.test.ts @@ -1,3 +1,5 @@ +// SPDX-License-Identifier: Apache-2.0 + import { describe, expect, it } from 'vitest' import { requiresLspRestart } from './configurationChange' diff --git a/vscode/extension/src/utilities/common/configurationChange.ts b/vscode/extension/src/utilities/common/configurationChange.ts index 3676efd85f..7e3ab1f3e0 100644 --- a/vscode/extension/src/utilities/common/configurationChange.ts +++ b/vscode/extension/src/utilities/common/configurationChange.ts @@ -1,3 +1,5 @@ +// SPDX-License-Identifier: Apache-2.0 + /** * Configuration sections the language server reads, so a change to any of them * needs the server restarted to take effect.