diff options
| author | Adam Malczewski <[email protected]> | 2026-06-25 18:11:17 +0900 |
|---|---|---|
| committer | Adam Malczewski <[email protected]> | 2026-06-25 18:11:17 +0900 |
| commit | 7626c7f3adf940ee871c4fd2ba2d342f19d9d60b (patch) | |
| tree | 17af00238fd8020bf27a482415f37fb79baf0b59 /packages/lsp/src/client.test.ts | |
| parent | c1bc7bfaaca7bdf4d9b2973f5dc88605217a7866 (diff) | |
| download | dispatch-7626c7f3adf940ee871c4fd2ba2d342f19d9d60b.tar.gz dispatch-7626c7f3adf940ee871c4fd2ba2d342f19d9d60b.zip | |
fix(lsp): stop per-edit hangs on dead/slow servers (10s cap + skip + self-heal)
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.
Diffstat (limited to 'packages/lsp/src/client.test.ts')
| -rw-r--r-- | packages/lsp/src/client.test.ts | 146 |
1 files changed, 146 insertions, 0 deletions
diff --git a/packages/lsp/src/client.test.ts b/packages/lsp/src/client.test.ts index 681860f..338ef0b 100644 --- a/packages/lsp/src/client.test.ts +++ b/packages/lsp/src/client.test.ts @@ -3,6 +3,7 @@ import { type FileWatcher, type FsAccess, LanguageServerClient, + type ProcessExitHandler, type SpawnProcess, } from "./client.js"; import { encode } from "./framing.js"; @@ -288,4 +289,149 @@ describe("client", () => { client.shutdown(); expect(state.killed).toBe(true); }); + + it("onExit marks the client broken (error) so callers stop querying a corpse", async () => { + const state = { killed: false }; + let exitCb: ProcessExitHandler | null = null; + const stdoutHolder: { cb: ((data: Uint8Array) => void) | null } = { cb: null }; + + const spawnWithExit: SpawnProcess = () => ({ + stdin: { write: () => {} }, + stdout: { + on: (_event: string, cb: (data: Uint8Array) => void) => { + stdoutHolder.cb = cb; + }, + }, + pid: 999, + kill: () => { + state.killed = true; + }, + onExit: (handler) => { + exitCb = handler; + }, + }); + + const { client } = makeClient({ spawn: spawnWithExit }); + const startPromise = client.start(); + await new Promise((r) => setTimeout(r, 50)); + stdoutHolder.cb?.( + encode(JSON.stringify({ jsonrpc: "2.0", id: 1, result: { capabilities: {} } })), + ); + await startPromise; + expect(client.getState()).toBe("connected"); + + // Simulate the process dying (user kill / crash). + exitCb?.({ code: 1 }); + + expect(client.getState()).toBe("error"); + expect(client.getStateError()).toMatch(/process exited/); + // The (still-alive-in-test) process was killed to avoid a zombie. + expect(state.killed).toBe(true); + }); + + it("a dead client is skipped by waitForDiagnostics callers (state !== connected)", async () => { + // Build a client, connect, kill via onExit, then assert a diagnostics + // query would not block: getState() is "error" so the matching filter + // (state === "connected") excludes it. We assert the state guard. + const stdoutHolder: { cb: ((data: Uint8Array) => void) | null } = { cb: null }; + let exitCb: ProcessExitHandler | null = null; + const spawnWithExit: SpawnProcess = () => ({ + stdin: { write: () => {} }, + stdout: { + on: (_e, cb) => { + stdoutHolder.cb = cb; + }, + }, + pid: 1, + kill: () => {}, + onExit: (handler) => { + exitCb = handler; + }, + }); + + const { client } = makeClient({ spawn: spawnWithExit }); + const startPromise = client.start(); + await new Promise((r) => setTimeout(r, 50)); + stdoutHolder.cb?.( + encode(JSON.stringify({ jsonrpc: "2.0", id: 1, result: { capabilities: {} } })), + ); + await startPromise; + + exitCb?.({ code: null }); + expect(client.getState()).toBe("error"); + // The aggregate / getDiagnostics matching filter requires "connected". + expect(client.getState() === "connected").toBe(false); + }); + + it("corruption detector marks the client broken after repeated identical diagnostics despite text changes", async () => { + // A healthy server would change diagnostics as the file changes; a + // corrupted one re-emits the SAME non-empty set. Drive 5 edits with + // different text but identical diagnostics → client flips to error. + const { client, serverResponses } = makeClient(); + const startPromise = client.start(); + await new Promise((r) => setTimeout(r, 50)); + serverResponses(JSON.stringify({ jsonrpc: "2.0", id: 1, result: { capabilities: {} } })); + await startPromise; + + const phantom = JSON.stringify({ + jsonrpc: "2.0", + method: "textDocument/publishDiagnostics", + params: { + uri: "file:///project/game.rb", + diagnostics: [ + { + range: { start: { line: 0, character: 27 }, end: { line: 0, character: 28 } }, + severity: 1, + message: "SyntaxError: unexpected token", + }, + ], + }, + }); + + const path = "/project/game.rb"; + // The first call establishes the baseline snapshot (no increment). + // Each subsequent call with identical diagnostics + changed text + // increments; the 6th call (5th increment) trips the threshold. + for (let i = 1; i <= 5; i++) { + const p = client.waitForDiagnostics(path, { text: `buf-v${i}`, timeoutMs: 2000 }); + // Push the identical phantom diagnostics so the poll resolves. + await new Promise((r) => setTimeout(r, 30)); + serverResponses(phantom); + await p; + expect(client.getState()).toBe("connected"); + } + // 6th identical-across-changed-text repeat trips the threshold. + const p6 = client.waitForDiagnostics(path, { text: "buf-v6", timeoutMs: 2000 }); + await new Promise((r) => setTimeout(r, 30)); + serverResponses(phantom); + await p6; + + expect(client.getState()).toBe("error"); + expect(client.getStateError()).toMatch(/repeated stale diagnostics/i); + }); + + it("corruption detector does NOT trip on a clean file (empty diagnostics stay identical)", async () => { + const { client, serverResponses } = makeClient(); + const startPromise = client.start(); + await new Promise((r) => setTimeout(r, 50)); + serverResponses(JSON.stringify({ jsonrpc: "2.0", id: 1, result: { capabilities: {} } })); + await startPromise; + + const clean = JSON.stringify({ + jsonrpc: "2.0", + method: "textDocument/publishDiagnostics", + params: { uri: "file:///project/game.rb", diagnostics: [] }, + }); + const path = "/project/game.rb"; + + for (let i = 1; i <= 6; i++) { + const p = client.waitForDiagnostics(path, { text: `clean-v${i}`, timeoutMs: 2000 }); + await new Promise((r) => setTimeout(r, 30)); + serverResponses(clean); + await p; + } + // Empty diagnostics never count as "stale" — a clean file staying clean + // is normal, not corruption. + expect(client.getState()).toBe("connected"); + }); }); |
