diff --git a/plugins/platforms/whatsapp/adapter.py b/plugins/platforms/whatsapp/adapter.py index 3dc040d26a..4a8b085979 100644 --- a/plugins/platforms/whatsapp/adapter.py +++ b/plugins/platforms/whatsapp/adapter.py @@ -72,6 +72,11 @@ def _windows_listener_pids(port: int) -> list: return _safe_ints(p[4] for p in rows if len(p) >= 5 and p[3] == "LISTENING" and p[1].endswith(f":{port}")) +def _port_listener_pids(port: int) -> list: + """Listening PIDs on ``port`` for this platform: netstat on Windows, lsof/ss elsewhere.""" + return _windows_listener_pids(port) if _IS_WINDOWS else _listener_pids_on_port(port) + + def _pid_looks_like_node_bridge(pid: int) -> bool: """Fail-closed: the live process must be a ``node`` executable (a scan-time PID can be a stranger by kill time). @@ -92,7 +97,7 @@ def _pid_looks_like_node_bridge(pid: int) -> bool: def _kill_port_process(port: int) -> None: """Kill any node bridge *listening* on the given TCP port (never a client); SIGTERM on POSIX, taskkill /F on Windows.""" with suppress(Exception): - for pid in (_windows_listener_pids(port) if _IS_WINDOWS else _listener_pids_on_port(port)): + for pid in _port_listener_pids(port): # Killing a mistyped or recycled PID is unrecoverable — verify first. if pid <= 0 or not _pid_looks_like_node_bridge(pid): logger.warning("[whatsapp] Not killing PID %s on port %d: process is not a node bridge (or identity unverifiable)", pid, port) @@ -350,13 +355,15 @@ class WhatsAppAdapter(WhatsAppBehaviorMixin, BasePlatformAdapter): return getattr(self._http_session, method)(self._bridge_url(path), **kwargs, timeout=aiohttp.ClientTimeout(total=timeout)) async def _probe_bridge_health(self) -> tuple[bool, Any]: - """GET /health with a fresh session → ``(http_200, json)``; unparseable 200 body → ``(True, None)``; connection errors propagate.""" + """GET /health with a fresh session → ``(http_200, json)``; unparseable 200 body → ``(True, None)``; connection errors and timeouts propagate.""" import aiohttp async with aiohttp.ClientSession() as session, session.get(self._bridge_url("health"), timeout=aiohttp.ClientTimeout(total=2)) as resp: if resp.status != 200: return False, None try: return True, await resp.json() + except asyncio.TimeoutError: + raise # a body that never arrives is no answer, the same as headers that never arrive except Exception: return True, None @@ -600,7 +607,11 @@ class WhatsAppAdapter(WhatsAppBehaviorMixin, BasePlatformAdapter): # No /health answer is no ownership evidence (another profile's busy bridge looks the same), and # our own stale bridge was just reaped by its pidfile: a holder that remains is left running. await asyncio.sleep(1) - if _listener_pids_on_port(self._bridge_port): + try: + held = bool(_port_listener_pids(self._bridge_port)) + except Exception: + held = True # listeners could not be listed: still no evidence to kill on + if held: self._set_fatal_error( "whatsapp_bridge_unresponsive", f"Port {self._bridge_port} is held by a process that did not answer /health in time, so " @@ -1059,7 +1070,10 @@ async def _standalone_send(pconfig, chat_id, message, *, thread_id=None, media_f f"http://localhost:{bridge_port}/health", timeout=aiohttp.ClientTimeout(total=5), ) as resp: - health = await resp.json() if resp.status == 200 else {} + if resp.status != 200: + return send_error( + f"WhatsApp bridge on port {bridge_port} answered /health with HTTP {resp.status}; nothing was sent.") + health = await resp.json() reported_session = health.get("session") if reported_session and reported_session != own_session: return send_error( diff --git a/tests/gateway/test_whatsapp_stale_bridge.py b/tests/gateway/test_whatsapp_stale_bridge.py index 6137057c7f..778977defc 100644 --- a/tests/gateway/test_whatsapp_stale_bridge.py +++ b/tests/gateway/test_whatsapp_stale_bridge.py @@ -18,7 +18,6 @@ package.json changes, not only when node_modules is missing. import asyncio import os -from contextlib import nullcontext from pathlib import Path from unittest.mock import AsyncMock, MagicMock, patch @@ -83,13 +82,14 @@ def _make_adapter(bridge_script: str = "/tmp/test-bridge.js", return adapter -def _mock_health(json_data): - """Mock aiohttp.ClientSession whose GET returns 200 + *json_data*.""" +def _mock_health(json_data, raise_on=None): + """Mock aiohttp.ClientSession whose GET returns 200 + *json_data*; ``raise_on`` "headers"/"body" times out that phase.""" mock_resp = MagicMock() mock_resp.status = 200 - mock_resp.json = AsyncMock(return_value=json_data) + mock_resp.json = AsyncMock(return_value=json_data, side_effect=asyncio.TimeoutError if raise_on == "body" else None) mock_session = MagicMock() - mock_session.get = MagicMock(return_value=_AsyncCM(mock_resp)) + mock_session.get = MagicMock( + return_value=_AsyncCM(mock_resp), side_effect=asyncio.TimeoutError if raise_on == "headers" else None) mock_session.close = AsyncMock() return MagicMock(return_value=_AsyncCM(mock_session)) @@ -162,8 +162,11 @@ class TestStaleBridgeHandshake: ("own", "connected", None), ("other", "connected", "whatsapp_bridge_foreign_session"), ("other", "disconnected", "whatsapp_bridge_foreign_session"), - ("unanswered", None, "whatsapp_bridge_unresponsive"), - ], ids=["own-session-adopted", "other-session-connected", "other-session-starting", "no-health-answer"]) + ("headers-timeout", None, "whatsapp_bridge_unresponsive"), + ("body-timeout", None, "whatsapp_bridge_unresponsive"), + ("body-timeout-windows", None, "whatsapp_bridge_unresponsive"), + ], ids=["own-session-adopted", "other-session-connected", "other-session-starting", + "health-headers-timeout", "health-body-timeout", "health-body-timeout-windows"]) async def test_bridge_is_adopted_or_left_running_by_its_session(self, tmp_path, holder, status, fatal): """Two profiles default to one bridge_port; only this profile's own session may be adopted. Another profile's bridge is never killed, even while it reports ``disconnected`` (startup, reconnect, QR wait), and neither is a @@ -183,19 +186,20 @@ class TestStaleBridgeHandshake: "scriptHash": _file_content_hash(bridge_dir / "bridge.js"), "sendReadReceipts": False, "session": str(reported), - } + }, + raise_on=holder.split("-")[0] if "timeout" in holder else None, ) - unanswered = (patch.object(adapter, "_probe_bridge_health", AsyncMock(side_effect=asyncio.TimeoutError)) - if holder == "unanswered" else nullcontext()) + windows = holder.endswith("-windows") # the listener scan must be the platform's own, or Windows still kills with ( - unanswered, patch("plugins.platforms.whatsapp.adapter.check_whatsapp_requirements", return_value=True), patch("aiohttp.ClientSession", mock_client), patch("plugins.platforms.whatsapp.adapter.asyncio.sleep", new_callable=AsyncMock), patch("plugins.platforms.whatsapp.adapter._kill_stale_bridge_by_pidfile") as mock_kill_pidfile, patch("plugins.platforms.whatsapp.adapter._kill_port_process") as mock_kill_port, - patch("plugins.platforms.whatsapp.adapter._listener_pids_on_port", return_value=[4242]), + patch("plugins.platforms.whatsapp.adapter._IS_WINDOWS", windows), + patch("plugins.platforms.whatsapp.adapter._listener_pids_on_port", return_value=[] if windows else [4242]), + patch("plugins.platforms.whatsapp.adapter._windows_listener_pids", return_value=[4242] if windows else []), patch("subprocess.Popen", return_value=MagicMock()) as mock_popen, patch.object(adapter, "_acquire_platform_lock", return_value=True, create=True), ): diff --git a/tests/tools/test_whatsapp_send_message_media.py b/tests/tools/test_whatsapp_send_message_media.py index e20a82c7e6..480a740646 100644 --- a/tests/tools/test_whatsapp_send_message_media.py +++ b/tests/tools/test_whatsapp_send_message_media.py @@ -233,23 +233,25 @@ def test_secondary_standalone_sends_use_active_profile_port_for_text_media_and_m assert record.read_text(encoding="utf-8") == str(persisted_port) -@pytest.mark.parametrize("reported", ["own", "other", None], ids=["own-session", "other-profile-session", "pre-session-bridge"]) +@pytest.mark.parametrize("reported", ["own", "other", None, "unhealthy"], + ids=["own-session", "other-profile-session", "pre-session-bridge", "health-503"]) def test_standalone_send_posts_only_through_this_profiles_bridge(tmp_path, reported): """Profiles sharing a bridge_port must not send from each other's WhatsApp account, text or media.""" own = tmp_path / "b" / "session" media = tmp_path / "image.png" media.write_bytes(b"image") - health = {"session": str(own if reported == "own" else tmp_path / "a" / "session")} if reported else {} + sessions = {"own": own, "other": tmp_path / "a" / "session"} + health = _resp(503) if reported == "unhealthy" else _resp(200, {"session": str(sessions[reported])} if reported else {}) session_ctx, calls = _session_with( - [_resp(200, {"messageId": "text"}), _resp(200, {"messageId": "media"})], health=_resp(200, health)) + [_resp(200, {"messageId": "text"}), _resp(200, {"messageId": "media"})], health=health) with patch("aiohttp.ClientSession", return_value=session_ctx): result = asyncio.run(_standalone_send( SimpleNamespace(token="", extra={"bridge_port": 3000, "session_path": str(own)}), "12345", "hello", media_files=[(str(media), False)], )) posted = [url for url, _ in calls if not url.endswith("/health")] - if reported == "other": - assert "another profile's session" in result["error"] + if reported in ("other", "unhealthy"): + assert "nothing was sent" in result["error"] assert posted == [] else: assert result.get("success") is True, result diff --git a/website/docs/user-guide/messaging/whatsapp.md b/website/docs/user-guide/messaging/whatsapp.md index b1f8724d3c..23b33ff06e 100644 --- a/website/docs/user-guide/messaging/whatsapp.md +++ b/website/docs/user-guide/messaging/whatsapp.md @@ -44,7 +44,7 @@ record claims, and saves it in its own `platforms/whatsapp/bridge_port` file for subsequent starts. Operators can pre-create that file with a port number; delete it to have a new port allocated. The launch profile uses port 3000 unless it already has that file (from serving as a secondary), in which case its gateway -and `hermes whatsapp send` both keep using the recorded port. +and `hermes send --to whatsapp:` both keep using the recorded port. A secondary adopts a bridge already running on its port only when its own session pidfile identifies that process (pid, kernel start time, and the port it @@ -70,11 +70,12 @@ platforms: A gateway identifies a running bridge by the session directory the bridge reports in `/health`. A bridge serving another profile's session is never adopted and never stopped: the second profile's WhatsApp fails to start with -`whatsapp_bridge_foreign_session`, naming the port and the other session. On -Linux and macOS, a process that holds the port but does not answer `/health` in -time is left running too, and WhatsApp fails with the retryable -`whatsapp_bridge_unresponsive`. `hermes whatsapp send` and cron delivery check -the same field and refuse to send through another profile's bridge. If you +`whatsapp_bridge_foreign_session`, naming the port and the other session. A +process that holds the port but does not answer `/health` in time is left +running too, and WhatsApp fails with the retryable +`whatsapp_bridge_unresponsive`. `hermes send --to whatsapp:` and cron +delivery check the same field and send nothing through another profile's bridge +or through one whose `/health` fails. If you override `session_path`, keep it distinct per profile, or the profiles share one WhatsApp login. Bridges started by an older Hermes report no session directory and are restarted once, as after a bridge update.