diff --git a/CHANGELOG.md b/CHANGELOG.md index c39a82d7..0a42b5c7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,9 +9,6 @@ Status of the `main` branch. Changes prior to the next official version change w - The `languages` key in project configurations was changed to `language_servers` to better reflect the actual semantics (configurations are automatically migrated) - Fix: glob matching bare `*` and `?` in non-`**` patterns matched across `/`, contradicting documented behaviour #1732 - - Language servers and their dependency providers now go through the `subprocess_run` helper instead of - calling `subprocess.run` directly, so all such subprocesses get `stdin=DEVNULL` and can no longer - interfere with the stdio MCP connection (as happened for Julia, #1577) #1748 * Language Servers: - Allow language server priorities to be configured in `serena_config.yml` (for auto-detection during @@ -30,6 +27,11 @@ Status of the `main` branch. Changes prior to the next official version change w for projects where the initial project-graph resolution itself takes longer than that. The grace is now `indexing_start_grace` (default 5.0s), configurable the same way as `indexing_timeout` and `server_ready_timeout` #1586 + - Fix: On Linux, a language server process spawned in its own session (the default) is no longer + orphaned when Serena is killed without a chance to shut down cleanly (e.g. SIGKILL, OOM) #1490 + - Language servers and their dependency providers now go through the `subprocess_run` helper instead of + calling `subprocess.run` directly (e.g. for installation processes), so all such subprocesses get + `stdin=DEVNULL` and can no longer interfere with the stdio MCP connection #1748 * Hooks: - Add `serena-hooks --client=grok`, including Grok-native PreToolUse allow/deny output. diff --git a/src/solidlsp/ls_process.py b/src/solidlsp/ls_process.py index b369ff11..3acec5a5 100644 --- a/src/solidlsp/ls_process.py +++ b/src/solidlsp/ls_process.py @@ -1,7 +1,6 @@ import asyncio import json import logging -import os import socket import subprocess import threading @@ -10,7 +9,7 @@ from abc import ABC, abstractmethod from collections.abc import Callable, Iterable from dataclasses import dataclass from queue import Empty, Queue -from typing import IO, Any, AnyStr, cast +from typing import IO, Any, AnyStr from sensai.util.string import ToStringMixin @@ -512,27 +511,10 @@ class StdioLanguageServer(LanguageServerInterface): return self.process is not None and self.process.returncode is None def _start(self) -> None: - child_proc_env = os.environ.copy() - child_proc_env.update(self.process_launch_info.env) - - cmd = subprocess_util.convert_shell_cmd(self.process_launch_info.cmd) log.info("Starting language server process via command: %s", self.process_launch_info.cmd) - kwargs = subprocess_util.subprocess_kwargs() - kwargs["start_new_session"] = self.start_independent_lsp_process - # the language server is launched with binary (bytes) pipes; the cast is needed because the - # presence of platform-specific **kwargs prevents ty from selecting the bytes Popen overload - process = cast( - "subprocess.Popen[bytes]", - subprocess.Popen( - cmd, - stdout=subprocess.PIPE, - stdin=subprocess.PIPE, - stderr=subprocess.PIPE, - env=child_proc_env, - cwd=self.process_launch_info.cwd, - shell=True, - **kwargs, - ), + + process = subprocess_util.LanguageServerSubprocessLauncher.get_instance().launch( + self.process_launch_info, start_new_session=self.start_independent_lsp_process ) self.process = process diff --git a/src/solidlsp/util/subprocess_util.py b/src/solidlsp/util/subprocess_util.py index e88b9a4f..e1e48598 100644 --- a/src/solidlsp/util/subprocess_util.py +++ b/src/solidlsp/util/subprocess_util.py @@ -1,10 +1,21 @@ import logging +import os import platform +import queue +import signal import subprocess +import threading +from collections.abc import Callable +from typing import TYPE_CHECKING, Any, cast import oslex import psutil +if TYPE_CHECKING: + import ctypes + + from solidlsp.lsp_protocol_handler.server import ProcessLaunchInfo + log = logging.getLogger(__name__) @@ -56,10 +67,150 @@ def convert_shell_cmd(cmd: str | list[str]) -> str: :param cmd: the command to convert, specified as a list of arguments :return: a suitable representation of the command for subprocess calls on the current platform """ - if isinstance(cmd, list): - return oslex.join(cmd) - else: - return cmd + return oslex.join(cmd) if isinstance(cmd, list) else cmd + + +class LanguageServerSubprocessLauncher: + """ + Launcher for language server subprocesses, which are started for stdio-based communication. + It is home to the concern of launching a subprocess with well-defined lifecycle properties, + ensuring, in particular, that a launched subprocess cannot outlive this process (insofar as + the platform allows) -- even if the subprocess is started in its own session (see + :meth:`launch`) and this process is terminated forcefully without the opportunity to perform + cleanup (e.g. SIGKILL). + + The class is a singleton, as it is (potentially) home to a persistent worker thread. + """ + + _PR_SET_PDEATHSIG = 1 + """the PR_SET_PDEATHSIG option value for prctl(2)""" + + _instance: "LanguageServerSubprocessLauncher | None" = None + _instance_lock = threading.Lock() + + def __init__(self) -> None: + self._libc = self._load_libc() + self._spawner: "LanguageServerSubprocessLauncher._PDeathSigSpawner | None" = None + self._spawner_lock = threading.Lock() + + @classmethod + def get_instance(cls) -> "LanguageServerSubprocessLauncher": + if cls._instance is None: + with cls._instance_lock: + if cls._instance is None: + cls._instance = cls() + return cls._instance + + @staticmethod + def _load_libc() -> "ctypes.CDLL | None": + """ + Loads libc if on Linux, where it is needed for pdeathsig protection. + """ + if platform.system() != "Linux": + return None + import ctypes + + try: + # resolve libc, passing None to resolve symbols from the current process image (which is linked against libc on any Linux) + return ctypes.CDLL(None) + except OSError as e: + log.warning( + "Could not load libc (%s); language server processes will not be protected against " + "orphaning if this process is killed without a chance to shut down cleanly", + e, + ) + return None + + def _set_pdeathsig_on_parent_exit(self) -> None: + """ + preexec_fn for subprocess.Popen (no-op if libc is unavailable), which asks the kernel, via + prctl(PR_SET_PDEATHSIG), to send this process SIGTERM when its parent dies for any reason, + including SIGKILL. + """ + if self._libc is not None: + self._libc.prctl(self._PR_SET_PDEATHSIG, signal.SIGTERM) + + def launch(self, process_launch_info: "ProcessLaunchInfo", start_new_session: bool) -> subprocess.Popen[bytes]: + """ + Launches a language server process from ``process_launch_info``. + + :param process_launch_info: the command, environment and working directory to launch with + :param start_new_session: whether to start the process in its own session (own process + group, detached from ours) + """ + # build the child environment from ours, overridden by the launch info's entries + child_proc_env = os.environ.copy() + child_proc_env.update(process_launch_info.env) + + # convert the command for shell=True execution, prefixing `exec` when pdeathsig applies + use_pdeathsig = start_new_session and self._libc is not None + cmd = convert_shell_cmd(process_launch_info.cmd) + if use_pdeathsig: + # `exec` makes the shell replace its own process image with the program (execve) + # instead of forking it as a child, so the PID -- and therefore the PR_SET_PDEATHSIG + # registration below, which execve preserves -- carries through to the actual language + # server process rather than protecting only the intermediate shell + cmd = f"exec {cmd}" + + # assemble platform kwargs and lifecycle settings + kwargs: dict[str, Any] = subprocess_kwargs() + kwargs["start_new_session"] = start_new_session + if use_pdeathsig: + kwargs["preexec_fn"] = self._set_pdeathsig_on_parent_exit + + def do_popen() -> subprocess.Popen[bytes]: + return cast( + "subprocess.Popen[bytes]", + subprocess.Popen( + cmd, + stdout=subprocess.PIPE, + stdin=subprocess.PIPE, + stderr=subprocess.PIPE, + env=child_proc_env, + cwd=process_launch_info.cwd, + shell=True, + **kwargs, + ), + ) + + # perform the actual Popen call, funneling the fork() through the dedicated spawner thread + # when pdeathsig applies + if not use_pdeathsig: + return do_popen() + else: + with self._spawner_lock: + if self._spawner is None: + self._spawner = self._PDeathSigSpawner() + spawner = self._spawner + return spawner.spawn(do_popen) + + class _PDeathSigSpawner: + """ + Runs subprocess.Popen() calls that register PR_SET_PDEATHSIG on one dedicated, permanently + running daemon thread, so the "parent thread" the kernel ties the registration to is one + that has the same lifetime as the process + """ + + def __init__(self) -> None: + self._queue: queue.Queue[tuple[Callable[[], subprocess.Popen], "queue.Queue"]] = queue.Queue() + self._thread = threading.Thread(target=self._run, name="solidlsp-pdeathsig-spawner", daemon=True) + self._thread.start() + + def _run(self) -> None: + while True: + func, result_queue = self._queue.get() + try: + result_queue.put((None, func())) + except BaseException as e: + result_queue.put((e, None)) + + def spawn(self, func: Callable[[], subprocess.Popen]) -> subprocess.Popen: + result_queue: queue.Queue = queue.Queue(maxsize=1) + self._queue.put((func, result_queue)) + error, process = result_queue.get() + if error is not None: + raise error + return process def _signal_process_tree(process: subprocess.Popen[bytes], terminate: bool = True) -> None: diff --git a/test/solidlsp/test_pdeathsig.py b/test/solidlsp/test_pdeathsig.py new file mode 100644 index 00000000..662143c9 --- /dev/null +++ b/test/solidlsp/test_pdeathsig.py @@ -0,0 +1,167 @@ +"""Regression test for issue #1490: a SIGKILLed Serena leaves its language server process running. + +``StdioLanguageServer`` starts every LSP process in its own session +(``start_independent_lsp_process`` defaults to True, see ``ls_config.py``) so that our own +graceful-shutdown code can walk and terminate the whole process tree on a clean exit +(``subprocess_util.terminate_process_tree_with_kill_fallback``). That code never runs if Serena +itself is killed (SIGKILL, OOM, crash), and being in its own session means no signal to our +process group reaches the child either, so it survives as a PPID=1 orphan. + +This drives a real ``StdioLanguageServer`` in a child ("fake Serena") process, SIGKILLs that +child, and checks whether the language server process it started is still alive afterwards. +Linux only: the fix is ``prctl(PR_SET_PDEATHSIG)``, which has no equivalent exercised here on +other platforms. +""" + +from __future__ import annotations + +import platform +import subprocess +import sys +import textwrap +import time +import uuid + +import psutil +import pytest + +pytestmark = pytest.mark.skipif(platform.system() != "Linux", reason="PR_SET_PDEATHSIG is Linux-specific") + +_DRIVER_SRC = textwrap.dedent( + """ + import sys + from solidlsp.ls_config import LanguageServerId + from solidlsp.ls_process import StdioLanguageServer + from solidlsp.lsp_protocol_handler.server import ProcessLaunchInfo + + marker = sys.argv[1] + ls = StdioLanguageServer( + process_launch_info=ProcessLaunchInfo( + cmd=[sys.executable, "-c", f"import time; time.sleep(300) # {marker}"], + cwd="/tmp", + ), + ls_id=LanguageServerId.PYTHON, + determine_log_level=lambda _line: 0, + ) + ls._start() + print("READY", flush=True) + time.sleep(600) + """ +) + + +def _find_marked_processes(marker: str) -> list[psutil.Process]: + found = [] + for proc in psutil.process_iter(["cmdline"]): + try: + cmdline = proc.info["cmdline"] or [] + except (psutil.NoSuchProcess, psutil.AccessDenied): + continue + if any(marker in arg for arg in cmdline): + found.append(proc) + return found + + +_THREAD_DRIVER_SRC = textwrap.dedent( + """ + import sys + import threading + import time + from solidlsp.ls_config import LanguageServerId + from solidlsp.ls_process import StdioLanguageServer + from solidlsp.lsp_protocol_handler.server import ProcessLaunchInfo + + marker = sys.argv[1] + + # Mirrors LanguageServerManager.from_languages's StartLSThread: the real code path spawns one + # Thread per language server, that thread calls language_server.start() (-> ._start(), where + # the process is actually launched), and the thread then simply returns -- terminating almost + # immediately after Popen() returns, not on the process's main thread. + def start_language_server(): + ls = StdioLanguageServer( + process_launch_info=ProcessLaunchInfo( + cmd=[sys.executable, "-c", f"import time; time.sleep(300) # {marker}"], + cwd="/tmp", + ), + ls_id=LanguageServerId.PYTHON, + determine_log_level=lambda _line: 0, + ) + ls.start() + + t = threading.Thread(target=start_language_server) + t.start() + t.join() # the calling thread is gone from here on; the driver process itself lives on + print("READY", flush=True) + time.sleep(600) + """ +) + + +def test_language_server_process_survives_a_short_lived_calling_thread() -> None: + """ + PR_SET_PDEATHSIG (per prctl(2)) ties the registration to the specific calling *thread*, not + the process: if that thread terminates while the rest of the process lives on, the kernel + delivers the death signal right then, even though "Serena" (the process) never died. Starting + a language server is dispatched through TaskExecutor onto exactly such a short-lived thread, so + a naive preexec_fn registration kills the language server itself within milliseconds of a normal + startup, independent of any real SIGKILL scenario -- this is the CI-only regression this test + guards against. + """ + marker = f"pdeathsig-thread-test-{uuid.uuid4().hex}" + driver = subprocess.Popen( + [sys.executable, "-c", _THREAD_DRIVER_SRC, marker], + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True, + ) + try: + ready_line = driver.stdout.readline() + assert ready_line.strip() == "READY", f"driver failed to start the language server: {driver.stderr.read()}" + + time.sleep(2) + # The driver's own argv also contains `marker` (it's passed as sys.argv[1]), so exclude + # the driver's own pid or it would satisfy the assertion below on its own, regardless of + # whether the actual language server process (its child) is still alive. + survivors = [p for p in _find_marked_processes(marker) if p.pid != driver.pid] + assert survivors, ( + "language server process died on its own even though the driver ('Serena') process " + "is still alive -- the calling thread's exit, not a real parent death, killed it" + ) + finally: + for proc in _find_marked_processes(marker): + proc.kill() + if driver.poll() is None: + driver.kill() + + +def test_language_server_process_dies_with_a_sigkilled_serena() -> None: + marker = f"pdeathsig-test-{uuid.uuid4().hex}" + driver = subprocess.Popen( + [sys.executable, "-c", _DRIVER_SRC, marker], + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True, + ) + try: + ready_line = driver.stdout.readline() + assert ready_line.strip() == "READY", f"driver failed to start the language server: {driver.stderr.read()}" + assert _find_marked_processes(marker), "language server process never started" + + driver.kill() # SIGKILL: simulates Serena being killed without a chance to clean up + driver.wait(timeout=5) + + deadline = time.monotonic() + 5 + survivors = _find_marked_processes(marker) + while survivors and time.monotonic() < deadline: + time.sleep(0.2) + survivors = _find_marked_processes(marker) + + assert not survivors, ( + f"language server process(es) {[p.pid for p in survivors]} survived a SIGKILLed Serena " + "(orphaned, will run until manually killed -- issue #1490)" + ) + finally: + for proc in _find_marked_processes(marker): + proc.kill() + if driver.poll() is None: + driver.kill()