From 0f3f0efd9c08b755d6127ef8b77100a34170a512 Mon Sep 17 00:00:00 2001 From: Dudu-0223 Date: Thu, 16 Jul 2026 13:34:37 +0800 Subject: [PATCH] fix(lsp): address codex review round 2 Further lifecycle/safety hardening of the local provider: - Tear the instance down when the initialize handshake is aborted, so a poisoned pending `ready` can't make later queries for that workspace re-wait. - Make the serialized-queue wait itself abortable, so a query blocked behind hung earlier work can still observe its own timeout. - Spawn the server detached and signal the whole process group on teardown, so helper processes (e.g. tsserver) can't outlive dispose(). - Open the source with O_NOFOLLOW and cap the read at maxDocumentBytes+1, closing the symlink-swap and concurrent-grow windows the fd-based read left open. - Honor an already-aborted signal before any host I/O or startup. - Validate maxStderrBytes positive at load; surface the retained stderr tail in the "language server exited" error so a fatal startup diagnostic is visible. --- packages/lsp/lsp-local/src/connection.ts | 38 ++++++++++++++++--- packages/lsp/lsp-local/src/host.ts | 28 ++++++++++++-- packages/lsp/lsp-local/src/index.ts | 7 +++- packages/lsp/lsp-local/src/instance.ts | 32 +++++++++++++--- .../lsp/lsp-local/tests/lifecycle.spec.ts | 19 ++++++++++ 5 files changed, 109 insertions(+), 15 deletions(-) diff --git a/packages/lsp/lsp-local/src/connection.ts b/packages/lsp/lsp-local/src/connection.ts index 4f1f4d9b3a..272109d2c6 100644 --- a/packages/lsp/lsp-local/src/connection.ts +++ b/packages/lsp/lsp-local/src/connection.ts @@ -55,14 +55,17 @@ export class LspConnection { private readonly onServerRequest: (method: string, params: unknown) => Promise, ) { this.decoder = new MessageDecoder(spec.maxMessageBytes) + // `detached` puts the server in its own process group so teardown can signal the WHOLE group + // (via `process.kill(-pid)`), reaching helper processes a language server spawns (e.g. tsserver). this.child = spawn(spec.command, [...spec.args], { cwd: spec.cwd, env: spec.env, stdio: ['pipe', 'pipe', 'pipe'], + detached: true, }) this.closed = new Promise((resolve) => { this.child.on('close', () => { - const reason = this.closeReason ?? new Error('language server exited') + const reason = this.closeReason ?? new Error(this.exitMessage()) // Record the reason so any request issued AFTER close rejects immediately instead of hanging // (a closed process sends no further responses). this.closeReason = reason @@ -150,14 +153,33 @@ export class LspConnection { return this.nextId } - /** Send SIGTERM to the child (idempotent-safe; a dead child ignores it). */ + /** Send SIGTERM to the server's process group (idempotent-safe; a dead group ignores it). */ terminate(): void { - this.child.kill('SIGTERM') + this.signalGroup('SIGTERM') } - /** Send SIGKILL to the child. */ + /** Send SIGKILL to the server's process group. */ kill(): void { - this.child.kill('SIGKILL') + this.signalGroup('SIGKILL') + } + + /** + * Signal the whole process group (negative pid) so helper processes are reached; fall back to the + * direct child if the group send fails. Never throws — teardown races process exit. + */ + private signalGroup(sig: NodeJS.Signals): void { + const pid = this.child.pid + if (pid === undefined) return + try { + process.kill(-pid, sig) + } catch { + // The group is gone (already exited) or could not be signalled; try the direct child. + try { + this.child.kill(sig) + } catch { + // Already dead; nothing to signal. + } + } } private onStdout(chunk: Buffer): void { @@ -221,6 +243,12 @@ export class LspConnection { this.child.stdin.write(encodeMessage(message)) } + /** The exit-close error message, appending the retained stderr tail when the server wrote any. */ + private exitMessage(): string { + const tail = this.stderr.trim() + return tail === '' ? 'language server exited' : `language server exited; stderr: ${tail}` + } + private fail(error: Error): void { /* v8 ignore next -- the second arm (closeReason already set) needs two fail() calls before close; defensive. */ if (this.closeReason === undefined) this.closeReason = error diff --git a/packages/lsp/lsp-local/src/host.ts b/packages/lsp/lsp-local/src/host.ts index 8638926860..949a1b838b 100644 --- a/packages/lsp/lsp-local/src/host.ts +++ b/packages/lsp/lsp-local/src/host.ts @@ -9,7 +9,9 @@ * @module @deepseek-ai/dsh-lsp-local/host */ +import { constants } from 'node:fs' import { open, realpath, stat } from 'node:fs/promises' +import type { FileHandle } from 'node:fs/promises' import { isAbsolute, resolve as resolvePath, sep } from 'node:path' /** A validated source: its canonical absolute path and current UTF-8 text. */ @@ -70,8 +72,9 @@ export async function readHostSource( } // Open ONE handle after containment, then stat and read through it: a concurrent replace between // realpath and read cannot swap the target, so the regular-file and size checks bind the bytes we - // actually read (no path-based TOCTOU). - const handle = await open(canonicalPath, 'r') + // actually read (no path-based TOCTOU). O_NOFOLLOW rejects the final component being swapped for a + // symlink between realpath and open (which would otherwise escape the workspace). + const handle = await open(canonicalPath, constants.O_RDONLY | constants.O_NOFOLLOW) try { const info = await handle.stat() if (!info.isFile()) { @@ -80,7 +83,9 @@ export async function readHostSource( if (info.size > maxDocumentBytes) { throw new Error(`source "${filePath}" is ${info.size} bytes, over the ${maxDocumentBytes}-byte limit`) } - const buffer = await handle.readFile() + // Bound the read to the cap even if the file grew after stat: read one extra byte and reject on + // overflow, so a concurrent grow cannot defeat the memory bound. + const buffer = await readCapped(handle, maxDocumentBytes, filePath) const text = decodeUtf8Strict(buffer, filePath) return { canonicalPath, text } } finally { @@ -88,6 +93,23 @@ export async function readHostSource( } } +/** Read at most `maxBytes` from the handle, rejecting when the source overflows the cap. */ +async function readCapped(handle: FileHandle, maxBytes: number, filePath: string): Promise { + const limit = maxBytes + 1 + const chunk = Buffer.allocUnsafe(limit) + let total = 0 + for (;;) { + const { bytesRead } = await handle.read(chunk, total, limit - total, total) + if (bytesRead === 0) break + total += bytesRead + /* v8 ignore next 3 -- overflow requires the file to grow past the cap between stat and read (a concurrent mutation); defensive. */ + if (total > maxBytes) { + throw new Error(`source "${filePath}" grew past the ${maxBytes}-byte limit while reading`) + } + } + return chunk.subarray(0, total) +} + /** Whether `child` is the workspace itself or a descendant of it (both already canonical). */ function isInside(workspace: string, child: string): boolean { if (child === workspace) return true diff --git a/packages/lsp/lsp-local/src/index.ts b/packages/lsp/lsp-local/src/index.ts index 598254f937..dc6e02d3c6 100644 --- a/packages/lsp/lsp-local/src/index.ts +++ b/packages/lsp/lsp-local/src/index.ts @@ -24,7 +24,7 @@ import type { // Side-effect type import: declaration-merges `ctx.lsp` onto Context. import type {} from '@deepseek-ai/dsh-lsp' import { canonicalizeWorkspace, readHostSource } from './host.ts' -import { LspInstance } from './instance.ts' +import { abortError, LspInstance } from './instance.ts' import type { InstanceSpec } from './instance.ts' export { canonicalizeWorkspace, readHostSource } from './host.ts' @@ -114,6 +114,8 @@ export function apply(ctx: Context, config: Config): void { // nonpositive value would let a server that ignores shutdown hang disposal forever. Fail at load. assertPositiveInteger('shutdownTimeoutMs', resolved.shutdownTimeoutMs) assertPositiveInteger('killGraceMs', resolved.killGraceMs) + // A nonpositive stderr cap defeats the retained-tail bound (`slice(-0)` keeps everything). + assertPositiveInteger('maxStderrBytes', resolved.maxStderrBytes) const childEnv = buildChildEnv(resolved.env) // Resolve the executable eagerly so a misconfigured command fails at load, not on first query. const executable = resolveExecutable(resolved.command, childEnv) @@ -160,6 +162,9 @@ class LocalLspProvider implements LspProvider { async query(request: LspProviderQuery, signal?: AbortSignal): Promise { /* v8 ignore next -- the seam unregisters this provider on dispose, so a query never reaches it disposed; defensive. */ if (this.isDisposed()) throw new Error('lsp-local provider is disposed') + // Honor an already-aborted signal before any host I/O so a canceled request neither reads nor + // spawns a server. + if (signal?.aborted) throw abortError(signal) const workspace = await canonicalizeWorkspace(request.workspaceRoot) // Validate and read the source BEFORE spawning a server: a missing/external/non-regular/oversized // source must fail without leaving an idle process pooled (the pre-start rejection contract), and diff --git a/packages/lsp/lsp-local/src/instance.ts b/packages/lsp/lsp-local/src/instance.ts index 83266e9c8f..ecf2782a12 100644 --- a/packages/lsp/lsp-local/src/instance.ts +++ b/packages/lsp/lsp-local/src/instance.ts @@ -78,9 +78,14 @@ export class LspInstance { * @returns the normalized result. */ query(request: LspProviderQuery, source: HostSource, signal?: AbortSignal): Promise { - const run = this.queue.then(() => this.runQuery(request, source, signal)) - // Keep the tail alive regardless of this query's outcome so the next caller still serializes. - this.queue = run.then(() => undefined, () => undefined) + // Serialize behind prior work, but observe abort DURING the queue wait too: if an earlier query + // hangs (e.g. a signal-less seam caller), a later tool's timeout must still be able to give up + // rather than block on the shared tail forever. + const run = this.abortable(this.queue, signal).then(() => this.runQuery(request, source, signal)) + // Keep the tail alive regardless of this query's outcome so the next caller still serializes. The + // tail follows the ACTUAL prior work (this.queue), not the abortable view, so a caller giving up + // on the wait does not deserialize the queue. + this.queue = this.queue.then(() => run).then(() => undefined, () => undefined) return run } @@ -101,10 +106,21 @@ export class LspInstance { private async runQuery(request: LspProviderQuery, source: HostSource, signal?: AbortSignal): Promise { if (this.disposed) throw new Error('LSP instance was disposed') + /* v8 ignore next -- the abortable queue wait rejects a pre-aborted signal before runQuery; this is a belt-and-suspenders guard. */ if (signal?.aborted) throw abortError(signal) // Observe abort during the handshake wait: a server that never answers `initialize` must not // block the tool-timeout signal here (the timeout policy awaits our quiescence, not the promise). - await this.abortable(this.ready, signal) + // If abort wins, the handshake is still pending on a live process, so tear the instance down — + // otherwise its poisoned `ready` would make every later query for this workspace re-wait. + try { + await this.abortable(this.ready, signal) + } catch (error) { + if (signal?.aborted && !this.dead) { + this.disposed = true + await this.tearDown(abortError(signal)) + } + throw error + } const capabilities = this.capabilities /* v8 ignore next -- `ready` resolves only after capabilities are set, else it rejects above; defensive. */ if (capabilities === undefined) throw new Error('LSP instance is not initialized') @@ -303,8 +319,12 @@ function markSettled(): boolean { return true } -/** Build an abort Error carrying the signal's reason (preserving a timeout classification). */ -function abortError(signal: AbortSignal): Error { +/** + * Build an abort Error carrying the signal's reason (preserving a timeout classification). + * @param signal - the aborted signal whose reason to surface. + * @returns the timeout reason if the signal carries one, else the signal's Error reason, else a generic aborted Error. + */ +export function abortError(signal: AbortSignal): Error { const timeout = timeoutOf(signal) if (timeout !== undefined) return timeout const reason: unknown = signal.reason diff --git a/packages/lsp/lsp-local/tests/lifecycle.spec.ts b/packages/lsp/lsp-local/tests/lifecycle.spec.ts index c5631f2a95..66a18584a1 100644 --- a/packages/lsp/lsp-local/tests/lifecycle.spec.ts +++ b/packages/lsp/lsp-local/tests/lifecycle.spec.ts @@ -151,6 +151,25 @@ describe('lsp-local end to end over a fake server', () => { await ctx.fiber.dispose() }) + it('honors an already-aborted signal before any host I/O or startup', async () => { + const ctx = await mount({ LSP_FAKE_DEF: 'null' }) + const controller = new AbortController() + controller.abort(new Error('pre-aborted')) + await expect(ctx.lsp.query(query('definition'), controller.signal)).rejects.toThrow(/pre-aborted/) + await ctx.fiber.dispose() + }) + + it('surfaces the server stderr tail in the exit error', async () => { + // A server that writes to stderr then exits without answering: the query rejection carries the + // retained stderr tail so the failure is diagnosable. + const ctx = await mount({}, { + command: process.execPath, + args: ['-e', 'process.stderr.write("FATAL: boom\\n"); setTimeout(()=>process.exit(1), 50)'], + }) + await expect(ctx.lsp.query(query('definition'))).rejects.toThrow(/FATAL: boom/) + await ctx.fiber.dispose() + }) + it('classifies a timeout deadline as the abort reason', async () => { const ctx = await mount({ LSP_FAKE_HANG: '1' }) using d = deadline(undefined, 50, 'TEST_TIMEOUT')