From daf90ba76457cd9d9cda36bee75ef5fc79746acb Mon Sep 17 00:00:00 2001 From: CleverThis Date: Sat, 16 May 2026 16:21:12 +0000 Subject: [PATCH 1/5] 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 From 421fa4060c1fb05bd68f6da503190f421256e8b5 Mon Sep 17 00:00:00 2001 From: controller-ci-rerun Date: Thu, 28 May 2026 09:45:44 -0400 Subject: [PATCH 2/5] chore: re-trigger CI [controller] -- 2.52.0 From a61d132ac0347af1fcf4a1ca922ebae940114309 Mon Sep 17 00:00:00 2001 From: CleverThis Date: Thu, 28 May 2026 10:21:03 -0400 Subject: [PATCH 3/5] fix(lsp): fix relative imports and remove unused imports in BDD step files - features/steps/lsp_transport_coverage_steps.py: change relative import `from ._ltcov_helpers import build_lsp_frame, ...` to absolute `from _ltcov_helpers import make_mock_process ...`, removing unused `build_lsp_frame` - features/steps/lsp_transport_post_spawn_cleanup_steps.py: remove unused `subprocess` and `MagicMock` imports; fix relative import to absolute; collapse short assert messages to single lines; remove trailing blank line Behave's exec_file loader does not set __name__ in globals, causing relative imports to raise KeyError at step-module load time. Absolute imports work because Behave adds features/steps/ to sys.path. ISSUES CLOSED: #11237 --- features/steps/lsp_transport_coverage_steps.py | 2 +- .../lsp_transport_post_spawn_cleanup_steps.py | 18 +++++------------- 2 files changed, 6 insertions(+), 14 deletions(-) diff --git a/features/steps/lsp_transport_coverage_steps.py b/features/steps/lsp_transport_coverage_steps.py index a13567886..8d94467a8 100644 --- a/features/steps/lsp_transport_coverage_steps.py +++ b/features/steps/lsp_transport_coverage_steps.py @@ -20,7 +20,7 @@ from behave.runner import Context from cleveragents.lsp.errors import LspError from cleveragents.lsp.transport import _MAX_CONTENT_LENGTH, StdioTransport -from ._ltcov_helpers import build_lsp_frame, make_mock_process as _make_mock_process +from _ltcov_helpers import 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 index 9b2ed05f8..8f813bccd 100644 --- a/features/steps/lsp_transport_post_spawn_cleanup_steps.py +++ b/features/steps/lsp_transport_post_spawn_cleanup_steps.py @@ -7,13 +7,12 @@ Shared helper utilities are imported from :mod:`_ltcov_helpers`. from __future__ import annotations -import subprocess -from unittest.mock import MagicMock, patch +from unittest.mock import patch from behave import given, then, when from behave.runner import Context -from ._ltcov_helpers import make_mock_process as _make_mock_process +from _ltcov_helpers import make_mock_process as _make_mock_process # --------------------------------------------------------------------------- # Given steps @@ -72,27 +71,21 @@ def step_ltcov_check_alive(context: Context) -> None: @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" - ) + 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" - ) + 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" - ) + proc.wait.assert_called_once(), "Expected the mock process to be waited on" @then("ltcov the transport should be alive") @@ -117,4 +110,3 @@ def step_ltcov_alive_result_false(context: Context) -> None: assert context.ltcov_is_alive_result is False, ( f"Expected False, got {context.ltcov_is_alive_result}" ) - -- 2.52.0 From c2459772eb473513118982881b25d876485cebe4 Mon Sep 17 00:00:00 2001 From: CleverThis Date: Thu, 28 May 2026 11:03:11 -0400 Subject: [PATCH 4/5] fix(lsp): restore missing else branch for stderr in _make_mock_process The `else: proc.stderr = stderr` clause was dropped when the helper was extracted into `_ltcov_helpers.py`. `stdin` and `stdout` both have the corresponding else branches; this restores parity so that callers passing a non-"auto" stderr value have it honoured. --- features/steps/_ltcov_helpers.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/features/steps/_ltcov_helpers.py b/features/steps/_ltcov_helpers.py index a4927c97f..8554fcd69 100644 --- a/features/steps/_ltcov_helpers.py +++ b/features/steps/_ltcov_helpers.py @@ -49,6 +49,8 @@ def make_mock_process( proc.stdout = stdout if stderr == "auto": proc.stderr = MagicMock() + else: + proc.stderr = stderr return proc -- 2.52.0 From 22831e4f5095a0a0feee6ec71eebc9f7e4189fc4 Mon Sep 17 00:00:00 2001 From: CleverThis Date: Thu, 28 May 2026 11:36:14 -0400 Subject: [PATCH 5/5] fix(lsp): make post-spawn logger.info mock selective and remove tdd_expected_fail The _info_raises side_effect previously raised RuntimeError on every logger.info call, which caused the pre-Popen "lsp.transport.starting" call (transport.py:109) to raise before subprocess.Popen was ever reached. This meant the cleanup guard at lines 160-186 was never exercised, making the scenario a permanent no-op TDD stub rather than a genuine regression guard. Fix: make _info_raises conditional on the first positional argument being "lsp.transport.started" (the post-Popen message). The pre-Popen "lsp.transport.starting" call now passes through normally, Popen succeeds (mocked), and the RuntimeError is raised inside the guarded try/except block, triggering terminate() + wait() cleanup. Remove @tdd_expected_fail from the scenario since the cleanup code at transport.py:160-186 is in place and the scenario now passes. Closes #7044 --- features/lsp_transport_coverage.feature | 2 +- features/steps/lsp_transport_post_spawn_cleanup_steps.py | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/features/lsp_transport_coverage.feature b/features/lsp_transport_coverage.feature index 258e2f5a8..07e2186c6 100644 --- a/features/lsp_transport_coverage.feature +++ b/features/lsp_transport_coverage.feature @@ -5,7 +5,7 @@ Feature: LSP StdioTransport coverage # ── start() error paths ───────────────────────────────────────── - @tdd_issue @tdd_issue_7044 @tdd_expected_fail + @tdd_issue @tdd_issue_7044 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 diff --git a/features/steps/lsp_transport_post_spawn_cleanup_steps.py b/features/steps/lsp_transport_post_spawn_cleanup_steps.py index 8f813bccd..3daf3b6a8 100644 --- a/features/steps/lsp_transport_post_spawn_cleanup_steps.py +++ b/features/steps/lsp_transport_post_spawn_cleanup_steps.py @@ -42,7 +42,8 @@ def step_ltcov_logger_info_raises(context: Context) -> None: """ def _info_raises(*args, **kwargs): - raise RuntimeError("simulated post-Popen initialization failure") + if args and args[0] == "lsp.transport.started": + raise RuntimeError("simulated post-Popen initialization failure") patcher = patch( "cleveragents.lsp.transport.logger.info", -- 2.52.0