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.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.ts')
| -rw-r--r-- | packages/lsp/src/client.ts | 153 |
1 files changed, 139 insertions, 14 deletions
diff --git a/packages/lsp/src/client.ts b/packages/lsp/src/client.ts index 677a22a..ac7d025 100644 --- a/packages/lsp/src/client.ts +++ b/packages/lsp/src/client.ts @@ -4,13 +4,22 @@ * FileWatcher, and forwards matching disk changes. */ -import { DiagnosticsStore, type PublishDiagnosticsParams } from "./diagnostics.js"; +import { DiagnosticsStore, diagnosticKey, type PublishDiagnosticsParams } from "./diagnostics.js"; import { computeChangeRange } from "./diff.js"; import { FrameDecoder } from "./framing.js"; import { languageId as resolveLanguageId } from "./language.js"; import { JsonRpcConnection, type WriteFn } from "./rpc.js"; import { FileChangeType, WatchedFilesRegistry } from "./watched-files.js"; +/** Info delivered to an `onExit` handler when the child process terminates. */ +export interface ProcessExitInfo { + readonly code: number | null; + readonly signal?: string; +} + +/** A handler registered to be called when the child process exits. */ +export type ProcessExitHandler = (info: ProcessExitInfo) => void; + export interface SpawnedProcess { readonly stdin: { readonly write: (bytes: Uint8Array) => void }; readonly stdout: @@ -22,6 +31,13 @@ export interface SpawnedProcess { | undefined; readonly pid: number | undefined; readonly kill: () => void; + /** + * Register a handler fired when the child process exits (code|signal). + * Optional: when absent, death is detected via stdout-end instead. Wires + * Bun's `proc.exited` in production; tests invoke it directly to simulate + * a crash. Lets the client stop querying a dead server (no per-edit hang). + */ + readonly onExit?: ((handler: ProcessExitHandler) => void) | undefined; } export type SpawnProcess = ( @@ -102,6 +118,18 @@ export class LanguageServerClient { private openDocuments = new Map<string, { version: number; text: string }>(); /** Sync mode captured from the server's initialize capabilities: 1=Full, 2=Incremental. */ private textDocumentChange: 1 | 2 = 1; + /** + * Corruption detection: the last diagnostic-key set + synced text per URI. + * A healthy server's diagnostics change when the file changes; a corrupted + * one (e.g. Steep's ~3h phantom-SyntaxError drift) re-emits the identical + * non-empty set across edits. `staleRepeat` counts consecutive such repeats + * across URIs; at the threshold the client is marked broken (→ respawn). + */ + private lastDiagSnapshot = new Map<string, { keys: Set<string>; text: string }>(); + private staleRepeat = 0; + private static readonly STALE_REPEAT_THRESHOLD = 5; + /** Default timeout for outbound requests (hover/definition/references). */ + private static readonly REQUEST_TIMEOUT_MS = 10_000; constructor(deps: ClientDeps) { this.deps = deps; @@ -128,6 +156,12 @@ export class LanguageServerClient { } const proc = this.deps.spawn(this.deps.command as string[], spawnOpts); this.process = proc; + // Detect process death so we stop querying a corpse (fixes the + // per-edit hang after a server is killed/crashes). onExit is the + // primary signal; stdout-end is the defence-in-depth fallback. + if (proc.onExit) { + proc.onExit((info) => this.handleExit(info)); + } const writeFn: WriteFn = (bytes) => proc.stdin.write(bytes); const rpc = new JsonRpcConnection(writeFn); @@ -158,8 +192,11 @@ export class LanguageServerClient { for await (const chunk of source) { this.handleBytes(chunk); } + // stdout closed — the process is gone (defence-in-depth alongside onExit, + // which some edges never call). Idempotent via handleExit's guard. + this.handleExit({ code: null }); } catch { - // process exited + this.handleExit({ code: null }); } })(); } @@ -172,6 +209,65 @@ export class LanguageServerClient { }); } + /** + * The server process exited (onExit or stdout-end). Transition to a broken + * state so callers skip it and the manager re-spawns after backoff — instead + * of polling a corpse for the full timeout on every edit. Idempotent. + */ + private handleExit(info: ProcessExitInfo): void { + if (this.state === "error" || this.state === "not-started") return; + const detail = info.signal !== undefined ? `signal ${info.signal}` : `code ${info.code ?? "?"}`; + this.markBroken(`language server process exited (${detail})`); + } + + /** + * Mark this client permanently broken: kill the process if still alive + * (corruption case), dispose the rpc (rejects pending requests), and drop + * edge handles. The manager's status() observes state:"error" and re-spawns + * after the bounded backoff. Called on process death AND on corruption. + */ + private markBroken(reason: string): void { + if (this.state === "error") return; + this.state = "error"; + this.stateError = reason; + this.fileWatcherHandle?.close(); + this.fileWatcherHandle = null; + this.process?.kill(); + this.process = null; + this.rpc?.dispose(); + this.rpc = null; + } + + /** + * Detect a server stuck re-emitting identical non-empty diagnostics + * despite the file content changing between calls — the signature of a + * corrupted parse/type-check state (e.g. Steep's ~3h phantom-SyntaxError + * drift, where a fresh CLI reports green on the same project). After + * STALE_REPEAT_THRESHOLD consecutive such repeats, mark the client broken + * so it is skipped + re-spawned. A clean file (empty diagnostics) or a + * genuinely changing diagnostic set resets the counter. Note the + * tradeoff: a real, unfixed error on an untouched line also "stays the + * same across edits", so this can false-positive on a healthy server — + * the threshold is set conservatively and the CLI type-check gate remains + * authoritative either way. + */ + private detectStaleDiagnostics(uri: string, text: string): void { + const merged = this.diagnostics.getMerged(uri); + const keys = new Set(merged.map((d) => diagnosticKey(d))); + const prev = this.lastDiagSnapshot.get(uri); + if (prev && keys.size > 0 && setsEqual(keys, prev.keys) && text !== prev.text) { + this.staleRepeat++; + } else { + this.staleRepeat = 0; + } + this.lastDiagSnapshot.set(uri, { keys, text }); + if (this.staleRepeat >= LanguageServerClient.STALE_REPEAT_THRESHOLD) { + this.markBroken( + "language server emitting repeated stale diagnostics despite file changes — likely corrupted; restarting", + ); + } + } + private handleBytes(chunk: Uint8Array): void { const messages = this.decoder.decode(chunk); for (const msg of messages) { @@ -403,26 +499,37 @@ export class LanguageServerClient { await this.open(filePath); } - const slowThreshold = 10_000; const start = Date.now(); - // Poll until the server pushes diagnostics (even empty = done) or timeout. - return new Promise((resolve) => { + // Poll until the server pushes diagnostics (even empty = done) or the + // per-server cap elapses (then we skip it — see aggregateDiagnostics). + const received = await new Promise<boolean>((resolve) => { const check = () => { const elapsed = Date.now() - start; - const received = this.diagnostics.hasReceivedPush(uri); - if (received || elapsed >= timeoutMs) { - resolve({ - formatted: this.diagnostics.formatFiltered(uri, opts?.minSeverity), - slow: elapsed > slowThreshold, - timedOut: !received, - }); + const got = this.diagnostics.hasReceivedPush(uri); + if (got || elapsed >= timeoutMs) { + resolve(got); return; } setTimeout(check, 100); }; check(); }); + + // Only a server that actually pushed can be corruption-checked. + if (received) { + this.detectStaleDiagnostics(uri, opts?.text ?? ""); + } + + // `slow` is structurally false now: the per-server cap is 10s, so + // elapsed can never exceed the old "unusually long" threshold. That + // warning is superseded by the timeout→skip notice produced in + // aggregateDiagnostics. The field is kept for contract compatibility. + return { + formatted: this.diagnostics.formatFiltered(uri, opts?.minSeverity), + slow: false, + timedOut: !received, + }; } getWatchedFilesRegistry(): WatchedFilesRegistry { @@ -433,11 +540,21 @@ export class LanguageServerClient { return this.diagnostics; } - async request(method: string, params?: unknown): Promise<unknown> { + /** + * Send a request (hover/definition/references/documentSymbol). Capped at + * REQUEST_TIMEOUT_MS so a dead/slow server can't hang the turn — the + * initialize handshake bypasses this (it calls rpc.sendRequest directly + * with its own 45s race). + */ + async request( + method: string, + params?: unknown, + timeoutMs: number = LanguageServerClient.REQUEST_TIMEOUT_MS, + ): Promise<unknown> { if (!this.rpc || this.state !== "connected") { throw new Error("Client not connected"); } - return this.rpc.sendRequest(method, params); + return this.rpc.sendRequest(method, params, timeoutMs); } shutdown(): void { @@ -450,3 +567,11 @@ export class LanguageServerClient { this.state = "not-started"; } } + +function setsEqual<T>(a: Set<T>, b: Set<T>): boolean { + if (a.size !== b.size) return false; + for (const v of a) { + if (!b.has(v)) return false; + } + return true; +} |
