Fix: Exceptions raised during LanguageServerManager.start did not stop the language server subprocess if it was already started

start() now catches an exception from _start_server(), stops the process if it is
already running, and re-raises. 

Fixes #1949

CHANGELOG.md

@@ -8,6 +8,8 @@ Status of the `main` branch. Changes prior to the next official version change w
   - Fix: `read_only` restriction in project definition was not applied to base tool set when in single-project context (#1938)
 
 * Language Servers:
+  - Fix: Exceptions raised during `LanguageServerManager.start` did not stop the language server subprocess if it was
+    already started (#1949)
   - Fix: Dart's `$/analyzerStatus` notifications were logged as unhandled-method warnings during analysis (#1855)
   - Fix: clojure-lsp was not told that Serena sends `workspace/didChangeWatchedFiles`, so changes made
     outside Serena's own edit tools (a git checkout, another editor, a build step) need not invalidate

src/serena/ls_manager.py

@@ -144,6 +144,9 @@ class LanguageServerManager:
                 language_servers[thread.ls_id] = thread.language_server
 
         # If any server failed to start up, raise an exception and stop all started language servers.
+        # A server whose own thread raised has already stopped its own process, since
+        # SolidLanguageServer.start() cleans up after itself on failure; only the servers that
+        # started successfully (and are therefore absent from `exceptions`) still need stopping.
         # We intentionally fail fast here. The user's intention is to work with all the specified languages,
         # so if any of them is not available, it is better to make symbolic tool calls fail, bringing the issue to the
         # user's attention instead of silently continuing with a subset of the language servers and potentially

src/solidlsp/ls.py

@@ -3179,7 +3179,17 @@ class SolidLanguageServer(ABC):
         """
         log.info(f"Starting language server {self.language_server.ls_id} for {self.language_server.repository_root_path}")
         self.server_started = True
-        self._start_server()
+        try:
+            self._start_server()
+        except Exception:
+            # `_start_server()` may raise after already spawning the underlying process (a
+            # capability assertion or an initialize() timeout firing post-spawn). Stop it here
+            # so a failed start never leaves an orphaned process behind, regardless of caller.
+            if self.is_running():
+                self.stop()
+            else:
+                self.server_started = False
+            raise
         return self
 
     def stop(self, shutdown_timeout: float = 2.0) -> None:

test/serena/test_ls_manager.py

@@ -0,0 +1,103 @@
+import subprocess
+import sys
+import time
+
+import psutil
+import pytest
+
+from serena.ls_manager import LanguageServerManager, LanguageServerManagerInitialisationError
+from solidlsp.ls_config import LanguageServerId
+
+
+def _pid_alive(pid: int) -> bool:
+    # Not `os.kill(pid, 0)`: that is a no-op liveness probe on POSIX, but on Windows
+    # `os.kill` has no signal-0 special case and falls through to `TerminateProcess(handle,
+    # 0)`, so the "check" can itself kill (or, if the pid was already recycled, terminate an
+    # unrelated process) instead of only reading process-table state.
+    return psutil.pid_exists(pid)
+
+
+class _FakeLanguageServer:
+    """Duck-typed stand-in for SolidLanguageServer: `from_languages` only calls
+    `.start()`/`.is_running()`/`.stop()` on it, never isinstance-checks the object.
+    """
+
+    def __init__(self, ls_id: LanguageServerId, should_fail: bool):
+        self.ls_id = ls_id
+        self.should_fail = should_fail
+        self.proc: subprocess.Popen | None = None
+        self._running = False
+
+    def start(self) -> "_FakeLanguageServer":
+        # Mirrors SolidLanguageServer.start(): the OS subprocess is spawned as part of
+        # start(), a capability/initialize() check can still raise after that process is
+        # already running, and start() stops the process it just spawned before re-raising
+        # (see SolidLanguageServer.start).
+        self.proc = subprocess.Popen([sys.executable, "-c", "import time; time.sleep(100)"])
+        if self.should_fail:
+            self._running = True
+            self.stop()
+            raise RuntimeError(f"simulated: capability assertion failed after initialize() ({self.ls_id.value})")
+        self._running = True
+        return self
+
+    def is_running(self) -> bool:
+        return self._running
+
+    def stop(self, shutdown_timeout: float = 2.0) -> None:
+        if self.proc is not None and self.proc.poll() is None:
+            self.proc.terminate()
+            try:
+                self.proc.wait(timeout=shutdown_timeout)
+            except subprocess.TimeoutExpired:
+                self.proc.kill()
+        self._running = False
+
+
+class _FakeLanguageServerFactory:
+    def __init__(self, fail_ids: set[LanguageServerId]):
+        self.fail_ids = fail_ids
+        self.created: dict[LanguageServerId, _FakeLanguageServer] = {}
+
+    def create_language_server(self, ls_id: LanguageServerId) -> _FakeLanguageServer:
+        ls = _FakeLanguageServer(ls_id, should_fail=ls_id in self.fail_ids)
+        self.created[ls_id] = ls
+        return ls
+
+
+@pytest.fixture
+def _cleanup_leftover_pids():
+    """Belt-and-braces: kill any spawned test subprocess still alive after the test body,
+    so a regression in the fix under test cannot leak a real OS process past this test.
+    """
+    pids: list[int] = []
+    yield pids
+    for pid in pids:
+        try:
+            psutil.Process(pid).kill()
+        except psutil.NoSuchProcess:
+            pass
+
+
+def test_from_languages_stops_process_of_server_that_raises_after_spawning(_cleanup_leftover_pids):
+    """End-to-end: a language server whose `start()` spawns its OS subprocess and then raises
+    (e.g. a capability assertion or an initialize() timeout firing after the process is
+    already up) must not leak that process, and a sibling that started successfully must be
+    stopped too since `from_languages` fails the whole batch. The failing server cleans up its
+    own process (SolidLanguageServer.start()); `from_languages` is responsible only for
+    stopping the siblings that succeeded.
+    """
+    ok_id = LanguageServerId("python")
+    failing_id = LanguageServerId("rust")
+    factory = _FakeLanguageServerFactory(fail_ids={failing_id})
+
+    with pytest.raises(LanguageServerManagerInitialisationError):
+        LanguageServerManager.from_languages([ok_id, failing_id], factory, project=None)
+
+    time.sleep(0.3)
+    pids = {ls_id: ls.proc.pid for ls_id, ls in factory.created.items() if ls.proc is not None}
+    _cleanup_leftover_pids.extend(pids.values())
+
+    assert set(pids) == {ok_id, failing_id}
+    assert not _pid_alive(pids[ok_id]), "the successfully-started server's process should be stopped"
+    assert not _pid_alive(pids[failing_id]), "the process spawned by the server that raised post-spawn must not leak"

test/solidlsp/test_ls_start_cleanup.py

@@ -0,0 +1,55 @@
+from unittest.mock import MagicMock
+
+import pytest
+
+from solidlsp.ls import SolidLanguageServer
+
+
+class _RaisingLanguageServer(SolidLanguageServer):
+    """Bypasses SolidLanguageServer.__init__ (like DummyLanguageServer in
+    test_rename_didopen.py) so _start_server can be made to raise after the underlying
+    process is already considered running, without spinning up a real language server.
+    """
+
+    def _start_server(self) -> None:
+        raise RuntimeError("simulated: capability assertion failed after initialize()")
+
+    def _create_base_initialize_params(self) -> dict:
+        return {}
+
+
+def _make_server(is_running: bool) -> _RaisingLanguageServer:
+    server = object.__new__(_RaisingLanguageServer)
+    server.ls_id = "python"
+    server.repository_root_path = "/tmp/project"
+    server.server_started = False
+    server.server = MagicMock()
+    server.server.is_running.return_value = is_running
+    return server
+
+
+def test_start_stops_process_when_start_server_raises_after_spawning():
+    """_start_server() spawning the OS process and then raising (e.g. a capability
+    assertion or an initialize() timeout firing post-spawn) must not leak that process:
+    start() itself stops it before re-raising.
+    """
+    server = _make_server(is_running=True)
+
+    with pytest.raises(RuntimeError, match="capability assertion"):
+        server.start()
+
+    server.server.stop.assert_called_once()
+    assert server.server_started is False
+
+
+def test_start_does_not_call_stop_when_start_server_raises_before_spawning():
+    """When _start_server() raises before any process was ever spawned (e.g. the language
+    server binary is missing), there is nothing to stop.
+    """
+    server = _make_server(is_running=False)
+
+    with pytest.raises(RuntimeError, match="capability assertion"):
+        server.start()
+
+    server.server.stop.assert_not_called()
+    assert server.server_started is False