1
0
Эх сурвалжийг харах

test: add reproducer for Npm.which-forever hang in Pyright spawn

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
Kit Langton 4 сар өмнө
parent
commit
617c9841ef

+ 16 - 3
packages/opencode/src/lsp/lsp.ts

@@ -13,6 +13,7 @@ import { Process } from "../util"
 import { spawn as lspspawn } from "./launch"
 import { spawn as lspspawn } from "./launch"
 import { Effect, Layer, Context } from "effect"
 import { Effect, Layer, Context } from "effect"
 import { InstanceState } from "@/effect"
 import { InstanceState } from "@/effect"
+import { Npm as EffectNpm } from "@opencode-ai/shared/npm"
 
 
 const log = Log.create({ service: "lsp" })
 const log = Log.create({ service: "lsp" })
 
 
@@ -160,6 +161,10 @@ export const layer = Layer.effect(
   Service,
   Service,
   Effect.gen(function* () {
   Effect.gen(function* () {
     const config = yield* Config.Service
     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<State>(
     const state = yield* InstanceState.make<State>(
       Effect.fn("LSP.state")(function* () {
       Effect.fn("LSP.state")(function* () {
@@ -229,9 +234,17 @@ export const layer = Layer.effect(
         const extension = path.parse(file).ext || file
         const extension = path.parse(file).ext || file
         const result: LSPClient.Info[] = []
         const result: LSPClient.Info[] = []
 
 
+        const runSpawn = (server: LSPServer.Info, root: string): Promise<LSPServer.Handle | undefined> => {
+          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) {
         async function schedule(server: LSPServer.Info, root: string, key: string) {
-          const handle = await server
-            .spawn(root)
+          const handle = await runSpawn(server, root)
             .then((value) => {
             .then((value) => {
               if (!value) s.broken.add(key)
               if (!value) s.broken.add(key)
               return value
               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 {
 export namespace Diagnostic {
   const MAX_PER_FILE = 20
   const MAX_PER_FILE = 20

+ 47 - 26
packages/opencode/src/lsp/server.ts

@@ -1,6 +1,7 @@
 import type { ChildProcessWithoutNullStreams } from "child_process"
 import type { ChildProcessWithoutNullStreams } from "child_process"
 import path from "path"
 import path from "path"
 import os from "os"
 import os from "os"
+import { Effect, Option } from "effect"
 import { Global } from "../global"
 import { Global } from "../global"
 import { Log } from "../util"
 import { Log } from "../util"
 import { text } from "node:stream/consumers"
 import { text } from "node:stream/consumers"
@@ -13,6 +14,7 @@ import { Process } from "../util"
 import { which } from "../util/which"
 import { which } from "../util/which"
 import { Module } from "@opencode-ai/shared/util/module"
 import { Module } from "@opencode-ai/shared/util/module"
 import { spawn } from "./launch"
 import { spawn } from "./launch"
+import { Npm as EffectNpm } from "@opencode-ai/shared/npm"
 import { Npm } from "../npm"
 import { Npm } from "../npm"
 
 
 const log = Log.create({ service: "lsp.server" })
 const log = Log.create({ service: "lsp.server" })
@@ -61,6 +63,14 @@ export interface Info {
   global?: boolean
   global?: boolean
   root: RootFunction
   root: RootFunction
   spawn(root: string): Promise<Handle | undefined>
   spawn(root: string): Promise<Handle | undefined>
+  /**
+   * 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<Handle | undefined, never, EffectNpm.Service>
 }
 }
 
 
 export const Deno: Info = {
 export const Deno: Info = {
@@ -486,6 +496,9 @@ export const Pyright: Info = {
   extensions: [".py", ".pyi"],
   extensions: [".py", ".pyi"],
   root: NearestRoot(["pyproject.toml", "setup.py", "setup.cfg", "requirements.txt", "Pipfile", "pyrightconfig.json"]),
   root: NearestRoot(["pyproject.toml", "setup.py", "setup.cfg", "requirements.txt", "Pipfile", "pyrightconfig.json"]),
   async spawn(root) {
   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")
     let binary = which("pyright-langserver")
     const args = []
     const args = []
     if (!binary) {
     if (!binary) {
@@ -495,34 +508,42 @@ export const Pyright: Info = {
       binary = resolved
       binary = resolved
     }
     }
     args.push("--stdio")
     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<string, string> = {}
-
-    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<Record<string, string>> {
+  const initialization: Record<string, string> = {}
+  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 = {
 export const ElixirLS: Info = {

+ 122 - 0
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<Option.Option<string>>,
+})
+
+// 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<typeof WriteTool>,
+  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,
+  )
+})