From 24940a1560fa346620e4c7676701648eeba08c3c Mon Sep 17 00:00:00 2001 From: Max Date: Thu, 3 Sep 2026 13:28:05 +0000 Subject: [PATCH] fix(process): reap language server descendants (#1918) * fix(process): reap language server descendants * fix(process): preserve descendant snapshot for kill fallback * fix(process): share termination timeout deadline * fix(process): reap descendants in process-group cleanup * fix(process): reap leader after descendant kill timeout --------- Co-authored-by: Yu-0312 <2.66498311e+08+Yu-0312@users.noreply.github.com> --- CHANGELOG.md | 2 + src/solidlsp/util/subprocess_util.py | 103 ++++++-- test/solidlsp/test_process_group_cleanup.py | 276 ++++++++++++++++++++ 3 files changed, 356 insertions(+), 25 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index dc7f35ec..0b597cff 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,8 @@ Status of the `main` branch. Changes prior to the next official version change w - Fix: MCP `initialize` now reports Serena's version instead of the installed mcp SDK version (#1889) - Fix: Parallel agents auto-registering projects could overwrite each other's changes to the global project list in `serena_config.yml` + - Fix: process-tree cleanup signaled descendant language-server processes without waiting for them, + which could leave grandchildren as zombies; cleanup now waits for the discovered descendants (#1464) - Fix: `read_only` restriction in project definition was not applied to base tool set when in single-project context (#1938) * Memories: diff --git a/src/solidlsp/util/subprocess_util.py b/src/solidlsp/util/subprocess_util.py index f8e9377a..a6c5905f 100644 --- a/src/solidlsp/util/subprocess_util.py +++ b/src/solidlsp/util/subprocess_util.py @@ -6,6 +6,7 @@ import signal import subprocess import threading from collections.abc import Callable +from time import monotonic from typing import IO, TYPE_CHECKING, Any, Generic, TypeVar, cast import oslex @@ -281,11 +282,20 @@ class ManagedSubprocessLauncher: return process -def _signal_process_tree(process: subprocess.Popen[bytes], terminate: bool = True) -> None: +def _signal_process_tree( + process: subprocess.Popen[bytes], + terminate: bool = True, + descendants: list[psutil.Process] | None = None, +) -> None: """ - Sends a signal (terminate or kill) to the given process and all its children. + Sends a signal (terminate or kill) to the given process and its descendants. + + ``descendants`` is an optional snapshot captured before the leader is signaled. + It is needed for kill fallback: the leader may have exited and been reaped before + the fallback runs, in which case re-enumerating from ``process.pid`` loses the tree. :param terminate: if True, signal terminate, otherwise signal kill + :param descendants: previously discovered descendants to signal even if the leader is gone """ def signal_process(p: subprocess.Popen | psutil.Process) -> None: @@ -294,24 +304,23 @@ def _signal_process_tree(process: subprocess.Popen[bytes], terminate: bool = Tru p.terminate() else: p.kill() - except: + except (psutil.Error, OSError): pass - # Try to get the parent process - parent = None - try: - parent = psutil.Process(process.pid) - except (psutil.NoSuchProcess, psutil.AccessDenied, Exception): - pass + if descendants is None: + descendants = [] + try: + parent = psutil.Process(process.pid) + if parent.is_running(): + descendants = parent.children(recursive=True) + except (psutil.Error, OSError): + pass - # If we have the parent process and it's running, signal the entire tree - if parent and parent.is_running(): - for child in parent.children(recursive=True): - signal_process(child) - signal_process(parent) - # Otherwise, fall back to direct process signaling - else: - signal_process(process) + # Signal the snapshot first, then the leader. The snapshot remains usable after + # the leader has exited, while newly spawned children are outside its guarantee. + for child in descendants: + signal_process(child) + signal_process(process) def _signal_process_group(pgid: int, terminate: bool = True) -> None: @@ -335,6 +344,40 @@ def _signal_process_group(pgid: int, terminate: bool = True) -> None: log.warning(f"Unexpected error signaling process group {pgid} with {sig.name}: {e}") +def _get_process_descendants(process: subprocess.Popen) -> list[psutil.Process]: + """Return a snapshot of descendants that should be waited on after signaling.""" + try: + return psutil.Process(process.pid).children(recursive=True) + except (psutil.Error, OSError): + return [] + + +def _wait_for_processes_until(processes: list[psutil.Process], deadline: float) -> bool: + """Wait for a process snapshot until an absolute monotonic deadline. + + Descendants are waited in reverse depth-first discovery order so that a child + has a chance to exit before its parent is reaped. A process that disappears + or cannot be waited by the current OS process is already clean for our + purposes; a timeout is returned to trigger the caller's kill fallback. + """ + for child in reversed(processes): + remaining = deadline - monotonic() + if remaining <= 0: + return False + try: + child.wait(timeout=remaining) + except psutil.TimeoutExpired: + return False + except (psutil.Error, OSError): + continue + return True + + +def _wait_for_processes(processes: list[psutil.Process], timeout: float) -> bool: + """Wait for a process snapshot within one shared timeout budget.""" + return _wait_for_processes_until(processes, monotonic() + timeout) + + def terminate_process_tree_with_kill_fallback( process: subprocess.Popen, terminate_timeout: float, @@ -352,31 +395,41 @@ def terminate_process_tree_with_kill_fallback( :param process_name: the name of the process (used for logging purposes); should start with capital letter :param process_group_id: if given, the POSIX process group ID that ``process`` leads (i.e. it was launched with ``start_new_session=True``, so its PGID equals its PID). When set, cleanup - signals the whole group directly via ``os.killpg`` instead of walking the process tree with - ``psutil``, which requires system-wide process-table enumeration (``sysctl(KERN_PROC_ALL)`` on - macOS) that can be denied even for processes we started and own. Only pass this for a process - that was started in its own session: signaling the group of a process that shares ours would - also signal us. + signals the whole group directly via ``os.killpg`` rather than walking the process tree to + signal it. Descendant discovery remains best-effort so cleanup can wait for children after + the group is signaled; ``_get_process_descendants`` safely returns an empty list when the + platform denies process-table enumeration (for example, ``sysctl(KERN_PROC_ALL)`` on macOS). + Only pass this for a process that was started in its own session: signaling the group of a + process that shares ours would also signal us. """ log.debug(f"Terminating process {process.pid}, current status: {process.poll()}") + descendants = _get_process_descendants(process) def signal_tree(terminate: bool) -> None: if process_group_id is not None: _signal_process_group(process_group_id, terminate=terminate) else: - _signal_process_tree(process, terminate=terminate) + _signal_process_tree(process, terminate=terminate, descendants=None if terminate else descendants) signal_tree(terminate=True) try: log.debug(f"Waiting for process {process.pid} to terminate...") - exit_code = process.wait(timeout=terminate_timeout) + terminate_deadline = monotonic() + terminate_timeout + descendants_finished = _wait_for_processes_until(descendants, terminate_deadline) + remaining = terminate_deadline - monotonic() + if not descendants_finished or remaining <= 0: + raise subprocess.TimeoutExpired(process.args, terminate_timeout) + exit_code = process.wait(timeout=remaining) log.info(f"{process_name} terminated successfully with exit code {exit_code}.") except subprocess.TimeoutExpired: # If termination failed, forcefully kill the process log.warning(f"{process_name} (pid={process.pid}) termination timed out, killing process forcefully...") signal_tree(terminate=False) + kill_deadline = monotonic() + 2.0 + _wait_for_processes_until(descendants, kill_deadline) + remaining = max(kill_deadline - monotonic(), 0.1) try: - exit_code = process.wait(timeout=2.0) + exit_code = process.wait(timeout=remaining) log.info(f"{process_name} killed successfully with exit code {exit_code}.") except subprocess.TimeoutExpired: log.error(f"{process_name} (pid={process.pid}) could not be killed within timeout.") diff --git a/test/solidlsp/test_process_group_cleanup.py b/test/solidlsp/test_process_group_cleanup.py index 0a37ccc0..9bd4098c 100644 --- a/test/solidlsp/test_process_group_cleanup.py +++ b/test/solidlsp/test_process_group_cleanup.py @@ -25,6 +25,7 @@ import time import psutil import pytest +from solidlsp.util import subprocess_util from solidlsp.util.subprocess_util import _signal_process_group, terminate_process_tree_with_kill_fallback pytestmark = pytest.mark.skipif(platform.system() == "Windows", reason="process groups / os.killpg are POSIX-specific") @@ -55,6 +56,14 @@ def _process_alive(pid: int) -> bool: return False +def _pid_is_gone(pid: int) -> bool: + try: + psutil.Process(pid) + return False + except psutil.NoSuchProcess: + return True + + class _DenyingProcess: """Stand-in for ``psutil.Process`` that always raises ``AccessDenied``, used to simulate a sandboxed environment denying process-table enumeration without needing one. A real class @@ -114,6 +123,157 @@ class TestTerminateProcessTreeWithKillFallback: proc.kill() proc.wait(timeout=2.0) + def test_graceful_termination_uses_one_shared_deadline(self, monkeypatch: pytest.MonkeyPatch) -> None: + """The descendant wait and leader wait must share ``terminate_timeout``.""" + now = [0.0] + descendant_wait_timeouts: list[float] = [] + leader_wait_timeouts: list[float] = [] + + class FakeDescendant: + def wait(self, timeout: float) -> None: + descendant_wait_timeouts.append(timeout) + now[0] += 3.0 + + class FakePopen: + pid = 123 + args = ["fake-language-server"] + + def poll(self) -> None: + return None + + def wait(self, timeout: float) -> int: + leader_wait_timeouts.append(timeout) + return 0 + + monkeypatch.setattr(subprocess_util, "monotonic", lambda: now[0]) + monkeypatch.setattr(subprocess_util, "_get_process_descendants", lambda _process: [FakeDescendant()]) + monkeypatch.setattr(subprocess_util, "_signal_process_tree", lambda *args, **kwargs: None) + + terminate_process_tree_with_kill_fallback(FakePopen(), terminate_timeout=5.0) + + assert descendant_wait_timeouts == [pytest.approx(5.0)] + assert leader_wait_timeouts == [pytest.approx(2.0)] + + def test_kill_fallback_uses_one_shared_deadline(self, monkeypatch: pytest.MonkeyPatch) -> None: + """The forceful descendant wait and leader wait must share the 2s fallback budget.""" + now = [0.0] + descendant_wait_timeouts: list[float] = [] + leader_wait_timeouts: list[float] = [] + + class FakeDescendant: + wait_count = 0 + + def wait(self, timeout: float) -> None: + descendant_wait_timeouts.append(timeout) + self.wait_count += 1 + if self.wait_count == 1: + now[0] += timeout + raise psutil.TimeoutExpired(timeout, 456) + now[0] += 1.5 + + class FakePopen: + pid = 123 + args = ["fake-language-server"] + + def poll(self) -> None: + return None + + def wait(self, timeout: float) -> int: + leader_wait_timeouts.append(timeout) + return 0 + + monkeypatch.setattr(subprocess_util, "monotonic", lambda: now[0]) + monkeypatch.setattr(subprocess_util, "_get_process_descendants", lambda _process: [FakeDescendant()]) + monkeypatch.setattr(subprocess_util, "_signal_process_tree", lambda *args, **kwargs: None) + + terminate_process_tree_with_kill_fallback(FakePopen(), terminate_timeout=5.0) + + assert descendant_wait_timeouts == [pytest.approx(5.0), pytest.approx(2.0)] + assert leader_wait_timeouts == [pytest.approx(0.5)] + + def test_group_cleanup_kills_descendant_after_leader_exits(self) -> None: + """A group leader may exit after SIGTERM while a descendant keeps running. + + Cleanup must snapshot descendants before signaling, wait for them, and use the + group kill fallback instead of treating the exited leader as sufficient. + """ + child_code = textwrap.dedent( + """ + import signal, time + signal.signal(signal.SIGTERM, signal.SIG_IGN) + time.sleep(300) + """ + ) + leader_code = textwrap.dedent( + f""" + import subprocess, sys, time + subprocess.Popen([sys.executable, "-c", {child_code!r}], stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL) + print("READY", flush=True) + time.sleep(300) + """ + ) + proc = _spawn_ready(leader_code) + child_pid: int | None = None + try: + deadline = time.monotonic() + 5.0 + while time.monotonic() < deadline: + children = psutil.Process(proc.pid).children(recursive=True) + if children: + child_pid = children[0].pid + break + time.sleep(0.05) + assert child_pid is not None, "expected the process-group leader to have a child" + child_pid_value = child_pid + + terminate_process_tree_with_kill_fallback(proc, terminate_timeout=0.2, process_group_id=proc.pid) + + assert _wait_until(lambda: not _process_alive(child_pid_value)), "descendant survived group cleanup" + finally: + try: + os.killpg(proc.pid, signal.SIGKILL) + except ProcessLookupError: + pass + if proc.poll() is None: + proc.wait(timeout=2.0) + if child_pid is not None and _process_alive(child_pid): + try: + os.kill(child_pid, signal.SIGKILL) + except ProcessLookupError: + pass + + def test_kill_fallback_reaps_leader_after_descendant_wait_expires(self, monkeypatch: pytest.MonkeyPatch) -> None: + """The leader must still be reaped if descendants consume the kill budget.""" + now = [0.0] + descendant_wait_timeouts: list[float] = [] + leader_wait_timeouts: list[float] = [] + + class FakeDescendant: + def wait(self, timeout: float) -> None: + descendant_wait_timeouts.append(timeout) + now[0] += timeout + if len(descendant_wait_timeouts) == 1: + raise psutil.TimeoutExpired(timeout, 456) + + class FakePopen: + pid = 123 + args = ["fake-language-server"] + + def poll(self) -> None: + return None + + def wait(self, timeout: float) -> int: + leader_wait_timeouts.append(timeout) + return -signal.SIGKILL + + monkeypatch.setattr(subprocess_util, "monotonic", lambda: now[0]) + monkeypatch.setattr(subprocess_util, "_get_process_descendants", lambda _process: [FakeDescendant()]) + monkeypatch.setattr(subprocess_util, "_signal_process_tree", lambda *args, **kwargs: None) + + terminate_process_tree_with_kill_fallback(FakePopen(), terminate_timeout=5.0) + + assert descendant_wait_timeouts == [pytest.approx(5.0), pytest.approx(2.0)] + assert leader_wait_timeouts == [pytest.approx(0.1)] + def test_falls_back_to_kill_when_group_ignores_sigterm(self) -> None: src = textwrap.dedent( """ @@ -225,3 +385,119 @@ class TestPsutilDenialConsequences: pass if proc.poll() is None: proc.wait(timeout=2.0) + + +class TestProcessTreeDescendantReaping: + def test_kill_fallback_signals_snapshot_after_leader_exits(self) -> None: + """A leader can exit before the fallback runs while a child ignores SIGTERM. + + The fallback must use the pre-signal descendant snapshot; re-enumerating from + the exited leader would leave the child and its zombie grandchild behind. + """ + child_code = textwrap.dedent( + """ + import signal, subprocess, sys, time + signal.signal(signal.SIGTERM, signal.SIG_IGN) + subprocess.Popen([sys.executable, "-c", "import sys; sys.exit(17)"], stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL) + time.sleep(300) + """ + ) + leader_code = textwrap.dedent( + f""" + import subprocess, sys, time + subprocess.Popen([sys.executable, "-c", {child_code!r}], stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL) + print("READY", flush=True) + time.sleep(300) + """ + ) + proc = _spawn_ready(leader_code) + try: + descendants: list[psutil.Process] = [] + deadline = time.monotonic() + 5.0 + while time.monotonic() < deadline: + descendants = psutil.Process(proc.pid).children(recursive=True) + if len(descendants) >= 2 and any(child.status() == psutil.STATUS_ZOMBIE for child in descendants): + break + time.sleep(0.05) + assert len(descendants) >= 2, "expected leader, child, and grandchild processes" + assert any(child.status() == psutil.STATUS_ZOMBIE for child in descendants), "expected a zombie grandchild" + + terminate_process_tree_with_kill_fallback(proc, terminate_timeout=0.2, process_name="leader") + + assert _wait_until(lambda: all(_pid_is_gone(child.pid) for child in descendants)), ( + "kill fallback must not leave the saved child or zombie grandchild behind" + ) + finally: + for child in descendants: + try: + child.kill() + except psutil.Error: + pass + if proc.poll() is None: + proc.kill() + proc.wait(timeout=2.0) + + def test_waits_for_discovered_descendants_after_signaling(self, monkeypatch: pytest.MonkeyPatch) -> None: + events: list[str] = [] + + class FakeDescendant: + def __init__(self, name: str) -> None: + self.name = name + + def terminate(self) -> None: + events.append(f"terminate:{self.name}") + + def kill(self) -> None: + events.append(f"kill:{self.name}") + + def wait(self, timeout: float) -> None: + events.append(f"wait:{self.name}") + + descendants = [FakeDescendant("child"), FakeDescendant("grandchild")] + + class FakePsutilProcess: + def __init__(self, pid: int) -> None: + self.pid = pid + + def is_running(self) -> bool: + return True + + def children(self, recursive: bool) -> list[FakeDescendant]: + assert recursive + return descendants + + def terminate(self) -> None: + events.append("terminate:leader") + + def kill(self) -> None: + events.append("kill:leader") + + class FakePopen: + pid = 123 + args = ["fake-language-server"] + + def poll(self) -> None: + return None + + def terminate(self) -> None: + events.append("terminate:leader") + + def kill(self) -> None: + events.append("kill:leader") + + def wait(self, timeout: float) -> int: + events.append("wait:leader") + return 0 + + monkeypatch.setattr(subprocess_util.psutil, "Process", FakePsutilProcess) + + subprocess_util.terminate_process_tree_with_kill_fallback(FakePopen(), terminate_timeout=1.0) + + assert events == [ + "terminate:child", + "terminate:grandchild", + "terminate:leader", + "wait:grandchild", + "wait:child", + "wait:leader", + ]