fix: discard unconsumed rshell server output

README.md

@@ -1166,6 +1166,10 @@ If code cannot establish connection, it will start deployment of python using [D
 
 `RShellConnection` is a connection type that leverages `httplib.client` library to establish and manage connections using RESTful API over HTTP protocol in EFI Shell environment. `rshell_client.py` must be present on the EFI Shell target system.
 
+When `RShellConnection` starts a local server automatically, its stdout and stderr are discarded
+to prevent blocking on unread pipes. Command results still return through the HTTP API.
+For server diagnostics, start the server manually with output directed to a terminal or a managed log file.
+
 `rshell_server.py` is a server script that needs to be executed on the host machine to facilitate communication between the host and the EFI Shell target system. Server works on queue with address ip -> command to execute, it provides a RESTful API that allows the host to send commands and receive responses from the EFI Shell:
 
   * `/execute_command` - Endpoint to execute commands on the EFI Shell target system:

mfd_connect/rshell.py

@@ -125,14 +125,18 @@ def _run_server(self) -> LocalProcess:
         """Run RShell server locally."""
         conn = LocalConnection()
         conn.enable_sudo()
-        process: LocalProcess = conn.start_process(f"{conn.modules().sys.executable} -m mfd_connect.rshell_server")
+        process: LocalProcess = conn.start_process(
+            f"{conn.modules().sys.executable} -m mfd_connect.rshell_server",
+            discard_stdout=True,
+            discard_stderr=True,
+        )
         conn.disable_sudo()
         timeout = TimeoutCounter(1)
         while not timeout:
             if not process.running:
                 raise RuntimeError(
                     f"RShell server failed to start. Exit code: {process.return_code}\n"
-                    f"STDOUT: {process.stdout_text}\nSTDERR: {process.stderr_text}"
+                    "Server stdout and stderr are discarded. Run the server manually to inspect its diagnostics."
                 )
         return process
 
@@ -389,9 +393,3 @@ def stop_server(self) -> None:
                 raise RuntimeError("RShell server did not stop within timeout")
 
             logger.log(level=log_levels.MODULE_DEBUG, msg="RShell server stopped")
-            logger.log(
-                level=log_levels.MODULE_DEBUG,
-                msg=self.server_process.stdout_text
-                if self.server_process.stdout_text
-                else "RShell server had no stdout",
-            )

tests/unit/test_mfd_connect/test_rshell.py

@@ -5,6 +5,7 @@
 import sys
 import types
 import runpy
+import shlex
 from pathlib import Path
 from subprocess import CalledProcessError
 from unittest.mock import Mock, patch
@@ -15,6 +16,7 @@
 from mfd_typing.os_values import OSBitness, OSName, OSType
 
 from mfd_connect.base import ConnectionCompletedProcess
+from mfd_connect.local import LocalConnection
 from mfd_connect.exceptions import ConnectionCalledProcessError, OsNotSupported
 from mfd_connect.rshell import (
     PLATFORM_POWER_TRANSITION_DELAY_SECONDS,
@@ -147,7 +149,37 @@ def __bool__(self):
         result = rshell._run_server()
 
         assert result == process
-        conn.start_process.assert_called_once_with("python -m mfd_connect.rshell_server")
+        conn.start_process.assert_called_once_with(
+            "python -m mfd_connect.rshell_server", discard_stdout=True, discard_stderr=True
+        )
+
+    @pytest.mark.skipif(sys.platform == "win32", reason="POSIX pipe regression")
+    def test_run_server_discards_output_larger_than_pipe_capacity(self, rshell, mocker, tmp_path):
+        script = tmp_path / "verbose_server.py"
+        script.write_text("import sys\nsys.stdout.write('x' * 1048576)\nsys.stderr.write('x' * 1048576)\n")
+        conn = LocalConnection()
+        start_process = conn.start_process
+        command = f"{shlex.quote(sys.executable)} {shlex.quote(str(script))}"
+        mocker.patch.object(conn, "enable_sudo")
+        mocker.patch.object(conn, "disable_sudo")
+        mocker.patch.object(conn, "start_process", side_effect=lambda _, **kwargs: start_process(command, **kwargs))
+        mocker.patch("mfd_connect.rshell.LocalConnection", return_value=conn)
+        mocker.patch("mfd_connect.rshell.TimeoutCounter", return_value=True)
+
+        process = rshell._run_server()
+        try:
+            assert process.wait(timeout=5) == 0
+            rshell.server_process = process
+            mocker.patch(
+                "mfd_connect.rshell.requests.post", return_value=Mock(status_code=200, text="", headers={"rc": "0"})
+            )
+            kill_process = mocker.patch("mfd_connect.rshell.kill_process_by_pid")
+            mocker.patch("mfd_connect.rshell.TimeoutCounter", return_value=False)
+            rshell.disconnect(stop_client=True, stop_server=True)
+            kill_process.assert_called_once_with(conn, process.pid)
+        finally:
+            if process.running:
+                process.stop(wait=5)
 
     def test_run_server_timeout_loop_executes(self, rshell, mocker):
         """Test that the timeout loop in _run_server executes at least once."""
@@ -186,8 +218,10 @@ def test_run_server_server_not_running_raises_error(self, rshell, mocker):
         process = mocker.Mock()
         process.running = False  # Process crashed
         process.return_code = 1
-        process.stdout_text = "stdout output"
-        process.stderr_text = "stderr output"
+        stdout = mocker.PropertyMock(side_effect=AssertionError("Discarded stdout must not be read"))
+        stderr = mocker.PropertyMock(side_effect=AssertionError("Discarded stderr must not be read"))
+        type(process).stdout_text = stdout
+        type(process).stderr_text = stderr
 
         conn = mocker.Mock()
         conn.modules().sys.executable = "python"
@@ -212,6 +246,8 @@ def __bool__(self):
 
         with pytest.raises(RuntimeError, match="RShell server failed to start.*Exit code: 1"):
             rshell._run_server()
+        stdout.assert_not_called()
+        stderr.assert_not_called()
 
     def test_type_checking_import_block_executes(self, monkeypatch):
         class _FakeBaseModel:
@@ -508,11 +544,16 @@ def test_stop_server_with_and_without_process(self, rshell, mocker):
         server_process = mocker.Mock()
         server_process.pid = 1234
         server_process.running = False
-        server_process.stdout_text = "server out"
+        stdout = mocker.PropertyMock(side_effect=AssertionError("Discarded stdout must not be read"))
+        stderr = mocker.PropertyMock(side_effect=AssertionError("Discarded stderr must not be read"))
+        type(server_process).stdout_text = stdout
+        type(server_process).stderr_text = stderr
         rshell.server_process = server_process
 
         rshell.stop_server()
 
         local_connection.enable_sudo.assert_called_once()
         kill_process_by_pid.assert_called_once_with(local_connection, 1234)
         local_connection.disable_sudo.assert_called_once()
+        stdout.assert_not_called()
+        stderr.assert_not_called()