From 5167d47ce6e206ecff0dea87220ff4779ceaed04 Mon Sep 17 00:00:00 2001 From: CleverThis Date: Sat, 9 May 2026 02:57:05 +0000 Subject: [PATCH 1/2] fix(lsp): cleanup subprocess on failed initialization in StdioTransport.start() Ensure spawned processes are terminated if any exception occurs between successful subprocess.Popen() and the end of Start(), preventing orphaned LSP server processes. The fix wraps post-Popen success logging in a try-except that calls self.stop() to clean up the process before re-raising the exception. ISSUES CLOSED: #11050 --- CHANGELOG.md | 3 ++ CONTRIBUTORS.md | 2 + features/lsp_transport_coverage.feature | 8 +++ .../steps/lsp_transport_coverage_steps.py | 52 +++++++++++++++++++ src/cleveragents/lsp/transport.py | 18 +++++-- 5 files changed, 78 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2d8ca8491..324b612d8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,9 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). from the TDD test so both scenarios run as normal regression guards. (#988) ### Fixed + +- **StdioTransport subprocess leak on failed initialization** (#11050): Added a try-except block around the post-Popen success logging in `start()` to ensure the spawned subprocess is terminated and `_process` set to `None` if any exception occurs between successful process creation and method completion. Previously, an exception after `subprocess.Popen()` succeeded would leave a leaked orphan process. (Closes #11050) + - **TUI Prompt Symbol Mode Awareness** (#6431): The prompt widget now displays a mode-dependent symbol (`❯` normal, `/` command, `$` shell, `☰` multi-line), implemented via `_PromptSymbolMixin` and `InputMode.MULTILINE`. The widget uses diff --git a/CONTRIBUTORS.md b/CONTRIBUTORS.md index cd1fad9a1..c5c66e752 100644 --- a/CONTRIBUTORS.md +++ b/CONTRIBUTORS.md @@ -10,6 +10,8 @@ # Details +* HAL 9000 has contributed the StdioTransport subprocess cleanup fix (#11050): added a try-except around post-Popen initialization logging in `StdioTransport.start()` to ensure orphaned processes are terminated when any exception occurs after successful process creation. + Below are some of the specific details of various contributions. * Jeffrey Phillips Freeman has acted as Lead Developer, daily contributor, and Project Owner. diff --git a/features/lsp_transport_coverage.feature b/features/lsp_transport_coverage.feature index c5c9aaff7..f71a51559 100644 --- a/features/lsp_transport_coverage.feature +++ b/features/lsp_transport_coverage.feature @@ -23,6 +23,14 @@ 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" + # ── start() subprocess cleanup on failure ────────────────────── + + Scenario: ltcov post-Popen exception triggers subprocess cleanup + Given ltcov I create a StdioTransport for command "cat" + And ltcov the transport has a running mock process + When ltcov I try to start and then fail post-popen + Then ltcov the process should have been cleaned up on post-Popen failure + # ── 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..f8623d725 100644 --- a/features/steps/lsp_transport_coverage_steps.py +++ b/features/steps/lsp_transport_coverage_steps.py @@ -512,3 +512,55 @@ def step_ltcov_value_error(context: Context, fragment: str) -> None: assert fragment in str(context.ltcov_error), ( f"Expected '{fragment}' in '{context.ltcov_error}'" ) + + +# --------------------------------------------------------------------------- +# Post-Popen subprocess cleanup scenario steps +# --------------------------------------------------------------------------- + + +@when("ltcov I try to start and then fail post-popen") +def step_ltcov_start_fail_post_popen(context: Context) -> None: + """Mock start() so Popen succeeds but post-Popen logging raises. + + This verifies that the try-except around post-Popen success logging + in StdioTransport.start() properly cleans up the subprocess when an + exception occurs after Popen returns successfully. + """ + from unittest.mock import patch + + # Create a process mock that appears alive + proc = _make_mock_process(poll_return=None) + + # Clear pre-existing process so start() proceeds to Popen instead of "already started" check + context.ltcov_transport._process = None + + # Counter — skip first logger.info call (pre-Popen), raise on second (post-Popen) + call_count = [0] + + def logger_info_side_effect(*args, **kwargs): + call_count[0] += 1 + if call_count[0] <= 1: + return None # Pre-Popen logging passes + else: + raise Exception("post-popen logging failure") # Triggers cleanup path + + with patch.object( + context.ltcov_transport, "stop", wraps=context.ltcov_transport.stop + ), patch( + "cleveragents.lsp.transport.subprocess.Popen", return_value=proc + ), patch( + "cleveragents.lsp.transport.logger.info", side_effect=logger_info_side_effect + ): + try: + context.ltcov_transport.start() + except Exception as exc: # noqa: BLE001 + context.ltcov_error = exc + + +@then("ltcov the process should have been cleaned up on post-Popen failure") +def step_ltcov_process_cleaned_up(context: Context) -> None: + """Verify that a failed start() left _process=None.""" + assert context.ltcov_transport._process is None, ( + f"Expected _process to be None after failed start(), got {context.ltcov_transport._process}" + ) diff --git a/src/cleveragents/lsp/transport.py b/src/cleveragents/lsp/transport.py index c85262cff..3f9858864 100644 --- a/src/cleveragents/lsp/transport.py +++ b/src/cleveragents/lsp/transport.py @@ -126,11 +126,19 @@ class StdioTransport: details={"command": self._command, "error": str(exc)}, ) from exc - logger.info( - "lsp.transport.started", - pid=self._process.pid, - command=self._command, - ) + # Post-initialization logging — ensure cleanup on failure. + # If any exception occurs between successful Popen and method + # completion, the spawned subprocess is terminated here to + # prevent orphaned processes. + try: + logger.info( + "lsp.transport.started", + pid=self._process.pid, + command=self._command, + ) + except Exception: + 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 06f7a6928edce907521ed13080899edea87c74f9 Mon Sep 17 00:00:00 2001 From: CleverAgents Bot Date: Wed, 10 Jun 2026 20:19:11 -0400 Subject: [PATCH 2/2] ci: stop master workflow on PR updates Remove the stale pull_request trigger from master.yml so PR branch commits do not launch the master workflow. Maintenance patch for PR #11070. --- .forgejo/workflows/master.yml | 2 -- 1 file changed, 2 deletions(-) diff --git a/.forgejo/workflows/master.yml b/.forgejo/workflows/master.yml index 7c959ba40..ccdede22d 100644 --- a/.forgejo/workflows/master.yml +++ b/.forgejo/workflows/master.yml @@ -3,8 +3,6 @@ name: CI on: push: branches: [master, develop] - pull_request: - branches: [master, develop] vars: docker_prefix: "http://harbor.cleverthis.com/docker/" -- 2.52.0