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

@@ -55,14 +55,17 @@ export class LspConnection {
     private readonly onServerRequest: (method: string, params: unknown) => Promise<unknown>,
   ) {
     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<void>((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

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<Buffer> {
+  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

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<LspQueryResult> {
     /* 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

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<LspQueryResult> {
-    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<LspQueryResult> {
     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

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')