From 769710cfb990fa93376cfaa3e9042be38f028f9f Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Wed, 22 Jul 2026 15:56:29 +0800 Subject: [PATCH] fix(lsp): make transport recovery ownership-safe --- ...-22-cross-platform-test-fixtures.i18n.yaml | 4 +-- ...2026-07-22-cross-platform-test-fixtures.md | 4 +-- ...6-07-22-cross-platform-test-fixtures.zh.md | 4 +-- packages/lsp/lsp-local/README.md | 4 +-- packages/lsp/lsp-local/src/connection.ts | 30 ++++++++++++++----- packages/lsp/lsp-local/src/index.ts | 18 ++++++----- packages/lsp/lsp-local/src/instance.ts | 16 +++++++++- .../lsp/lsp-local/tests/connection.spec.ts | 28 +++++++++++------ packages/lsp/lsp-local/tests/instance.spec.ts | 11 +++++++ .../lsp/lsp-local/tests/lifecycle.spec.ts | 10 +++++-- 10 files changed, 94 insertions(+), 35 deletions(-) diff --git a/.agents/notes/implemented/testing/2026-07-22-cross-platform-test-fixtures.i18n.yaml b/.agents/notes/implemented/testing/2026-07-22-cross-platform-test-fixtures.i18n.yaml index 511b66e345..f5fc9ecef6 100644 --- a/.agents/notes/implemented/testing/2026-07-22-cross-platform-test-fixtures.i18n.yaml +++ b/.agents/notes/implemented/testing/2026-07-22-cross-platform-test-fixtures.i18n.yaml @@ -2,5 +2,5 @@ # side as of the last confirmed-consistent state. Both languages carry equal authority; # after editing either side, bring the other along and re-record with: # pnpm run verify-translation-pairing --write -2026-07-22-cross-platform-test-fixtures.md: 56deaf6306e15c6cf17e83fcbaf36137e5c543f4 -2026-07-22-cross-platform-test-fixtures.zh.md: f61441e2dbe86fa5a580e666fcd08d23b0fadf0b +2026-07-22-cross-platform-test-fixtures.md: 6217aabfdbe8f14f869004c8dafb7e19f4b7443a +2026-07-22-cross-platform-test-fixtures.zh.md: 43942ec0468df822d04b39e318010c2b260c734f diff --git a/.agents/notes/implemented/testing/2026-07-22-cross-platform-test-fixtures.md b/.agents/notes/implemented/testing/2026-07-22-cross-platform-test-fixtures.md index 56deaf6306..6217aabfdb 100644 --- a/.agents/notes/implemented/testing/2026-07-22-cross-platform-test-fixtures.md +++ b/.agents/notes/implemented/testing/2026-07-22-cross-platform-test-fixtures.md @@ -16,7 +16,7 @@ Tests of platform-neutral behavior construct absolute paths and `file:` URIs wit Transport-failure tests inject the connection's message writer and deliver the same asynchronous write callback error that a real Node stream would report. The production writer still writes framed messages to child stdin. This keeps a real child alive while the test deterministically distinguishes transport failure from process exit without reaching into platform-specific pipe handles. -Language-server teardown targets the whole descendant tree through a negative process-group id on POSIX and synchronous `taskkill /T /F` on Windows, with a direct-child fallback when the tree is already gone. A read-only provider query retries once only when its pooled transport becomes dead after the liveness check; errors from a still-live server are not replayed. Terminal tests wait for their observable rendered output instead of assuming one event-loop turn is sufficient. +Language-server teardown targets the whole descendant tree through a negative process-group id on POSIX and synchronous `taskkill /T /F` on Windows. Windows suppresses only taskkill's already-absent-tree status; command, permission, and other tree-kill failures remain teardown failures. A read-only provider query retries once only when its selected pooled transport fails before or during that query; errors from a still-live server are not replayed. Terminal tests wait for their observable rendered output instead of assuming one event-loop turn is sufficient. Tests for a genuinely POSIX-only primitive use a narrow Windows exclusion on that case. Adjacent cross-platform cases continue to pin non-regular file rejection, unavailable command rejection, and inaccessible working-directory rejection. Supported Windows paths remain inside the per-file coverage gate rather than being excluded with their test files. @@ -30,4 +30,4 @@ Tests for a genuinely POSIX-only primitive use a narrow Windows exclusion on tha ## Consequences -Portable fixtures are slightly more explicit because expected paths derive from shared native constants and transport failures enter through a narrow writer seam. Platform-only exclusions require a neighboring cross-platform assertion for the product behavior they support. Windows teardown depends on the host `taskkill` command after graceful protocol shutdown has failed; a synchronous result keeps disposal bounded and makes descendant exit observable before cleanup returns. +Portable fixtures are slightly more explicit because expected paths derive from shared native constants and transport failures enter through a narrow writer seam. Platform-only exclusions require a neighboring cross-platform assertion for the product behavior they support. Windows teardown depends on the host `taskkill` command after graceful protocol shutdown has failed; a successful synchronous result keeps disposal bounded and makes descendant exit observable before cleanup returns, while a failed tree kill remains visible to the disposer. diff --git a/.agents/notes/implemented/testing/2026-07-22-cross-platform-test-fixtures.zh.md b/.agents/notes/implemented/testing/2026-07-22-cross-platform-test-fixtures.zh.md index f61441e2db..43942ec046 100644 --- a/.agents/notes/implemented/testing/2026-07-22-cross-platform-test-fixtures.zh.md +++ b/.agents/notes/implemented/testing/2026-07-22-cross-platform-test-fixtures.zh.md @@ -16,7 +16,7 @@ Status: implemented 传输故障测试会注入连接的消息写入器，并传入与真实 Node 流相同的异步写入回调错误。生产写入器仍会把分帧消息写入子进程 stdin。这种方式让真实子进程保持存活，使测试无需触及平台特有的管道句柄，也能确定性地区分传输故障与进程退出。 -语言服务器的资源清理会终止整棵后代进程树：POSIX 使用负数进程组 ID，Windows 同步执行 `taskkill /T /F`；若进程树已经不存在，则回退到直接终止子进程。只读的提供方查询仅在池化传输于存活检查后失效时重试一次；服务器仍存活时返回的错误不会重放。终端测试会等待可观察的渲染输出，不假设一次事件循环轮转已经足够。 +语言服务器的资源清理会终止整棵后代进程树：POSIX 使用负数进程组 ID，Windows 同步执行 `taskkill /T /F`。Windows 只会忽略 taskkill 返回的「进程树已经不存在」状态；命令执行失败、权限错误及其他终止进程树的失败仍属于资源清理失败。只读的提供方查询仅在选定的池化传输于该次查询开始前或执行期间失效时重试一次；服务器仍存活时返回的错误不会重放。终端测试会等待可观察的渲染输出，不假设一次事件循环轮转已经足够。 对于真正仅存在于 POSIX 的原语，测试只在该用例上排除 Windows。相邻的跨平台用例仍会固定拒绝非普通文件、不可用命令和无法访问的工作目录的行为。Windows 上受支持的路径仍受逐文件覆盖率门禁约束，不会随测试文件一起排除。 @@ -30,4 +30,4 @@ Status: implemented ## 后果 -可移植 fixture 需要更显式地构造，因为预期路径要从共享的原生常量派生，传输故障则通过狭窄的写入器 seam 注入。仅适用于特定平台的排除项必须配有相邻的跨平台断言，以继续覆盖相应的产品行为。协议级优雅关停失败后，Windows 上的资源清理依赖宿主的 `taskkill` 命令；同步取得命令结果让 dispose 的完成边界明确，并确保清理返回前即可观察到后代进程退出。 +可移植 fixture 需要更显式地构造，因为预期路径要从共享的原生常量派生，传输故障则通过狭窄的写入器 seam 注入。仅适用于特定平台的排除项必须配有相邻的跨平台断言，以继续覆盖相应的产品行为。协议级优雅关停失败后，Windows 上的资源清理依赖宿主的 `taskkill` 命令；命令同步执行成功时，dispose 的完成边界明确，并确保清理返回前即可观察到后代进程退出；若进程树终止失败，dispose 的调用方仍能观察到该失败。 diff --git a/packages/lsp/lsp-local/README.md b/packages/lsp/lsp-local/README.md index 269fe6b666..7c6c05b7df 100644 --- a/packages/lsp/lsp-local/README.md +++ b/packages/lsp/lsp-local/README.md @@ -7,10 +7,10 @@ Namespace plugin (`name` / `inject` / `Config` / `apply`, no default export). ## What it does - Resolves every server-local setting before registration; an invalid mapping or registration conflict rolls back earlier entries, so a failed load leaves no provider routes. -- Lazily single-flights one server process per `(server id, canonical workspace realpath)`. A live server error is not replayed; if the transport becomes dead between the pool's liveness check and a read-only query, the provider evicts it and retries that query once on a fresh process. +- Lazily single-flights one server process per `(server id, canonical workspace realpath)`. A live server error is not replayed; if the selected pooled transport fails before or during a read-only query, the provider awaits its disposal and retries that query once on a fresh process. - Uses a compatibility-first **transient-open** sequence per query: canonicalize and read the source with Node APIs, `textDocument/didOpen` (version 1, full text), the requested request, then `textDocument/didClose` in `finally`. A failed or canceled `didOpen` write terminates the instance before the pool can reuse it. Documents close after each call, so the first version needs no `didChange`, content cache, or document LRU. - Serializes each source-read/open/query/close lifecycle through one abortable per-workspace queue so queued calls read current source only when their turn starts; distinct workspaces run in parallel. -- After protocol shutdown fails, terminates the server's descendant tree through POSIX process-group signaling or synchronous Windows `taskkill /T /F`, with a direct-child fallback for teardown races. +- After protocol shutdown fails, terminates the server's descendant tree through POSIX process-group signaling or synchronous Windows `taskkill /T /F`. Windows suppresses only taskkill's already-absent-tree result; command, permission, and other tree-kill failures remain visible. - Reads sources through Node filesystem APIs in the subprocess's host namespace — NOT `ctx.fs`, and emits no `fs/observed`: only the LSP result is model-visible, so a query does not satisfy read-before-write policy. ## Configuration diff --git a/packages/lsp/lsp-local/src/connection.ts b/packages/lsp/lsp-local/src/connection.ts index 7ed3e09054..1103c4dbd2 100644 --- a/packages/lsp/lsp-local/src/connection.ts +++ b/packages/lsp/lsp-local/src/connection.ts @@ -52,7 +52,7 @@ export type ConnectionWriter = ( export interface ProcessTreeOperations { /** Signal a POSIX process group. */ readonly signal: (target: number, signal: NodeJS.Signals) => void - /** Signal the direct child when group/tree signalling is unavailable. */ + /** Signal the direct child when POSIX group signaling is unavailable. */ readonly killChild: (signal: NodeJS.Signals) => void /** Terminate a Windows process tree by root pid. */ readonly taskkill: (pid: number) => void @@ -78,6 +78,9 @@ export type ProcessSignalRunner = (target: number, signal: NodeJS.Signals) => bo const processSignalRunner: ProcessSignalRunner = process.kill.bind(process) +/** taskkill status for "process not found": the requested process tree is already absent. */ +const TASKKILL_TREE_NOT_FOUND_STATUS = 128 + const writeConnectionMessage: ConnectionWriter = (stdin, message, done) => { stdin.write(encodeMessage(message), done) } @@ -93,6 +96,7 @@ export function taskkillProcessTree( ): void { const result = run('taskkill', ['/PID', String(pid), '/T', '/F'], { stdio: 'ignore' }) if (result.error !== undefined) throw result.error + if (result.status === TASKKILL_TREE_NOT_FOUND_STATUS) return if (result.status !== 0) throw new Error(`taskkill exited with status ${String(result.status)}`) } @@ -130,7 +134,8 @@ export async function waitForTreeExit( } /** - * Signal a detached process tree with platform-correct semantics and a direct-child fallback. + * Signal a detached process tree with platform-correct semantics. POSIX falls back to the direct + * child; Windows requires taskkill to reach the full tree. * @param platform - host platform. * @param pid - detached root process id. * @param signal - requested termination signal. @@ -142,9 +147,12 @@ export function signalProcessTree( signal: NodeJS.Signals, operations: ProcessTreeOperations, ): void { + if (platform === 'win32') { + operations.taskkill(pid) + return + } try { - if (platform === 'win32') operations.taskkill(pid) - else operations.signal(-pid, signal) + operations.signal(-pid, signal) } catch { try { operations.killChild(signal) @@ -219,6 +227,15 @@ export class LspConnection { return this.closeReason !== undefined } + /** + * Test whether a caught error is this connection's retained fatal transport cause. + * @param error - error caught by the instance or provider. + * @returns `true` only when this connection produced that exact failure. + */ + failedWith(error: unknown): boolean { + return this.closeReason === error + } + /** * Send a request and await its result. * @param method - the JSON-RPC method. @@ -291,10 +308,7 @@ export class LspConnection { return await waitForTreeExit(this.processTreeAlive.bind(this), signal) } - /** - * Signal the whole process tree so helper processes are reached; fall back to the direct child if - * tree signaling fails. Never throws because teardown races process exit. - */ + /** Signal the whole process tree. */ private signalTree(sig: NodeJS.Signals): void { const pid = this.child.pid if (pid === undefined) return diff --git a/packages/lsp/lsp-local/src/index.ts b/packages/lsp/lsp-local/src/index.ts index 7a4b1b6b6d..dda3558130 100644 --- a/packages/lsp/lsp-local/src/index.ts +++ b/packages/lsp/lsp-local/src/index.ts @@ -2,8 +2,8 @@ * Generic stdio language-server backend for `ctx.lsp`. One plugin instance configures a named table * of server commands and registers one isolated provider for each entry. Every provider lazily * single-flights one server process per canonical workspace realpath, serves transient-open queries - * through it, and replaces a transport that dies between a pool liveness check and the next - * read-only query. Providers read sources through Node APIs in the host namespace (not `ctx.fs`) + * through it, and replaces a selected transport that fails before or during the next read-only + * query. Providers read sources through Node APIs in the host namespace (not `ctx.fs`) * and trust their configured servers — no sandbox confinement. * * Namespace plugin (named exports, no default export). Lifecycle is effect-scoped: disposal @@ -224,16 +224,20 @@ class LocalLspProvider implements LspProvider { try { return await instance.query(request, source, signal) } catch (error) { - // A child can die after the pre-query liveness check but before or during the next write. - // Queries are read-only, so replace a newly failed transport once and retry transparently. - if (!instance.dead) throw error + // A selected child can have died while idle or fail during the next write. Queries are + // read-only, so replace that transport once and retry transparently. + if (!instance.isTransportFailure(error)) throw error + await instance.dispose() this.evictIfCurrent(workspace, instance) this.assertActive(signal) instance = this.instanceFor(workspace) return await instance.query(request, source, signal) } finally { - // Drop a crashed slot only when it still owns this instance; a replacement must survive. - if (instance.dead) this.evictIfCurrent(workspace, instance) + // Reach quiescence before dropping a dead slot; a replacement must survive this ownership check. + if (instance.dead) { + await instance.dispose() + this.evictIfCurrent(workspace, instance) + } } }) } diff --git a/packages/lsp/lsp-local/src/instance.ts b/packages/lsp/lsp-local/src/instance.ts index eecc5d4e5e..266dd3c59f 100644 --- a/packages/lsp/lsp-local/src/instance.ts +++ b/packages/lsp/lsp-local/src/instance.ts @@ -83,6 +83,15 @@ export class LspInstance { return this.processClosed || this.disposed || this.connection.failed } + /** + * Test whether a caught query error came from this instance's transport. + * @param error - error caught by the provider. + * @returns `true` only for the connection's retained fatal transport cause. + */ + isTransportFailure(error: unknown): boolean { + return this.connection.failedWith(error) + } + /** * Run one query through the serialized queue. * @param request - the resolved provider query. @@ -94,7 +103,12 @@ export class LspInstance { // 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 = abortable(this.queue, signal).then(() => this.runQuery(request, source, signal)) + const run = abortable(this.queue, signal) + .then(() => this.runQuery(request, source, signal)) + .catch(async (error: unknown) => { + if (this.isTransportFailure(error)) await this.startTeardown() + throw error + }) // 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. diff --git a/packages/lsp/lsp-local/tests/connection.spec.ts b/packages/lsp/lsp-local/tests/connection.spec.ts index 9fea82b43f..aa7e819cb6 100644 --- a/packages/lsp/lsp-local/tests/connection.spec.ts +++ b/packages/lsp/lsp-local/tests/connection.spec.ts @@ -65,6 +65,12 @@ describe('LspConnection', () => { await expect(conn.request('textDocument/hover', {})).rejects.toThrow(/server refused the request/) }) + it('treats signaling an already-closed child as a teardown race', async () => { + const conn = connectScript('') + await conn.closed + expect(() => { conn.kill() }).not.toThrow() + }) + it('answers a server workspace/configuration request from static config', async () => { const seen: SeenRequest[] = [] const conn = connect( @@ -272,23 +278,27 @@ describe('process-tree signaling', () => { expect(operations.signal).toHaveBeenCalledWith(-42, 'SIGKILL') }) - it('falls back to the direct child and tolerates an already-dead child', () => { + it('surfaces a Windows taskkill failure without downgrading to the direct child', () => { const fallback = fakeProcessTreeOperations() vi.mocked(fallback.taskkill).mockImplementation(() => { throw new Error('taskkill unavailable') }) - signalProcessTree('win32', 42, 'SIGTERM', fallback) - expect(fallback.killChild).toHaveBeenCalledWith('SIGTERM') - - const gone = fakeProcessTreeOperations() - vi.mocked(gone.signal).mockImplementation(() => { throw new Error('group gone') }) - vi.mocked(gone.killChild).mockImplementation(() => { throw new Error('child gone') }) - expect(() => { signalProcessTree('linux', 42, 'SIGKILL', gone) }).not.toThrow() + expect(() => { signalProcessTree('win32', 42, 'SIGTERM', fallback) }).toThrow(/taskkill unavailable/) + expect(fallback.killChild).not.toHaveBeenCalled() }) - it('runs taskkill for the full tree and rejects command failures', () => { + it('tolerates a POSIX tree-signaling race after the direct child is already gone', () => { + const posixGone = fakeProcessTreeOperations() + vi.mocked(posixGone.signal).mockImplementation(() => { throw new Error('group gone') }) + vi.mocked(posixGone.killChild).mockImplementation(() => { throw new Error('child gone') }) + expect(() => { signalProcessTree('linux', 42, 'SIGKILL', posixGone) }).not.toThrow() + }) + + it('runs taskkill for the full tree, accepts an absent tree, and rejects command failures', () => { const success: TaskkillRunner = vi.fn(() => ({ status: 0 })) taskkillProcessTree(42, success) expect(success).toHaveBeenCalledWith('taskkill', ['/PID', '42', '/T', '/F'], { stdio: 'ignore' }) + expect(() => { taskkillProcessTree(42, () => ({ status: 128 })) }).not.toThrow() + const spawnFailure = new Error('cannot spawn taskkill') expect(() => { taskkillProcessTree(42, () => ({ status: null, error: spawnFailure })) }).toThrow(spawnFailure) expect(() => { taskkillProcessTree(42, () => ({ status: 1 })) }).toThrow(/status 1/) diff --git a/packages/lsp/lsp-local/tests/instance.spec.ts b/packages/lsp/lsp-local/tests/instance.spec.ts index d08a431192..9f246e602a 100644 --- a/packages/lsp/lsp-local/tests/instance.spec.ts +++ b/packages/lsp/lsp-local/tests/instance.spec.ts @@ -216,6 +216,17 @@ describe('LspInstance query and abort', () => { expect(instance.dead).toBe(true) }) + it('awaits process exit before rejecting a request write failure', async () => { + const instance = makeInstance({}, { + shutdownTimeoutMs: 100, + killGraceMs: 100, + }, failingWriter('textDocument/definition')) + // The pid is observed only to prove the owned subprocess reached quiescence before rejection. + const pid = (instance as unknown as { connection: { pid: number } }).connection.pid + await expect(run(instance, 'goToDefinition')).rejects.toThrow(/fixture textDocument\/definition failure/) + expect(processAlive(pid)).toBe(false) + }) + it('rejects when the server lacks the operation capability', async () => { const instance = makeInstance({ LSP_FAKE_CAPS: JSON.stringify({ definitionProvider: false }), LSP_FAKE_DEF: 'null' }) await expect(run(instance, 'goToDefinition')).rejects.toThrow(/does not support goToDefinition/) diff --git a/packages/lsp/lsp-local/tests/lifecycle.spec.ts b/packages/lsp/lsp-local/tests/lifecycle.spec.ts index 826905ed26..47b8d78bb4 100644 --- a/packages/lsp/lsp-local/tests/lifecycle.spec.ts +++ b/packages/lsp/lsp-local/tests/lifecycle.spec.ts @@ -122,9 +122,15 @@ describe('lsp-local end to end over a fake server', () => { await ctx.fiber.dispose() }) - it('rejects a non-utf-16 position encoding at initialize', async () => { - const ctx = await mount({ LSP_FAKE_ENCODING: 'utf-8', LSP_FAKE_DEF: 'null' }) + it('rejects a non-utf-16 position encoding at initialize without retrying', async () => { + const marker = join(root, 'initialize-rejection-exit.log') + const ctx = await mount({ + LSP_FAKE_ENCODING: 'utf-8', + LSP_FAKE_DEF: 'null', + LSP_FAKE_EXIT_MARKER: marker, + }) await expect(ctx.lsp.query(query('goToDefinition'))).rejects.toThrow(/unsupported position encoding/) + expect(await readFile(marker, 'utf8')).toBe('EXIT\nCLEAN\n') await ctx.fiber.dispose() })