diff options
| author | Adam Malczewski <[email protected]> | 2026-06-27 21:14:54 +0900 |
|---|---|---|
| committer | Adam Malczewski <[email protected]> | 2026-06-27 21:14:54 +0900 |
| commit | 05ff2566eba1b4f22e2a2c22c7bd408150b0a60a (patch) | |
| tree | 4560d18d7851e5c5e02829cf002e97b0f30a802b | |
| parent | 04356c8678ae8dd1d7ddca2d0460b514116adc2e (diff) | |
| download | dispatch-05ff2566eba1b4f22e2a2c22c7bd408150b0a60a.tar.gz dispatch-05ff2566eba1b4f22e2a2c22c7bd408150b0a60a.zip | |
fix(lsp): fix crashes (optional chaining, fs.watch error) + memory leak (document lifecycle) + leaked init promises
| -rw-r--r-- | ai-review-report.md | 92 | ||||
| -rw-r--r-- | packages/lsp/src/client.test.ts | 168 | ||||
| -rw-r--r-- | packages/lsp/src/client.ts | 119 | ||||
| -rw-r--r-- | packages/lsp/src/diagnostics.test.ts | 36 | ||||
| -rw-r--r-- | packages/lsp/src/diagnostics.ts | 13 | ||||
| -rw-r--r-- | packages/lsp/src/extension.test.ts | 89 | ||||
| -rw-r--r-- | packages/lsp/src/extension.ts | 44 | ||||
| -rw-r--r-- | packages/lsp/src/rpc.test.ts | 26 |
8 files changed, 571 insertions, 16 deletions
diff --git a/ai-review-report.md b/ai-review-report.md new file mode 100644 index 0000000..2f52074 --- /dev/null +++ b/ai-review-report.md @@ -0,0 +1,92 @@ +# LSP Subsystem Review + +## Executive Summary + +The production `dispatch.service` is destabilizing due to three distinct issues in the LSP subsystem: +1. **Crash Type 1 (The "Defence-in-Depth" TypeError):** The original `SyntaxError` from malformed JSON has been mitigated by a `try/catch` in `rpc.ts`, but the subsequent "defence-in-depth" boundary added in `client.ts` introduced a fatal `TypeError` via improper optional chaining. +2. **Crash Type 2 (Unhandled Watcher ENOENT):** The recursive `node:fs` watcher (`extension.ts`) throws an uncaught exception when it encounters deleted transient files (e.g., during `bun install`). +3. **Severe Memory Leak (Unbounded Caches):** The `LanguageServerClient` retains the full text and diagnostics of every file it ever interacts with in memory indefinitely. When an AI agent scans many files, this memory footprint grows linearly, causing the 9.5 GB usage spike. + +--- + +## Root Causes & Detailed Technical Analysis + +### 1. Crash Type 1: Unhandled JSON Parse / TypeError (`client.ts`) + +**Location:** `packages/lsp/src/client.ts`, line 278 + +```typescript +// Current Code +void this.rpc?.handleMessage(msg).catch(() => {}); +``` + +**The Bug:** The stack trace in the journal logs (`at handleMessage ... at handleBytes`) points to an error in the byte stream handler. While the developer correctly added a `try/catch` inside `rpc.ts` to prevent malformed JSON from crashing the app, they introduced a fatal flaw in `client.ts` when trying to suppress unhandled promise rejections. + +In JavaScript, optional chaining (`?.`) short-circuits the evaluation of the entire expression if the left-hand side is null/undefined. When the server process dies, `handleExit` sets `this.rpc = null`. If standard output flushes a final chunk *after* this occurs, `this.rpc?.handleMessage(msg)` evaluates to `undefined`. Appending `.catch(() => {})` to this results in `undefined.catch()`, which throws a synchronous `TypeError: Cannot read properties of undefined (reading 'catch')`. This exception is entirely unhandled and bypasses all safeguards, crashing the Bun process. + +**Recommendation:** +Properly chain the `.catch` statement using optional chaining, or use a traditional `if` block: +```typescript +// Fix option 1: +void this.rpc?.handleMessage(msg)?.catch(() => {}); + +// Fix option 2: +if (this.rpc) { + void this.rpc.handleMessage(msg).catch(() => {}); +} +``` + +### 2. Crash Type 2: Unhandled ENOENT from File Watcher (`extension.ts`) + +**Location:** `packages/lsp/src/extension.ts`, lines 63-68 + +```typescript +// Current Code +const watcher = watch(root, { recursive: true }, (eventType: string, filename: string | null) => { + // ... +}); +return { close: () => watcher.close() }; +``` + +**The Bug:** Node's native `fs.watch` backend natively emits `'error'` events when it loses track of files—such as when watching a directory that is rapidly deleted (e.g., transient `.old_modules` directories during `bun install`). Since the returned `FSWatcher` instance does not have an `.on("error", ...)` event listener attached, Node/Bun escalates the emitted `'error'` event into an Uncaught Exception in the main event loop, abruptly killing the process. + +**Recommendation:** +Attach a no-op error listener to the watcher to prevent the event loop from panicking. +```typescript +const watcher = watch(root, { recursive: true }, (eventType: string, filename: string | null) => { + // ... +}); +watcher.on("error", () => { + // Gracefully ignore transient FS errors +}); +return { close: () => watcher.close() }; +``` + +### 3. The 9.5 GB Memory Leak (`client.ts` & `diagnostics.ts`) + +**Location:** `packages/lsp/src/client.ts` and `packages/lsp/src/diagnostics.ts` + +```typescript +// client.ts +private openDocuments = new Map<string, { version: number; text: string }>(); +private lastDiagSnapshot = new Map<string, { keys: Set<string>; text: string }>(); + +// diagnostics.ts +private pushDiagnostics = new Map<string, readonly Diagnostic[]>(); +``` + +**The Bug:** The design of `LanguageServerClient` is strictly append-only. When an agent opens a file or queries diagnostics (`waitForDiagnostics`), the full string contents of the file are loaded and stored in `openDocuments` and `lastDiagSnapshot`. + +Because there is no `.close()` method implemented, no eviction policy (e.g., LRU cache), and no code that issues a `textDocument/didClose` notification, these `Map` instances hold references to every file touched throughout the server's lifespan. Over a 12-hour session where an agent iteratively scans thousands of files across a large monorepo, these caches balloon uncontrollably, resulting in the observed 9.5 GB memory footprint. + +**Recommendation:** +- Implement a document lifecycle strategy. When an agent is done with a file (or when an LRU threshold is reached), invoke a new `closeDocument` method that: + 1. Sends a `textDocument/didClose` notification to the language server. + 2. Deletes the corresponding entry from `openDocuments` and `lastDiagSnapshot`. + 3. Commands the `DiagnosticsStore` to purge the related `pushDiagnostics`. + +### Minor: Leaking Promises on Initialize Timeout (`rpc.ts`) + +**Location:** `packages/lsp/src/rpc.ts` (line 49) & `packages/lsp/src/client.ts` (line 356) + +When `initialize` executes, it wraps `rpc.sendRequest("initialize")` in a `Promise.race` with a 45-second timeout. If the server times out, the `initialize` method rejects, but the original promise inside `this.pending` inside `rpc.ts` is never cleared. It will leak (along with its closure) in the `Map` forever. While minor compared to the document cache leak, it should be addressed by allowing `timeoutMs` to be passed into the initial `sendRequest` call directly instead of relying on `Promise.race`. diff --git a/packages/lsp/src/client.test.ts b/packages/lsp/src/client.test.ts index 9495888..7ba28b0 100644 --- a/packages/lsp/src/client.test.ts +++ b/packages/lsp/src/client.test.ts @@ -13,6 +13,7 @@ function makeClient(overrides?: { readonly fileWatcher?: FileWatcher; readonly fs?: FsAccess; readonly initialization?: Record<string, unknown>; + readonly initializeTimeoutMs?: number; }): { client: LanguageServerClient; stdinChunks: Uint8Array[]; @@ -52,6 +53,9 @@ function makeClient(overrides?: { root: "/project", serverId: "test", ...(overrides?.initialization ? { initialization: overrides.initialization } : {}), + ...(overrides?.initializeTimeoutMs !== undefined + ? { initializeTimeoutMs: overrides.initializeTimeoutMs } + : {}), }); return { @@ -434,4 +438,168 @@ describe("client", () => { // is normal, not corruption. expect(client.getState()).toBe("connected"); }); + + it("handleBytes does not crash when the server dies and rpc is null (Bug 1)", async () => { + // When the process dies, markBroken() sets this.rpc = null. If stdout + // then flushes a final chunk, the old code `this.rpc?.handleMessage(msg).catch()` + // threw a synchronous TypeError: Cannot read properties of undefined + // (reading 'catch') — undefined.catch. The fix adds a second `?.`. + 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; + expect(client.getState()).toBe("connected"); + + // Kill the server — rpc is now null. + exitCb?.({ code: 1 }); + expect(client.getState()).toBe("error"); + + // A final stdout chunk must NOT throw (the regression crashed here). + expect(() => { + stdoutHolder.cb?.(encode(JSON.stringify({ jsonrpc: "2.0", method: "foo", params: {} }))); + }).not.toThrow(); + }); + + it("closeDocument sends textDocument/didClose and purges cached text + diagnostics (Bug 3)", async () => { + const { client, stdinChunks, 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; + + // Open a doc + receive some diagnostics. + await client.openWithText("/project/a.ts", "const x = 1;\n"); + serverResponses( + JSON.stringify({ + jsonrpc: "2.0", + method: "textDocument/publishDiagnostics", + params: { + uri: "file:///project/a.ts", + diagnostics: [ + { + range: { start: { line: 0, character: 0 }, end: { line: 0, character: 5 } }, + severity: 1, + message: "unused", + }, + ], + }, + }), + ); + await new Promise((r) => setTimeout(r, 30)); + const store = client.getDiagnosticsStore(); + expect(store.format("file:///project/a.ts")).toContain("unused"); + + client.closeDocument("/project/a.ts"); + + // didClose was sent. + const sent = stdinChunks.map((chunk) => { + const decoded = new TextDecoder().decode(chunk); + const headerEnd = decoded.indexOf("\r\n\r\n"); + return JSON.parse(decoded.slice(headerEnd + 4)); + }); + const didClose = sent.find((m: { method?: string }) => m.method === "textDocument/didClose"); + expect(didClose).toBeDefined(); + expect(didClose.params.textDocument.uri).toBe("file:///project/a.ts"); + + // Cached text + diagnostics are gone. + expect(store.format("file:///project/a.ts")).toBe(""); + expect(store.hasReceivedPush("file:///project/a.ts")).toBe(false); + }); + + it("opening more than the LRU cap evicts the least-recently-used document (Bug 3)", async () => { + const { client, stdinChunks, 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 CAP = 50; + // Open `CAP` documents (the first is the LRU eviction candidate). + for (let i = 0; i < CAP; i++) { + await client.openWithText(`/project/file${i}.ts`, `content ${i}`); + } + + const sentBefore = stdinChunks.length; + + // Touching an early doc (file1) promotes it: it should NOT be evicted + // when we then open one more (file50) past the cap. Instead file0 (the + // oldest untouched) is evicted. + await client.change("/project/file1.ts", "content 1 updated"); + + // Open one beyond the cap → eviction. + await client.openWithText("/project/file50.ts", "content 50"); + + const sentAfter = stdinChunks.slice(sentBefore).map((chunk) => { + const decoded = new TextDecoder().decode(chunk); + const headerEnd = decoded.indexOf("\r\n\r\n"); + return JSON.parse(decoded.slice(headerEnd + 4)); + }); + + // file0 was evicted (didClose sent); file1 was NOT evicted. + const didCloses = sentAfter.filter( + (m: { method?: string }) => m.method === "textDocument/didClose", + ); + const closedUris = didCloses.map( + (m: { params: { textDocument: { uri: string } } }) => m.params.textDocument.uri, + ); + expect(closedUris).toContain("file:///project/file0.ts"); + expect(closedUris).not.toContain("file:///project/file1.ts"); + // Exactly one eviction for one overflow open. + expect(didCloses.length).toBe(1); + }); + + it("initialize timeout clears the pending rpc entry and errors the client (Bug 4)", async () => { + // A short, injectable initialize timeout lets us drive the timeout path + // fast. The fix passes the timeout into rpc.sendRequest (not Promise.race), + // so the pending entry is cleared on expiry — no leak. + const stdoutHolder: { cb: ((data: Uint8Array) => void) | null } = { cb: null }; + const spawnNoInit: SpawnProcess = () => ({ + stdin: { write: () => {} }, + stdout: { + on: (_e, cb) => { + stdoutHolder.cb = cb; + }, + }, + pid: 7, + kill: () => {}, + }); + + const { client } = makeClient({ spawn: spawnNoInit, initializeTimeoutMs: 80 }); + const startPromise = client.start(); + await new Promise((r) => setTimeout(r, 50)); + + // Never answer initialize. The client should time out → error. + await expect(startPromise).resolves.toBeUndefined(); + expect(client.getState()).toBe("error"); + expect(client.getStateError()).toMatch(/timed out/i); + + // A LATE initialize response must not resolve/dangle anything (the + // pending entry was cleared on timeout). Feeding it is a safe no-op. + expect(() => { + stdoutHolder.cb?.( + encode(JSON.stringify({ jsonrpc: "2.0", id: 1, result: { capabilities: {} } })), + ); + }).not.toThrow(); + // State stays errored; the stale response didn't flip it to connected. + expect(client.getState()).toBe("error"); + }); }); diff --git a/packages/lsp/src/client.ts b/packages/lsp/src/client.ts index f0e40ff..b86ecc2 100644 --- a/packages/lsp/src/client.ts +++ b/packages/lsp/src/client.ts @@ -99,6 +99,13 @@ export interface ClientDeps { readonly root: string; readonly initialization?: Readonly<Record<string, unknown>> | undefined; readonly serverId: string; + /** + * Timeout for the initialize handshake, in ms (default 45_000). Passed + * straight into `rpc.sendRequest` so a no-show server's pending entry is + * cleared on expiry (no Promise.race leak). Exposed mainly so tests can + * drive the timeout path quickly. + */ + readonly initializeTimeoutMs?: number | undefined; } export type ClientState = "starting" | "connected" | "error" | "not-started"; @@ -115,6 +122,13 @@ export class LanguageServerClient { private state: ClientState = "not-started"; private stateError: string | undefined; private deps: ClientDeps; + /** + * Open documents keyed by filePath. Insertion order = LRU recency order: + * the first entry is the least-recently-used (eviction candidate). Access + * (`change`) re-inserts to move a key to the tail (most-recently-used); + * `closeDocument` removes it. Capped at MAX_OPEN_DOCUMENTS — overflow + * evicts the LRU entry via a textDocument/didClose + purge. + */ 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; @@ -130,6 +144,13 @@ export class LanguageServerClient { private static readonly STALE_REPEAT_THRESHOLD = 5; /** Default timeout for outbound requests (hover/definition/references). */ private static readonly REQUEST_TIMEOUT_MS = 10_000; + /** + * Bounded open-document set: once more than this many files are open, the + * least-recently-used is closed (textDocument/didClose) and evicted. The + * maps were previously append-only — an agent scanning a large monorepo + * held every file's text + diagnostics forever (9.5 GB over 12h). + */ + private static readonly MAX_OPEN_DOCUMENTS = 50; constructor(deps: ClientDeps) { this.deps = deps; @@ -236,6 +257,11 @@ export class LanguageServerClient { this.process = null; this.rpc?.dispose(); this.rpc = null; + // Release cached document text + diagnostics — the server is dead, so + // the contents are stale anyway. Keeps a repeatedly-crashed client from + // accumulating memory across re-spawn cycles. + this.openDocuments.clear(); + this.lastDiagSnapshot.clear(); } /** @@ -275,7 +301,13 @@ export class LanguageServerClient { // message never becomes an unhandled rejection that crashes // the server. (handleMessage also has its own try/catch around // JSON.parse, but this is the defence-in-depth boundary.) - void this.rpc?.handleMessage(msg).catch(() => {}); + // NOTE the second `?.` before `.catch`: when the server process + // dies, `markBroken` sets `this.rpc = null`. If stdout then + // flushes a final chunk, `this.rpc?.handleMessage(msg)` short- + // circuits to `undefined`, and a plain `.catch()` on `undefined` + // throws a synchronous TypeError that crashes the process. The + // extra `?.` makes it `undefined?.catch()` → `undefined`. + void this.rpc?.handleMessage(msg)?.catch(() => {}); } } @@ -353,20 +385,23 @@ export class LanguageServerClient { } private async initialize(rpc: JsonRpcConnection): Promise<void> { - const timeout = 45_000; - - const initPromise = rpc.sendRequest("initialize", { - processId: this.process?.pid ?? null, - rootUri: `file://${this.root}`, - workspaceFolders: [{ uri: `file://${this.root}`, name: this.root }], - capabilities: CLIENT_CAPABILITIES, - }); - - const timeoutPromise = new Promise<never>((_, reject) => { - setTimeout(() => reject(new Error("Initialize timeout")), timeout); - }); - - const result = (await Promise.race([initPromise, timeoutPromise])) as { + const timeout = this.deps.initializeTimeoutMs ?? 45_000; + + // Pass the timeout straight into sendRequest (rather than wrapping in a + // Promise.race) so that, on expiry, rpc.ts's own timeout handler deletes + // the pending entry from its `pending` Map. The old Promise.race path + // rejected the caller but left the original promise (and its closure) + // lodged in `pending` forever — a slow leak across re-spawns. + const result = (await rpc.sendRequest( + "initialize", + { + processId: this.process?.pid ?? null, + rootUri: `file://${this.root}`, + workspaceFolders: [{ uri: `file://${this.root}`, name: this.root }], + capabilities: CLIENT_CAPABILITIES, + }, + timeout, + )) as { readonly capabilities?: { readonly textDocumentSync?: | number @@ -449,6 +484,50 @@ export class LanguageServerClient { text, }, }); + + // Bound the open-document set: evict the least-recently-used (the head + // of the insertion-ordered Map) when the cap is exceeded. Eviction + // closes the document on the server and purges its cached text + + // diagnostics so the maps can't grow without bound. + this.evictIfOverCap(); + } + + /** + * If the open-document set exceeds MAX_OPEN_DOCUMENTS, close + purge the + * least-recently-used entry (the first key in insertion order). No-op + * while at or below the cap. + */ + private evictIfOverCap(): void { + while (this.openDocuments.size > LanguageServerClient.MAX_OPEN_DOCUMENTS) { + const oldest = this.openDocuments.keys().next().value; + if (oldest === undefined) break; + this.closeDocument(oldest); + } + } + + /** + * Close an open document: send textDocument/didClose to the server and + * release every cached reference to it (openDocuments, lastDiagSnapshot, + * and the diagnostics store). Idempotent — a no-op for a path that isn't + * open. This is the lifecycle hook that keeps memory bounded: without it + * the maps retained every file an agent ever touched (9.5 GB over 12h). + * Safe to call on a broken/disconnected client (sends nothing, still frees + * local state). + */ + closeDocument(filePath: string): void { + const wasOpen = this.openDocuments.has(filePath); + this.openDocuments.delete(filePath); + this.lastDiagSnapshot.delete(filePath); + const uri = `file://${filePath}`; + this.diagnostics.purge(uri); + + if (!wasOpen) return; + const rpc = this.rpc; + if (rpc && this.state === "connected") { + rpc.sendNotification("textDocument/didClose", { + textDocument: { uri }, + }); + } } async change(filePath: string, newText: string): Promise<void> { @@ -463,6 +542,11 @@ export class LanguageServerClient { } const version = existing.version + 1; + // Re-insert (delete + set) to move this key to the tail of the insertion- + // ordered Map = most-recently-used. A plain `set` on an existing key + // updates the value but leaves its LRU position unchanged, so a hot file + // opened early could still be evicted first. Deleting first reorders it. + this.openDocuments.delete(filePath); this.openDocuments.set(filePath, { version, text: newText }); if (this.textDocumentChange === 2) { @@ -564,6 +648,11 @@ export class LanguageServerClient { this.process = null; this.rpc?.dispose(); this.rpc = null; + // Drop all cached document text + diagnostics so a shut-down client + // releases its memory immediately (no lingering references until GC). + // We don't send didClose here — the server process is being killed. + this.openDocuments.clear(); + this.lastDiagSnapshot.clear(); this.state = "not-started"; } } diff --git a/packages/lsp/src/diagnostics.test.ts b/packages/lsp/src/diagnostics.test.ts index e72e007..7f0365b 100644 --- a/packages/lsp/src/diagnostics.test.ts +++ b/packages/lsp/src/diagnostics.test.ts @@ -48,4 +48,40 @@ describe("diagnostics", () => { const store = new DiagnosticsStore(); expect(store.format("file:///nonexistent.ts")).toBe(""); }); + + it("purge drops push diagnostics, pull diagnostics, and the received flag (Bug 3)", () => { + const store = new DiagnosticsStore(); + store.setPushDiagnostics({ + uri: "file:///project/a.ts", + diagnostics: [ + { + range: { start: { line: 0, character: 0 }, end: { line: 0, character: 1 } }, + severity: 1, + message: "boom", + }, + ], + }); + store.setPullDiagnostics("file:///project/a.ts", { + kind: "full", + items: [ + { + range: { start: { line: 1, character: 0 }, end: { line: 1, character: 1 } }, + severity: 2, + message: "warn", + }, + ], + }); + expect(store.hasReceivedPush("file:///project/a.ts")).toBe(true); + expect(store.format("file:///project/a.ts")).toContain("boom"); + + store.purge("file:///project/a.ts"); + + // Everything for that URI is gone — the store no longer retains it. + expect(store.hasReceivedPush("file:///project/a.ts")).toBe(false); + expect(store.format("file:///project/a.ts")).toBe(""); + expect(store.getMerged("file:///project/a.ts")).toHaveLength(0); + + // Other URIs are untouched. + expect(store.format("file:///project/b.ts")).toBe(""); + }); }); diff --git a/packages/lsp/src/diagnostics.ts b/packages/lsp/src/diagnostics.ts index ccd695f..09ec1ae 100644 --- a/packages/lsp/src/diagnostics.ts +++ b/packages/lsp/src/diagnostics.ts @@ -57,6 +57,19 @@ export class DiagnosticsStore { this.pushReceived.delete(uri); } + /** + * Drop ALL state for a URI — push diagnostics, pull diagnostics, and the + * received flag. Called when a document is closed (textDocument/didClose) + * so the store stops retaining the file's diagnostics forever. Without + * this, the maps grow unboundedly as an agent touches thousands of files + * (the 9.5 GB leak). + */ + purge(uri: string): void { + this.pushDiagnostics.delete(uri); + this.pullDiagnostics.delete(uri); + this.pushReceived.delete(uri); + } + getMerged(uri: string): readonly Diagnostic[] { const push = this.pushDiagnostics.get(uri) ?? []; const pull = this.pullDiagnostics.get(uri) ?? []; diff --git a/packages/lsp/src/extension.test.ts b/packages/lsp/src/extension.test.ts new file mode 100644 index 0000000..16b9df7 --- /dev/null +++ b/packages/lsp/src/extension.test.ts @@ -0,0 +1,89 @@ +import { EventEmitter } from "node:events"; +import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { describe, expect, it } from "vitest"; +import type { FsWatcherHandle, WatchFn } from "./extension.js"; + +// `realFileWatcher` is a module-private function; the exported seam below +// re-exposes it for testing. Import through the module to exercise the real +// production path (the error-listener attachment is what we're verifying). +import { __test__realFileWatcher } from "./extension.js"; + +describe("realFileWatcher (Bug 2 — unhandled fs.watch 'error' event)", () => { + it("swallows a watcher 'error' event instead of crashing (injected watcher)", () => { + // A fake fs.watch: returns an EventEmitter we control. Without an + // 'error' listener, Node EventEmitter throws an uncaughtException on + // emit('error'). The fix attaches a no-op 'error' listener, so emitting + // here must NOT throw. The fake wires the watch callback to the + // EventEmitter's "change" event (args: eventType, filename), mirroring + // how node:fs.watch invokes its callback. + const watcher = new EventEmitter() as unknown as FsWatcherHandle & { + close: () => void; + }; + (watcher as { close: () => void }).close = () => { + watcher.removeAllListeners(); + }; + const fakeWatch: WatchFn = (_root, _opts, cb) => { + watcher.on("change", (eventType: string, filename: string | null) => cb(eventType, filename)); + return watcher; + }; + + const events: { type: string; path: string }[] = []; + const handle = __test__realFileWatcher("/project", (e) => events.push(e), fakeWatch); + + // A transient FS error (e.g. bun install deleting a watched dir) — must + // be a no-op, NOT an uncaught exception. + expect(() => watcher.emit("error", new Error("ENOENT transient"))).not.toThrow(); + + // The watcher still forwards normal change events. + watcher.emit("change", "change", "src/a.ts"); + expect(events).toEqual([{ type: "change", path: "/project/src/a.ts" }]); + + handle.close(); + }); + + it("ignores a null filename (no spurious event)", () => { + const watcher = new EventEmitter() as unknown as FsWatcherHandle & { + close: () => void; + }; + (watcher as { close: () => void }).close = () => { + watcher.removeAllListeners(); + }; + const fakeWatch: WatchFn = (_root, _opts, cb) => { + watcher.on("change", (eventType: string, filename: string | null) => cb(eventType, filename)); + return watcher; + }; + + const events: { type: string; path: string }[] = []; + const handle = __test__realFileWatcher("/project", (e) => events.push(e), fakeWatch); + + watcher.emit("change", "change", null); + expect(events).toHaveLength(0); + + handle.close(); + }); + + it("integration: watches a real temp directory and fires on file change", async () => { + // A real-FS smoke test of the production adapter's happy path. Uses the + // real node:fs.watch (recursive on a temp dir). Best-effort: some + // platforms coalesce events, so we only assert the adapter runs and + // closes cleanly without throwing — we do not hard-assert an event + // arrived (that would be flaky across inotify/kqueue/Win backends). + const dir = mkdtempSync(join(tmpdir(), "lsp-watch-")); + try { + const events: { type: string; path: string }[] = []; + const handle = __test__realFileWatcher(dir, (e) => events.push(e)); + + // Touch a file; give the watcher a moment. + writeFileSync(join(dir, "hello.txt"), "hi"); + await new Promise((r) => setTimeout(r, 150)); + + handle.close(); + // No assertion on events.length — the point is no throw + clean close. + expect(true).toBe(true); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); +}); diff --git a/packages/lsp/src/extension.ts b/packages/lsp/src/extension.ts index b4eb71b..31fd6b7 100644 --- a/packages/lsp/src/extension.ts +++ b/packages/lsp/src/extension.ts @@ -55,17 +55,51 @@ function realSpawn( }; } +/** + * The minimal slice of `node:fs.watch`'s return we depend on — narrow enough + * that a test can supply an EventEmitter stand-in. `on('error', …)` is the + * crucial bit: without a listener, Node/Bun escalates a watcher 'error' + * event (e.g. from `bun install` deleting transient `.old_modules-*` dirs) + * into an uncaught exception that kills the process. + */ +export interface FsWatcherHandle { + readonly on: (event: string, cb: (err: Error) => void) => FsWatcherHandle; + readonly close: () => void; +} + +export type WatchFn = ( + root: string, + opts: { readonly recursive: boolean }, + cb: (eventType: string, filename: string | null) => void, +) => FsWatcherHandle; + +function defaultWatch(): WatchFn { + // Loaded lazily so the kernel-style import graph never statically pulls + // `node:fs` (the extension imports it at call time, matching the prior + // `require` form). + const { watch } = require("node:fs") as { + watch: WatchFn; + }; + return watch; +} + function realFileWatcher( root: string, onEvent: (e: { readonly type: "create" | "change" | "delete"; readonly path: string }) => void, + watch: WatchFn = defaultWatch(), ): { readonly close: () => void } { - const { watch } = require("node:fs"); const watcher = watch(root, { recursive: true }, (eventType: string, filename: string | null) => { if (!filename) return; const fullPath = root.endsWith("/") ? `${root}${filename}` : `${root}/${filename}`; const type = eventType === "rename" ? "create" : "change"; onEvent({ type, path: fullPath }); }); + // Attach a no-op 'error' listener so a transient FS error (e.g. a watched + // directory vanishing mid-`bun install`) is swallowed instead of being + // escalated to an uncaught exception that crashes the server. + watcher.on("error", () => { + // Gracefully ignore transient FS errors — the watcher is best-effort. + }); return { close: () => watcher.close() }; } @@ -167,3 +201,11 @@ export const extension: Extension = { // Module-scoped store for deactivate const lspManagerStore: { manager: LspManager | null } = { manager: null }; + +/** + * Test-only re-export of the production `realFileWatcher` adapter, so the + * fs.watch error-listener behavior (Bug 2) can be exercised with an injected + * fake watcher without poking module internals. NOT part of the public + * extension surface — the `__test__` prefix signals test-only use. + */ +export const __test__realFileWatcher = realFileWatcher; diff --git a/packages/lsp/src/rpc.test.ts b/packages/lsp/src/rpc.test.ts index 5db5f97..37cadf2 100644 --- a/packages/lsp/src/rpc.test.ts +++ b/packages/lsp/src/rpc.test.ts @@ -109,4 +109,30 @@ describe("sendRequest timeout", () => { conn.handleMessage(frameResponse(1, { capabilities: {} })); await expect(promise).resolves.toEqual({ capabilities: {} }); }); + + it("clears the pending entry on timeout so it does not leak (Bug 4)", async () => { + // A timed-out request must drop its pending entry: a late response for + // that id is then a no-op (entry gone), and the connection keeps working + // (next id resolves). If the entry leaked, dispose() would later reject a + // phantom promise — the initialize handshake relies on this. + const { conn } = makeConnection(); + const promise = conn.sendRequest("initialize", {}, 50); + await expect(promise).rejects.toThrow(/timed out/); + + // id 1's entry is gone: a stray response for it resolves nothing and + // does not throw. + conn.handleMessage(frameResponse(1, { stale: true })); + + // The connection is un-wedged: a fresh request (id 2) resolves normally. + const promise2 = conn.sendRequest("second", {}, 5000); + conn.handleMessage(frameResponse(2, { ok: true })); + await expect(promise2).resolves.toEqual({ ok: true }); + + // dispose() rejects nothing extra: the only live pending entry is id 2 + // (already resolved + cleared), so disposal is a clean no-op of rejects. + // (If id 1 had leaked, this would still be fine since handleResponse + // guards on missing entries — the leak is a memory issue, not a crash; + // the assertions above prove functional correctness post-timeout.) + conn.dispose(); + }); }); |
