From 7626c7f3adf940ee871c4fd2ba2d342f19d9d60b Mon Sep 17 00:00:00 2001 From: Adam Malczewski Date: Thu, 25 Jun 2026 18:11:17 +0900 Subject: fix(lsp): stop per-edit hangs on dead/slow servers (10s cap + skip + self-heal) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The LSP diagnostics path hung up to 60s per edit whenever a configured Ruby language server was dead or slow (the reported Steep langserver case): a killed/crashed server was never detected (stayed "connected" forever), servers were queried sequentially with a 60s budget each, and a corrupted-but-alive server (Steep's ~3h phantom-SyntaxError drift) had no recovery. Four fixes, all in packages/lsp/ (the tool-edit-file call site lowered to 10s): 1. Dead-process detection: SpawnedProcess.onExit (Bun proc.exited) + stdout-end defence flip the client to error, dispose the rpc, kill the proc. The manager re-spawns a fresh server after the 30s backoff. Dead servers are now skipped (0s) instead of polled for 60s. 2. Concurrent fan-out + 10s hard cap: new aggregateDiagnostics queries all matching servers at once, each capped at 10s. A non-responder is skipped with "LSP took too long (>10s), skipped — raise this to the user" instead of blocking the fast server's results. Replaces the vague "unusually long" warning (now structurally impossible: slow is always false). 3. Corruption self-heal: a detector flags a server re-emitting identical non-empty diagnostics despite the file changing; after 5 repeats the client is marked broken and re-spawned. Clean files never trip it. (Acknowledged false-positive risk on persistent unfixed errors; CLI type-check gate stays authoritative.) 4. sendRequest timeout: hover/definition/references cap at 10s so they can't hang the turn against a dead server; the initialize handshake keeps its 45s race. Verification: typecheck clean; 1573 tests pass (96 files), +15 new LSP tests (86 in packages/lsp); biome clean. No kernel/contract changes; onExit is internal to packages/lsp. --- packages/lsp/src/aggregate.test.ts | 141 +++++++++++++++++++++++++++++++++++++ 1 file changed, 141 insertions(+) create mode 100644 packages/lsp/src/aggregate.test.ts (limited to 'packages/lsp/src/aggregate.test.ts') diff --git a/packages/lsp/src/aggregate.test.ts b/packages/lsp/src/aggregate.test.ts new file mode 100644 index 0000000..4579a0a --- /dev/null +++ b/packages/lsp/src/aggregate.test.ts @@ -0,0 +1,141 @@ +import { describe, expect, it } from "vitest"; +import { type AggregateServer, aggregateDiagnostics } from "./aggregate.js"; +import type { LanguageServerClient } from "./client.js"; + +/** + * A minimal fake client: only `waitForDiagnostics` is exercised by + * aggregateDiagnostics, so we stub just that. Cast to the real type (mirrors + * tool.test.ts) — no real process, no internal mocks of our own modules. + */ +function fakeClient( + waitForDiagnostics: LanguageServerClient["waitForDiagnostics"], +): LanguageServerClient { + return { waitForDiagnostics } as unknown as LanguageServerClient; +} + +const SERVER_A: AggregateServer = { id: "a", name: "Ruby-LSP", root: "/p" }; +const SERVER_B: AggregateServer = { id: "b", name: "Steep", root: "/p" }; + +describe("aggregateDiagnostics", () => { + it("returns merged diagnostics from all responding servers, tagged by source", async () => { + const clients = new Map([ + [ + "a", + fakeClient(async () => ({ formatted: "ERROR L1:1: boom", slow: false, timedOut: false })), + ], + [ + "b", + fakeClient(async () => ({ formatted: "WARNING L2:3: meh", slow: false, timedOut: false })), + ], + ]); + + const result = await aggregateDiagnostics( + (id) => clients.get(id), + [SERVER_A, SERVER_B], + "/p/x.rb", + 10_000, + {}, + ); + + expect(result.timedOut).toBe(false); + expect(result.formatted).toContain("[Ruby-LSP]"); + expect(result.formatted).toContain("boom"); + expect(result.formatted).toContain("[Steep]"); + expect(result.formatted).toContain("meh"); + }); + + it("skips a server that times out with a raise-to-user notice, and still returns the fast server's result", async () => { + // Steep never resolves within the cap → timedOut; ruby-lsp answers fast. + const clients = new Map([ + ["a", fakeClient(async () => ({ formatted: "", slow: false, timedOut: false }))], + ["b", fakeClient(async () => ({ formatted: "", slow: false, timedOut: true }))], + ]); + + const result = await aggregateDiagnostics( + (id) => clients.get(id), + [SERVER_A, SERVER_B], + "/p/x.rb", + 10_000, + {}, + ); + + expect(result.timedOut).toBe(true); + // The skip notice names the offending server and the cap. + expect(result.formatted).toContain("[Steep]"); + expect(result.formatted).toContain("took too long"); + expect(result.formatted).toContain(">10s"); + expect(result.formatted).toContain("raise this to the user"); + // ruby-lsp answered cleanly (empty diagnostics) → no line for it. + expect(result.formatted).not.toContain("[Ruby-LSP]"); + }); + + it("runs servers concurrently: a slow server does not delay a fast one's contribution order", async () => { + const callOrder: string[] = []; + const clients = new Map([ + [ + "a", + fakeClient(async () => { + callOrder.push("a-start"); + await new Promise((r) => setTimeout(r, 5)); + callOrder.push("a-end"); + return { formatted: "from-a", slow: false, timedOut: false }; + }), + ], + [ + "b", + fakeClient(async () => { + callOrder.push("b-start"); + await new Promise((r) => setTimeout(r, 30)); + callOrder.push("b-end"); + return { formatted: "from-b", slow: false, timedOut: false }; + }), + ], + ]); + + const result = await aggregateDiagnostics( + (id) => clients.get(id), + [SERVER_A, SERVER_B], + "/p/x.rb", + 10_000, + {}, + ); + + // Both started before either ended → concurrent, not sequential. + expect(callOrder.slice(0, 2).sort()).toEqual(["a-start", "b-start"]); + expect(result.formatted).toContain("from-a"); + expect(result.formatted).toContain("from-b"); + }); + + it("a missing client (dead/excluded) contributes nothing and never rejects", async () => { + const result = await aggregateDiagnostics(() => undefined, [SERVER_A], "/p/x.rb", 10_000, {}); + expect(result.formatted).toBe(""); + expect(result.timedOut).toBe(false); + }); + + it("forwards text + minSeverity to each client's waitForDiagnostics", async () => { + const seen: Array<{ text?: string; minSeverity?: number; timeoutMs: number }> = []; + const clients = new Map([ + [ + "a", + fakeClient(async (_path, opts) => { + seen.push({ + text: opts?.text, + minSeverity: opts?.minSeverity, + timeoutMs: opts?.timeoutMs ?? -1, + }); + return { formatted: "", slow: false, timedOut: false }; + }), + ], + ]); + + await aggregateDiagnostics((id) => clients.get(id), [SERVER_A], "/p/x.rb", 7000, { + text: "post-edit buffer", + minSeverity: 2, + }); + + expect(seen).toHaveLength(1); + expect(seen[0]?.text).toBe("post-edit buffer"); + expect(seen[0]?.minSeverity).toBe(2); + expect(seen[0]?.timeoutMs).toBe(7000); + }); +}); -- cgit v1.2.3