diff --git a/packages/opencode/src/lsp/client.ts b/packages/opencode/src/lsp/client.ts index 59a64ca1ed2e..64545657fd7c 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 d4d1e7563492..f32dffc04077 100644 --- a/packages/opencode/src/lsp/lsp.ts +++ b/packages/opencode/src/lsp/lsp.ts @@ -13,6 +13,7 @@ import { Process } from "../util" import { spawn as lspspawn } from "./launch" import { Effect, Layer, Context } from "effect" import { InstanceState } from "@/effect" +import { Npm as EffectNpm } from "@opencode-ai/shared/npm" const log = Log.create({ service: "lsp" }) @@ -160,6 +161,10 @@ export const layer = Layer.effect( Service, Effect.gen(function* () { const config = yield* Config.Service + // Resolve Npm.Service once at layer construction so per-call methods + // (getClients → server.spawnEffect) can capture a stable reference + // without propagating Npm.Service into the public LSP Interface. + const npm = yield* EffectNpm.Service const state = yield* InstanceState.make( Effect.fn("LSP.state")(function* () { @@ -229,10 +234,32 @@ export const layer = Layer.effect( const extension = path.parse(file).ext || file const result: LSPClient.Info[] = [] + const runSpawn = (server: LSPServer.Info, root: string): Promise => { + if (server.spawnEffect) { + return Effect.runPromise( + server.spawnEffect(root).pipe(Effect.provideService(EffectNpm.Service, npm)), + ) + } + return server.spawn(root) + } + async function schedule(server: LSPServer.Info, root: string, key: string) { - const handle = await server - .spawn(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 }) @@ -241,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 }) @@ -504,7 +534,7 @@ export const layer = Layer.effect( }), ) -export const defaultLayer = layer.pipe(Layer.provide(Config.defaultLayer)) +export const defaultLayer = layer.pipe(Layer.provide(Config.defaultLayer), Layer.provide(EffectNpm.defaultLayer)) export namespace Diagnostic { const MAX_PER_FILE = 20 diff --git a/packages/opencode/src/lsp/server.ts b/packages/opencode/src/lsp/server.ts index 390c5f24287f..e98d9e190a50 100644 --- a/packages/opencode/src/lsp/server.ts +++ b/packages/opencode/src/lsp/server.ts @@ -1,6 +1,7 @@ import type { ChildProcessWithoutNullStreams } from "child_process" import path from "path" import os from "os" +import { Effect, Option } from "effect" import { Global } from "../global" import { Log } from "../util" import { text } from "node:stream/consumers" @@ -13,6 +14,7 @@ import { Process } from "../util" import { which } from "../util/which" import { Module } from "@opencode-ai/shared/util/module" import { spawn } from "./launch" +import { Npm as EffectNpm } from "@opencode-ai/shared/npm" import { Npm } from "../npm" const log = Log.create({ service: "lsp.server" }) @@ -61,6 +63,14 @@ export interface Info { global?: boolean root: RootFunction spawn(root: string): Promise + /** + * Optional Effect-based variant of `spawn`. When present, the LSP + * runtime uses this instead of `spawn` so the server body can yield + * from platform services like `Npm.Service.which`. Use for servers + * distributed via npm so tests can inject fakes. Existing `spawn` + * implementations continue to work unchanged. + */ + spawnEffect?(root: string): Effect.Effect } export const Deno: Info = { @@ -486,6 +496,9 @@ export const Pyright: Info = { extensions: [".py", ".pyi"], root: NearestRoot(["pyproject.toml", "setup.py", "setup.cfg", "requirements.txt", "Pipfile", "pyrightconfig.json"]), async spawn(root) { + // Legacy async entry point — kept so the Info interface stays + // backwards-compatible. The live runtime always prefers spawnEffect + // below; this path only runs if someone invokes spawn() directly. let binary = which("pyright-langserver") const args = [] if (!binary) { @@ -495,34 +508,42 @@ export const Pyright: Info = { binary = resolved } args.push("--stdio") + const initialization = await pyrightVenvInitialization(root) + const proc = spawn(binary, args, { cwd: root, env: { ...process.env } }) + return { process: proc, initialization } + }, + spawnEffect: (root) => + Effect.gen(function* () { + let binary = which("pyright-langserver") + if (!binary) { + if (Flag.OPENCODE_DISABLE_LSP_DOWNLOAD) return undefined + const npm = yield* EffectNpm.Service + const resolved = yield* npm.which("pyright") + if (Option.isNone(resolved)) return undefined + binary = resolved.value + } + const initialization = yield* Effect.promise(() => pyrightVenvInitialization(root)) + const proc = spawn(binary, ["--stdio"], { cwd: root, env: { ...process.env } }) + return { process: proc, initialization } + }), +} - const initialization: Record = {} - - const potentialVenvPaths = [process.env["VIRTUAL_ENV"], path.join(root, ".venv"), path.join(root, "venv")].filter( - (p): p is string => p !== undefined, - ) - for (const venvPath of potentialVenvPaths) { - const isWindows = process.platform === "win32" - const potentialPythonPath = isWindows - ? path.join(venvPath, "Scripts", "python.exe") - : path.join(venvPath, "bin", "python") - if (await Filesystem.exists(potentialPythonPath)) { - initialization["pythonPath"] = potentialPythonPath - break - } - } - - const proc = spawn(binary, args, { - cwd: root, - env: { - ...process.env, - }, - }) - return { - process: proc, - initialization, +async function pyrightVenvInitialization(root: string): Promise> { + const initialization: Record = {} + const potentialVenvPaths = [process.env["VIRTUAL_ENV"], path.join(root, ".venv"), path.join(root, "venv")].filter( + (p): p is string => p !== undefined, + ) + for (const venvPath of potentialVenvPaths) { + const isWindows = process.platform === "win32" + const potentialPythonPath = isWindows + ? path.join(venvPath, "Scripts", "python.exe") + : path.join(venvPath, "bin", "python") + if (await Filesystem.exists(potentialPythonPath)) { + initialization["pythonPath"] = potentialPythonPath + break } - }, + } + return initialization } export const ElixirLS: Info = { diff --git a/packages/opencode/src/tool/apply_patch.ts b/packages/opencode/src/tool/apply_patch.ts index 7da7dd255c52..14ecec078431 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 62b96cba820c..84f2e3b24e59 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 c5871eb0ef26..ed3075f0e40a 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/fixture/lsp/hanging-lsp-server.js b/packages/opencode/test/fixture/lsp/hanging-lsp-server.js new file mode 100644 index 000000000000..73b3cb2fee50 --- /dev/null +++ b/packages/opencode/test/fixture/lsp/hanging-lsp-server.js @@ -0,0 +1,44 @@ +// Fake LSP server that intentionally never responds to `initialize`. +// Used by tests that reproduce hangs in the LSP touchFile flow when an +// LSP server process spawns successfully but the handshake stalls. The +// process also ignores SIGTERM for a short period to surface any teardown +// issues, but exits cleanly on SIGKILL. + +let readBuffer = Buffer.alloc(0) + +function decodeFrames(buffer) { + const results = [] + let idx + while ((idx = buffer.indexOf("\r\n\r\n")) !== -1) { + const header = buffer.slice(0, idx).toString("utf8") + const m = /Content-Length:\s*(\d+)/i.exec(header) + const len = m ? parseInt(m[1], 10) : 0 + const bodyStart = idx + 4 + const bodyEnd = bodyStart + len + if (buffer.length < bodyEnd) break + results.push(buffer.slice(bodyStart, bodyEnd).toString("utf8")) + buffer = buffer.slice(bodyEnd) + } + return { messages: results, rest: buffer } +} + +process.stdin.on("data", (chunk) => { + readBuffer = Buffer.concat([readBuffer, chunk]) + const { messages, rest } = decodeFrames(readBuffer) + readBuffer = rest + // Swallow everything — including `initialize`. Never reply. + for (const _ of messages) { + // no-op + } +}) + +// Keep the process alive until parent terminates us or closes stdin. +const keepalive = setInterval(() => {}, 60_000) +process.stdin.on("end", () => { + clearInterval(keepalive) + process.exit(0) +}) +process.stdin.on("close", () => { + clearInterval(keepalive) + process.exit(0) +}) diff --git a/packages/opencode/test/tool/write-lsp-hang.test.ts b/packages/opencode/test/tool/write-lsp-hang.test.ts new file mode 100644 index 000000000000..ae87b27367ee --- /dev/null +++ b/packages/opencode/test/tool/write-lsp-hang.test.ts @@ -0,0 +1,106 @@ +import { afterEach, describe, expect } from "bun:test" +import { Effect, Layer } from "effect" +import path from "path" +import fs from "fs/promises" +import { WriteTool } from "../../src/tool/write" +import { Instance } from "../../src/project/instance" +import { LSP } from "../../src/lsp" +import { AppFileSystem } from "@opencode-ai/shared/filesystem" +import { FileTime } from "../../src/file/time" +import { Bus } from "../../src/bus" +import { Format } from "../../src/format" +import { Truncate } from "../../src/tool" +import { Tool } from "../../src/tool" +import { Agent } from "../../src/agent/agent" +import { SessionID, MessageID } from "../../src/session/schema" +import * as CrossSpawnSpawner from "../../src/effect/cross-spawn-spawner" +import { provideTmpdirInstance } from "../fixture/fixture" +import { testEffect } from "../lib/effect" + +// Reproduces issue #22872 — the write tool hangs when an LSP server for the +// file's extension spawns successfully but never answers the `initialize` +// request. The fake LSP here swallows every message, mimicking pyright in +// the reporter's Docker container. If the write tool correctly bounds the +// diagnostic-enrichment tail (lsp.touchFile + lsp.diagnostics) the whole +// call should finish quickly, well before the 45s LSPClient.create timeout. + +const HANGING_SERVER = path.resolve(__dirname, "..", "fixture", "lsp", "hanging-lsp-server.js") + +const ctx = { + sessionID: SessionID.make("ses_test-write-lsp-hang"), + messageID: MessageID.make(""), + callID: "", + agent: "build", + abort: AbortSignal.any([]), + messages: [], + metadata: () => Effect.void, + ask: () => Effect.void, +} + +afterEach(async () => { + await Instance.disposeAll() +}) + +const it = testEffect( + Layer.mergeAll( + LSP.defaultLayer, + AppFileSystem.defaultLayer, + FileTime.defaultLayer, + Bus.layer, + Format.defaultLayer, + CrossSpawnSpawner.defaultLayer, + Truncate.defaultLayer, + Agent.defaultLayer, + ), +) + +const init = Effect.fn("WriteLspHangTest.init")(function* () { + const info = yield* WriteTool + return yield* info.init() +}) + +const run = Effect.fn("WriteLspHangTest.run")(function* ( + args: Tool.InferParameters, + next: Tool.Context = ctx, +) { + const tool = yield* init() + return yield* tool.execute(args, next) +}) + +describe("tool.write (LSP hang — issue #22872)", () => { + it.live( + "completes promptly when the LSP server for this extension never finishes initialize", + () => + provideTmpdirInstance( + (dir) => + Effect.gen(function* () { + const filepath = path.join(dir, "hello.hang") + const started = Date.now() + const result = yield* run({ filePath: filepath, content: "print('hi')" }) + const elapsed = Date.now() - started + + // On disk content is correct. + const content = yield* Effect.promise(() => fs.readFile(filepath, "utf-8")) + expect(content).toBe("print('hi')") + expect(result.output).toContain("Wrote file successfully") + + // 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: { + lsp: { + "hang-ls": { + command: ["node", HANGING_SERVER], + extensions: [".hang"], + }, + }, + }, + }, + ), + 60_000, + ) +}) diff --git a/packages/opencode/test/tool/write-lsp-spawn-hang.test.ts b/packages/opencode/test/tool/write-lsp-spawn-hang.test.ts new file mode 100644 index 000000000000..d55c0ef81573 --- /dev/null +++ b/packages/opencode/test/tool/write-lsp-spawn-hang.test.ts @@ -0,0 +1,123 @@ +import { afterEach, beforeAll, afterAll, describe, expect } from "bun:test" +import { Effect, Layer, Option } from "effect" +import path from "path" +import fs from "fs/promises" +import { Npm } from "@opencode-ai/shared/npm" +import { Config } from "../../src/config" +import { WriteTool } from "../../src/tool/write" +import { Instance } from "../../src/project/instance" +import * as LSP from "../../src/lsp/lsp" +import { AppFileSystem } from "@opencode-ai/shared/filesystem" +import { FileTime } from "../../src/file/time" +import { Bus } from "../../src/bus" +import { Format } from "../../src/format" +import { Truncate } from "../../src/tool" +import { Tool } from "../../src/tool" +import { Agent } from "../../src/agent/agent" +import { SessionID, MessageID } from "../../src/session/schema" +import * as CrossSpawnSpawner from "../../src/effect/cross-spawn-spawner" +import { provideTmpdirInstance } from "../fixture/fixture" +import { testEffect } from "../lib/effect" + +// Reproduces the "forever" branch of issue #22872 — in a sandboxed +// container with no network and no cached pyright binary, Pyright.spawn +// calls `Npm.Service.which("pyright")` which internally uses +// `arborist.reify()` with no timeout. If the npm registry is +// unreachable, that promise never resolves and the write tool blocks +// indefinitely. +// +// Here we mock Npm.Service so `which("pyright")` returns Effect.never, +// simulating the unbounded network block. The write tool must still +// return quickly for the fix to be correct — shortening the 45s +// LSPClient.create initialize timeout would NOT help this case, so +// the fix must bound the touchFile enrichment tail itself. + +const ctx = { + sessionID: SessionID.make("ses_test-write-lsp-spawn-hang"), + messageID: MessageID.make(""), + callID: "", + agent: "build", + abort: AbortSignal.any([]), + messages: [], + metadata: () => Effect.void, + ask: () => Effect.void, +} + +// Ensure pyright-langserver isn't picked up from the user's real PATH +// during the test — we want the spawn to fall through to Npm.which. +let savedPath: string | undefined +beforeAll(() => { + savedPath = process.env.PATH + process.env.PATH = "" +}) +afterAll(() => { + process.env.PATH = savedPath +}) + +afterEach(async () => { + await Instance.disposeAll() +}) + +const hangingNpm = Layer.mock(Npm.Service)({ + add: () => Effect.never, + install: () => Effect.never, + outdated: () => Effect.succeed(false), + which: () => Effect.never as unknown as Effect.Effect>, +}) + +// Build the LSP layer with the hanging Npm mock in place of the real one. +// LSP.defaultLayer pre-provides the real EffectNpm.defaultLayer which would +// shadow any outer provide, so we wire the mock directly into LSP.layer. +const lspWithHangingNpm = LSP.layer.pipe(Layer.provide(Config.defaultLayer), Layer.provide(hangingNpm)) + +const it = testEffect( + Layer.mergeAll( + lspWithHangingNpm, + AppFileSystem.defaultLayer, + FileTime.defaultLayer, + Bus.layer, + Format.defaultLayer, + CrossSpawnSpawner.defaultLayer, + Truncate.defaultLayer, + Agent.defaultLayer, + ), +) + +const init = Effect.fn("WriteLspSpawnHangTest.init")(function* () { + const info = yield* WriteTool + return yield* info.init() +}) + +const run = Effect.fn("WriteLspSpawnHangTest.run")(function* ( + args: Tool.InferParameters, + next: Tool.Context = ctx, +) { + const tool = yield* init() + return yield* tool.execute(args, next) +}) + +describe("tool.write (LSP spawn hang — issue #22872 forever branch)", () => { + it.live( + "completes promptly when Npm.Service.which hangs forever during LSP spawn", + () => + provideTmpdirInstance((dir) => + Effect.gen(function* () { + const filepath = path.join(dir, "hello.py") + const started = Date.now() + const result = yield* run({ filePath: filepath, content: "print('hi')" }) + const elapsed = Date.now() - started + + // File is on disk even though LSP spawn is wedged. + const content = yield* Effect.promise(() => fs.readFile(filepath, "utf-8")) + expect(content).toBe("print('hi')") + expect(result.output).toContain("Wrote file successfully") + + // 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, + ) +})