From 69f7182f053698260aacc0bc6086caddb6f83f11 Mon Sep 17 00:00:00 2001 From: Kit Langton Date: Thu, 16 Apr 2026 17:36:04 -0400 Subject: [PATCH] fix: bound LSP diagnostic enrichment in write/edit/apply_patch (#22872) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Issue #22872 reports the write tool hanging indefinitely after a file is written. Two underlying causes, both in the post-write LSP enrichment path: 1. LSPClient.create wraps the `initialize` request in a 45s withTimeout. If the spawned LSP process is wedged (happens with pyright under certain conditions), every write that matches that LSP blocks the tool for up to 45s even though the file is on disk. 2. Server.spawn for npm-distributed LSPs (pyright, tsserver, biome, ...) calls Npm.which, which internally uses arborist.reify with no timeout. In sandboxed containers with no network access this promise never resolves — the write tool hangs forever. Fix applied at three layers of defense: - write.ts / edit.ts / apply_patch.ts: wrap the touchFile + diagnostics tail in a 5s Effect.timeout with catch-to-empty. Diagnostics are a best-effort enrichment; they must not block the tool's return after the file is already written. - lsp.ts schedule(): bound server.spawn with a 10s Promise.race timeout. On timeout the server is added to s.broken so subsequent touches short-circuit instantly instead of re-racing. - client.ts: lower the `initialize` withTimeout from 45_000 to 10_000. If a server hasn't responded to initialize in 10s it's wedged; 45s was punishing for no benefit. Reproducer tests (added in earlier commits on this branch) now pass: - write-lsp-hang.test.ts (branch A, 45s initialize timeout) - write-lsp-spawn-hang.test.ts (branch B, forever Npm.which) Both complete in ~5s. Full opencode test suite: 1934 pass, 0 fail. --- packages/opencode/src/lsp/client.ts | 8 +++++- packages/opencode/src/lsp/lsp.ts | 19 +++++++++++++- packages/opencode/src/tool/apply_patch.ts | 25 +++++++++++++------ packages/opencode/src/tool/edit.ts | 14 ++++++++--- packages/opencode/src/tool/write.ts | 18 ++++++++++--- .../opencode/test/tool/write-lsp-hang.test.ts | 8 +++--- .../test/tool/write-lsp-spawn-hang.test.ts | 7 +++--- 7 files changed, 77 insertions(+), 22 deletions(-) diff --git a/packages/opencode/src/lsp/client.ts b/packages/opencode/src/lsp/client.ts index 59a64ca1ed..64545657fd 100644 --- a/packages/opencode/src/lsp/client.ts +++ b/packages/opencode/src/lsp/client.ts @@ -113,7 +113,13 @@ export async function create(input: { serverID: string; server: LSPServer.Handle }, }, }), - 45_000, + // 10s budget for LSP `initialize`. Previously 45s, which is far + // longer than a healthy server needs and effectively froze tools + // that await this path (e.g. write → lsp.touchFile). Servers that + // genuinely need more time for first-boot indexing publish their + // progress via window/workDoneProgress after initialize returns. + // See issue #22872. + 10_000, ).catch((err) => { l.error("initialize error", { error: err }) throw new InitializeError( diff --git a/packages/opencode/src/lsp/lsp.ts b/packages/opencode/src/lsp/lsp.ts index c946ed2b0f..f32dffc040 100644 --- a/packages/opencode/src/lsp/lsp.ts +++ b/packages/opencode/src/lsp/lsp.ts @@ -244,8 +244,22 @@ export const layer = Layer.effect( } async function schedule(server: LSPServer.Info, root: string, key: string) { - const handle = await runSpawn(server, root) + // Bound server.spawn so an unresponsive provisioning step + // (e.g. pyright's Npm.which → arborist.reify with no network) + // cannot hang touchFile forever. A 10s budget is generous for + // a cold start and an eternity for a wedged one. See #22872. + const SPAWN_TIMEOUT_MS = 10_000 + let spawnTimer: ReturnType | undefined + const timeoutP = new Promise<"__lsp_spawn_timeout__">((resolve) => { + spawnTimer = setTimeout(() => resolve("__lsp_spawn_timeout__"), SPAWN_TIMEOUT_MS) + }) + const handle = await Promise.race([runSpawn(server, root), timeoutP]) .then((value) => { + if (value === "__lsp_spawn_timeout__") { + s.broken.add(key) + log.error(`LSP server ${server.id} spawn timed out after ${SPAWN_TIMEOUT_MS}ms`, { root }) + return undefined + } if (!value) s.broken.add(key) return value }) @@ -254,6 +268,9 @@ export const layer = Layer.effect( log.error(`Failed to spawn LSP server ${server.id}`, { error: err }) return undefined }) + .finally(() => { + if (spawnTimer) clearTimeout(spawnTimer) + }) if (!handle) return undefined log.info("spawned lsp server", { serverID: server.id, root }) diff --git a/packages/opencode/src/tool/apply_patch.ts b/packages/opencode/src/tool/apply_patch.ts index 7da7dd255c..14ecec0784 100644 --- a/packages/opencode/src/tool/apply_patch.ts +++ b/packages/opencode/src/tool/apply_patch.ts @@ -1,6 +1,7 @@ import z from "zod" import * as path from "path" -import { Effect } from "effect" +import { Duration, Effect } from "effect" +import type * as LSPClient from "../lsp/client" import * as Tool from "./tool" import { Bus } from "../bus" import { FileWatcher } from "../file/watcher" @@ -244,13 +245,21 @@ export const ApplyPatchTool = Tool.define( yield* bus.publish(FileWatcher.Event.Updated, update) } - // Notify LSP of file changes and collect diagnostics - for (const change of fileChanges) { - if (change.type === "delete") continue - const target = change.movePath ?? change.filePath - yield* lsp.touchFile(target, true) - } - const diagnostics = yield* lsp.diagnostics() + // Notify LSP of file changes and collect diagnostics. Best-effort; + // bounded at 5s total so a slow or wedged LSP cannot block the + // tool result after the patches have already been applied. See + // issue #22872 and write.ts for the same pattern. + const diagnostics = yield* Effect.gen(function* () { + for (const change of fileChanges) { + if (change.type === "delete") continue + const target = change.movePath ?? change.filePath + yield* lsp.touchFile(target, true) + } + return yield* lsp.diagnostics() + }).pipe( + Effect.timeout(Duration.seconds(5)), + Effect.catch(() => Effect.succeed({} as Record)), + ) // Generate output summary const summaryLines = fileChanges.map((change) => { diff --git a/packages/opencode/src/tool/edit.ts b/packages/opencode/src/tool/edit.ts index 62b96cba82..84f2e3b24e 100644 --- a/packages/opencode/src/tool/edit.ts +++ b/packages/opencode/src/tool/edit.ts @@ -5,9 +5,10 @@ import z from "zod" import * as path from "path" -import { Effect } from "effect" +import { Duration, Effect } from "effect" import * as Tool from "./tool" import { LSP } from "../lsp" +import type * as LSPClient from "../lsp/client" import { createTwoFilesPatch, diffLines } from "diff" import DESCRIPTION from "./edit.txt" import { File } from "../file" @@ -166,8 +167,15 @@ export const EditTool = Tool.define( }) let output = "Edit applied successfully." - yield* lsp.touchFile(filePath, true) - const diagnostics = yield* lsp.diagnostics() + // LSP diagnostic enrichment is best-effort; see write.ts for + // rationale. Never block the tool on a slow or wedged LSP. + const diagnostics = yield* Effect.gen(function* () { + yield* lsp.touchFile(filePath, true) + return yield* lsp.diagnostics() + }).pipe( + Effect.timeout(Duration.seconds(5)), + Effect.catch(() => Effect.succeed({} as Record)), + ) const normalizedFilePath = AppFileSystem.normalizePath(filePath) const block = LSP.Diagnostic.report(filePath, diagnostics[normalizedFilePath] ?? []) if (block) output += `\n\nLSP errors detected in this file, please fix:\n${block}` diff --git a/packages/opencode/src/tool/write.ts b/packages/opencode/src/tool/write.ts index c5871eb0ef..ed3075f0e4 100644 --- a/packages/opencode/src/tool/write.ts +++ b/packages/opencode/src/tool/write.ts @@ -1,8 +1,9 @@ import z from "zod" import * as path from "path" -import { Effect } from "effect" +import { Duration, Effect } from "effect" import * as Tool from "./tool" import { LSP } from "../lsp" +import type * as LSPClient from "../lsp/client" import { createTwoFilesPatch } from "diff" import DESCRIPTION from "./write.txt" import { Bus } from "../bus" @@ -64,8 +65,19 @@ export const WriteTool = Tool.define( yield* filetime.read(ctx.sessionID, filepath) let output = "Wrote file successfully." - yield* lsp.touchFile(filepath, true) - const diagnostics = yield* lsp.diagnostics() + // LSP diagnostic enrichment is best-effort. If the LSP server is + // slow to spawn, slow to initialize, or wedged entirely (e.g. a + // pyright install hanging on network in a sandboxed container) + // we must not block the tool's return on it — the file is + // already on disk. Bound at 5s and fall back to an empty + // diagnostics set. See issue #22872. + const diagnostics = yield* Effect.gen(function* () { + yield* lsp.touchFile(filepath, true) + return yield* lsp.diagnostics() + }).pipe( + Effect.timeout(Duration.seconds(5)), + Effect.catch(() => Effect.succeed({} as Record)), + ) const normalizedFilepath = AppFileSystem.normalizePath(filepath) let projectDiagnosticsCount = 0 for (const [file, issues] of Object.entries(diagnostics)) { diff --git a/packages/opencode/test/tool/write-lsp-hang.test.ts b/packages/opencode/test/tool/write-lsp-hang.test.ts index a5a3f3f904..ae87b27367 100644 --- a/packages/opencode/test/tool/write-lsp-hang.test.ts +++ b/packages/opencode/test/tool/write-lsp-hang.test.ts @@ -84,9 +84,11 @@ describe("tool.write (LSP hang — issue #22872)", () => { expect(content).toBe("print('hi')") expect(result.output).toContain("Wrote file successfully") - // Regression guard: touchFile/diagnostics must not block the tool - // on the 45s LSPClient.create initialize timeout. - expect(elapsed).toBeLessThan(10_000) + // Regression guard: touchFile/diagnostics must not block the + // tool on the LSP initialize timeout. The write tool wraps + // its enrichment tail in a 5s Effect.timeout, so the tool + // must return within roughly 5s regardless of LSP state. + expect(elapsed).toBeLessThan(7_000) }), { config: { diff --git a/packages/opencode/test/tool/write-lsp-spawn-hang.test.ts b/packages/opencode/test/tool/write-lsp-spawn-hang.test.ts index 82f167270a..d55c0ef815 100644 --- a/packages/opencode/test/tool/write-lsp-spawn-hang.test.ts +++ b/packages/opencode/test/tool/write-lsp-spawn-hang.test.ts @@ -112,9 +112,10 @@ describe("tool.write (LSP spawn hang — issue #22872 forever branch)", () => { expect(content).toBe("print('hi')") expect(result.output).toContain("Wrote file successfully") - // The LSP spawn path is now blocked forever (Npm.Service.which - // returns Effect.never). The write tool must not wait on it. - expect(elapsed).toBeLessThan(10_000) + // The LSP spawn path is blocked forever (Npm.Service.which + // returns Effect.never). The write tool's 5s enrichment + // timeout must win, so the tool returns within roughly 5s. + expect(elapsed).toBeLessThan(7_000) }), ), 15_000,