From cbc7f286b305cb59a762a0649e750d3db1d24487 Mon Sep 17 00:00:00 2001 From: Dominik Jain Date: Fri, 24 Jul 2026 18:17:49 +0200 Subject: [PATCH] Refactoring: Fully internalise the pdeathsig machinery into LanguageServerSubprocessLauncher --- src/solidlsp/ls_process.py | 8 +- src/solidlsp/util/subprocess_util.py | 212 ++++++++++++--------------- test/solidlsp/test_pdeathsig.py | 42 ------ 3 files changed, 91 insertions(+), 171 deletions(-) diff --git a/src/solidlsp/ls_process.py b/src/solidlsp/ls_process.py index a5df5d38..3acec5a5 100644 --- a/src/solidlsp/ls_process.py +++ b/src/solidlsp/ls_process.py @@ -513,13 +513,7 @@ class StdioLanguageServer(LanguageServerInterface): def _start(self) -> None: log.info("Starting language server process via command: %s", self.process_launch_info.cmd) - # An independent session (its own process group, detached from ours) is what lets a - # SIGKILLed Serena leave the language server behind as an orphan: our own graceful - # shutdown code (subprocess_util.terminate_process_tree_with_kill_fallback) never runs in - # that case, and being in its own session means no signal to our process group reaches it - # either. LanguageServerSubprocessLauncher closes that gap on Linux at spawn time via - # PR_SET_PDEATHSIG when start_new_session is set. - process = subprocess_util.LanguageServerSubprocessLauncher.instance().launch( + 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 86629fac..e1e48598 100644 --- a/src/solidlsp/util/subprocess_util.py +++ b/src/solidlsp/util/subprocess_util.py @@ -12,86 +12,12 @@ import oslex import psutil if TYPE_CHECKING: + import ctypes + from solidlsp.lsp_protocol_handler.server import ProcessLaunchInfo log = logging.getLogger(__name__) -# Resolved once at import time rather than inside the preexec_fn below: preexec_fn runs in the -# forked child before exec(), where only async-signal-safe operations are safe to perform, and an -# import/ctypes.CDLL lookup at that point is not. -_libc = None -if platform.system() == "Linux": - import ctypes - - _libc = ctypes.CDLL("libc.so.6", use_errno=True) -_PR_SET_PDEATHSIG = 1 - - -def set_pdeathsig_on_parent_exit() -> None: - """ - preexec_fn for subprocess.Popen (Linux only, no-op elsewhere): asks the kernel to send this - process SIGTERM if its parent dies for any reason, including SIGKILL, via - prctl(PR_SET_PDEATHSIG). This is the only mechanism that can free the child when the parent - is killed rather than exiting gracefully, since a graceful exit is required to run any - signal-the-tree cleanup code ourselves. - - Only protects the immediate process this is passed to. When launching with shell=True, that - process is the shell, not the program it runs, so the registration alone will not protect a - program the shell forks as a child; see LanguageServerSubprocessLauncher's `exec` prefix, which - makes the shell replace itself with the program instead of forking it, so the same PID (and - therefore the same PDEATHSIG registration, which execve preserves) carries through. - - Warning this function's caller (LanguageServerSubprocessLauncher below) exists to guard - against: per `man 2 prctl`, PR_SET_PDEATHSIG's "parent" is specifically the OS THREAD that - called fork() to create this process, not the parent process as a whole -- if that thread - terminates while the rest of the parent process (e.g. its main thread) keeps running, the death - signal fires anyway. - """ - if _libc is not None: - _libc.prctl(_PR_SET_PDEATHSIG, signal.SIGTERM) - - -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 never a - short-lived one. - - Why this exists: LanguageServerSubprocessLauncher.launch() (the sole caller of this class) is - very often invoked from a thread that is *intentionally* short-lived. For example, - LanguageServerManager.from_languages spawns one StartLSThread per language server; each one - calls language_server.start() (-> ._start(), where Popen() happens) and then simply returns, - terminating moments after Popen() itself returns. Because PR_SET_PDEATHSIG's delivery is tied - to that specific calling thread (see the warning on set_pdeathsig_on_parent_exit above), the - language server would then receive the parent-death signal and exit almost immediately, even - though Serena's process is still fully alive -- the exact CI-only regression this class fixes. - Funneling the fork()/exec() itself through one thread that never voluntarily exits (it only - ever terminates alongside the whole process, which is exactly the case PR_SET_PDEATHSIG is - meant to detect) decouples "which caller thread happened to invoke _start()" from "does the - language server survive." - """ - - 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 subprocess_kwargs() -> dict: """ @@ -146,75 +72,93 @@ def convert_shell_cmd(cmd: str | list[str]) -> str: class LanguageServerSubprocessLauncher: """ - Singleton that launches a language server subprocess whose lifecycle is tied to this process - (on Linux, when requested via ``start_new_session``). This reifies, as one unit, several - pieces that only work correctly when combined and would otherwise have to be assembled by hand - at every call site: the `exec` prefixing of the shell command, the libc/prctl preexec_fn that - registers PR_SET_PDEATHSIG, the platform/own-session gate that decides whether any of this - applies, and the dedicated long-lived spawner thread that the PR_SET_PDEATHSIG registration - requires (see _PDeathSigSpawner: its "parent" is the calling OS *thread*, not the process, so a - short-lived calling thread would defeat it). + 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 because the spawner thread it owns must be a single, permanently - running thread for the entire process: PR_SET_PDEATHSIG's registration is tied to whichever OS - thread performs the fork(), so there can only be one such thread doing pdeathsig-registering - spawns if the registration is to reliably outlive the caller. ``launch()`` takes - ``start_new_session`` per call and the instance otherwise carries no per-call configuration, so - sharing one instance across all callers is safe. + 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: - # Created lazily (see _spawn_with_pdeathsig): the dedicated spawner thread is only needed - # for own-session launches on Linux, which not every caller requests. - self._spawner: _PDeathSigSpawner | None = None + self._libc = self._load_libc() + self._spawner: "LanguageServerSubprocessLauncher._PDeathSigSpawner | None" = None self._spawner_lock = threading.Lock() @classmethod - def instance(cls) -> "LanguageServerSubprocessLauncher": + 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). This is what leaves an orphan behind if we are SIGKILLed - without a chance to clean up, so on Linux, requesting it also arms - prctl(PR_SET_PDEATHSIG) on the actual language server process (not the intermediate - shell -- see the `exec` prefix below), ensuring the kernel sends it SIGTERM if this - process dies for any reason. On other platforms, this parameter only controls the - process-group behavior; there is no equivalent kernel-level protection available. - :return: the started process, with binary (bytes) stdio pipes + 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) - use_pdeathsig = start_new_session and platform.system() == "Linux" + # 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: - # 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. + # `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"] = set_pdeathsig_on_parent_exit + kwargs["preexec_fn"] = self._set_pdeathsig_on_parent_exit def do_popen() -> subprocess.Popen[bytes]: - # 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 return cast( "subprocess.Popen[bytes]", subprocess.Popen( @@ -229,20 +173,44 @@ class LanguageServerSubprocessLauncher: ), ) - # Funneling the fork() itself through the dedicated spawner thread (rather than calling - # Popen() on the calling thread directly) matters specifically when use_pdeathsig is set: - # the actual fork() has to happen on a thread that outlives the calling thread, which may - # itself be short-lived (e.g. LanguageServerManager's per-server StartLSThread) -- see - # _PDeathSigSpawner's docstring for why a plain Popen() call here would defeat the fix for - # #1490 in exactly that case. When pdeathsig isn't needed, calling Popen() directly is - # exactly equivalent and avoids spinning up the spawner thread for no reason. + # perform the actual Popen call, funneling the fork() through the dedicated spawner thread + # when pdeathsig applies if not use_pdeathsig: return do_popen() - with self._spawner_lock: - if self._spawner is None: - self._spawner = _PDeathSigSpawner() - spawner = self._spawner - return spawner.spawn(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 index d5e18240..662143c9 100644 --- a/test/solidlsp/test_pdeathsig.py +++ b/test/solidlsp/test_pdeathsig.py @@ -15,7 +15,6 @@ other platforms. from __future__ import annotations -import inspect import platform import subprocess import sys @@ -26,9 +25,6 @@ import uuid import psutil import pytest -from solidlsp.lsp_protocol_handler.server import ProcessLaunchInfo -from solidlsp.util.subprocess_util import LanguageServerSubprocessLauncher, convert_shell_cmd - pytestmark = pytest.mark.skipif(platform.system() != "Linux", reason="PR_SET_PDEATHSIG is Linux-specific") _DRIVER_SRC = textwrap.dedent( @@ -138,44 +134,6 @@ def test_language_server_process_survives_a_short_lived_calling_thread() -> None driver.kill() -def test_convert_shell_cmd_has_no_lifecycle_concern() -> None: - """ - convert_shell_cmd is a pure command-formatting utility; its other callers (e.g. - solidlsp.language_servers.common, serena.agent) have no use for the `exec` prefixing that - PR_SET_PDEATHSIG needs, and prefixing with `exec` is invalid on Windows. That concern now - lives entirely inside LanguageServerSubprocessLauncher, applied to this function's output, so - it can never be requested without the preexec_fn/dedicated-thread machinery it depends on. - """ - assert "use_exec" not in inspect.signature(convert_shell_cmd).parameters - assert convert_shell_cmd(["python3", "-c", "pass"]) == "python3 -c pass" - - -def test_language_server_subprocess_launcher_is_a_singleton() -> None: - assert LanguageServerSubprocessLauncher.instance() is LanguageServerSubprocessLauncher.instance() - - -def test_language_server_subprocess_launcher_spawner_thread_is_lazy() -> None: - """ - The dedicated spawner thread is only needed for own-session launches on Linux; a fresh - launcher instance that has only ever performed a non-own-session launch should not have - created it. - """ - launcher = LanguageServerSubprocessLauncher() - assert launcher._spawner is None - - process = launcher.launch( - ProcessLaunchInfo(cmd=[sys.executable, "-c", "print('hi')"], cwd="/tmp"), - start_new_session=False, - ) - try: - process.wait(timeout=5) - finally: - if process.poll() is None: - process.kill() - - assert launcher._spawner is None - - def test_language_server_process_dies_with_a_sigkilled_serena() -> None: marker = f"pdeathsig-test-{uuid.uuid4().hex}" driver = subprocess.Popen(