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(