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..295da64ff 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 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}" + ) 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.