fix: isolate subprocess stdin and decouple startup to prevent MCP stdio deadlocks
- Set stdin=subprocess.DEVNULL across runtime, client, and utility subprocess calls to avoid hijacking the parent process's stdin. - Run awake service initialization asynchronously on a daemon thread in MCP server startup. - Redirect logger header printing to stderr to ensure stdio JSON-RPC purity. - Add unit and lifecycle tests for MCP stdio protocol handshake and stdin isolation.
This commit is contained in:
1 parent
8fdecf23d7
commit
e984d17635
12 files changed
+237
-13
No files matched your search
@@ -174,6 +174,7 @@ def _is_package_installed(device_id: str, pkg: str) -> bool:
|
||||
try:
|
||||
result = subprocess.run(
|
||||
["adb", "-s", device_id, "shell", "pm", "list", "packages"],
|
||||
stdin=subprocess.DEVNULL,
|
||||
capture_output=True,
|
||||
text=True,
|
||||
timeout=10,
|
||||
@@ -208,6 +209,7 @@ def _uninstall_package(device_id: str, pkg: str) -> bool:
|
||||
"0",
|
||||
pkg,
|
||||
],
|
||||
stdin=subprocess.DEVNULL,
|
||||
capture_output=True,
|
||||
text=True,
|
||||
timeout=30,
|
||||
@@ -379,6 +381,7 @@ class UIAutomatorClient:
|
||||
try:
|
||||
result = subprocess.run(
|
||||
["adb", "-s", self._device_id, "exec-out", "screencap", "-p"],
|
||||
stdin=subprocess.DEVNULL,
|
||||
capture_output=True,
|
||||
timeout=10,
|
||||
check=True,
|
||||
|
||||
@@ -19,6 +19,7 @@ import os
|
||||
from pathlib import Path
|
||||
import re
|
||||
import sys
|
||||
import threading
|
||||
import tomllib
|
||||
from typing import Annotated
|
||||
|
||||
@@ -724,7 +725,9 @@ def mcp_command(
|
||||
console.print(syntax)
|
||||
raise typer.Exit(0)
|
||||
|
||||
start_awake_service()
|
||||
threading.Thread(
|
||||
target=start_awake_service, daemon=True, name="artemis-awake-init"
|
||||
).start()
|
||||
try:
|
||||
st = server_type.lower()
|
||||
if st in ("agent", "mobile", "artemis", "default"):
|
||||
|
||||
@@ -153,6 +153,7 @@ class ScreenAwakeLease:
|
||||
try:
|
||||
result = subprocess.run(
|
||||
["adb", "-s", self.device_id, *args],
|
||||
stdin=subprocess.DEVNULL,
|
||||
capture_output=True,
|
||||
text=True,
|
||||
timeout=10,
|
||||
|
||||
@@ -48,6 +48,7 @@ def _run_awake_adb_command(
|
||||
try:
|
||||
result = subprocess.run(
|
||||
["adb", "-s", device_id, *args],
|
||||
stdin=subprocess.DEVNULL,
|
||||
capture_output=True,
|
||||
text=True,
|
||||
timeout=10,
|
||||
@@ -185,21 +186,25 @@ class ScreenAwakeService:
|
||||
self._heartbeat_threads: dict[str, threading.Thread] = {}
|
||||
self._monitor_stop: threading.Event | None = None
|
||||
self._monitor_thread: threading.Thread | None = None
|
||||
self._shutdown_requested = False
|
||||
|
||||
def start(self, device_ids: Iterable[str] | None = None) -> dict[str, str | None]:
|
||||
if self._shutdown_requested:
|
||||
return {}
|
||||
selected = list(device_ids) if device_ids is not None else _discover_connected_device_ids()
|
||||
results = {
|
||||
device_id: self.ensure_device(device_id)
|
||||
for device_id in dict.fromkeys(selected)
|
||||
if device_id
|
||||
}
|
||||
if _awake_enabled() and os.environ.get("ARTEMIS_CLOUD_MODE") != "1":
|
||||
results = {}
|
||||
for device_id in dict.fromkeys(selected):
|
||||
if self._shutdown_requested:
|
||||
break
|
||||
if device_id:
|
||||
results[device_id] = self.ensure_device(device_id)
|
||||
if _awake_enabled() and os.environ.get("ARTEMIS_CLOUD_MODE") != "1" and not self._shutdown_requested:
|
||||
self._start_device_monitor()
|
||||
return results
|
||||
|
||||
def ensure_device(self, device_id: str) -> str | None:
|
||||
"""Configure one device once per process; Android owns unplug behavior."""
|
||||
if not device_id or not _awake_enabled() or os.environ.get("ARTEMIS_CLOUD_MODE") == "1":
|
||||
if not device_id or not _awake_enabled() or os.environ.get("ARTEMIS_CLOUD_MODE") == "1" or self._shutdown_requested:
|
||||
return None
|
||||
with self._lock:
|
||||
existing = self._strategies.get(device_id)
|
||||
@@ -277,6 +282,7 @@ class ScreenAwakeService:
|
||||
|
||||
def shutdown(self) -> None:
|
||||
"""Stop host work; unplug behavior remains owned by Android's USB policy."""
|
||||
self._shutdown_requested = True
|
||||
with self._lock:
|
||||
monitor_stop = self._monitor_stop
|
||||
monitor_thread = self._monitor_thread
|
||||
|
||||
@@ -81,6 +81,7 @@ class DevicePool:
|
||||
try:
|
||||
res = subprocess.run(
|
||||
[adb, "devices", "-l"],
|
||||
stdin=subprocess.DEVNULL,
|
||||
capture_output=True,
|
||||
text=True,
|
||||
timeout=timeout,
|
||||
|
||||
@@ -135,6 +135,7 @@ def find_server_pids(port: int = 8000) -> list[int]:
|
||||
try:
|
||||
res = subprocess.run(
|
||||
["lsof", "-ti", f":{port}", "-sTCP:LISTEN"],
|
||||
stdin=subprocess.DEVNULL,
|
||||
capture_output=True,
|
||||
text=True,
|
||||
timeout=2.0,
|
||||
@@ -152,6 +153,7 @@ def find_server_pids(port: int = 8000) -> list[int]:
|
||||
try:
|
||||
res = subprocess.run(
|
||||
["fuser", f"{port}/tcp"],
|
||||
stdin=subprocess.DEVNULL,
|
||||
capture_output=True,
|
||||
text=True,
|
||||
timeout=2.0,
|
||||
@@ -168,6 +170,7 @@ def find_server_pids(port: int = 8000) -> list[int]:
|
||||
try:
|
||||
res = subprocess.run(
|
||||
["netstat", "-ano"],
|
||||
stdin=subprocess.DEVNULL,
|
||||
capture_output=True,
|
||||
text=True,
|
||||
timeout=3.0,
|
||||
|
||||
@@ -168,9 +168,9 @@ class ArtemisLogger:
|
||||
colored_separator = f"{Fore.CYAN}{separator}{Style.RESET_ALL}"
|
||||
colored_message = f"{Fore.CYAN}{message}{Style.RESET_ALL}"
|
||||
|
||||
print(colored_separator)
|
||||
print(colored_message)
|
||||
print(colored_separator)
|
||||
print(colored_separator, file=sys.stderr)
|
||||
print(colored_message, file=sys.stderr)
|
||||
print(colored_separator, file=sys.stderr)
|
||||
self.logger.info(f"\n{separator}\n{message}\n{separator}")
|
||||
|
||||
|
||||
|
||||
@@ -20,6 +20,7 @@ def run_shell_command_on_host(command: str) -> str:
|
||||
try:
|
||||
result = subprocess.run(
|
||||
command,
|
||||
stdin=subprocess.DEVNULL,
|
||||
shell=True,
|
||||
check=True,
|
||||
capture_output=True,
|
||||
|
||||
@@ -59,9 +59,13 @@ import mcp_server.tools # noqa: F401
|
||||
from artemis.runtime import shutdown_awake_service, start_awake_service
|
||||
|
||||
|
||||
import threading
|
||||
|
||||
def main(transport: str = "stdio", host: str = "127.0.0.1", port: int = 8001):
|
||||
"""Main entrypoint to run the Artemis Mobile Agent MCP server."""
|
||||
start_awake_service()
|
||||
threading.Thread(
|
||||
target=start_awake_service, daemon=True, name="artemis-awake-init"
|
||||
).start()
|
||||
try:
|
||||
if transport.lower() == "sse":
|
||||
mcp.run(transport="sse", host=host, port=port)
|
||||
|
||||
@@ -99,6 +99,7 @@ def get_connected_devices(adb_path: str | None = None) -> list[str]:
|
||||
try:
|
||||
res = subprocess.run(
|
||||
[adb, "devices"],
|
||||
stdin=subprocess.DEVNULL,
|
||||
capture_output=True,
|
||||
text=True,
|
||||
timeout=5,
|
||||
@@ -137,7 +138,7 @@ def ensure_emulator(
|
||||
if not emu_exe or not os.path.exists(emu_exe):
|
||||
return False
|
||||
|
||||
popen_kwargs = {"stdout": subprocess.DEVNULL, "stderr": subprocess.DEVNULL}
|
||||
popen_kwargs = {"stdin": subprocess.DEVNULL, "stdout": subprocess.DEVNULL, "stderr": subprocess.DEVNULL}
|
||||
if sys.platform == "win32":
|
||||
popen_kwargs["creationflags"] = (
|
||||
subprocess.CREATE_NEW_PROCESS_GROUP | subprocess.DETACHED_PROCESS # type: ignore[attr-defined]
|
||||
@@ -158,6 +159,7 @@ def ensure_emulator(
|
||||
try:
|
||||
res = subprocess.run(
|
||||
[adb, "-s", "emulator-5554", "shell", "getprop", "sys.boot_completed"],
|
||||
stdin=subprocess.DEVNULL,
|
||||
capture_output=True,
|
||||
text=True,
|
||||
timeout=3,
|
||||
|
||||
@@ -49,6 +49,7 @@ def resolve_python_executable(project_root: str | None = None) -> str:
|
||||
def get_detached_process_kwargs() -> dict[str, Any]:
|
||||
"""Returns kwargs for subprocess.Popen to run a fully detached background process cross-platform."""
|
||||
kwargs: dict[str, Any] = {
|
||||
"stdin": subprocess.DEVNULL,
|
||||
"stdout": subprocess.DEVNULL,
|
||||
"stderr": subprocess.DEVNULL,
|
||||
}
|
||||
|
||||
@@ -0,0 +1,199 @@
|
||||
# Copyright 2026 Google LLC
|
||||
#
|
||||
# Licensed under the Apache License, Version 2.0 (the "License");
|
||||
# you may not use this file except in compliance with the License.
|
||||
# You may obtain a copy of the License at
|
||||
#
|
||||
# http://www.apache.org/licenses/LICENSE-2.0
|
||||
#
|
||||
# Unless required by applicable law or agreed to in writing, software
|
||||
# distributed under the License is distributed on an "AS IS" BASIS,
|
||||
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
|
||||
# See the License for the specific language governing permissions and
|
||||
# limitations under the License.
|
||||
|
||||
"""Unit and integration tests verifying MCP stdio handshake reliability,
|
||||
|
||||
subprocess stdin isolation against pipe hijacking, and stdout purity.
|
||||
"""
|
||||
|
||||
import io
|
||||
import json
|
||||
import os
|
||||
import subprocess
|
||||
import sys
|
||||
import time
|
||||
from unittest.mock import MagicMock, patch
|
||||
|
||||
import pytest
|
||||
|
||||
from artemis.clients import ui_automator_client
|
||||
from artemis.runtime.awake_lease import ScreenAwakeLease
|
||||
from artemis.runtime.awake_service import _run_awake_adb_command
|
||||
from artemis.utils.logger import get_logger
|
||||
from mcp_server.utils import device_utils, env_utils
|
||||
|
||||
|
||||
def test_mcp_stdio_handshake_immediate_input():
|
||||
"""Verify that MCP server over stdio responds to an immediate initialize request
|
||||
|
||||
without hanging, even when stdin data is pushed concurrently with process startup.
|
||||
"""
|
||||
project_root = os.path.dirname(os.path.dirname(os.path.dirname(os.path.dirname(os.path.abspath(__file__)))))
|
||||
python_exe = sys.executable
|
||||
|
||||
p = subprocess.Popen(
|
||||
[python_exe, "-m", "mcp_server"],
|
||||
stdin=subprocess.PIPE,
|
||||
stdout=subprocess.PIPE,
|
||||
stderr=subprocess.PIPE,
|
||||
env={
|
||||
"PYTHONUNBUFFERED": "1",
|
||||
"PATH": os.environ.get("PATH", ""),
|
||||
"PYTHONPATH": project_root,
|
||||
},
|
||||
cwd=project_root,
|
||||
)
|
||||
|
||||
try:
|
||||
# Immediately write the JSON-RPC initialize request into stdin
|
||||
init_req = {
|
||||
"jsonrpc": "2.0",
|
||||
"id": 1,
|
||||
"method": "initialize",
|
||||
"params": {
|
||||
"protocolVersion": "2024-11-05",
|
||||
"capabilities": {},
|
||||
"clientInfo": {"name": "test-client", "version": "1.0.0"},
|
||||
},
|
||||
}
|
||||
p.stdin.write(json.dumps(init_req).encode("utf-8") + b"\n")
|
||||
p.stdin.flush()
|
||||
|
||||
# Read initialize response with a strict timeout
|
||||
start = time.time()
|
||||
init_resp_line = None
|
||||
while time.time() - start < 6.0:
|
||||
import select
|
||||
|
||||
r, _, _ = select.select([p.stdout], [], [], 0.2)
|
||||
if r:
|
||||
init_resp_line = p.stdout.readline().decode("utf-8").strip()
|
||||
break
|
||||
|
||||
assert init_resp_line is not None, "MCP server failed to respond to initialize request within 6 seconds (deadlock detected)!"
|
||||
init_data = json.loads(init_resp_line)
|
||||
assert init_data.get("id") == 1
|
||||
assert "result" in init_data
|
||||
|
||||
# Follow up with initialized notification and tools/list request
|
||||
notif = {"jsonrpc": "2.0", "method": "notifications/initialized", "params": {}}
|
||||
tools_req = {"jsonrpc": "2.0", "id": 2, "method": "tools/list", "params": {}}
|
||||
p.stdin.write(json.dumps(notif).encode("utf-8") + b"\n")
|
||||
p.stdin.write(json.dumps(tools_req).encode("utf-8") + b"\n")
|
||||
p.stdin.flush()
|
||||
|
||||
tools_resp_line = None
|
||||
start = time.time()
|
||||
while time.time() - start < 4.0:
|
||||
import select
|
||||
|
||||
r, _, _ = select.select([p.stdout], [], [], 0.2)
|
||||
if r:
|
||||
tools_resp_line = p.stdout.readline().decode("utf-8").strip()
|
||||
break
|
||||
|
||||
assert tools_resp_line is not None, "MCP server failed to respond to tools/list request!"
|
||||
tools_data = json.loads(tools_resp_line)
|
||||
assert tools_data.get("id") == 2
|
||||
tools = tools_data.get("result", {}).get("tools", [])
|
||||
tool_names = {t["name"] for t in tools}
|
||||
assert {
|
||||
"mobile_run_task",
|
||||
"mobile_manage_task",
|
||||
"mobile_get_device_state",
|
||||
"mobile_inspect_trace",
|
||||
}.issubset(tool_names)
|
||||
|
||||
finally:
|
||||
p.terminate()
|
||||
try:
|
||||
p.wait(timeout=3)
|
||||
except subprocess.TimeoutExpired:
|
||||
p.kill()
|
||||
|
||||
|
||||
def test_awake_service_adb_command_isolates_stdin():
|
||||
"""Verify _run_awake_adb_command always sets stdin=subprocess.DEVNULL."""
|
||||
with patch("artemis.runtime.awake_service.subprocess.run") as mock_run:
|
||||
mock_run.return_value = MagicMock(returncode=0, stdout="ok", stderr="")
|
||||
_run_awake_adb_command("test-dev-1", ["shell", "date"], "test command")
|
||||
|
||||
assert mock_run.called
|
||||
kwargs = mock_run.call_args.kwargs
|
||||
assert kwargs.get("stdin") == subprocess.DEVNULL, "Expected stdin=subprocess.DEVNULL to prevent stdin hijacking!"
|
||||
|
||||
|
||||
def test_awake_lease_run_isolates_stdin():
|
||||
"""Verify ScreenAwakeLease._run always sets stdin=subprocess.DEVNULL."""
|
||||
lease = ScreenAwakeLease("test-dev-1")
|
||||
with patch("artemis.runtime.awake_lease.subprocess.run") as mock_run:
|
||||
mock_run.return_value = MagicMock(returncode=0, stdout="ok", stderr="")
|
||||
lease._run(["shell", "date"], "test lease command")
|
||||
|
||||
assert mock_run.called
|
||||
kwargs = mock_run.call_args.kwargs
|
||||
assert kwargs.get("stdin") == subprocess.DEVNULL, "Expected stdin=subprocess.DEVNULL to prevent stdin hijacking!"
|
||||
|
||||
|
||||
def test_detached_process_kwargs_isolates_stdin():
|
||||
"""Verify get_detached_process_kwargs always sets stdin=subprocess.DEVNULL."""
|
||||
kwargs = env_utils.get_detached_process_kwargs()
|
||||
assert kwargs.get("stdin") == subprocess.DEVNULL, "Expected stdin=subprocess.DEVNULL for detached background tasks!"
|
||||
|
||||
|
||||
def test_device_utils_isolates_stdin():
|
||||
"""Verify device_utils subprocess calls always set stdin=subprocess.DEVNULL."""
|
||||
with patch("mcp_server.utils.device_utils.subprocess.run") as mock_run:
|
||||
mock_run.return_value = MagicMock(returncode=0, stdout="List of devices attached\n", stderr="")
|
||||
device_utils.get_connected_devices()
|
||||
assert mock_run.called
|
||||
assert mock_run.call_args.kwargs.get("stdin") == subprocess.DEVNULL
|
||||
|
||||
|
||||
def test_ui_automator_client_isolates_stdin():
|
||||
"""Verify ui_automator_client helper commands isolate stdin."""
|
||||
with patch("artemis.clients.ui_automator_client.subprocess.run") as mock_run:
|
||||
mock_run.return_value = MagicMock(returncode=0, stdout="package:com.test\n", stderr="")
|
||||
ui_automator_client._is_package_installed("dev-1", "com.test")
|
||||
assert mock_run.called
|
||||
assert mock_run.call_args.kwargs.get("stdin") == subprocess.DEVNULL
|
||||
|
||||
with patch("artemis.clients.ui_automator_client.subprocess.run") as mock_run:
|
||||
mock_run.return_value = MagicMock(returncode=0, stdout="", stderr="")
|
||||
ui_automator_client._uninstall_package("dev-1", "com.test")
|
||||
assert mock_run.called
|
||||
assert mock_run.call_args.kwargs.get("stdin") == subprocess.DEVNULL
|
||||
|
||||
|
||||
def test_logger_header_does_not_pollute_stdout():
|
||||
"""Verify logger.header directs output to stderr and never pollutes sys.stdout."""
|
||||
logger = get_logger("test_purity_logger")
|
||||
captured_stdout = io.StringIO()
|
||||
captured_stderr = io.StringIO()
|
||||
|
||||
orig_stdout = sys.stdout
|
||||
orig_stderr = sys.stderr
|
||||
try:
|
||||
sys.stdout = captured_stdout
|
||||
sys.stderr = captured_stderr
|
||||
logger.header("IMPORTANT PROTOCOL BANNER")
|
||||
finally:
|
||||
sys.stdout = orig_stdout
|
||||
sys.stderr = orig_stderr
|
||||
|
||||
assert captured_stdout.getvalue() == "", (
|
||||
f"Detected stdout pollution from logger.header: {captured_stdout.getvalue()!r}. "
|
||||
"In MCP stdio mode, stdout MUST be reserved exclusively for JSON-RPC messages!"
|
||||
)
|
||||
assert "IMPORTANT PROTOCOL BANNER" in captured_stderr.getvalue()
|
||||
Reference in new issue
Block a user