From 8e41165223bbd97adc103b5a24f5e4e06294c013 Mon Sep 17 00:00:00 2001 From: CleverThis Date: Wed, 13 May 2026 01:19:58 +0000 Subject: [PATCH 1/3] fix(lsp): cleanup subprocess on failed initialization in StdioTransport.start() After Popen() succeeds, verify the spawned process is still alive via poll(). If it died immediately during initialization (binary corrupted, missing shared libraries, etc.), call stop() to release the handle and raise LspError with exit code. This prevents resource leaks where _process was left pointing to a terminated Popen object. ISSUES CLOSED: #11160 --- CHANGELOG.md | 10 +++ CONTRIBUTORS.md | 1 + .../lsp_transport_subprocess_cleanup.feature | 40 ++++++++++ .../steps/lsp_transport_coverage_steps.py | 79 +++++++++++++++++++ src/cleveragents/lsp/transport.py | 23 +++++- 5 files changed, 152 insertions(+), 1 deletion(-) create mode 100644 features/lsp_transport_subprocess_cleanup.feature 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..8bdd8baa9 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 #11160 / 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..869102381 --- /dev/null +++ b/features/lsp_transport_subprocess_cleanup.feature @@ -0,0 +1,40 @@ +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 returnscode 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 "dye_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..2b2c21cdc 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,41 @@ 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 the transport should be alive") +def step_ltcov_alive(context: Context) -> None: + assert context.ltcov_transport.is_alive, "Expected transport to be alive" + + +@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..31c17bbc1 100644 --- a/src/cleveragents/lsp/transport.py +++ b/src/cleveragents/lsp/transport.py @@ -98,13 +98,17 @@ 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). """ if self._process is not None and self.is_alive: raise RuntimeError("Transport already started") + from cleveragents.lsp.errors import LspError + merged_env = {**os.environ, **self._env} cmd = [self._command, *self._args] @@ -135,6 +139,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. -- 2.52.0 From 007fd66b90b0487bc5f762b37bb10117c66b6ef9 Mon Sep 17 00:00:00 2001 From: controller-ci-rerun Date: Sat, 13 Jun 2026 16:45:12 -0400 Subject: [PATCH 2/3] chore: re-trigger CI [controller] -- 2.52.0 From 10e8167ef733701708d451ffe704dd0f8579db2a Mon Sep 17 00:00:00 2001 From: CleverThis Date: Mon, 15 Jun 2026 00:39:43 -0400 Subject: [PATCH 3/3] =?UTF-8?q?fix(lsp):=20address=20reviewer=20blockers?= =?UTF-8?q?=20=E2=80=94=20remove=20duplicate=20step,=20move=20LspError=20i?= =?UTF-8?q?mport,=20fix=20metadata?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Remove duplicate @then("ltcov the transport should be alive") in features/steps/lsp_transport_coverage_steps.py; the same step is already defined at features/steps/lsp_transport_post_spawn_cleanup_steps.py:92 for issue #7044. The duplicate caused AmbiguousStep at behave load time, erroring every BDD feature in the suite (root cause of the CI unit_tests + coverage gate failures). - Remove redundant inline `from cleveragents.lsp.errors import LspError` inside StdioTransport.start(); the module-level import at line 30 is sufficient and inline imports violate the project import rule. - Update CONTRIBUTORS.md: correct the PR number from #11160 (the linked issue) to #11185 (this PR). - features/lsp_transport_subprocess_cleanup.feature: add the required `@tdd_issue @tdd_issue_11160` tags per the TDD bug-fix workflow, fix the `returnscode` typo in a scenario name, and rename the `dye_on_start` command label to `die_on_start`. ISSUES CLOSED: #11160 --- CONTRIBUTORS.md | 2 +- features/lsp_transport_subprocess_cleanup.feature | 5 +++-- features/steps/lsp_transport_coverage_steps.py | 5 ----- src/cleveragents/lsp/transport.py | 2 -- 4 files changed, 4 insertions(+), 10 deletions(-) diff --git a/CONTRIBUTORS.md b/CONTRIBUTORS.md index 8bdd8baa9..44dd3e8bf 100644 --- a/CONTRIBUTORS.md +++ b/CONTRIBUTORS.md @@ -120,4 +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 #11160 / 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``. +* 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 index 869102381..32cdb82fa 100644 --- a/features/lsp_transport_subprocess_cleanup.feature +++ b/features/lsp_transport_subprocess_cleanup.feature @@ -1,3 +1,4 @@ +@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() @@ -27,14 +28,14 @@ Feature: StdioTransport subprocess cleanup on failed initialization 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 returnscode is zero (clean exit) + 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 "dye_on_start" + 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 2b2c21cdc..5bb7fa672 100644 --- a/features/steps/lsp_transport_coverage_steps.py +++ b/features/steps/lsp_transport_coverage_steps.py @@ -543,11 +543,6 @@ def step_ltcov_process_none_after_failed_start(context: Context) -> None: # --------------------------------------------------------------------------- -@then("ltcov the transport should be alive") -def step_ltcov_alive(context: Context) -> None: - assert context.ltcov_transport.is_alive, "Expected transport to be alive" - - @then("ltcov no error should have been raised") def step_ltcov_no_error(context: Context) -> None: assert context.ltcov_error is None, ( diff --git a/src/cleveragents/lsp/transport.py b/src/cleveragents/lsp/transport.py index 31c17bbc1..c34219757 100644 --- a/src/cleveragents/lsp/transport.py +++ b/src/cleveragents/lsp/transport.py @@ -107,8 +107,6 @@ class StdioTransport: if self._process is not None and self.is_alive: raise RuntimeError("Transport already started") - from cleveragents.lsp.errors import LspError - merged_env = {**os.environ, **self._env} cmd = [self._command, *self._args] -- 2.52.0