From 3ac66bb8c13bc613f0ce31805b2fbb80f3370a54 Mon Sep 17 00:00:00 2001 From: Amir Fathi Date: Thu, 23 Jul 2026 22:12:44 +0000 Subject: [PATCH 1/5] fix(ls): don't orphan the language server process when Serena is killed On Linux, a SIGKILLed Serena (or an OOM kill, or a crash) leaves its language server process running: it is launched in its own session so our graceful-shutdown code can clean up the whole tree, but that code never runs if Serena itself dies uncleanly, and no signal from our process group reaches a detached session either. Fix via prctl(PR_SET_PDEATHSIG) at spawn time. Since the process runs under shell=True, the registration alone only protects the intermediate shell; prefixing the command with `exec` makes the shell replace itself with the language server so the same PID (and PDEATHSIG registration) carries through. Fixes #1490 --- CHANGELOG.md | 2 + src/solidlsp/ls_process.py | 13 +++- src/solidlsp/util/subprocess_util.py | 43 +++++++++++-- test/solidlsp/test_pdeathsig.py | 95 ++++++++++++++++++++++++++++ 4 files changed, 147 insertions(+), 6 deletions(-) create mode 100644 test/solidlsp/test_pdeathsig.py diff --git a/CHANGELOG.md b/CHANGELOG.md index e214da84..3d96597c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,8 @@ 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 + - 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: - Allow language server priorities to be configured in `serena_config.yml` (for auto-detection during diff --git a/src/solidlsp/ls_process.py b/src/solidlsp/ls_process.py index b369ff11..4f9b046c 100644 --- a/src/solidlsp/ls_process.py +++ b/src/solidlsp/ls_process.py @@ -2,6 +2,7 @@ import asyncio import json import logging import os +import platform import socket import subprocess import threading @@ -515,10 +516,20 @@ class StdioLanguageServer(LanguageServerInterface): 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) + # 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. On Linux, close that gap at spawn time via PR_SET_PDEATHSIG, which needs the + # actual language server process (not the intermediate shell that runs it) to be the one + # holding the registration; see convert_shell_cmd's `exec` prefix. + use_pdeathsig = self.start_independent_lsp_process and platform.system() == "Linux" + cmd = subprocess_util.convert_shell_cmd(self.process_launch_info.cmd, use_exec=use_pdeathsig) 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 + if use_pdeathsig: + kwargs["preexec_fn"] = subprocess_util.set_pdeathsig_on_parent_exit # 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( diff --git a/src/solidlsp/util/subprocess_util.py b/src/solidlsp/util/subprocess_util.py index e88b9a4f..92a0ce53 100644 --- a/src/solidlsp/util/subprocess_util.py +++ b/src/solidlsp/util/subprocess_util.py @@ -1,5 +1,6 @@ import logging import platform +import signal import subprocess import oslex @@ -7,6 +8,34 @@ import psutil 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 convert_shell_cmd'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. + """ + if _libc is not None: + _libc.prctl(_PR_SET_PDEATHSIG, signal.SIGTERM) + def subprocess_kwargs() -> dict: """ @@ -48,18 +77,22 @@ def subprocess_run( return subprocess.run(cmd, check=check, **kwargs) -def convert_shell_cmd(cmd: str | list[str]) -> str: +def convert_shell_cmd(cmd: str | list[str], use_exec: bool = False) -> str: """ Converts a command (specified as a list or string) to a format supported by subprocess calls with shell=True on the current platform, applying necessary escaping and quoting if the command is specified as a list of arguments. :param cmd: the command to convert, specified as a list of arguments + :param use_exec: if True (POSIX only; must not be set on Windows), prefix the command with the shell's + `exec` builtin, so the shell replaces its own process image with the command instead of forking it + as a child. Combined with set_pdeathsig_on_parent_exit, this is what makes the parent-death signal + reach the actual program rather than only the intermediate shell. :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 + cmd_str = oslex.join(cmd) if isinstance(cmd, list) else cmd + if use_exec: + cmd_str = f"exec {cmd_str}" + return cmd_str 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..6b5a3090 --- /dev/null +++ b/test/solidlsp/test_pdeathsig.py @@ -0,0 +1,95 @@ +"""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 + + +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() From 81d19bdd27e58c273165d94eb694b7bcba4b55e4 Mon Sep 17 00:00:00 2001 From: Amir Fathi Date: Fri, 24 Jul 2026 01:00:41 +0000 Subject: [PATCH 2/5] fix(ls): keep the PDEATHSIG registration alive across the calling thread's own exit Tool calls and language server startup dispatch Popen() onto a short-lived thread (e.g. LanguageServerManager's per-server StartLSThread), which terminates moments after Popen() returns. PR_SET_PDEATHSIG ties its registration to that specific calling thread, not the process, so the language server received SIGTERM and died within milliseconds of a normal startup on Linux CI, independent of any real SIGKILL scenario. Route Popen() calls that set preexec_fn=set_pdeathsig_on_parent_exit through one dedicated, permanently-running daemon thread instead, so the registration stays tied to a thread that only ever dies alongside the whole process. Added a regression test that starts a language server on a short-lived thread and checks it survives past that thread's exit. --- src/solidlsp/ls_process.py | 7 ++- src/solidlsp/util/subprocess_util.py | 77 ++++++++++++++++++++++++++++ test/solidlsp/test_pdeathsig.py | 72 ++++++++++++++++++++++++++ 3 files changed, 155 insertions(+), 1 deletion(-) diff --git a/src/solidlsp/ls_process.py b/src/solidlsp/ls_process.py index 4f9b046c..c45279a8 100644 --- a/src/solidlsp/ls_process.py +++ b/src/solidlsp/ls_process.py @@ -532,9 +532,14 @@ class StdioLanguageServer(LanguageServerInterface): kwargs["preexec_fn"] = subprocess_util.set_pdeathsig_on_parent_exit # 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 + # popen_preserving_pdeathsig (rather than a plain subprocess.Popen call) 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 its docstring for why a plain Popen call here would defeat the fix + # for #1490 in exactly that case. process = cast( "subprocess.Popen[bytes]", - subprocess.Popen( + subprocess_util.popen_preserving_pdeathsig( cmd, stdout=subprocess.PIPE, stdin=subprocess.PIPE, diff --git a/src/solidlsp/util/subprocess_util.py b/src/solidlsp/util/subprocess_util.py index 92a0ce53..18eb9237 100644 --- a/src/solidlsp/util/subprocess_util.py +++ b/src/solidlsp/util/subprocess_util.py @@ -1,7 +1,10 @@ import logging import platform +import queue import signal import subprocess +import threading +from collections.abc import Callable import oslex import psutil @@ -32,11 +35,85 @@ def set_pdeathsig_on_parent_exit() -> None: program the shell forks as a child; see convert_shell_cmd'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 (popen_preserving_pdeathsig 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: StdioLanguageServer._start() (the sole caller of popen_preserving_pdeathsig) + 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 + + +class _SpawnerHolder: + instance: "_PDeathSigSpawner | None" = None + + +_pdeathsig_spawner_holder = _SpawnerHolder() +_pdeathsig_spawner_lock = threading.Lock() + + +def popen_preserving_pdeathsig(cmd: str | list[str], **kwargs) -> subprocess.Popen: + """ + subprocess.Popen wrapper for spawns that pass preexec_fn=set_pdeathsig_on_parent_exit. + + On Linux, delegates the actual Popen() call to a single dedicated daemon thread (see + _PDeathSigSpawner) instead of performing it on the calling thread directly, so that the + PR_SET_PDEATHSIG registration set up by the preexec_fn remains tied to a thread that lives as + long as the process does. On any other platform (or if no preexec_fn is passed), this is + exactly equivalent to calling subprocess.Popen(cmd, **kwargs) directly. + """ + if platform.system() != "Linux" or kwargs.get("preexec_fn") is None: + return subprocess.Popen(cmd, **kwargs) + with _pdeathsig_spawner_lock: + if _pdeathsig_spawner_holder.instance is None: + _pdeathsig_spawner_holder.instance = _PDeathSigSpawner() + spawner = _pdeathsig_spawner_holder.instance + return spawner.spawn(lambda: subprocess.Popen(cmd, **kwargs)) + + def subprocess_kwargs() -> dict: """ Returns a dictionary of keyword arguments for subprocess calls, adding platform-specific diff --git a/test/solidlsp/test_pdeathsig.py b/test/solidlsp/test_pdeathsig.py index 6b5a3090..662143c9 100644 --- a/test/solidlsp/test_pdeathsig.py +++ b/test/solidlsp/test_pdeathsig.py @@ -62,6 +62,78 @@ def _find_marked_processes(marker: str) -> list[psutil.Process]: 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( From ce685c8979f7d34388d2b0292934ec860e0319fc Mon Sep 17 00:00:00 2001 From: Amir Fathi Date: Fri, 24 Jul 2026 14:05:26 +0000 Subject: [PATCH 3/5] fix(ls): reify the pdeathsig launch ritual into LanguageServerSubprocessLauncher The previous fix for #1490 depended on four fragments being combined correctly by hand at the _start() call site: convert_shell_cmd's exec prefix, the preexec_fn registration, routing through popen_preserving_pdeathsig, and the Linux/own-session condition. Nothing enforced their co-occurrence. Reify "launch a subprocess whose lifecycle is tied to the parent process (on Linux)" as LanguageServerSubprocessLauncher, a lazily constructed singleton with a single public method: launch(process_launch_info: ProcessLaunchInfo, start_new_session: bool) -> Popen[bytes] Internalizes the libc/prctl preexec_fn, the exec prefixing, the platform/own-session gate, and the dedicated long-lived spawner thread (created lazily, only for own-session launches on Linux). The singleton is required because the spawner thread it owns must be the one thread in the process performing pdeathsig-registering fork() calls; launch() takes start_new_session per call so the instance itself carries no configuration state. convert_shell_cmd loses the use_exec flag it gained for this fix: its other callers (solidlsp.language_servers.common, serena.agent) never used it, and it is invalid on Windows. The exec prefix now lives entirely inside the launcher, applied to convert_shell_cmd's plain output, so it can never be requested without the machinery it depends on. StdioLanguageServer._start() now calls LanguageServerSubprocessLauncher.instance().launch(...) instead of assembling the four fragments itself. Adapted test/solidlsp/test_pdeathsig.py: the two existing end-to-end regression tests (short-lived calling thread, SIGKILLed driver) are unchanged in behavior since they exercise _start()/start() end-to-end and now transparently go through the new launcher. Added three targeted tests: convert_shell_cmd's reverted signature, the launcher's singleton identity, and that its spawner thread is not created for a non-own-session launch. --- src/solidlsp/ls_process.py | 40 +------ src/solidlsp/util/subprocess_util.py | 163 +++++++++++++++++++-------- test/solidlsp/test_pdeathsig.py | 42 +++++++ 3 files changed, 166 insertions(+), 79 deletions(-) diff --git a/src/solidlsp/ls_process.py b/src/solidlsp/ls_process.py index c45279a8..a5df5d38 100644 --- a/src/solidlsp/ls_process.py +++ b/src/solidlsp/ls_process.py @@ -1,8 +1,6 @@ import asyncio import json import logging -import os -import platform import socket import subprocess import threading @@ -11,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 @@ -513,42 +511,16 @@ 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) + 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. On Linux, close that gap at spawn time via PR_SET_PDEATHSIG, which needs the - # actual language server process (not the intermediate shell that runs it) to be the one - # holding the registration; see convert_shell_cmd's `exec` prefix. - use_pdeathsig = self.start_independent_lsp_process and platform.system() == "Linux" - cmd = subprocess_util.convert_shell_cmd(self.process_launch_info.cmd, use_exec=use_pdeathsig) - 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 - if use_pdeathsig: - kwargs["preexec_fn"] = subprocess_util.set_pdeathsig_on_parent_exit - # 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 - # popen_preserving_pdeathsig (rather than a plain subprocess.Popen call) 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 its docstring for why a plain Popen call here would defeat the fix - # for #1490 in exactly that case. - process = cast( - "subprocess.Popen[bytes]", - subprocess_util.popen_preserving_pdeathsig( - cmd, - stdout=subprocess.PIPE, - stdin=subprocess.PIPE, - stderr=subprocess.PIPE, - env=child_proc_env, - cwd=self.process_launch_info.cwd, - shell=True, - **kwargs, - ), + # 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( + 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 18eb9237..86629fac 100644 --- a/src/solidlsp/util/subprocess_util.py +++ b/src/solidlsp/util/subprocess_util.py @@ -1,14 +1,19 @@ 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: + 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 @@ -32,14 +37,15 @@ def set_pdeathsig_on_parent_exit() -> None: 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 convert_shell_cmd'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. + 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 (popen_preserving_pdeathsig 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. + 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) @@ -51,8 +57,8 @@ class _PDeathSigSpawner: running daemon thread, so the "parent thread" the kernel ties the registration to is never a short-lived one. - Why this exists: StdioLanguageServer._start() (the sole caller of popen_preserving_pdeathsig) - is very often invoked from a thread that is *intentionally* short-lived. For example, + 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 @@ -87,33 +93,6 @@ class _PDeathSigSpawner: return process -class _SpawnerHolder: - instance: "_PDeathSigSpawner | None" = None - - -_pdeathsig_spawner_holder = _SpawnerHolder() -_pdeathsig_spawner_lock = threading.Lock() - - -def popen_preserving_pdeathsig(cmd: str | list[str], **kwargs) -> subprocess.Popen: - """ - subprocess.Popen wrapper for spawns that pass preexec_fn=set_pdeathsig_on_parent_exit. - - On Linux, delegates the actual Popen() call to a single dedicated daemon thread (see - _PDeathSigSpawner) instead of performing it on the calling thread directly, so that the - PR_SET_PDEATHSIG registration set up by the preexec_fn remains tied to a thread that lives as - long as the process does. On any other platform (or if no preexec_fn is passed), this is - exactly equivalent to calling subprocess.Popen(cmd, **kwargs) directly. - """ - if platform.system() != "Linux" or kwargs.get("preexec_fn") is None: - return subprocess.Popen(cmd, **kwargs) - with _pdeathsig_spawner_lock: - if _pdeathsig_spawner_holder.instance is None: - _pdeathsig_spawner_holder.instance = _PDeathSigSpawner() - spawner = _pdeathsig_spawner_holder.instance - return spawner.spawn(lambda: subprocess.Popen(cmd, **kwargs)) - - def subprocess_kwargs() -> dict: """ Returns a dictionary of keyword arguments for subprocess calls, adding platform-specific @@ -154,22 +133,116 @@ def subprocess_run( return subprocess.run(cmd, check=check, **kwargs) -def convert_shell_cmd(cmd: str | list[str], use_exec: bool = False) -> str: +def convert_shell_cmd(cmd: str | list[str]) -> str: """ Converts a command (specified as a list or string) to a format supported by subprocess calls with shell=True on the current platform, applying necessary escaping and quoting if the command is specified as a list of arguments. :param cmd: the command to convert, specified as a list of arguments - :param use_exec: if True (POSIX only; must not be set on Windows), prefix the command with the shell's - `exec` builtin, so the shell replaces its own process image with the command instead of forking it - as a child. Combined with set_pdeathsig_on_parent_exit, this is what makes the parent-death signal - reach the actual program rather than only the intermediate shell. :return: a suitable representation of the command for subprocess calls on the current platform """ - cmd_str = oslex.join(cmd) if isinstance(cmd, list) else cmd - if use_exec: - cmd_str = f"exec {cmd_str}" - return cmd_str + return oslex.join(cmd) if isinstance(cmd, list) else cmd + + +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). + + 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. + """ + + _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._spawner_lock = threading.Lock() + + @classmethod + def instance(cls) -> "LanguageServerSubprocessLauncher": + if cls._instance is None: + with cls._instance_lock: + if cls._instance is None: + cls._instance = cls() + return cls._instance + + 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 + """ + child_proc_env = os.environ.copy() + child_proc_env.update(process_launch_info.env) + + use_pdeathsig = start_new_session and platform.system() == "Linux" + 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. + cmd = f"exec {cmd}" + + kwargs: dict[str, Any] = subprocess_kwargs() + kwargs["start_new_session"] = start_new_session + if use_pdeathsig: + kwargs["preexec_fn"] = 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( + cmd, + stdout=subprocess.PIPE, + stdin=subprocess.PIPE, + stderr=subprocess.PIPE, + env=child_proc_env, + cwd=process_launch_info.cwd, + shell=True, + **kwargs, + ), + ) + + # 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. + 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) 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 662143c9..d5e18240 100644 --- a/test/solidlsp/test_pdeathsig.py +++ b/test/solidlsp/test_pdeathsig.py @@ -15,6 +15,7 @@ other platforms. from __future__ import annotations +import inspect import platform import subprocess import sys @@ -25,6 +26,9 @@ 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( @@ -134,6 +138,44 @@ 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( From cbc7f286b305cb59a762a0649e750d3db1d24487 Mon Sep 17 00:00:00 2001 From: Dominik Jain Date: Fri, 24 Jul 2026 18:17:49 +0200 Subject: [PATCH 4/5] 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( From aeb73c789d11bf2dacb68513bc637a71137f825b Mon Sep 17 00:00:00 2001 From: Dominik Jain Date: Sun, 26 Jul 2026 11:34:42 +0200 Subject: [PATCH 5/5] Move changelog entries to their proper place, rewording --- CHANGELOG.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 380e9ba2..0a42b5c7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,11 +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 - - 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, 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 @@ -32,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.