fix(lsp): address lifecycle review feedback

packages/lsp/lsp-local/src/connection.ts

@@ -10,6 +10,7 @@
 import type { ChildProcessByStdio } from 'node:child_process'
 import { spawn } from 'node:child_process'
 import type { Readable, Writable } from 'node:stream'
+import { setImmediate as yieldToEventLoop } from 'node:timers/promises'
 import { encodeMessage, MessageDecoder } from './framing.ts'
 
 /** How to launch the server and answer its config requests. */
@@ -163,6 +164,19 @@ export class LspConnection {
     this.signalGroup('SIGKILL')
   }
 
+  /**
+   * Wait until the owned process group has no members.
+   * @param signal - optional bound for the wait.
+   * @returns `true` when the group exited, or `false` when the signal aborted first.
+   */
+  async waitForProcessGroupExit(signal?: AbortSignal): Promise<boolean> {
+    while (this.processGroupAlive()) {
+      if (signal?.aborted) return false
+      await yieldToEventLoop()
+    }
+    return true
+  }
+
   /**
    * 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.
@@ -182,6 +196,25 @@ export class LspConnection {
     }
   }
 
+  /** Whether the detached process group still has at least one member. */
+  private processGroupAlive(): boolean {
+    const pid = this.child.pid
+    /* v8 ignore next -- only an asynchronous spawn failure omits pid; its close path owns cleanup. */
+    if (pid === undefined) return false
+    try {
+      process.kill(-pid, 0)
+      return true
+    } catch (error) {
+      const code = (error as NodeJS.ErrnoException).code
+      if (code === 'ESRCH') return false
+      /* v8 ignore start -- EPERM and non-POSIX negative-pid failures are platform defenses; CI runs
+         process-group lifecycle tests on POSIX hosts where absence reports ESRCH. */
+      if (code === 'EPERM') return true
+      return this.child.exitCode === null && this.child.signalCode === null
+      /* v8 ignore stop */
+    }
+  }
+
   private onStdout(chunk: Buffer): void {
     let messages: unknown[]
     try {

packages/lsp/lsp-local/src/index.ts

@@ -169,8 +169,8 @@ function assertPositiveInteger(providerId: string, name: string, value: number):
 class LocalLspProvider implements LspProvider {
   readonly id: LspProviderId
   readonly extensionToLanguage: Readonly<Record<string, string>>
-  /** Single-flight map: canonical workspace realpath → the (pending) instance for it. */
-  private readonly instances = new Map<string, Promise<LspInstance>>()
+  /** One live instance per canonical workspace realpath. */
+  private readonly instances = new Map<string, LspInstance>()
   private disposed = false
 
   constructor(
@@ -188,12 +188,17 @@ class LocalLspProvider implements LspProvider {
     return this.disposed
   }
 
-  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. */
+  /** Reject work that cannot publish or use a provider-owned instance. */
+  private assertActive(signal?: AbortSignal): void {
+    /* v8 ignore next -- the seam unregisters this provider before disposal; direct in-flight calls
+       exercise the post-await check instead. */
     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)
+  }
+
+  async query(request: LspProviderQuery, signal?: AbortSignal): Promise<LspQueryResult> {
+    // Honor an already-aborted signal before host I/O so a canceled request never starts a server.
+    this.assertActive(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
@@ -201,49 +206,37 @@ class LocalLspProvider implements LspProvider {
     const source = await readHostSource(request.filePath, workspace, this.config.maxDocumentBytes)
     // Re-check disposal after the awaits: disposeAll() may have snapshotted the instance map while we
     // were canonicalizing/reading, so creating a server now would leave it unowned by teardown.
-    /* v8 ignore next -- guards a dispose landing during the canonicalize/read await; not a reproducible unit race. */
-    if (this.isDisposed()) throw new Error('lsp-local provider is disposed')
-    // Re-check cancellation too: an abort during the canonicalize/read awaits must not go on to spawn
-    // (or pool) a server solely for an operation the caller already gave up on.
-    if (signal?.aborted) throw abortError(signal)
-    let instance = await this.instanceFor(workspace)
-    // A pooled server that exited while idle resolves to a dead instance: evict it and create a fresh
-    // one before dispatch, so this query does not have to fail on a closed connection first. One retry
-    // suffices — the replacement was just constructed and has not been used.
+    this.assertActive(signal)
+    let instance = this.instanceFor(workspace)
+    // Eviction and replacement are synchronous so disposal cannot snapshot the pool between them and
+    // miss a newly spawned process.
     if (instance.dead) {
-      await this.evictIfCurrent(workspace, instance)
-      instance = await this.instanceFor(workspace)
+      this.evictIfCurrent(workspace, instance)
+      instance = this.instanceFor(workspace)
     }
     try {
       return await instance.query(request, source, signal)
     } finally {
       // A crashed/closed process must not be reused: drop its slot so the next query starts fresh,
       // but only if the slot still holds THIS instance (a concurrent replacement must survive).
-      if (instance.dead) await this.evictIfCurrent(workspace, instance)
+      if (instance.dead) this.evictIfCurrent(workspace, instance)
     }
   }
 
-  /** Single-flight one instance per canonical workspace; a rejected creation clears the slot. */
-  private instanceFor(workspace: string): Promise<LspInstance> {
+  /** Return or synchronously publish the one instance for a canonical workspace. */
+  private instanceFor(workspace: string): LspInstance {
+    this.assertActive()
     const existing = this.instances.get(workspace)
     if (existing !== undefined) return existing
-    const created = Promise.resolve().then(() => this.createInstance(workspace))
+    const created = this.createInstance(workspace)
     this.instances.set(workspace, created)
-    /* v8 ignore next 3 -- createInstance (the LspInstance constructor) does not throw; spawn failures
-       surface asynchronously through the instance, so this creation-rejection cleanup is defensive. */
-    created.catch(() => {
-      if (this.instances.get(workspace) === created) this.instances.delete(workspace)
-    })
     return created
   }
 
-  /** Drop the slot for `workspace` iff it still resolves to `instance` (a concurrent replacement survives). */
-  private async evictIfCurrent(workspace: string, instance: LspInstance): Promise<void> {
-    const slot = this.instances.get(workspace)
-    /* v8 ignore next -- the slot-undefined/mismatch arm needs a concurrent eviction of the same slot; defensive. */
-    if (slot !== undefined && (await settledInstance(slot)) === instance) {
-      this.instances.delete(workspace)
-    }
+  /** Drop the slot iff it still contains this instance. */
+  private evictIfCurrent(workspace: string, instance: LspInstance): void {
+    /* v8 ignore next -- mismatch requires another query to replace the slot before this finally runs. */
+    if (this.instances.get(workspace) === instance) this.instances.delete(workspace)
   }
 
   private createInstance(workspace: string): LspInstance {
@@ -265,26 +258,9 @@ class LocalLspProvider implements LspProvider {
   /** Dispose every live instance and block further queries. */
   async disposeAll(): Promise<void> {
     this.disposed = true
-    const pending = [...this.instances.values()]
+    const live = [...this.instances.values()]
     this.instances.clear()
-    await Promise.all(pending.map(async (entry) => {
-      try {
-        const instance = await entry
-        await instance.dispose()
-      } catch {
-        // A never-initialized instance already rejected; nothing to tear down.
-      }
-    }))
-  }
-}
-
-/** Resolve a slot promise to its instance for identity comparison, tolerating a pending rejection. */
-async function settledInstance(slot: Promise<LspInstance>): Promise<LspInstance | undefined> {
-  try {
-    return await slot
-  } catch {
-    /* v8 ignore next -- a slot promise only rejects if createInstance throws, which it never does; defensive. */
-    return undefined
+    await Promise.all(live.map(instance => instance.dispose()))
   }
 }
 

packages/lsp/lsp-local/src/instance.ts

@@ -48,6 +48,8 @@ export class LspInstance {
   /** The serialization tail: each query awaits the prior one, so lifecycles never interleave. */
   private queue: Promise<unknown> = Promise.resolve()
   private disposed = false
+  /** The one teardown transaction shared by abort, failure, and explicit disposal. */
+  private teardownPromise: Promise<void> | undefined
   /** Set once the process closes, so the pool can synchronously skip a dead instance. */
   private processClosed = false
   /** Populated once `initialize` succeeds; a failed handshake rejects every query. */
@@ -116,9 +118,8 @@ export class LspInstance {
       await this.abortable(this.ready, signal)
     } catch (error) {
       if (!this.dead) {
-        this.disposed = true
         /* v8 ignore next -- ready rejects with an Error (abort reason or initialize failure); the String() fallback is defensive. */
-        await this.tearDown(error instanceof Error ? error : new Error(String(error)))
+        await this.startTeardown(error instanceof Error ? error : new Error(String(error)))
       }
       throw error
     }
@@ -155,8 +156,7 @@ export class LspInstance {
              listener, so a synchronous didClose write failure is a defensive path. */
           // A close-write failure does not replace the settled result/error, but the instance can no
           // longer be trusted: invalidate it and await bounded process termination.
-          this.disposed = true
-          void this.tearDown(error instanceof Error ? error : new Error(String(error)))
+          void this.startTeardown(error instanceof Error ? error : new Error(String(error)))
           /* v8 ignore stop */
         }
       }
@@ -210,19 +210,20 @@ export class LspInstance {
       this.connection.cancel(requestId)
       // Wait, bounded, for the server to honor the cancellation. If it does not, the request is still
       // running: terminate the instance (disposal awaits process close) so nothing outlives the query.
-      using grace = deadline(undefined, this.spec.killGraceMs, 'LSP_CANCEL_GRACE')
-      // `settled` is true if the request finished (either outcome) before the grace elapsed.
-      const settled = await Promise.race([
-        send.then(markSettled, markSettled),
-        new Promise<boolean>((resolve) => {
-          /* v8 ignore next -- the cancel-grace deadline signal is freshly armed and not yet aborted here; defensive. */
-          if (grace.signal.aborted) { resolve(false); return }
-          grace.signal.addEventListener('abort', () => { resolve(false) }, { once: true })
-        }),
-      ])
-      if (!settled && !this.disposed) {
-        this.disposed = true
-        await this.tearDown(abortError(signal))
+      const grace = deadline(undefined, this.spec.killGraceMs, 'LSP_CANCEL_GRACE')
+      try {
+        // `settled` is true if the request finished (either outcome) before the grace elapsed.
+        const settled = await Promise.race([
+          send.then(markSettled, markSettled),
+          new Promise<boolean>((resolve) => {
+            /* v8 ignore next -- the cancel-grace deadline signal is freshly armed and not yet aborted here; defensive. */
+            if (grace.signal.aborted) { resolve(false); return }
+            grace.signal.addEventListener('abort', () => { resolve(false) }, { once: true })
+          }),
+        ])
+        if (!settled) await this.startTeardown(abortError(signal))
+      } finally {
+        grace[Symbol.dispose]()
       }
       throw error
     }
@@ -262,21 +263,24 @@ export class LspInstance {
    * process close so nothing outlives disposal.
    */
   async dispose(): Promise<void> {
-    if (this.disposed) {
-      await this.connection.closed
-      return
-    }
+    await this.startTeardown(new Error('LSP instance disposed'))
+  }
+
+  /** Publish disposal once and make every caller await the same quiescence boundary. */
+  private startTeardown(reason: Error): Promise<void> {
     this.disposed = true
-    await this.tearDown(new Error('LSP instance disposed'))
+    this.teardownPromise ??= this.tearDown(reason)
+    return this.teardownPromise
   }
 
   private async tearDown(_reason: Error): Promise<void> {
+    const shutdownDeadline = deadline(undefined, this.spec.shutdownTimeoutMs, 'LSP_SHUTDOWN')
     try {
-      using shutdownDeadline = deadline(undefined, this.spec.shutdownTimeoutMs, 'LSP_SHUTDOWN')
       await this.gracefulShutdown(shutdownDeadline.signal)
-      return
     } catch {
-      // Graceful shutdown failed or timed out: fall through to signal escalation.
+      // Graceful shutdown failed or timed out; process-group cleanup below remains authoritative.
+    } finally {
+      shutdownDeadline[Symbol.dispose]()
     }
     await this.forceTerminate()
   }
@@ -288,20 +292,21 @@ export class LspInstance {
     await this.abortable(this.connection.closed, signal)
   }
 
-  /** SIGTERM, wait `killGraceMs` for close, then SIGKILL; await full process close either way. */
+  /** SIGTERM the group, escalate after `killGraceMs`, then await leader and helper exit. */
   private async forceTerminate(): Promise<void> {
     this.connection.terminate()
-    using graceDeadline = deadline(undefined, this.spec.killGraceMs, 'LSP_KILL_GRACE')
-    const closedInTime = await Promise.race([
-      this.connection.closed.then(() => true),
-      new Promise<boolean>((resolve) => {
-        /* v8 ignore next -- the kill-grace deadline signal is freshly armed and not yet aborted here; defensive. */
-        if (graceDeadline.signal.aborted) { resolve(false); return }
-        graceDeadline.signal.addEventListener('abort', () => { resolve(false) }, { once: true })
-      }),
+    const graceDeadline = deadline(undefined, this.spec.killGraceMs, 'LSP_KILL_GRACE')
+    let groupExited: boolean
+    try {
+      groupExited = await this.connection.waitForProcessGroupExit(graceDeadline.signal)
+    } finally {
+      graceDeadline[Symbol.dispose]()
+    }
+    if (!groupExited) this.connection.kill()
+    await Promise.all([
+      this.connection.closed,
+      this.connection.waitForProcessGroupExit(),
     ])
-    if (!closedInTime) this.connection.kill()
-    await this.connection.closed
   }
 }
 

packages/lsp/lsp-local/tests/instance.spec.ts

@@ -236,6 +236,26 @@ describe('LspInstance disposal', () => {
     await expect(instance.dispose()).resolves.toBeUndefined()
   })
 
+  it('awaits a surviving process-group helper on every concurrent dispose', async () => {
+    const marker = join(root, 'helper.pid')
+    const helper = 'process.on("SIGTERM",()=>{});setInterval(()=>{},1000);'
+    const script = 'const{spawn}=require("node:child_process");const{writeFileSync}=require("node:fs");'
+      + `const helper=spawn(process.execPath,["-e",${JSON.stringify(helper)}],{stdio:"ignore"});`
+      + `writeFileSync(${JSON.stringify(marker)},String(helper.pid));`
+      + RESPONDING_SERVER
+    const instance = scriptInstance(script, { shutdownTimeoutMs: 100, killGraceMs: 100 })
+    await run(instance, 'definition')
+    const helperPid = Number(await readFile(marker, 'utf8'))
+    try {
+      const first = instance.dispose()
+      await instance.dispose()
+      expect(processAlive(helperPid)).toBe(false)
+      await first
+    } finally {
+      if (processAlive(helperPid)) process.kill(helperPid, 'SIGKILL')
+    }
+  })
+
   it('carries a non-Error abort reason as a generic aborted error', async () => {
     const instance = makeInstance({ LSP_FAKE_HANG: '1' })
     const controller = new AbortController()
@@ -245,3 +265,14 @@ describe('LspInstance disposal', () => {
     await expect(pending).rejects.toThrow(/aborted/)
   })
 })
+
+/** Probe a pid without changing its state. */
+function processAlive(pid: number): boolean {
+  try {
+    process.kill(pid, 0)
+    return true
+  } catch (error) {
+    if ((error as NodeJS.ErrnoException).code === 'ESRCH') return false
+    throw error
+  }
+}

packages/lsp/tool-lsp/tests/integration.spec.ts

@@ -12,10 +12,8 @@ import * as TimeoutPolicy from '@deepseek-ai/dsh-timeout-policy'
 import * as ToolLsp from '@deepseek-ai/dsh-tool-lsp'
 
 /**
- * Real-composition integration: the model-facing `lsp` tool over the real seam, the real
- * `dsh-lsp-local` provider (driving an inline stdio server), and the real `dsh-timeout-policy`, all
- * driven only through `ctx.tools.execute()`. Pins that a query round-trips end to end and that the
- * policy's `TOOL_TIMEOUT` budget wins when the server hangs.
+ * Focused in-process integration of the model-facing tool, seam, local provider, and timeout policy.
+ * The `lsp-definition` ACP snapshot owns the shipped Loader/app entry path.
  */
 
 let root: string
@@ -77,7 +75,7 @@ function call(ctx: Context, args: unknown) {
   })
 }
 
-describe('tool-lsp real composition', () => {
+describe('tool-lsp integration', () => {
   it('round-trips a definition query through the real provider and renders a location', async () => {
     const ctx = await mount(false)
     const result = await call(ctx, { operation: 'definition', file_path: 'a.ts', line: 1, character: 7 })

packages/lsp/tool-lsp/tests/load-path.spec.ts

@@ -1,15 +1,15 @@
 /**
- * Real-load-path guard for @deepseek-ai/dsh-tool-lsp. It is a NAMESPACE plugin with `inject`, so a
+ * Loader export-shape guard for @deepseek-ai/dsh-tool-lsp. It is a NAMESPACE plugin with `inject`, so a
  * stray `export default apply` would make the Loader's `unwrapExports` collapse the module to the
- * bare `apply`, dropping `inject` (postmortem 0001). This unwraps through the REAL
- * `Loader.prototype.unwrapExports` and verifies the namespace shape survives.
+ * bare `apply`, dropping `inject` (postmortem 0001). This verifies the namespace survives
+ * `Loader.prototype.unwrapExports`; the `lsp-definition` ACP snapshot owns full app composition.
  */
 
 import { describe, expect, it } from 'vitest'
 import Loader from '@cordisjs/plugin-loader'
 import * as toolLsp from '@deepseek-ai/dsh-tool-lsp'
 
-describe('dsh-tool-lsp real-load-path guard', () => {
+describe('dsh-tool-lsp Loader export-shape guard', () => {
   it('has no default export and keeps name/inject/Config through unwrapExports', () => {
     expect('default' in toolLsp).toBe(false)