From 617c9841efe03d1661c7de38696cefea7e7cf122 Mon Sep 17 00:00:00 2001 From: Kit Langton Date: Thu, 16 Apr 2026 17:03:19 -0400 Subject: [PATCH] test: add reproducer for Npm.which-forever hang in Pyright spawn MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a second reproducer covering the 'forever' branch of issue #22872: when Pyright.spawn calls Npm.which('pyright') and the npm registry is unreachable (sandboxed container), arborist.reify blocks indefinitely with no timeout. Changes: - Adds optional Info.spawnEffect alongside the existing async Info.spawn. spawnEffect returns an Effect that can yield from Npm.Service, making npm lookups injectable for tests. - Migrates Pyright to use spawnEffect, pulling the venv probing logic into a reusable pyrightVenvInitialization helper. The legacy async spawn stays for backwards compatibility. - Threads Npm.Service through LSP.layer so getClients captures a stable reference and uses it for any server that provides spawnEffect. - Adds test/tool/write-lsp-spawn-hang.test.ts — mocks Npm.Service.which with Effect.never and asserts the write tool still returns in < 10s. Fails today (hangs forever); the fix must bound the touchFile tail so the tool cannot wait on a wedged LSP spawn. The two reproducers now cover both hang branches: - write-lsp-hang.test.ts: 45s LSPClient.create initialize timeout - write-lsp-spawn-hang.test.ts: unbounded Npm.which arborist.reify --- packages/opencode/src/lsp/lsp.ts | 19 ++- packages/opencode/src/lsp/server.ts | 75 +++++++---- .../test/tool/write-lsp-spawn-hang.test.ts | 122 ++++++++++++++++++ 3 files changed, 186 insertions(+), 30 deletions(-) create mode 100644 packages/opencode/test/tool/write-lsp-spawn-hang.test.ts diff --git a/packages/opencode/src/lsp/lsp.ts b/packages/opencode/src/lsp/lsp.ts index d4d1e75634..c946ed2b0f 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,9 +234,17 @@ 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) + const handle = await runSpawn(server, root) .then((value) => { if (!value) s.broken.add(key) return value @@ -504,7 +517,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 390c5f2428..e98d9e190a 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: 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, - } + 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 } + }), +} + +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/test/tool/write-lsp-spawn-hang.test.ts b/packages/opencode/test/tool/write-lsp-spawn-hang.test.ts new file mode 100644 index 0000000000..82f167270a --- /dev/null +++ b/packages/opencode/test/tool/write-lsp-spawn-hang.test.ts @@ -0,0 +1,122 @@ +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 now blocked forever (Npm.Service.which + // returns Effect.never). The write tool must not wait on it. + expect(elapsed).toBeLessThan(10_000) + }), + ), + 15_000, + ) +})