diff --git a/CHANGELOG.md b/CHANGELOG.md index 73736e9cb..5b5311121 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -592,6 +592,16 @@ _ALL_DATA_COLUMNS + ") " "SELECT " + _ALL_DATA_COLUMNS + " FROM v3_plans"`. it to a single batch call after all docs PRs are created per cycle, eliminating merge conflicts on `examples.json` when parallel UAT workers run simultaneously. +- **Subprocess resource leak on failed initialization in StdioTransport.start()** (#11160): + The ``start()`` method now verifies the spawned subprocess is still alive immediately + after ``subprocess.Popen()`` returns. If the process died before or during initialization + (e.g., binary missing shared libraries, corrupted executable, or permission denied), the + transport calls ``stop()`` to terminate and release the process handle, then raises + ``LspError`` with the exit code. This prevents zombie process accumulation, ensures + ``is_alive`` returns ``False`` for dead processes after failed start, and eliminates leaks + in error paths where ``_process`` was previously left pointing to a terminated Popen object. + Includes BDD test coverage verifying cleanup on immediate process death. + - **`invariant_enforced` decisions not propagated to child plans on subplan spawn** (#9131): Fixed `SubplanService.spawn()` to propagate all `invariant_enforced` decisions from the parent plan's decision tree to each child plan's decision tree. Previously, child plans diff --git a/CONTRIBUTORS.md b/CONTRIBUTORS.md index 2c474f369..44dd3e8bf 100644 --- a/CONTRIBUTORS.md +++ b/CONTRIBUTORS.md @@ -120,3 +120,4 @@ Below are some specific details of individual PR contributions. * Jeffrey Phillips Freeman has contributed the `--format`/`-f` flag to `agents session tell` (issue #10466): adds JSON envelope output for machine-readable workflows alongside existing Rich console output, with Behave BDD test coverage verifying all four non-rich format paths (JSON, YAML, plain, table) and the short `-f` flag alias. * HAL 9000 has contributed the Semgrep guard for broad exception suppression (PR #9185 / issue #9103): added two new Semgrep rules (`python-no-suppressed-exception` and `python-no-suppress-exception`) to automate enforcement of error propagation guidelines, integrated Semgrep into `nox -s lint` in audit mode with migration plan for ~337 existing violations, and comprehensive BDD test coverage across all rule patterns and escape hatch scenarios. * HAL 9000 has contributed the `ProviderRegistry.FALLBACK_ORDER` fix (#10906): added the missing `ProviderType.GEMINI` to the fallback provider order list so that when only a Gemini API key is configured, the registry correctly selects it as the default provider. Includes BDD regression scenarios in `features/fallback_gemini_provider.feature`. +* HAL 9000 has contributed the StdioTransport subprocess cleanup on failed initialization fix (PR #11185 / issue #11160): after ``start()`` spawns a process with ``subprocess.Popen()``, the method now verifies the process is still alive via ``poll()``. If it died immediately during initialization, ``stop()`` is called to release the handle and an ``LspError`` with exit code is raised, preventing zombie processes and resource leaks. Includes BDD test coverage in ``features/lsp_transport_subprocess_cleanup.feature``. diff --git a/features/lsp_transport_subprocess_cleanup.feature b/features/lsp_transport_subprocess_cleanup.feature new file mode 100644 index 000000000..32cdb82fa --- /dev/null +++ b/features/lsp_transport_subprocess_cleanup.feature @@ -0,0 +1,41 @@ +@tdd_issue @tdd_issue_11160 +Feature: StdioTransport subprocess cleanup on failed initialization + As a developer maintaining the LSP transport layer + I need proper cleanup of subprocess handles when the process dies during start() + So that resource leaks are prevented and is_alive returns False for dead processes + + # ── start() successful spawn paths ───────────────────────────────── + + Scenario: start succeeds when process stays alive after Popen + Given ltcov I create a StdioTransport for command "echo" + And ltcov Popen is mocked to return an alive process with pid 9999 + When ltcov I start the transport + Then ltcov the transport should be alive + And ltcov no error should have been raised + + # ── start() — process dies immediately on spawn (the new fix) ─────── + + Scenario: start raises LspError and cleans up when process exits before init completes + Given ltcov I create a StdioTransport for command "bad_binary" + And ltcov Popen is mocked to return a process that exited with code 127 + When ltcov I try to start the transport + Then ltcov the error should be an LspError with message "died on spawn" + And ltcov the internal process should be None + + Scenario: start raises LspError with exit_code detail when process dies with code -1 + Given ltcov I create a StdioTransport for command "sigkill_binary" + And ltcov Popen is mocked to return a process that exited with code -1 + When ltcov I try to start the transport + Then ltcov the error should be an LspError with message "died on spawn" + + Scenario: start raises LspError when process returncode is zero (clean exit) + Given ltcov I create a StdioTransport for command "quick_exit" + And ltcov Popen is mocked to return a process that exited with code 0 + When ltcov I try to start the transport + Then ltcov the error should be an LspError with message "died on spawn" + + Scenario: stop() handles already-exited process and cleans up _process + Given ltcov I create a StdioTransport for command "die_on_start" + And ltcov Popen is mocked to return a process that exited with code 42 + When ltcov I try to start the transport + Then ltcov the stop handles already-exited processes correctly diff --git a/features/steps/lsp_transport_coverage_steps.py b/features/steps/lsp_transport_coverage_steps.py index ce90e33a2..5bb7fa672 100644 --- a/features/steps/lsp_transport_coverage_steps.py +++ b/features/steps/lsp_transport_coverage_steps.py @@ -315,6 +315,37 @@ def step_ltcov_invalid_json_body(context: Context) -> None: context.add_cleanup(patcher.stop) +@given("ltcov Popen is mocked to return an alive process with pid {pid:d}") +def step_ltcov_popen_alive(context: Context, pid: int) -> None: + """Mock subprocess.Popen to return a MagicMock that represents an alive process.""" + proc = _make_mock_process(poll_return=None, pid=pid) + patcher = patch( + "cleveragents.lsp.transport.subprocess.Popen", + return_value=proc, + ) + patcher.start() + context.add_cleanup(patcher.stop) + context.ltcov_mock_process = proc + + +@given("ltcov Popen is mocked to return a process that exited with code {code:d}") +def step_ltcov_popen_exited(context: Context, code: int) -> None: + """Mock subprocess.Popen to return a MagicMock that represents a process that already exited.""" + proc = _make_mock_process(poll_return=code, returncode=code, pid=77777) + context._lsp_cleanup_exit_code = code + + def side_effect(*args, **kwargs): + return proc + + patcher = patch( + "cleveragents.lsp.transport.subprocess.Popen", + side_effect=side_effect, + ) + patcher.start() + context.add_cleanup(patcher.stop) + context.ltcov_mock_process = proc + + # --------------------------------------------------------------------------- # When steps # --------------------------------------------------------------------------- @@ -328,6 +359,16 @@ def step_ltcov_try_start(context: Context) -> None: context.ltcov_error = exc +@when("ltcov I start the transport") +def step_ltcov_start(context: Context) -> None: + """Start the transport (should succeed).""" + try: + context.ltcov_transport.start() + context.ltcov_error = None + except Exception as exc: + context.ltcov_error = exc + + @when("ltcov I stop the transport") def step_ltcov_stop(context: Context) -> None: context.ltcov_result = context.ltcov_transport.stop() @@ -495,3 +536,36 @@ def step_ltcov_process_none_after_failed_start(context: Context) -> None: f"_process should be None after failed start, but got " f"{context.ltcov_transport._process}" ) + + +# --------------------------------------------------------------------------- +# Cleanup-on-failed-init step definitions (lsp_transport_subprocess_cleanup.feature) +# --------------------------------------------------------------------------- + + +@then("ltcov no error should have been raised") +def step_ltcov_no_error(context: Context) -> None: + assert context.ltcov_error is None, ( + f"Unexpected error was raised: {context.ltcov_error}" + ) + + +@then("ltcov the stop handles already-exited processes correctly") +def step_ltcov_stop_handles_exited(context: Context) -> None: + """Verify that stop() properly handled the already-dead mock process.""" + assert context.ltcov_transport._process is None, ( + "Expected _process to be None after failed start cleanup" + ) + + +@then("ltcov the error should have exit_code in details") +def step_ltcov_error_has_exit_code_detail(context: Context) -> None: + """Verify the LspError contains an 'exit_code' detail.""" + assert context.ltcov_error is not None, "Expected an error but none was raised" + assert isinstance(context.ltcov_error, LspError), ( + f"Expected LspError, got {type(context.ltcov_error).__name__}" + ) + assert context.ltcov_error.details is not None, "LspError should have details" + assert "exit_code" in context.ltcov_error.details, ( + "LspError details should contain 'exit_code'" + ) diff --git a/src/cleveragents/lsp/transport.py b/src/cleveragents/lsp/transport.py index d90c31069..c34219757 100644 --- a/src/cleveragents/lsp/transport.py +++ b/src/cleveragents/lsp/transport.py @@ -98,7 +98,9 @@ class StdioTransport: callers always receive proper error semantics. Raises: - LspError: If the process cannot be started. + LspError: If the process cannot be started, or if the spawned + process exits immediately during initialization (e.g., + corrupted binary, missing shared libraries). RuntimeError: If already started or if a post-spawn init step fails (the subprocess is cleaned up before re-raising). """ @@ -135,6 +137,23 @@ class StdioTransport: details={"command": self._command, "error": str(exc)}, ) from exc + # Verify the process is still alive after Popen succeeds. If it has + # died immediately during initialization (common when the binary is + # corrupted, missing shared libraries, etc.), clean up the leaked + # subprocess handle to prevent resource leaks and ensure is_alive + # returns False for dead processes. + if self._process.poll() is not None: + logger.warning( + "lsp.transport.process_died_on_spawn", + pid=self._process.pid, + exit_code=self._process.returncode, + ) + code = self.stop() + raise LspError( + f"LSP server process died on spawn with exit code {code}", + details={"command": self._command, "exit_code": code}, + ) + # 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.