From dc59a8931c5ca90deb18bed2a5392ea2fc37add8 Mon Sep 17 00:00:00 2001 From: Amir Fathi Date: Wed, 2 Sep 2026 04:53:41 -0600 Subject: [PATCH] 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 | 2 + src/serena/ls_manager.py | 3 + src/solidlsp/ls.py | 12 ++- test/serena/test_ls_manager.py | 103 +++++++++++++++++++++++++ test/solidlsp/test_ls_start_cleanup.py | 55 +++++++++++++ 5 files changed, 174 insertions(+), 1 deletion(-) create mode 100644 test/serena/test_ls_manager.py create mode 100644 test/solidlsp/test_ls_start_cleanup.py diff --git a/CHANGELOG.md b/CHANGELOG.md index e60a96fb..7d8dfa43 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/src/serena/ls_manager.py b/src/serena/ls_manager.py index 2dd3aaf4..513e3651 100644 --- a/src/serena/ls_manager.py +++ b/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 diff --git a/src/solidlsp/ls.py b/src/solidlsp/ls.py index cba90723..13497121 100644 --- a/src/solidlsp/ls.py +++ b/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: diff --git a/test/serena/test_ls_manager.py b/test/serena/test_ls_manager.py new file mode 100644 index 00000000..ae6994b6 --- /dev/null +++ b/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" diff --git a/test/solidlsp/test_ls_start_cleanup.py b/test/solidlsp/test_ls_start_cleanup.py new file mode 100644 index 00000000..7e574311 --- /dev/null +++ b/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