From d01ddace832b2e3c58b9b120c43bb08cc42d428e Mon Sep 17 00:00:00 2001 From: Matt Hillsdon Date: Thu, 12 Nov 2026 09:39:31 +0000 Subject: [PATCH 1/2] Retry language server start-up when its locale script fails to load Pyright fetches its messages for the locale as the first step of initialize. When that fetch failed while the browser reported being online, the error was rethrown and the rejected promise kept, so the session had no completions, signature help or diagnostics until a reload. This is most of PYTHON-EDITOR-NEXT-3SJ. Retry initialize with the same back-off as other lazy loads. Nothing else has happened in the server at that point, so it can be sent again on the same connection. If every attempt fails, a later call starts afresh rather than replaying the failure. The offline fallback to the precached locale now awaits its second attempt and no longer loops if that fails too. --- src/common/chunk-util.ts | 6 +- src/e2e/language-server-retry.test.ts | 32 ++++ src/language-server/client.test.ts | 126 ++++++++++++++++ src/language-server/client.ts | 207 +++++++++++++++----------- 4 files changed, 279 insertions(+), 92 deletions(-) create mode 100644 src/e2e/language-server-retry.test.ts create mode 100644 src/language-server/client.test.ts diff --git a/src/common/chunk-util.ts b/src/common/chunk-util.ts index 8bb416654..36701c973 100644 --- a/src/common/chunk-util.ts +++ b/src/common/chunk-util.ts @@ -10,7 +10,8 @@ const defaultWaiter = (waitTime: number): Promise => export const retryAsyncLoad = async ( load: () => Promise, - waiter: (waitTime: number) => Promise = defaultWaiter + waiter: (waitTime: number) => Promise = defaultWaiter, + shouldRetry: (e: unknown) => boolean = () => true ): Promise => { let waitTime = 250; let attempts = 0; @@ -20,6 +21,9 @@ export const retryAsyncLoad = async ( // Must await here! return await load(); } catch (e) { + if (!shouldRetry(e)) { + throw e; + } // If the user is offline, bail immediately. if (!navigator.onLine) { throw new OfflineError(); diff --git a/src/e2e/language-server-retry.test.ts b/src/e2e/language-server-retry.test.ts new file mode 100644 index 000000000..81279dd87 --- /dev/null +++ b/src/e2e/language-server-retry.test.ts @@ -0,0 +1,32 @@ +/** + * (c) 2026, Micro:bit Educational Foundation and contributors + * + * SPDX-License-Identifier: MIT + */ +import { expect } from "@playwright/test"; +import { test } from "./app-test-fixtures.js"; + +test.describe("language server start-up", () => { + // A service worker would serve the script from its precache, out of + // reach of the route below. + test.use({ serviceWorkers: "block" }); + + let requests: number; + + test.beforeEach(async ({ context }) => { + requests = 0; + await context.route(/pyright-locale-/, (route) => + ++requests <= 2 ? route.abort("failed") : route.continue() + ); + }); + + test("recovers when its language script fails to load", async ({ app }) => { + // Two failures, then the foreground worker and the background worker it + // starts once initialized each load it. + await expect.poll(() => requests).toBe(4); + + await app.selectAllInEditor(); + await app.typeInEditor("from microbit import *\ndisplay.s"); + await app.expectCompletionOptions(["scroll", "set_pixel", "show"]); + }); +}); diff --git a/src/language-server/client.test.ts b/src/language-server/client.test.ts new file mode 100644 index 000000000..14da51e04 --- /dev/null +++ b/src/language-server/client.test.ts @@ -0,0 +1,126 @@ +/** + * (c) 2026, Micro:bit Educational Foundation and contributors + * + * SPDX-License-Identifier: MIT + */ +import { ToastFn } from "@microbit/ui"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { + ConnectionError, + ConnectionErrors, + MessageConnection, +} from "vscode-jsonrpc"; +import { + InitializedNotification, + InitializeParams, + InitializeRequest, +} from "vscode-languageserver-protocol"; +import { createUri, LanguageServerClient } from "./client"; + +const loadFailed = () => + new Error("Request initialize failed with message: Load failed"); + +const createClient = (locale: string = "fr") => { + const sendRequest = vi.fn(); + const sendNotification = vi.fn(); + const connection = { + sendRequest, + sendNotification, + onNotification: vi.fn(), + onRequest: vi.fn(), + } as unknown as MessageConnection; + const toast = vi.fn(); + const client = new LanguageServerClient( + connection, + locale, + createUri(""), + toast as unknown as ToastFn + ); + const requestedLocales = () => + sendRequest.mock.calls.map((call) => { + const [type, params] = call as [unknown, InitializeParams]; + expect(type).toBe(InitializeRequest.type); + return params.locale; + }); + return { client, sendRequest, sendNotification, toast, requestedLocales }; +}; + +describe("LanguageServerClient initialize", () => { + let onLine: boolean; + + beforeEach(() => { + onLine = true; + vi.spyOn(navigator, "onLine", "get").mockImplementation(() => onLine); + // Loading the real typeshed is slow and is I/O that fake timers can't + // advance past. + vi.spyOn( + LanguageServerClient.prototype as any, + "getInitializationOptions" + ).mockResolvedValue({}); + vi.useFakeTimers(); + }); + + afterEach(() => { + vi.useRealTimers(); + vi.restoreAllMocks(); + }); + + it("retries when the server fails to initialize", async () => { + const { client, sendRequest, sendNotification, toast, requestedLocales } = + createClient(); + const capabilities = { hoverProvider: true }; + sendRequest + .mockRejectedValueOnce(loadFailed()) + .mockRejectedValueOnce(loadFailed()) + .mockResolvedValueOnce({ capabilities }); + + const result = client.initialize(); + await vi.runAllTimersAsync(); + + expect(await result).toBe(true); + expect(requestedLocales()).toEqual(["fr", "fr", "fr"]); + expect(client.capabilities).toBe(capabilities); + expect(sendNotification).toHaveBeenCalledExactlyOnceWith( + InitializedNotification.type, + {} + ); + expect(toast).not.toHaveBeenCalled(); + }); + + it("can be tried again after giving up", async () => { + const { client, sendRequest } = createClient(); + sendRequest.mockRejectedValue(loadFailed()); + + const failed = expect(client.initialize()).rejects.toThrow("Load failed"); + await vi.runAllTimersAsync(); + await failed; + expect(sendRequest).toHaveBeenCalledTimes(5); + + sendRequest.mockReset().mockResolvedValue({ capabilities: {} }); + expect(await client.initialize()).toBe(true); + }); + + it("falls back to the precached locale when offline", async () => { + const { client, sendRequest, toast, requestedLocales } = createClient(); + onLine = false; + sendRequest + .mockRejectedValueOnce(loadFailed()) + .mockResolvedValueOnce({ capabilities: {} }); + + expect(await client.initialize()).toBe(true); + expect(requestedLocales()).toEqual(["fr", "en"]); + expect(client.locale).toBe("en"); + expect(toast).toHaveBeenCalledTimes(1); + }); + + it("does not retry when the connection was disposed", async () => { + const { client, sendRequest, sendNotification } = createClient(); + sendRequest.mockRejectedValue( + new ConnectionError(ConnectionErrors.Disposed, "Connection is disposed.") + ); + + expect(await client.initialize()).toBe(false); + expect(sendRequest).toHaveBeenCalledTimes(1); + expect(sendNotification).not.toHaveBeenCalled(); + }); +}); diff --git a/src/language-server/client.ts b/src/language-server/client.ts index 0e837087f..cd6f332c0 100644 --- a/src/language-server/client.ts +++ b/src/language-server/client.ts @@ -70,6 +70,7 @@ export class LanguageServerClient extends TypedEventTarget { private versions: Map = new Map(); private diagnostics: Map = new Map(); private initializePromise: Promise | undefined; + private handlersRegistered = false; constructor( public connection: MessageConnection, @@ -103,109 +104,133 @@ export class LanguageServerClient extends TypedEventTarget { } this.initializePromise = (async () => { try { - this.connection.onNotification(LogMessageNotification.type, (params) => - console.log("[LS]", params.message) - ); - - this.connection.onNotification( - PublishDiagnosticsNotification.type, - (params) => { - this.diagnostics.set(params.uri, params.diagnostics); - // Republish as you can't listen twice. - this.dispatchTypedEvent( - "diagnostics", - new DiagnosticsEvent(params) - ); - } - ); - this.connection.onRequest(RegistrationRequest.type, () => { - // Ignore. I don't think we should get these at all given our - // capabilities, but Pyright is sending one anyway. - }); - - const initializeParams: InitializeParams = { - // Our pyright fork names its message files in lowercase. - locale: this.locale.toLowerCase(), - capabilities: { - textDocument: { - moniker: {}, - synchronization: { - willSave: false, - didSave: false, - willSaveWaitUntil: false, - }, - completion: { - completionItem: { - snippetSupport: false, - commitCharactersSupport: true, - documentationFormat: ["markdown"], - deprecatedSupport: false, - preselectSupport: false, - }, - contextSupport: true, - }, - signatureHelp: { - signatureInformation: { - documentationFormat: ["markdown"], - activeParameterSupport: true, - parameterInformation: { - labelOffsetSupport: true, - }, - }, - }, - publishDiagnostics: { - tagSupport: { - valueSet: [ - DiagnosticTag.Unnecessary, - DiagnosticTag.Deprecated, - ], - }, - }, - }, - workspace: { - workspaceFolders: true, - didChangeConfiguration: {}, - configuration: true, - }, - }, - initializationOptions: await this.getInitializationOptions(), - processId: null, - // Do we need both of these? - rootUri: this.rootUri, - workspaceFolders: [ - { - name: "src", - uri: this.rootUri, - }, - ], - }; - const { capabilities } = await this.connection.sendRequest( - InitializeRequest.type, - initializeParams - ); - this.capabilities = capabilities; + this.registerHandlers(); + this.capabilities = await this.initializeWithRetry(); this.connection.sendNotification(InitializedNotification.type, {}); + return true; } catch (e) { if (isErrorDueToDispose(e)) { // We've intentionally disposed the connection because we're recreating the client. // This mostly happens due to React 18 strict mode but could happen due to language changes. return false; } - if (!navigator.onLine) { - showOfflineLanguageToast(this.toast); - // Fallback to the precached locale if user is offline. - this.locale = fallbackLocale; - this.initializePromise = undefined; - this.initialize(); - } else { - throw e; - } + // Let a later call try again rather than replaying this failure. + this.initializePromise = undefined; + throw e; } - return true; })(); return this.initializePromise; } + private registerHandlers() { + if (this.handlersRegistered) { + return; + } + this.handlersRegistered = true; + this.connection.onNotification(LogMessageNotification.type, (params) => + console.log("[LS]", params.message) + ); + this.connection.onNotification( + PublishDiagnosticsNotification.type, + (params) => { + this.diagnostics.set(params.uri, params.diagnostics); + // Republish as you can't listen twice. + this.dispatchTypedEvent("diagnostics", new DiagnosticsEvent(params)); + } + ); + this.connection.onRequest(RegistrationRequest.type, () => { + // Ignore. I don't think we should get these at all given our + // capabilities, but Pyright is sending one anyway. + }); + } + + /** + * The server loads its messages for the locale over the network before + * anything else, so initialize fails on a network error and is safe to + * send again on the same connection. + */ + private async initializeWithRetry(): Promise { + const sendInitialize = async (params: InitializeParams) => { + const { capabilities } = await this.connection.sendRequest( + InitializeRequest.type, + params + ); + return capabilities; + }; + const params = await this.getInitializeParams(); + try { + return await retryAsyncLoad( + () => sendInitialize(params), + undefined, + (e) => !isErrorDueToDispose(e) + ); + } catch (e) { + if (e instanceof OfflineError && this.locale !== fallbackLocale) { + showOfflineLanguageToast(this.toast); + // The fallback locale is precached. + this.locale = fallbackLocale; + return sendInitialize(await this.getInitializeParams()); + } + throw e; + } + } + + private async getInitializeParams(): Promise { + return { + // Our pyright fork names its message files in lowercase. + locale: this.locale.toLowerCase(), + capabilities: { + textDocument: { + moniker: {}, + synchronization: { + willSave: false, + didSave: false, + willSaveWaitUntil: false, + }, + completion: { + completionItem: { + snippetSupport: false, + commitCharactersSupport: true, + documentationFormat: ["markdown"], + deprecatedSupport: false, + preselectSupport: false, + }, + contextSupport: true, + }, + signatureHelp: { + signatureInformation: { + documentationFormat: ["markdown"], + activeParameterSupport: true, + parameterInformation: { + labelOffsetSupport: true, + }, + }, + }, + publishDiagnostics: { + tagSupport: { + valueSet: [DiagnosticTag.Unnecessary, DiagnosticTag.Deprecated], + }, + }, + }, + workspace: { + workspaceFolders: true, + didChangeConfiguration: {}, + configuration: true, + }, + }, + initializationOptions: await this.getInitializationOptions(), + processId: null, + // Do we need both of these? + rootUri: this.rootUri, + workspaceFolders: [ + { + name: "src", + uri: this.rootUri, + }, + ], + }; + } + private async getInitializationOptions(): Promise { const branch = microPythonConfig.stubs; let typeshed; From 28ab5df327d92b4da0203bdf09e608d65ae84425 Mon Sep 17 00:00:00 2001 From: Matt Hillsdon Date: Thu, 12 Nov 2026 10:04:29 +0000 Subject: [PATCH 2/2] Don't wait on a language server for a language we no longer want pyright() awaited the cached client's initialize before comparing languages. Now that a failed initialize can be tried again, a language change after a failure would run a full round of retries for the old locale, and give up without creating the new client if they failed. Compare first and dispose a client for another language straight away. Drop the handlersRegistered flag: vscode-jsonrpc keys handlers by method, so registering again replaces them. Relax the e2e request count so it doesn't depend on how many workers Pyright starts. --- src/e2e/language-server-retry.test.ts | 4 +--- src/language-server/client.ts | 5 ----- src/language-server/pyright.ts | 15 ++++++++------- 3 files changed, 9 insertions(+), 15 deletions(-) diff --git a/src/e2e/language-server-retry.test.ts b/src/e2e/language-server-retry.test.ts index 81279dd87..a20cfcd79 100644 --- a/src/e2e/language-server-retry.test.ts +++ b/src/e2e/language-server-retry.test.ts @@ -21,9 +21,7 @@ test.describe("language server start-up", () => { }); test("recovers when its language script fails to load", async ({ app }) => { - // Two failures, then the foreground worker and the background worker it - // starts once initialized each load it. - await expect.poll(() => requests).toBe(4); + await expect.poll(() => requests).toBeGreaterThan(2); await app.selectAllInEditor(); await app.typeInEditor("from microbit import *\ndisplay.s"); diff --git a/src/language-server/client.ts b/src/language-server/client.ts index cd6f332c0..459c5879e 100644 --- a/src/language-server/client.ts +++ b/src/language-server/client.ts @@ -70,7 +70,6 @@ export class LanguageServerClient extends TypedEventTarget { private versions: Map = new Map(); private diagnostics: Map = new Map(); private initializePromise: Promise | undefined; - private handlersRegistered = false; constructor( public connection: MessageConnection, @@ -123,10 +122,6 @@ export class LanguageServerClient extends TypedEventTarget { } private registerHandlers() { - if (this.handlersRegistered) { - return; - } - this.handlersRegistered = true; this.connection.onNotification(LogMessageNotification.type, (params) => console.log("[LS]", params.message) ); diff --git a/src/language-server/pyright.ts b/src/language-server/pyright.ts index 13a1ca60a..b5b6d8a04 100644 --- a/src/language-server/pyright.ts +++ b/src/language-server/pyright.ts @@ -45,15 +45,16 @@ export const pyright = async ( return undefined; } if (cache) { - // This is safe to call if already initialized. - await cache.client.initialize(); + const { client } = cache; if (cache.language === language) { - return cache.client; - } else { - // Dispose it, we'll create a new one. - cache?.client.dispose(); - cache = undefined; + // This is safe to call if already initialized. + await client.initialize(); + return client; } + // Dispose without waiting for it to initialize, which could be a long + // wait on retries for a language we no longer want. + client.dispose(); + cache = undefined; } const idSuffix = counter++;