From 41b9b893253fb7af00e1bf7cc0cb579b5a747964 Mon Sep 17 00:00:00 2001 From: binhnt92 Date: Mon, 6 Apr 2026 18:46:50 +0700 Subject: [PATCH] fix(gateway): release platform locks on Slack/Signal connect failure When Slack's connect() acquires a scoped lock on the app token and then fails (bad token, Socket Mode error), the lock is never released. The next gateway start sees "Slack app token already in use" and refuses to connect until the process dies. Same issue in Signal: if the health check fails after acquiring the phone lock, the lock is held permanently. Discord got this fix in PR #5302. Slack and Signal were missed. Add lock release to both exception/failure paths. Extract Signal's inline release logic into a reusable _release_phone_lock() helper. Also close the leaked httpx client on Signal health check failure. --- gateway/platforms/signal.py | 24 ++++-- gateway/platforms/slack.py | 8 ++ tests/gateway/test_platform_lock_release.py | 84 +++++++++++++++++++++ 3 files changed, 109 insertions(+), 7 deletions(-) create mode 100644 tests/gateway/test_platform_lock_release.py diff --git a/gateway/platforms/signal.py b/gateway/platforms/signal.py index 1629e08631e59..cc2b876736c04 100644 --- a/gateway/platforms/signal.py +++ b/gateway/platforms/signal.py @@ -230,9 +230,15 @@ async def connect(self) -> bool: resp = await self.client.get(f"{self.http_url}/api/v1/check", timeout=10.0) if resp.status_code != 200: logger.error("Signal: health check failed (status %d)", resp.status_code) + await self.client.aclose() + self.client = None + self._release_phone_lock() return False except Exception as e: logger.error("Signal: cannot reach signal-cli at %s: %s", self.http_url, e) + await self.client.aclose() + self.client = None + self._release_phone_lock() return False self._running = True @@ -243,6 +249,16 @@ async def connect(self) -> bool: logger.info("Signal: connected to %s", self.http_url) return True + def _release_phone_lock(self) -> None: + """Release the Signal phone scoped lock if held.""" + if self._phone_lock_identity: + try: + from gateway.status import release_scoped_lock + release_scoped_lock("signal-phone", self._phone_lock_identity) + except Exception as e: + logger.warning("Signal: Error releasing phone lock: %s", e, exc_info=True) + self._phone_lock_identity = None + async def disconnect(self) -> None: """Stop SSE listener and clean up.""" self._running = False @@ -270,13 +286,7 @@ async def disconnect(self) -> None: await self.client.aclose() self.client = None - if self._phone_lock_identity: - try: - from gateway.status import release_scoped_lock - release_scoped_lock("signal-phone", self._phone_lock_identity) - except Exception as e: - logger.warning("Signal: Error releasing phone lock: %s", e, exc_info=True) - self._phone_lock_identity = None + self._release_phone_lock() logger.info("Signal: disconnected") diff --git a/gateway/platforms/slack.py b/gateway/platforms/slack.py index 2e7bbee739ba4..35a46c4fa0519 100644 --- a/gateway/platforms/slack.py +++ b/gateway/platforms/slack.py @@ -189,6 +189,14 @@ async def handle_hermes_command(ack, command): except Exception as e: # pragma: no cover - defensive logging logger.error("[Slack] Connection failed: %s", e, exc_info=True) + # Release the platform lock so the next gateway start isn't blocked. + if self._token_lock_identity: + try: + from gateway.status import release_scoped_lock + release_scoped_lock('slack-app-token', self._token_lock_identity) + except Exception: + pass + self._token_lock_identity = None return False async def disconnect(self) -> None: diff --git a/tests/gateway/test_platform_lock_release.py b/tests/gateway/test_platform_lock_release.py new file mode 100644 index 0000000000000..90f561188aa1d --- /dev/null +++ b/tests/gateway/test_platform_lock_release.py @@ -0,0 +1,84 @@ +"""Tests for platform lock release on connect() failure. + +When a platform adapter acquires a scoped lock during connect() and then +fails (bad token, network error), the lock must be released so the next +gateway start isn't blocked with "already in use" errors. + +Discord got this fix in PR #5302. Slack and Signal were missed. +""" + +import asyncio +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + + +# --------------------------------------------------------------------------- +# Slack +# --------------------------------------------------------------------------- + +class TestSlackLockReleaseOnConnectFailure: + """gateway/platforms/slack.py — connect() must release lock on exception.""" + + @staticmethod + def _read_source() -> str: + import os + base = os.path.dirname(os.path.dirname(os.path.dirname(__file__))) + with open(os.path.join(base, "gateway", "platforms", "slack.py")) as f: + return f.read() + + def test_except_block_releases_lock(self): + """The except block in connect() should call release_scoped_lock.""" + src = self._read_source() + # Find the except block near the end of connect() + start = src.index("async def connect(") + end = src.index("\n async def disconnect", start) + connect_body = src[start:end] + + assert "release_scoped_lock" in connect_body, ( + "Slack connect() except block does not release the scoped lock — " + "next gateway start will be blocked" + ) + + +# --------------------------------------------------------------------------- +# Signal +# --------------------------------------------------------------------------- + +class TestSignalLockReleaseOnConnectFailure: + """gateway/platforms/signal.py — connect() must release lock on health check failure.""" + + @staticmethod + def _read_source() -> str: + import os + base = os.path.dirname(os.path.dirname(os.path.dirname(__file__))) + with open(os.path.join(base, "gateway", "platforms", "signal.py")) as f: + return f.read() + + def test_health_check_failure_releases_lock(self): + """Both return-False paths after health check must release the phone lock.""" + src = self._read_source() + start = src.index("# Health check") + end = src.index("self._running = True", start) + health_block = src[start:end] + + assert health_block.count("_release_phone_lock") >= 2, ( + "Signal health check failure paths do not release the phone lock — " + "both the non-200 and exception paths need _release_phone_lock()" + ) + + def test_release_phone_lock_method_exists(self): + """_release_phone_lock helper should exist for reuse.""" + src = self._read_source() + assert "def _release_phone_lock(self)" in src + + def test_health_check_failure_closes_client(self): + """Both return-False paths must also close the httpx client.""" + src = self._read_source() + start = src.index("# Health check") + end = src.index("self._running = True", start) + health_block = src[start:end] + + assert health_block.count("await self.client.aclose()") >= 2, ( + "Signal health check failure paths do not close the httpx client" + )