From 34badeb19c80fed56517d0199882bee9ff513f66 Mon Sep 17 00:00:00 2001 From: Farukest Date: Wed, 4 Mar 2026 19:11:48 +0300 Subject: [PATCH] fix(whatsapp): initialize data variable and close log handle on error paths --- gateway/platforms/whatsapp.py | 23 ++- tests/gateway/test_whatsapp_connect.py | 270 +++++++++++++++++++++++++ 2 files changed, 286 insertions(+), 7 deletions(-) create mode 100644 tests/gateway/test_whatsapp_connect.py diff --git a/gateway/platforms/whatsapp.py b/gateway/platforms/whatsapp.py index 6b057cd28..b3916aef0 100644 --- a/gateway/platforms/whatsapp.py +++ b/gateway/platforms/whatsapp.py @@ -186,11 +186,13 @@ class WhatsAppAdapter(BasePlatformAdapter): # Phase 2: wait for WhatsApp status: connected (up to 15s more). import aiohttp http_ready = False + data = {} for attempt in range(15): await asyncio.sleep(1) if self._bridge_process.poll() is not None: print(f"[{self.name}] Bridge process died (exit code {self._bridge_process.returncode})") print(f"[{self.name}] Check log: {self._bridge_log}") + self._close_bridge_log() return False try: async with aiohttp.ClientSession() as session: @@ -206,10 +208,11 @@ class WhatsAppAdapter(BasePlatformAdapter): break except Exception: continue - + if not http_ready: print(f"[{self.name}] Bridge HTTP server did not start in 15s") print(f"[{self.name}] Check log: {self._bridge_log}") + self._close_bridge_log() return False # Phase 2: HTTP is up but WhatsApp may still be connecting. @@ -221,6 +224,7 @@ class WhatsAppAdapter(BasePlatformAdapter): if self._bridge_process.poll() is not None: print(f"[{self.name}] Bridge process died during connection") print(f"[{self.name}] Check log: {self._bridge_log}") + self._close_bridge_log() return False try: async with aiohttp.ClientSession() as session: @@ -251,8 +255,18 @@ class WhatsAppAdapter(BasePlatformAdapter): except Exception as e: logger.error("[%s] Failed to start bridge: %s", self.name, e, exc_info=True) + self._close_bridge_log() return False + def _close_bridge_log(self) -> None: + """Close the bridge log file handle if open.""" + if self._bridge_log_fh: + try: + self._bridge_log_fh.close() + except Exception: + pass + self._bridge_log_fh = None + async def disconnect(self) -> None: """Stop the WhatsApp bridge and clean up any orphaned processes.""" if self._bridge_process: @@ -289,12 +303,7 @@ class WhatsAppAdapter(BasePlatformAdapter): self._running = False self._bridge_process = None - if self._bridge_log_fh: - try: - self._bridge_log_fh.close() - except Exception: - pass - self._bridge_log_fh = None + self._close_bridge_log() print(f"[{self.name}] Disconnected") async def send( diff --git a/tests/gateway/test_whatsapp_connect.py b/tests/gateway/test_whatsapp_connect.py new file mode 100644 index 000000000..2541ce508 --- /dev/null +++ b/tests/gateway/test_whatsapp_connect.py @@ -0,0 +1,270 @@ +"""Tests for WhatsApp connect() error handling. + +Regression tests for two bugs in WhatsAppAdapter.connect(): + +1. Uninitialized ``data`` variable: when ``resp.json()`` raised after the + health endpoint returned HTTP 200, ``http_ready`` was set to True but + ``data`` was never assigned. The subsequent ``data.get("status")`` + check raised ``NameError``. + +2. Bridge log file handle leaked on error paths: the file was opened before + the health-check loop but never closed when ``connect()`` returned False. + Repeated connection failures accumulated open file descriptors. +""" + +import asyncio +from pathlib import Path +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + +from gateway.config import Platform + + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + +class _AsyncCM: + """Minimal async context manager returning a fixed value.""" + + def __init__(self, value): + self.value = value + + async def __aenter__(self): + return self.value + + async def __aexit__(self, *exc): + return False + + +def _make_adapter(): + """Create a WhatsAppAdapter with test attributes (bypass __init__).""" + from gateway.platforms.whatsapp import WhatsAppAdapter + + adapter = WhatsAppAdapter.__new__(WhatsAppAdapter) + adapter.platform = Platform.WHATSAPP + adapter.config = MagicMock() + adapter._bridge_port = 19876 + adapter._bridge_script = "/tmp/test-bridge.js" + adapter._session_path = Path("/tmp/test-wa-session") + adapter._bridge_log_fh = None + adapter._bridge_log = None + adapter._bridge_process = None + adapter._running = False + adapter._message_queue = asyncio.Queue() + return adapter + + +def _mock_aiohttp(status=200, json_data=None, json_side_effect=None): + """Build a mock ``aiohttp.ClientSession`` returning a fixed response.""" + mock_resp = MagicMock() + mock_resp.status = status + if json_side_effect: + mock_resp.json = AsyncMock(side_effect=json_side_effect) + else: + mock_resp.json = AsyncMock(return_value=json_data or {}) + + mock_session = MagicMock() + mock_session.get = MagicMock(return_value=_AsyncCM(mock_resp)) + + return MagicMock(return_value=_AsyncCM(mock_session)) + + +def _connect_patches(mock_proc, mock_fh, mock_client_cls=None): + """Return a dict of common patches needed to reach the health-check loop.""" + patches = { + "gateway.platforms.whatsapp.check_whatsapp_requirements": True, + "gateway.platforms.whatsapp.asyncio.create_task": MagicMock(), + } + base = [ + patch("gateway.platforms.whatsapp.check_whatsapp_requirements", return_value=True), + patch.object(Path, "exists", return_value=True), + patch.object(Path, "mkdir", return_value=None), + patch("subprocess.run", return_value=MagicMock(returncode=0)), + patch("subprocess.Popen", return_value=mock_proc), + patch("builtins.open", return_value=mock_fh), + patch("gateway.platforms.whatsapp.asyncio.sleep", new_callable=AsyncMock), + patch("gateway.platforms.whatsapp.asyncio.create_task"), + ] + if mock_client_cls is not None: + base.append(patch("aiohttp.ClientSession", mock_client_cls)) + return base + + +# --------------------------------------------------------------------------- +# _close_bridge_log() unit tests +# --------------------------------------------------------------------------- + +class TestCloseBridgeLog: + """Direct tests for the _close_bridge_log() helper method.""" + + @staticmethod + def _bare_adapter(): + from gateway.platforms.whatsapp import WhatsAppAdapter + a = WhatsAppAdapter.__new__(WhatsAppAdapter) + a._bridge_log_fh = None + return a + + def test_closes_open_handle(self): + adapter = self._bare_adapter() + mock_fh = MagicMock() + adapter._bridge_log_fh = mock_fh + + adapter._close_bridge_log() + + mock_fh.close.assert_called_once() + assert adapter._bridge_log_fh is None + + def test_noop_when_no_handle(self): + adapter = self._bare_adapter() + + adapter._close_bridge_log() # must not raise + + assert adapter._bridge_log_fh is None + + def test_suppresses_close_exception(self): + adapter = self._bare_adapter() + mock_fh = MagicMock() + mock_fh.close.side_effect = OSError("already closed") + adapter._bridge_log_fh = mock_fh + + adapter._close_bridge_log() # must not raise + + assert adapter._bridge_log_fh is None + + +# --------------------------------------------------------------------------- +# data variable initialization +# --------------------------------------------------------------------------- + +class TestDataInitialized: + """Verify ``data = {}`` prevents NameError when resp.json() fails.""" + + @pytest.mark.asyncio + async def test_no_name_error_when_json_always_fails(self): + """HTTP 200 sets http_ready but json() always raises. + + Without the fix, ``data`` was never assigned and the Phase 2 check + ``data.get("status")`` raised NameError. With ``data = {}``, the + check evaluates to ``None != "connected"`` and Phase 2 runs normally. + """ + adapter = _make_adapter() + + mock_proc = MagicMock() + mock_proc.poll.return_value = None # bridge stays alive + + mock_client_cls = _mock_aiohttp( + status=200, json_side_effect=ValueError("bad json"), + ) + mock_fh = MagicMock() + + patches = _connect_patches(mock_proc, mock_fh, mock_client_cls) + + with patches[0], patches[1], patches[2], patches[3], patches[4], \ + patches[5], patches[6], patches[7], patches[8], \ + patch.object(type(adapter), "_poll_messages", return_value=MagicMock()): + # Must NOT raise NameError + result = await adapter.connect() + + # connect() returns True (warn-and-proceed path) + assert result is True + assert adapter._running is True + + +# --------------------------------------------------------------------------- +# File handle cleanup on error paths +# --------------------------------------------------------------------------- + +class TestFileHandleClosedOnError: + """Verify the bridge log file handle is closed on every failure path.""" + + @pytest.mark.asyncio + async def test_closed_when_bridge_dies_phase1(self): + """Bridge process exits during Phase 1 health-check loop.""" + adapter = _make_adapter() + + mock_proc = MagicMock() + mock_proc.poll.return_value = 1 # dead immediately + mock_proc.returncode = 1 + + mock_fh = MagicMock() + patches = _connect_patches(mock_proc, mock_fh) + + with patches[0], patches[1], patches[2], patches[3], patches[4], \ + patches[5], patches[6], patches[7]: + result = await adapter.connect() + + assert result is False + mock_fh.close.assert_called_once() + assert adapter._bridge_log_fh is None + + @pytest.mark.asyncio + async def test_closed_when_http_not_ready(self): + """Health endpoint never returns 200 within 15 attempts.""" + adapter = _make_adapter() + + mock_proc = MagicMock() + mock_proc.poll.return_value = None # bridge alive + + mock_client_cls = _mock_aiohttp(status=503) + mock_fh = MagicMock() + patches = _connect_patches(mock_proc, mock_fh, mock_client_cls) + + with patches[0], patches[1], patches[2], patches[3], patches[4], \ + patches[5], patches[6], patches[7], patches[8]: + result = await adapter.connect() + + assert result is False + mock_fh.close.assert_called_once() + assert adapter._bridge_log_fh is None + + @pytest.mark.asyncio + async def test_closed_when_bridge_dies_phase2(self): + """Bridge alive during Phase 1 but dies during Phase 2.""" + adapter = _make_adapter() + + # Phase 1 (15 iterations): alive. Phase 2 (iteration 16): dead. + call_count = [0] + + def poll_side_effect(): + call_count[0] += 1 + return None if call_count[0] <= 15 else 1 + + mock_proc = MagicMock() + mock_proc.poll.side_effect = poll_side_effect + mock_proc.returncode = 1 + + # Health returns 200 with status != "connected" -> triggers Phase 2 + mock_client_cls = _mock_aiohttp( + status=200, json_data={"status": "disconnected"}, + ) + mock_fh = MagicMock() + patches = _connect_patches(mock_proc, mock_fh, mock_client_cls) + + with patches[0], patches[1], patches[2], patches[3], patches[4], \ + patches[5], patches[6], patches[7], patches[8]: + result = await adapter.connect() + + assert result is False + mock_fh.close.assert_called_once() + assert adapter._bridge_log_fh is None + + @pytest.mark.asyncio + async def test_closed_on_unexpected_exception(self): + """Popen raises, outer except block must still close the handle.""" + adapter = _make_adapter() + + mock_fh = MagicMock() + + with patch("gateway.platforms.whatsapp.check_whatsapp_requirements", return_value=True), \ + patch.object(Path, "exists", return_value=True), \ + patch.object(Path, "mkdir", return_value=None), \ + patch("subprocess.run", return_value=MagicMock(returncode=0)), \ + patch("subprocess.Popen", side_effect=OSError("spawn failed")), \ + patch("builtins.open", return_value=mock_fh): + result = await adapter.connect() + + assert result is False + mock_fh.close.assert_called_once() + assert adapter._bridge_log_fh is None