From ed69291417a3d3c23a33cb438e2ab8d097790ceb Mon Sep 17 00:00:00 2001 From: CleverThis Date: Tue, 12 May 2026 17:17:42 +0000 Subject: [PATCH 1/2] fix(lsp): cleanup subprocess on failed initialization in StdioTransport.start() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After Popen succeeds, any exception in post-Popen code (e.g. logger.info) would leave the spawned process running untracked — a resource leak. The change wraps the post-Popen logger.info call in a try/except that calls stop() to clean up the process before re-raising. Two new Behave scenarios added: - "ltcov start cleans up subprocess when post-popen logging fails" - "ltcov start does not leak process when post-pogen succeeds normally" Closes #7044 --- CHANGELOG.md | 2 + CONTRIBUTORS.md | 2 + features/lsp_transport_coverage.feature | 14 ++++ .../steps/lsp_transport_coverage_steps.py | 71 +++++++++++++++++++ src/cleveragents/lsp/transport.py | 24 +++++-- 5 files changed, 108 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5cf3abfa7..70d4eef99 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,6 +43,8 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). - **Plan Rollback Command** (#8557): Implemented `agents plan rollback []` for checkpoint-based plan state restoration in Epic #8493. The command restores a plan's sandbox to the state captured at a given checkpoint, discarding all decisions made after that checkpoint. The checkpoint can be specified as an optional positional second argument or via the `--to-checkpoint` named option. Supports `--yes/-y` flag to skip confirmation prompts and `--format/-f` for output format selection (rich/plain/json/yaml). Included with comprehensive BDD test coverage (>= 97%) and spec-aligned output formatting showing rollback summary, changes reverted, impact analysis, and post-rollback state panels. ### Fixed +- **Subprocess leaked when post-popen logging fails in ``StdioTransport.start()``** (#7044): After ``subprocess.Popen`` succeeds, any subsequent exception (e.g. from structlog's ``logger.info``) would leave the spawned subprocess running untracked — a resource leak. Wrapping the post-Popen ``logger.info`` call in its own ``try/except`` ensures ``stop()`` is called to clean up the process before re-raising, preventing zombie LSP server processes. BDD test coverage added via two new scenarios in ``features/lsp_transport_coverage.feature``. + - **Guard cleanup_stale against execute/processing and execute/complete plans** (#11121): ``_create_sandbox_for_plan()`` in ``src/cleveragents/cli/commands/plan.py`` now skips ``GitWorktreeSandbox.cleanup_stale()`` when the plan is in diff --git a/CONTRIBUTORS.md b/CONTRIBUTORS.md index 735c33416..5d726e5c6 100644 --- a/CONTRIBUTORS.md +++ b/CONTRIBUTORS.md @@ -43,3 +43,5 @@ Below are some of the specific details of various contributions. * HAL 9000 has contributed database resource types (PostgreSQL, SQLite) with transaction-based sandbox strategy: implemented ``DatabaseResourceHandler`` providing full CRUD operations (`read`, `write`, `delete`, `list_children`) and connection validation with automatic credential masking for PostgreSQL and SQLite backends. Includes ``TransactionSandbox`` infrastructure wired into ``SandboxFactory``, BDD test coverage in ``features/database_resources.feature``, and Robot Framework integration tests in ``robot/database_resources.robot`` (PR #10591 / issue #8608, Epic #8568). * HAL 9000 has contributed the agents plan rollback command (PR #8674 / issue #8557): implemented checkpoint-based plan state restoration with the `agents plan rollback []` CLI command as part of Epic #8493, enabling plans to be restored to previous checkpoints, discarding post-checkpoint decisions, and resuming execution from the rolled-back state. Supported by `--yes/-y`, `--to-checkpoint`, and `--format/-f` flags. Includes comprehensive BDD test coverage (>= 97%) for rollback, decision discarding, and plan resume functionality. * HAL 9000 has contributed the PyYAML security upgrade (PR #11012 / issue #9055): added `pyyaml>=6.0.3` dependency constraint to address known YAML parsing vulnerabilities. + +* CleverThis (hal9000@cleverthis.com) has contributed a fix for LSP transport subprocess leak during failed initialization — wrapping post-Popen logging in a try/except guard to ensure ``stop()`` is called before re-raising, preventing zombie language server processes (#10597). diff --git a/features/lsp_transport_coverage.feature b/features/lsp_transport_coverage.feature index c5c9aaff7..cd8c85847 100644 --- a/features/lsp_transport_coverage.feature +++ b/features/lsp_transport_coverage.feature @@ -23,6 +23,20 @@ Feature: LSP StdioTransport coverage When ltcov I try to start the transport Then ltcov the error should be an LspError with message "Failed to start" + Scenario: ltcov start cleans up subprocess when post-popen logging fails + Given ltcov I create a StdioTransport for command "echo" + And ltcov Popen is mocked to return a process with pid 99999 + And ltcov logger.info on second call raises AttributeError + When ltcov I try to start the transport + Then ltcov the error should be an AttributeError + And ltcov the stop method should have been called on the mock process + + Scenario: ltcov start does not leak process when post-popen succeeds normally + Given ltcov I create a StdioTransport for command "echo" + And ltcov Popen is mocked to return a process with pid 88888 + When ltcov I try to start the transport + Then ltcov no error should occur + # ── stop() paths ──────────────────────────────────────────────── Scenario: ltcov stop returns None when not started diff --git a/features/steps/lsp_transport_coverage_steps.py b/features/steps/lsp_transport_coverage_steps.py index 7a0fefa3b..f15b4b28d 100644 --- a/features/steps/lsp_transport_coverage_steps.py +++ b/features/steps/lsp_transport_coverage_steps.py @@ -98,6 +98,50 @@ def step_ltcov_popen_oserror(context: Context, msg: str) -> None: context.add_cleanup(patcher.stop) +@given('ltcov Popen is mocked to return a process with pid {pid:d}') +def step_ltcov_popen_returns_process(context: Context, pid: int) -> None: + mock_proc = _make_mock_process(pid=pid) + patcher = patch( + "cleveragents.lsp.transport.subprocess.Popen", + return_value=mock_proc, + ) + patcher.start() + context.add_cleanup(patcher.stop) + + +@given('ltcov logger.info on second call raises {exception_name}') +def step_ltcov_logger_error(context: Context, exception_name: str) -> None: + """Patch logger.info so first call works (starting), second fails.""" + + def logger_side_effect(msg: str, **kwargs: object) -> None: + if msg == "lsp.transport.starting": + return # First call - log start + else: # lsp.transport.started or any other — raise + exc_classes = { + "AttributeError": AttributeError("logger broken"), + "RuntimeError": RuntimeError("logger broken"), + "ValueError": ValueError("logger broken"), + } + raise exc_classes.get(exception_name, Exception(exception_name)) + + import structlog as _structlog # local import to avoid side effects + + original_get_logger = _structlog.get_logger + + def mock_get_logger(*args: object, **kwargs: object) -> MagicMock: + mock_logger = MagicMock() + mock_logger.info.side_effect = logger_side_effect + return mock_logger + + patcher = patch.object( + _structlog, + "get_logger", + side_effect=mock_get_logger, + ) + patcher.start() + context.add_cleanup(patcher.stop) + + @given("ltcov the transport has a mock process that already exited with code {code:d}") def step_ltcov_exited_process(context: Context, code: int) -> None: proc = _make_mock_process(poll_return=code, returncode=code) @@ -512,3 +556,30 @@ def step_ltcov_value_error(context: Context, fragment: str) -> None: assert fragment in str(context.ltcov_error), ( f"Expected '{fragment}' in '{context.ltcov_error}'" ) + + +@then("ltcov the error should be an AttributeError") +def step_ltcov_attribute_error(context: Context) -> None: + """Verify that an AttributeError was raised.""" + assert context.ltcov_error is not None, "Expected an error but none was raised" + assert isinstance(context.ltcov_error, AttributeError), ( + f"Expected AttributeError, got {type(context.ltcov_error).__name__}" + ) + + +@then("ltcov the stop method should have been called on the mock process") +def step_ltcov_stop_called(context: Context) -> None: + """Verify stop() was invoked during cleanup.""" + trans = context.ltcov_transport + # After the error, _process should be None (stop cleared it) + assert trans._process is None, ( + "Expected _process to be None after post-popen cleanup" + ) + + +@then("ltcov no error should occur") +def step_ltcov_no_error(context: Context) -> None: + """Verify start() completed without raising.""" + assert context.ltcov_error is None, ( + f"Expected no error but got {type(context.ltcov_error).__name__}: {context.ltcov_error}" + ) diff --git a/src/cleveragents/lsp/transport.py b/src/cleveragents/lsp/transport.py index c85262cff..51222b65c 100644 --- a/src/cleveragents/lsp/transport.py +++ b/src/cleveragents/lsp/transport.py @@ -85,6 +85,10 @@ class StdioTransport: def start(self) -> None: """Spawn the LSP server subprocess. + Ensures that if any error occurs after ``Popen`` succeeds but before + the method returns normally (e.g. logging raises), the spawned + subprocess is cleaned up to avoid leaking a background process. + Raises: LspError: If the process cannot be started. RuntimeError: If already started. @@ -126,11 +130,21 @@ class StdioTransport: details={"command": self._command, "error": str(exc)}, ) from exc - logger.info( - "lsp.transport.started", - pid=self._process.pid, - command=self._command, - ) + # ── Post-Popen cleanup guard ──────────────────────────────────── + # If anything between Popen returning and here raises, the subprocess + # will leak. Wrap the remainder in a try/except so we call stop() + # before propagating the exception. + try: + logger.info( + "lsp.transport.started", + pid=self._process.pid, + command=self._command, + ) + except Exception: + # Popen succeeded but post-init code failed — reclaim the process. + logger.exception("lsp.transport.post_start_logging_failed") + self.stop() + raise def stop(self, timeout: float = _GRACEFUL_SHUTDOWN_TIMEOUT) -> int | None: """Terminate the subprocess gracefully, then force-kill if needed. -- 2.52.0 From f0d2db692a5e14ca770f0f101fe7a719da7c81c3 Mon Sep 17 00:00:00 2001 From: Jeffrey Phillips Freeman Date: Wed, 13 May 2026 00:15:17 +0000 Subject: [PATCH 2/2] fix(lsp): fix transport coverage BDD step for post-popen logger mock Patch the already-instantiated transport.logger.info directly instead of patching structlog.get_logger(), which takes effect too late after module-level import. Targeted exception-raising scope to only the 'lsp.transport.started' message so downstream stop() logging (stopping, stopped) succeeds and allows cleanup to complete. Also removed unused variable 'original_get_logger' that triggered F841 lint error. ISSUES CLOSED: #7044 --- .../steps/lsp_transport_coverage_steps.py | 32 +++++++++---------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/features/steps/lsp_transport_coverage_steps.py b/features/steps/lsp_transport_coverage_steps.py index f15b4b28d..295da64ff 100644 --- a/features/steps/lsp_transport_coverage_steps.py +++ b/features/steps/lsp_transport_coverage_steps.py @@ -111,33 +111,33 @@ def step_ltcov_popen_returns_process(context: Context, pid: int) -> None: @given('ltcov logger.info on second call raises {exception_name}') def step_ltcov_logger_error(context: Context, exception_name: str) -> None: - """Patch logger.info so first call works (starting), second fails.""" + """Patch the already-instantiated transport logger so first log call + (“starting”) succeeds, the second (started in start()) raises the + exception, and subsequent logging calls from stop()/other methods + succeed so cleanup can complete.""" + + _log_call_count: list[int] = [0] def logger_side_effect(msg: str, **kwargs: object) -> None: - if msg == "lsp.transport.starting": - return # First call - log start - else: # lsp.transport.started or any other — raise + if msg == "lsp.transport.started": exc_classes = { "AttributeError": AttributeError("logger broken"), "RuntimeError": RuntimeError("logger broken"), "ValueError": ValueError("logger broken"), } raise exc_classes.get(exception_name, Exception(exception_name)) + # All other log messages (starting, stopping, stopped, force_killing, + # post_start_logging_failed) are allowed through so cleanup runs. - import structlog as _structlog # local import to avoid side effects + from cleveragents.lsp import transport as _transport # local import - original_get_logger = _structlog.get_logger + def mock_info(msg: str, **kwargs: object) -> None: + if msg == "lsp.transport.started": + logger_side_effect(msg, **kwargs) + else: + pass # allow all other log messages through - def mock_get_logger(*args: object, **kwargs: object) -> MagicMock: - mock_logger = MagicMock() - mock_logger.info.side_effect = logger_side_effect - return mock_logger - - patcher = patch.object( - _structlog, - "get_logger", - side_effect=mock_get_logger, - ) + patcher = patch.object(_transport.logger, "info", side_effect=mock_info) patcher.start() context.add_cleanup(patcher.stop) -- 2.52.0