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

@@ -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:

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.")

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",
+        ]