Skip to content

fix(lsp): terminate LSP child process trees on failure #607

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Olusammytee wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

fix(lsp): terminate LSP child process trees on failure #607

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions packages/opencode/src/lsp/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { Config } from "../config/config"
import { spawn } from "child_process"
import { Instance } from "../project/instance"
import { Flag } from "@/flag/flag"
import { Shell } from "@/shell/shell"

export namespace LSP {
const log = Log.create({ service: "lsp" })
Expand Down Expand Up @@ -201,19 +202,19 @@ export namespace LSP {
root,
}).catch((err) => {
s.broken.add(key)
handle.process.kill()
void Shell.killTree(handle.process, { exited: () => handle.process.exitCode !== null })
Copy link

Copilot AI Feb 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The exited predicate only checks exitCode !== null, but child processes can also exit via signal (where signalCode is set and exitCode may remain null). That can cause Shell.killTree to unnecessarily attempt SIGKILL/taskkill after the process already exited. Consider treating the process as exited when either exitCode or signalCode is non-null (or attach an exit listener and use a boolean, like other killTree call sites).

Copilot uses AI. Check for mistakes.
log.error(`Failed to initialize LSP client ${server.id}`, { error: err })
return undefined
})

if (!client) {
handle.process.kill()
void Shell.killTree(handle.process, { exited: () => handle.process.exitCode !== null })
Comment on lines 203 to +211
Copy link

Copilot AI Feb 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change introduces new failure-path behavior (killing the spawned LSP process tree) but there isn’t a unit test verifying that Shell.killTree is invoked when LSPClient.create fails / when client is undefined. Since the repo already has LSP-related tests (e.g., test/lsp/client.test.ts), it would be good to add a focused test that stubs Shell.killTree and asserts it’s called in these startup failure paths.

Copilot uses AI. Check for mistakes.
return undefined
Comment on lines 210 to 212
Copy link

Copilot AI Feb 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shell.killTree tries process.kill(-pid, ...) on non-Windows to signal the process group, but LSP servers in this file are spawned without detached: true (so pid typically isn't a process group id). In practice this means the group kill will likely throw and killTree will fall back to proc.kill(), which won’t reliably terminate child processes on POSIX. If the goal is to kill process trees cross-platform, spawn LSP servers in their own process group (e.g., detached: process.platform !== "win32") so the negative-PID kill path actually works.

Copilot uses AI. Check for mistakes.
}

const existing = s.clients.find((x) => x.root === root && x.serverID === server.id)
if (existing) {
handle.process.kill()
void Shell.killTree(handle.process, { exited: () => handle.process.exitCode !== null })
return existing
}

Expand Down
Loading