From 2dda207409d18bd488bc7c02a698954fe0aa6972 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Sun, 4 Oct 2026 18:35:28 -0700 Subject: [PATCH] fix(whatsapp): install a missing aiohttp through PM and name every failed connect A gateway whose dependency environment lacks aiohttp (a sealed env from `hermes update` with a partial messaging extra, #126358) spawned the bridge, then every /health poll raised ModuleNotFoundError inside `_poll_bridge_health`'s blanket except. connect() returned False with no fatal code, so the reconnect watcher retried forever while the bridge answered /health 200, and telemetry counted each attempt as error_class "other". - connect() now obtains aiohttp through PM (`ensure_import("sms")`, the extra that is exactly aiohttp) before starting the bridge, the same lazy-install path Slack/Teams use. When PM cannot install it (lazy installs disabled, foreign interpreter, install failure) or aiohttp is a half-removed namespace shell (#71308), it fails with the named, non-retryable `whatsapp_aiohttp_missing` and never spawns the bridge. - The bridge-died / "did not start in 15s" paths set a retryable fatal code (`whatsapp_bridge_exited` / `whatsapp_bridge_timeout`), so reconnect status and connect telemetry name the cause instead of "other". - Folds the salvaged test file into test_whatsapp_connect.py as two invariant tests. --- plugins/platforms/whatsapp/adapter.py | 52 ++++++--- .../gateway/test_whatsapp_aiohttp_missing.py | 110 ------------------ tests/gateway/test_whatsapp_connect.py | 34 ++++++ 3 files changed, 72 insertions(+), 124 deletions(-) delete mode 100644 tests/gateway/test_whatsapp_aiohttp_missing.py diff --git a/plugins/platforms/whatsapp/adapter.py b/plugins/platforms/whatsapp/adapter.py index 35a20d37b1..6a216e140f 100644 --- a/plugins/platforms/whatsapp/adapter.py +++ b/plugins/platforms/whatsapp/adapter.py @@ -264,12 +264,33 @@ def check_whatsapp_requirements() -> bool: return False -def _aiohttp_available() -> bool: +def _import_aiohttp(): + import aiohttp + if not hasattr(aiohttp, "ClientSession"): # half-removed install leaves a namespace shell (#71308) + raise ImportError(f"aiohttp at {list(aiohttp.__path__)} is an incomplete install") + return aiohttp + + +def ensure_aiohttp() -> Optional[str]: + """None once aiohttp imports (PM installs the ``sms`` extra, which is exactly aiohttp, when it is missing), else why not. + + Every bridge call imports aiohttp; a sealed env without it used to fail each /health poll silently and loop + forever on "did not start in 15s" while the bridge was healthy (#126358). + """ try: - import aiohttp # noqa: F401 - return True - except ImportError: - return False + _import_aiohttp() + return None + except ImportError as exc: + if not isinstance(exc, ModuleNotFoundError): + return str(exc) # present but broken: PM sees it as installed, reinstalling is the user's call + try: + from pm.extras import ensure_import + + ensure_import("sms") + _import_aiohttp() + return None + except Exception as exc: + return str(exc) or type(exc).__name__ # Env vars bridge.js consumes; injected because a multiplexed subprocess's os.environ lacks the secondary profile's .env. @@ -489,10 +510,12 @@ class WhatsAppAdapter(WhatsAppBehaviorMixin, BasePlatformAdapter): bridge_env.update(HERMES_IMAGE_CACHE_DIR=str(img_dir), HERMES_AUDIO_CACHE_DIR=str(audio_dir), HERMES_DOCUMENT_CACHE_DIR=str(doc_dir)) return bridge_env - def _bridge_died(self, detail: str) -> bool: + def _bridge_died(self, detail: str, code: str = "whatsapp_bridge_exited") -> bool: print(f"[{self.name}] {detail}") print(f"[{self.name}] Check log: {self._bridge_log}") self._close_bridge_log() + # Named so the reconnect status and connect telemetry say why instead of an unclassified failure. + self._set_fatal_error(code, f"{detail} (see {self._bridge_log})", retryable=True) return False async def _poll_bridge_health(self, died_msg: str) -> tuple[Optional[bool], bool, dict]: @@ -522,7 +545,7 @@ class WhatsAppAdapter(WhatsAppBehaviorMixin, BasePlatformAdapter): if connected is False: return False if not http_ready: - return self._bridge_died("Bridge HTTP server did not start in 15s") + return self._bridge_died("Bridge HTTP server did not start in 15s", "whatsapp_bridge_timeout") if data.get("status") != "connected": print(f"[{self.name}] Bridge HTTP ready, waiting for WhatsApp connection...") connected, _, _ = await self._poll_bridge_health("Bridge process died during connection") @@ -535,18 +558,12 @@ class WhatsAppAdapter(WhatsAppBehaviorMixin, BasePlatformAdapter): return True def _preflight(self) -> bool: - """Node + aiohttp + bridge script + creds.json present, else a non-retryable fatal error (an unpaired bridge only prints QR codes; retries would pay 30s each).""" + """Node + bridge script + creds.json present, else a non-retryable fatal error (an unpaired bridge only prints QR codes; retries would pay 30s each).""" bridge_path = Path(self._bridge_script) creds_path = self._session_path / "creds.json" checks = ( (check_whatsapp_requirements, ("[%s] Node.js not found. WhatsApp requires Node.js.", self.name), "whatsapp_node_missing", "Node.js is not installed — install Node.js and re-run `hermes gateway`."), - # Every bridge health poll imports aiohttp; a sealed env that shipped a partial messaging extra - # used to swallow the ModuleNotFoundError and loop forever on "did not start in 15s" (#126358). - (_aiohttp_available, ("[%s] aiohttp not installed — the WhatsApp bridge health probe needs it.", self.name), - "whatsapp_aiohttp_missing", - "aiohttp is not installed — the WhatsApp bridge health probe needs it. " - "Run `hermes update` (or `uv pip install \"aiohttp==3.14.3\"`), then restart `hermes gateway`."), (bridge_path.exists, ("[%s] Bridge script not found: %s", self.name, bridge_path), "whatsapp_bridge_missing", f"WhatsApp bridge script missing at {bridge_path}."), (creds_path.exists, ("[%s] WhatsApp is enabled but not paired (no creds.json at %s). Pair from the dashboard or run " @@ -586,6 +603,13 @@ class WhatsAppAdapter(WhatsAppBehaviorMixin, BasePlatformAdapter): return False if not self._preflight(): return False + if (why := await asyncio.to_thread(ensure_aiohttp)) is not None: + message = (f"aiohttp is unavailable ({why}); the WhatsApp bridge client needs it. " + "Run `hermes pm install --extra sms` (it ships aiohttp; `hermes pm repair` for a damaged install), " + "then restart `hermes gateway`.") + logger.warning("[%s] %s", self.name, message) + self._set_fatal_error("whatsapp_aiohttp_missing", message, retryable=False) + return False bridge_path = Path(self._bridge_script) lock_acquired = False try: diff --git a/tests/gateway/test_whatsapp_aiohttp_missing.py b/tests/gateway/test_whatsapp_aiohttp_missing.py deleted file mode 100644 index ec5b0f4721..0000000000 --- a/tests/gateway/test_whatsapp_aiohttp_missing.py +++ /dev/null @@ -1,110 +0,0 @@ -"""WhatsApp must name a missing aiohttp instead of looping forever (#126358). - -A sealed env built by ``hermes update`` can ship a partial messaging extra -without aiohttp. The adapter's requirement check only verified Node.js, so -``connect()`` spawned the bridge and every health poll raised -``ModuleNotFoundError`` — swallowed by ``except Exception: continue`` — and -the gateway looped forever on the misleading -"Bridge HTTP server did not start in 15s". -""" - -import asyncio -import sys -from pathlib import Path -from unittest.mock import AsyncMock, MagicMock, patch - -import pytest - -from gateway.config import Platform - - -def _make_adapter(): - """Create a WhatsAppAdapter with test attributes (bypass __init__).""" - from plugins.platforms.whatsapp.adapter import WhatsAppAdapter - - adapter = WhatsAppAdapter.__new__(WhatsAppAdapter) - adapter.platform = Platform.WHATSAPP - adapter.config = MagicMock() - adapter._bridge_port = 19877 - adapter._bridge_script = "/tmp/test-bridge-126358.js" - adapter._session_path = Path("/tmp/test-wa-session-126358") - adapter._bridge_log_fh = None - adapter._bridge_log = None - adapter._bridge_process = None - adapter._reply_prefix = None - adapter._send_read_receipts = False - adapter._dm_policy = adapter._group_policy = "pairing" - adapter._allow_from = adapter._group_allow_from = set() - adapter._running = False - adapter._message_handler = None - adapter._fatal_error_code = None - adapter._fatal_error_message = None - adapter._fatal_error_retryable = True - adapter._fatal_error_handler = None - adapter._active_sessions = {} - adapter._pending_messages = {} - adapter._background_tasks = set() - adapter._auto_tts_disabled_chats = set() - adapter._message_queue = asyncio.Queue() - adapter._http_session = None - return adapter - - -class TestMissingAiohttpNamed: - @pytest.mark.asyncio - async def test_connect_fails_fast_with_named_aiohttp_error(self): - """Node present + aiohttp missing -> non-retryable fatal naming aiohttp, no bridge spawn.""" - from plugins.platforms.whatsapp.adapter import WhatsAppAdapter - - adapter = _make_adapter() - - with patch( - "plugins.platforms.whatsapp.adapter.find_node_executable", - return_value="/fake/node", - ), patch.object(Path, "exists", return_value=True), patch( - "subprocess.run", return_value=MagicMock(returncode=0) - ), patch( - "subprocess.Popen", return_value=MagicMock() - ) as mock_popen, patch( - "plugins.platforms.whatsapp.adapter.asyncio.sleep", - new_callable=AsyncMock, - ), patch.dict( - sys.modules, {"aiohttp": None} - ): - result = await adapter.connect() - - assert result is False - # Fail fast: the bridge process must never be spawned. - mock_popen.assert_not_called() - # Named error, not the misleading "did not start in 15s" loop. - assert adapter.fatal_error_code == "whatsapp_aiohttp_missing" - assert "aiohttp" in (adapter.fatal_error_message or "").lower() - assert adapter.fatal_error_retryable is False - - def test_preflight_names_missing_aiohttp(self): - """_preflight alone reports whatsapp_aiohttp_missing when the import fails.""" - adapter = _make_adapter() - - with patch( - "plugins.platforms.whatsapp.adapter.check_whatsapp_requirements", - return_value=True, - ), patch.object(Path, "exists", return_value=True), patch.dict( - sys.modules, {"aiohttp": None} - ): - ok = adapter._preflight() - - assert ok is False - assert adapter.fatal_error_code == "whatsapp_aiohttp_missing" - - def test_preflight_passes_with_aiohttp(self): - """With aiohttp importable the preflight keeps its Node/bridge/creds semantics.""" - adapter = _make_adapter() - - with patch( - "plugins.platforms.whatsapp.adapter.check_whatsapp_requirements", - return_value=True, - ), patch.object(Path, "exists", return_value=True): - ok = adapter._preflight() - - assert ok is True - assert adapter.fatal_error_code is None diff --git a/tests/gateway/test_whatsapp_connect.py b/tests/gateway/test_whatsapp_connect.py index 86f9454fd9..b1688a2e0a 100644 --- a/tests/gateway/test_whatsapp_connect.py +++ b/tests/gateway/test_whatsapp_connect.py @@ -14,6 +14,7 @@ Regression tests for two bugs in WhatsAppAdapter.connect(): import asyncio import signal +import sys from pathlib import Path from unittest.mock import AsyncMock, MagicMock, patch @@ -544,3 +545,36 @@ class TestNoCredsPreflight: # but the fatal-error code is NOT the "not paired" one. assert result is False assert adapter._fatal_error_code != "whatsapp_not_paired" + + +class TestNamedConnectFailures: + """A failed connect names its cause; an unnamed False loops forever as an unclassified failure (#126358).""" + + @pytest.mark.asyncio + async def test_missing_aiohttp_pm_cannot_install_is_named_and_final(self): + from pm.package import InstallError + + adapter = _make_adapter() + adapter._acquire_platform_lock = MagicMock(return_value=False) + ensure_import = MagicMock(side_effect=InstallError("venv", "lazy installs are disabled")) + with patch("plugins.platforms.whatsapp.adapter.check_whatsapp_requirements", return_value=True), \ + patch.object(Path, "exists", return_value=True), patch.dict(sys.modules, {"aiohttp": None}), \ + patch("pm.extras.ensure_import", ensure_import): + assert await adapter.connect() is False + + ensure_import.assert_called_once_with("sms") + adapter._acquire_platform_lock.assert_not_called() # bridge never started + assert adapter.fatal_error_code == "whatsapp_aiohttp_missing" + assert adapter.fatal_error_retryable is False + assert "lazy installs are disabled" in adapter.fatal_error_message + + @pytest.mark.asyncio + async def test_bridge_http_timeout_is_a_named_retryable_failure(self): + adapter = _make_adapter() + adapter._bridge_process = MagicMock(**{"poll.return_value": None}) + with patch.object(adapter, "_probe_bridge_health", new_callable=AsyncMock, side_effect=OSError("refused")), \ + patch("plugins.platforms.whatsapp.adapter.asyncio.sleep", new_callable=AsyncMock): + assert await adapter._wait_for_bridge() is False + + assert adapter.fatal_error_code == "whatsapp_bridge_timeout" + assert adapter.fatal_error_retryable is True