fix(lsp): bound restart shutdown, drop stale messages, de-flake tests

Restart was awaiting the graceful LSP shutdown handshake unbounded, so a
busy or cold server could stall a project-switch restart. Bound it to 3s
then hard-kill (the graceful path is kept for servers that need it). Also
drop inbound messages for a server that is stopping, so stale diagnostics
from a dying instance can't leak into the fresh one that replaces it.

Tests: warm up tsserver once in the TypeScript LSP integration beforeAll
(its one-time cold start can exceed a spec timeout on slow CI runners) and
add per-spec timeout headroom; replace the fixed-time wait in the
jump-to-definition spec with a retry until the LSP result lands.

Author: abose

src/extensions/default/TypeScriptSupport/unittests.js

@@ -59,7 +59,18 @@ define(function (require, exports, module) {
             await awaitsFor(function () {
                 return testWindow._TypeScriptSupportReadyToIntegTest;
             }, "TypeScript LSP server to start", 30000);
-        }, 30000);
+
+            // Warm up tsserver. Its very first request pays a large one-time cost - spawning node,
+            // launching vtsls, and loading the TypeScript library + project - which on a slow/loaded
+            // CI runner can exceed a single spec's timeout (fast dev machines never see it). Pay it
+            // once here with a generous budget so every spec below talks to an already-primed server;
+            // later project-switch restarts reuse the warm process and are fast.
+            await SpecRunnerUtils.loadProjectInTestWindow(testFolder + "ts/");
+            await awaitsForDone(SpecRunnerUtils.openProjectFiles(["type-error.ts"]), "warm-up: open type-error.ts");
+            await awaitsFor(function () {
+                return $("#problems-panel").text().includes("not assignable");
+            }, "tsserver to warm up on first cold start", 90000);
+        }, 100000);
 
         afterAll(async function () {
             testWindow = null;
@@ -85,34 +96,34 @@ define(function (require, exports, module) {
             // type-error.ts assigns a string to a `number` -> TS2322 "... not assignable ...".
             await awaitsFor(function () {
                 return panelText().includes("not assignable");
-            }, "TypeScript type error to be reported", 20000);
-        }, 30000);
+            }, "TypeScript type error to be reported", 30000);
+        }, 45000);
 
         it("should report implicit-any in a JS project that opts into checkJs", async function () {
             // js-checkjs has a jsconfig.json with checkJs + noImplicitAny, so the untyped parameter
             // in implicit.js IS flagged - and our diagnostic filter keeps it (the project opted in).
             await _openInProject("js-checkjs/", "implicit.js");
             await awaitsFor(function () {
                 return panelText().includes(IMPLICIT_ANY_MESSAGE);
-            }, "implicit-any to be reported under checkJs", 20000);
-        }, 30000);
+            }, "implicit-any to be reported under checkJs", 30000);
+        }, 45000);
 
         it("should NOT report implicit-any in a plain JS project", async function () {
             // Precondition: confirm the server actually produces implicit-any for this exact code
             // (under checkJs), so the plain-project assertion below reflects gating, not just timing.
             await _openInProject("js-checkjs/", "implicit.js");
             await awaitsFor(function () {
                 return panelText().includes(IMPLICIT_ANY_MESSAGE);
-            }, "implicit-any under checkJs (precondition)", 20000);
+            }, "implicit-any under checkJs (precondition)", 30000);
 
             // Same code in a plain JS project (no jsconfig / no @ts-check): the "go add types" nag
             // must not appear. Wait for inspection to settle clean, then assert it is absent.
             await _openInProject("js-plain/", "implicit.js");
             await awaitsFor(function () {
                 return $("#status-inspection").hasClass("inspection-valid");
-            }, "plain JS inspection to settle with no problems", 20000);
+            }, "plain JS inspection to settle with no problems", 30000);
             expect(panelText().includes(IMPLICIT_ANY_MESSAGE)).toBe(false);
-        }, 30000);
+        }, 75000);
 
         // ----- hover quick-actions (Go to Definition / Find Usages) -------------------------------
 
@@ -138,7 +149,7 @@ define(function (require, exports, module) {
                 await awaitsFor(async function () {
                     popover = await _hoverPopoverAt(editor, CALL_LINE, CALL_CH);
                     return !!(popover && popover.content && popover.content.find(".lsp-hover-action").length === 2);
-                }, "hover quick actions to appear", 20000);
+                }, "hover quick actions to appear", 30000);
 
                 const labels = popover.content.find(".lsp-hover-action-label").map(function () {
                     return $(this).text();
@@ -158,16 +169,16 @@ define(function (require, exports, module) {
                         $act.trigger("click");
                     }
                     return EditorManager.getCurrentFullEditor().getCursorPos().line === DECL_LINE;
-                }, "Go to Definition to navigate to the declaration", 25000);
+                }, "Go to Definition to navigate to the declaration", 30000);
                 expect(EditorManager.getCurrentFullEditor().getCursorPos().line).toBe(DECL_LINE);
-            }, 40000);
+            }, 75000);
 
             it("hover Find Usages opens the references panel (" + tc.ext + ")", async function () {
                 await _openInProject(tc.folder, tc.file);
                 const editor = EditorManager.getCurrentFullEditor();
                 await awaitsFor(async function () {
                     return !!(await _hoverPopoverAt(editor, CALL_LINE, CALL_CH));
-                }, "hover popover to be available", 20000);
+                }, "hover popover to be available", 30000);
 
                 // "Find Usages" is the right-aligned action; clicking it opens the references panel.
                 // Retry through the hover until the panel opens (the server may still be indexing).
@@ -181,9 +192,9 @@ define(function (require, exports, module) {
                         $end.trigger("click");
                     }
                     return $("#reference-in-files-results").is(":visible");
-                }, "references panel to open", 25000);
+                }, "references panel to open", 30000);
                 expect($("#reference-in-files-results").is(":visible")).toBe(true);
-            }, 40000);
+            }, 75000);
         });
     });
 });

src/languageTools/LSPClient.js

@@ -167,6 +167,12 @@ define(function (require, exports, module) {
         if (!client) {
             return;
         }
+        if (client._stopping) {
+            // The server is shutting down/restarting. Ignore any late messages it emits during the
+            // teardown window so stale diagnostics from the dying instance don't leak into the fresh
+            // one that replaces it (both share the same serverId).
+            return;
+        }
         if (data.method === "textDocument/publishDiagnostics" && client.lintingProvider) {
             const params = data.params || {};
             // Rewrite the URI to a VFS-based URI so the linting provider keys results by the
@@ -647,6 +653,27 @@ define(function (require, exports, module) {
      * @param {string} serverId
      * @return {Promise<void>}
      */
+    // How long to wait for a server to acknowledge a graceful `shutdown` before we hard-kill it.
+    // Healthy servers reply in well under this; the cap is a failsafe so a slow/buggy/hung server
+    // can't stall the restart indefinitely.
+    const SHUTDOWN_TIMEOUT_MS = 3000;
+
+    // Resolve/reject with `promise`, but reject with a timeout error if it doesn't settle in `ms`.
+    function _withTimeout(promise, ms) {
+        return new Promise(function (resolve, reject) {
+            const timer = setTimeout(function () {
+                reject(new Error("timeout"));
+            }, ms);
+            promise.then(function (value) {
+                clearTimeout(timer);
+                resolve(value);
+            }, function (err) {
+                clearTimeout(timer);
+                reject(err);
+            });
+        });
+    }
+
     async function restartLanguageServer(serverId) {
         const client = clients.get(serverId);
         if (!client) {
@@ -676,11 +703,19 @@ define(function (require, exports, module) {
         client.capabilities = null;
         client._completionCache = null;
         DocumentSync.clearServer(client);
+        // Attempt a graceful LSP shutdown - some servers need it to flush state or clean up child
+        // processes - but BOUND it. The `shutdown` request blocks until the server replies, and a
+        // busy or cold server can be slow (or never reply), which would stall the restart; on a
+        // project switch we'd end up waiting for the old server to finish booting just to tell it to
+        // die, then cold-start a new one (a double penalty on slow CI). Give it a short budget, then
+        // hard-kill regardless. The `exit` notification expects no reply, so it stays cheap.
         try {
-            await conn.execPeer("sendRequest", { serverId: client.serverId, method: "shutdown", params: null });
+            await _withTimeout(
+                conn.execPeer("sendRequest", { serverId: client.serverId, method: "shutdown", params: null }),
+                SHUTDOWN_TIMEOUT_MS);
             await conn.execPeer("sendNotification", { serverId: client.serverId, method: "exit", params: null });
         } catch (e) {
-            // Server may already be dead; fall through to a hard stop.
+            // Timed out, or the server is already dead - fall through to the hard stop.
         }
         await conn.execPeer("stopServer", { serverId: client.serverId });
         client._stopping = false;

test/spec/EditorCommandHandlers-integ-test.js

@@ -191,35 +191,44 @@ define(function (require, exports, module) {
 
         describe("Editor Navigation Commands", function () {
             it("should jump to definition", async function () {
-                var promise,
-                    selection;
-                promise = CommandManager.execute(Commands.CMD_ADD_TO_WORKINGSET_AND_OPEN, {fullPath: testPath + "/test.js"});
-                await awaitsForDone(promise, "Open into working set");
-
+                var selection;
+                await awaitsForDone(
+                    CommandManager.execute(Commands.CMD_ADD_TO_WORKINGSET_AND_OPEN, {fullPath: testPath + "/test.js"}),
+                    "Open into working set");
                 myEditor = EditorManager.getCurrentFullEditor();
-                myEditor.setCursorPos({line: 5, ch: 8});
-                await awaits(1500); // for the code intelligence framework to prime up
-                promise = CommandManager.execute(Commands.NAVIGATE_JUMPTO_DEFINITION);
-                await awaitsForDone(promise, "Jump To Definition");
 
-                selection = myEditor.getSelection();
                 if (window.Phoenix.isNativeApp) {
-                    // Desktop has the LSP server (vtsls), which now provides jump-to-definition
-                    // for JavaScript (higher priority than the built-in Tern provider). vtsls
-                    // returns the full declaration range of testMe and we jump to its start
-                    // (the `function` keyword), giving a collapsed cursor at {0,0} - whereas Tern
-                    // selects the identifier name. Both correctly land on the testMe definition.
+                    // Desktop has the LSP server (vtsls), which provides jump-to-definition for
+                    // JavaScript at a higher priority than the built-in Tern provider. vtsls returns
+                    // testMe's full declaration range and we jump to its start (the `function`
+                    // keyword), giving a collapsed cursor at {0,0} - whereas Tern selects the
+                    // identifier name. The server can take a moment to prime after the file opens, so
+                    // retry the jump until the LSP result lands rather than using a fixed wait (which
+                    // flakes on slow CI: before vtsls is ready, Tern answers with {0,9}-{0,15}).
+                    await awaitsFor(async function () {
+                        myEditor.setCursorPos({line: 5, ch: 8});
+                        await awaitsForDone(CommandManager.execute(Commands.NAVIGATE_JUMPTO_DEFINITION),
+                            "Jump To Definition");
+                        var sel = myEditor.getSelection();
+                        return sel.start.line === 0 && sel.start.ch === 0 &&
+                            sel.end.line === 0 && sel.end.ch === 0;
+                    }, "LSP jump-to-definition to land on the testMe declaration", 30000);
+                    selection = myEditor.getSelection();
                     expect(fixSel(selection)).toEql(fixSel({
                         start: {line: 0, ch: 0},
                         end: {line: 0, ch: 0}
                     }));
                 } else {
+                    myEditor.setCursorPos({line: 5, ch: 8});
+                    await awaitsForDone(CommandManager.execute(Commands.NAVIGATE_JUMPTO_DEFINITION),
+                        "Jump To Definition");
+                    selection = myEditor.getSelection();
                     expect(fixSel(selection)).toEql(fixSel({
                         start: {line: 0, ch: 9},
                         end: {line: 0, ch: 15}
                     }));
                 }
-            });
+            }, 35000);
         });
 
         if(window.Phoenix.isNativeApp) {