Merge pull request #1750 from AmirF194/fix/1490-kls-pdeathsig

fix(ls): don't orphan the language server process when Serena is killed

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.

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
 

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:

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()