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 <plan-id> [<checkpoint-id>]` 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

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 <plan-id> [<checkpoint-id>]` 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).

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

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 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.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.
+
+    from cleveragents.lsp import transport as _transport  # local import
+
+    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
+
+    patcher = patch.object(_transport.logger, "info", side_effect=mock_info)
+    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}"
+    )

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.