From 592cd360e741ddefe9d7e8ff367fe90566a89d9b Mon Sep 17 00:00:00 2001 From: CleverThis Date: Sat, 16 May 2026 16:21:12 +0000 Subject: [PATCH] fix(lsp): wrap post-Popen init in cleanup guard to prevent orphaned processes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #7044 The StdioTransport.start() method had an unprotected logger.info() call after successful Popen(). If that call raised, the subprocess would leak as an orphaned process. Wrap all post-spawn initialization in a try/except guard: on any exception after spawn, terminate and wait for the process (with kill fallback), reset state to None, then re-raise so callers still get proper error semantics. The existing stop() method cleanup pattern (terminate → wait → kill) is mirrored here for consistency across the transport lifecycle. Tests added: - TDD scenario with @tdd_issue_7044 verifying subprocess cleanup on post-Popen exception and state reset to None - Explicit is_alive() scenarios covering both alive and not-alive states Refactoring: - Extracted _make_mock_process and _build_lsp_frame helpers into a shared _ltcov_helpers module to keep step files under the 500-line CONTRIBUTING.md limit. ISSUES CLOSED: #7044 --- features/lsp_transport_coverage.feature | 23 ++++ features/steps/_ltcov_helpers.py | 66 ++++++++++ .../steps/lsp_transport_coverage_steps.py | 44 +------ .../lsp_transport_post_spawn_cleanup_steps.py | 120 ++++++++++++++++++ src/cleveragents/lsp/transport.py | 50 +++++++- 5 files changed, 255 insertions(+), 48 deletions(-) create mode 100644 features/steps/_ltcov_helpers.py create mode 100644 features/steps/lsp_transport_post_spawn_cleanup_steps.py diff --git a/features/lsp_transport_coverage.feature b/features/lsp_transport_coverage.feature index c5c9aaff7..258e2f5a8 100644 --- a/features/lsp_transport_coverage.feature +++ b/features/lsp_transport_coverage.feature @@ -5,12 +5,35 @@ Feature: LSP StdioTransport coverage # ── start() error paths ───────────────────────────────────────── + @tdd_issue @tdd_issue_7044 @tdd_expected_fail + Scenario: ltcov start cleans up subprocess when post-Popen init fails + Given ltcov I create a StdioTransport for command "cat" + And ltcov Popen is mocked to succeed with a running process + And ltcov logger.info after spawn raises RuntimeError + When ltcov I try to start the transport + Then ltcov an error occurred during start() + And ltcov the internal process should be None + And ltcov the mock process should have been terminated + And ltcov the mock process should have been waited on + Scenario: ltcov start raises RuntimeError when already alive Given ltcov I create a StdioTransport for command "cat" And ltcov the transport has a running mock process When ltcov I try to start the transport Then ltcov the error should be a RuntimeError with message "already started" + @tdd_issue @tdd_issue_7044 + Scenario: ltcov start is alive after successful spawn + Given ltcov I create a StdioTransport for command "cat" + When ltcov I try to start the transport + Then ltcov the transport should be alive + And ltcov the internal process should not be None + + Scenario: ltcov is_alive returns False when no process + Given ltcov I create a StdioTransport for command "cat" + When ltcov I check if the transport is alive + Then ltcov the is_alive result should be false + Scenario: ltcov start raises LspError on FileNotFoundError Given ltcov I create a StdioTransport for command "nonexistent_binary_xyz" And ltcov Popen is mocked to raise FileNotFoundError diff --git a/features/steps/_ltcov_helpers.py b/features/steps/_ltcov_helpers.py new file mode 100644 index 000000000..a4927c97f --- /dev/null +++ b/features/steps/_ltcov_helpers.py @@ -0,0 +1,66 @@ +"""Shared helper functions for LSP transport BDD step definitions. + +Extracted common mock-building utilities so individual step files stay +under the 500-line limit (see CONTRIBUTING.md file-size guideline). +""" + +from __future__ import annotations + +import json +import subprocess +from typing import Any +from unittest.mock import MagicMock + + +def make_mock_process( + *, + poll_return: int | None = None, + returncode: int = 0, + stdin: object | None = "auto", + stdout: object | None = "auto", + stderr: object | None = "auto", + pid: int = 12345, +) -> MagicMock: + """Create a ``MagicMock`` resembling ``subprocess.Popen``. + + Args: + poll_return: Value for :attr:`~MagicMock.poll` return value. + ``None`` means the process is still running. + returncode: The subprocess exit code when ``poll()`` returns. + stdin: Stdin mock object (or ``"auto"`` for auto-generated). + stdout: Stdout mock object (or ``"auto"`` for auto-generated). + stderr: Stderr mock object (or ``"auto"`` for auto-generated). + pid: Process ID to report. + + Returns: + A fully configured ``MagicMock`` with ``subprocess.Popen`` spec. + """ + proc = MagicMock(spec=subprocess.Popen) + proc.pid = pid + proc.poll.return_value = poll_return + proc.returncode = returncode + if stdin == "auto": + proc.stdin = MagicMock() + else: + proc.stdin = stdin + if stdout == "auto": + proc.stdout = MagicMock() + else: + proc.stdout = stdout + if stderr == "auto": + proc.stderr = MagicMock() + return proc + + +def build_lsp_frame(body_dict: dict[str, Any]) -> bytes: + """Encode a dict as a Content-Length framed LSP message. + + Args: + body_dict: The JSON-serialisable message body. + + Returns: + Raw bytes including the ``Content-Length`` header and empty line prefix. + """ + payload = json.dumps(body_dict, separators=(",", ":")).encode("utf-8") + header = f"Content-Length: {len(payload)}\r\n\r\n".encode("ascii") + return header + payload diff --git a/features/steps/lsp_transport_coverage_steps.py b/features/steps/lsp_transport_coverage_steps.py index 7a0fefa3b..a13567886 100644 --- a/features/steps/lsp_transport_coverage_steps.py +++ b/features/steps/lsp_transport_coverage_steps.py @@ -4,7 +4,8 @@ Exercises every uncovered branch in ``cleveragents.lsp.transport``: start() error paths, stop() variations, send_message(), read_message() header/body parsing, and all error handling paths. -Uses the ``ltcov`` prefix on all steps to avoid AmbiguousStep errors. +Uses the ``ltcov`` prefix on all steps to avoid AmbiguousStep errors. Shared +helper utilities are defined in :mod:`_ltcov_helpers`. """ from __future__ import annotations @@ -19,46 +20,7 @@ from behave.runner import Context from cleveragents.lsp.errors import LspError from cleveragents.lsp.transport import _MAX_CONTENT_LENGTH, StdioTransport -# --------------------------------------------------------------------------- -# Helpers -# --------------------------------------------------------------------------- - - -def _make_mock_process( - *, - poll_return: int | None = None, - returncode: int = 0, - stdin: object | None = "auto", - stdout: object | None = "auto", - stderr: object | None = "auto", - pid: int = 12345, -) -> MagicMock: - """Create a ``MagicMock`` resembling ``subprocess.Popen``.""" - proc = MagicMock(spec=subprocess.Popen) - proc.pid = pid - proc.poll.return_value = poll_return - proc.returncode = returncode - if stdin == "auto": - proc.stdin = MagicMock() - else: - proc.stdin = stdin - if stdout == "auto": - proc.stdout = MagicMock() - else: - proc.stdout = stdout - if stderr == "auto": - proc.stderr = MagicMock() - else: - proc.stderr = stderr - return proc - - -def _build_lsp_frame(body_dict: dict) -> bytes: - """Encode a dict as a Content-Length framed LSP message.""" - payload = json.dumps(body_dict, separators=(",", ":")).encode("utf-8") - header = f"Content-Length: {len(payload)}\r\n\r\n".encode("ascii") - return header + payload - +from ._ltcov_helpers import build_lsp_frame, make_mock_process as _make_mock_process # --------------------------------------------------------------------------- # Given steps diff --git a/features/steps/lsp_transport_post_spawn_cleanup_steps.py b/features/steps/lsp_transport_post_spawn_cleanup_steps.py new file mode 100644 index 000000000..9b2ed05f8 --- /dev/null +++ b/features/steps/lsp_transport_post_spawn_cleanup_steps.py @@ -0,0 +1,120 @@ +"""Step definitions for LSP StdioTransport post-spawn cleanup tests. + +Covers issue #7044: subprocess cleanup when post-Popen initialization fails, +plus explicit is_alive() test coverage. Uses the ``ltcov`` prefix on all steps. +Shared helper utilities are imported from :mod:`_ltcov_helpers`. +""" + +from __future__ import annotations + +import subprocess +from unittest.mock import MagicMock, patch + +from behave import given, then, when +from behave.runner import Context + +from ._ltcov_helpers import make_mock_process as _make_mock_process + +# --------------------------------------------------------------------------- +# Given steps +# --------------------------------------------------------------------------- + + +@given("ltcov Popen is mocked to succeed with a running process") +def step_ltcov_popen_succeed(context: Context) -> None: + """Mock subprocess.Popen to return a MagicMock (no exception), simulating + a successful spawn. The mock is stored on context for assertion. + """ + proc = _make_mock_process(poll_return=None, returncode=None) + + patcher = patch( + "cleveragents.lsp.transport.subprocess.Popen", + return_value=proc, + ) + patcher.start() + context.add_cleanup(patcher.stop) + context._ltcov_mock_popen_return_value = proc + + +@given("ltcov logger.info after spawn raises RuntimeError") +def step_ltcov_logger_info_raises(context: Context) -> None: + """Mock ``logger.info`` to raise a RuntimeError, simulating that post-Popen + initialization (e.g. logging the started message) fails. + """ + + def _info_raises(*args, **kwargs): + raise RuntimeError("simulated post-Popen initialization failure") + + patcher = patch( + "cleveragents.lsp.transport.logger.info", + side_effect=_info_raises, + ) + patcher.start() + context.add_cleanup(patcher.stop) + + +# --------------------------------------------------------------------------- +# When steps +# --------------------------------------------------------------------------- + + +@when("ltcov I check if the transport is alive") +def step_ltcov_check_alive(context: Context) -> None: + """Call is_alive() and store the boolean result.""" + context.ltcov_is_alive_result = context.ltcov_transport.is_alive + + +# --------------------------------------------------------------------------- +# Then steps +# --------------------------------------------------------------------------- + + +@then("ltcov an error occurred during start()") +def step_ltcov_error_occurred(context: Context) -> None: + """Assert that start() raised some exception (wrapped or unwrapped).""" + assert context.ltcov_error is not None, ( + "Expected an error but none was raised" + ) + + +@then("ltcov the mock process should have been terminated") +def step_ltcov_process_terminated(context: Context) -> None: + """Assert that terminate() was called on the mock subprocess.""" + proc = context._ltcov_mock_popen_return_value + proc.terminate.assert_called_once(), ( + "Expected the mock process to be terminated" + ) + + +@then("ltcov the mock process should have been waited on") +def step_ltcov_process_waited(context: Context) -> None: + """Assert that wait() was called on the mock subprocess.""" + proc = context._ltcov_mock_popen_return_value + proc.wait.assert_called_once(), ( + "Expected the mock process to be waited on" + ) + + +@then("ltcov the transport should be alive") +def step_ltcov_transport_alive(context: Context) -> None: + """Assert that is_alive() returns True.""" + assert context.ltcov_transport.is_alive, ( + "Expected transport to be alive but it reported not alive" + ) + + +@then("ltcov the internal process should not be None") +def step_ltcov_process_not_none(context: Context) -> None: + """Assert that _process is set to a non-None value.""" + assert context.ltcov_transport._process is not None, ( + "Expected _process to be set but it is None" + ) + + +@then("ltcov the is_alive result should be false") +def step_ltcov_alive_result_false(context: Context) -> None: + """Assert that is_alive() returns False.""" + assert context.ltcov_is_alive_result is False, ( + f"Expected False, got {context.ltcov_is_alive_result}" + ) + diff --git a/src/cleveragents/lsp/transport.py b/src/cleveragents/lsp/transport.py index 841475b1c..cc45c672f 100644 --- a/src/cleveragents/lsp/transport.py +++ b/src/cleveragents/lsp/transport.py @@ -83,11 +83,22 @@ class StdioTransport: return self._process is not None and self._process.poll() is None def start(self) -> None: - """Spawn the LSP server subprocess. + """Spawn the LSP server subprocess and verify connectivity. + + After ``subprocess.Popen`` succeeds, this method wraps all + subsequent initialization steps (e.g. logging) in a defensive + try/except guard. If any step raises an exception after a + successful spawn, the subprocess is terminated and waited on + to prevent orphaned LSP server processes leaking into the + background. + + The method re-raises the original exception after cleanup so + callers always receive proper error semantics. Raises: LspError: If the process cannot be started. - RuntimeError: If already started. + RuntimeError: If already started or if a post-spawn init + step fails (the subprocess is cleaned up before re-raising). """ if self._process is not None and self.is_alive: raise RuntimeError("Transport already started") @@ -143,11 +154,36 @@ class StdioTransport: self._process = None raise - logger.info( - "lsp.transport.started", - pid=self._process.pid, - command=self._command, - ) + # All post-Popen initialization logic must be placed within the + # guarded block below to prevent orphaned subprocesses if init + # steps fail after a successful spawn. + try: + logger.info( + "lsp.transport.started", + pid=self._process.pid, + command=self._command, + ) + except Exception: + # Post-spawn initialization failed — clean up the orphaned + # subprocess to prevent resource leaks. We re-raise the + # original exception so callers still get proper error + # semantics; this block only ensures resources are freed. + logger.warning( + "lsp.transport.start_post_init_failed", + pid=self._process.pid, + ) + self._process.terminate() + try: + self._process.wait(timeout=_GRACEFUL_SHUTDOWN_TIMEOUT) + except subprocess.TimeoutExpired: + logger.warning( + "lsp.transport.force_killing_cleanup", + pid=self._process.pid, + ) + self._process.kill() + self._process.wait(timeout=2.0) + self._process = None + raise def stop(self, timeout: float = _GRACEFUL_SHUTDOWN_TIMEOUT) -> int | None: """Terminate the subprocess gracefully, then force-kill if needed. -- 2.52.0