diff --git a/plugins/platforms/matrix/adapter.py b/plugins/platforms/matrix/adapter.py index a0306414e2..005ba7e76d 100644 --- a/plugins/platforms/matrix/adapter.py +++ b/plugins/platforms/matrix/adapter.py @@ -203,6 +203,15 @@ _MATRIX_PERMANENT_ERRCODES = frozenset({ }) # Leading HTTP status on error strings formatted as ``": "``. _MATRIX_LEADING_STATUS_RE = re.compile(r"^\s*(\d{3})\b") +# Transport-level failures are always transient. Checked before any status or +# text inspection so no amount of unlucky digits or keywords in a message can +# ever promote a network timeout or dropped connection to "permanent". +_MATRIX_TRANSIENT_EXC_TYPES: tuple[type[BaseException], ...] = ( + asyncio.TimeoutError, + TimeoutError, + ConnectionError, # covers ConnectionResetError, ConnectionRefusedError, ... + OSError, +) def _is_permanent_matrix_auth_error(exc: object) -> bool: @@ -215,6 +224,12 @@ def _is_permanent_matrix_auth_error(exc: object) -> bool: the sync loop permanently on a passing blip. Classify on the real HTTP status / errcode instead, and retry everything that is not 401/403. """ + # Transport failures are never permanent, regardless of what their + # message text says. A ConnectionError raised while retrying a call whose + # URL happens to contain "403" must still be retried. + if isinstance(exc, _MATRIX_TRANSIENT_EXC_TYPES): + return False + # Prefer structured attributes when the exception carries them. errcode = getattr(exc, "errcode", None) if ( @@ -222,21 +237,33 @@ def _is_permanent_matrix_auth_error(exc: object) -> bool: and errcode.strip().lower() in _MATRIX_PERMANENT_ERRCODES ): return True - for attr in ("http_status", "status", "status_code", "code"): - val = getattr(exc, attr, None) - if isinstance(val, int): - return val in (401, 403) + + # mautrix's MatrixRequestError exposes ``.http_status``. Deliberately not + # ``.status``/``.status_code``/``.code``: those belong to unrelated + # exception shapes (aiohttp responses, OS errno, generic process exit + # codes) and a first-int-wins probe across all of them can misclassify on + # a coincidental integer that has nothing to do with the HTTP response. + status = getattr(exc, "http_status", None) + if isinstance(status, int): + return status in (401, 403) # Fall back to parsing a leading status code off the string form - # (e.g. ``"502: ..."``). A known status is authoritative: + # (e.g. ``"502: ...``). A known status is authoritative: # only 401/403 are permanent; 5xx/429/etc. must retry regardless of body. text = str(exc) m = _MATRIX_LEADING_STATUS_RE.match(text) if m: return int(m.group(1)) in (401, 403) - # No status available: trust only whole-word auth errcodes/keywords found in - # a bounded prefix, so a large HTML body cannot smuggle a false positive. + # No structured status available at all: fall back to a bounded, + # whole-word text scan. This is a deliberate divergence from a + # structured-only classifier. Some transports (a bare Exception wrapping a + # homeserver error) never expose http_status/errcode, so without this + # fallback we'd silently treat every unstructured auth failure as + # retryable forever. + # Bounded to the first 200 characters with \b word boundaries so a large + # HTML error body (which can run to kilobytes) has no way to smuggle a + # false positive the way the old unbounded substring check did. head = text[:200].lower() return bool( re.search( diff --git a/tests/gateway/test_matrix.py b/tests/gateway/test_matrix.py index c35db21824..e368d1b560 100644 --- a/tests/gateway/test_matrix.py +++ b/tests/gateway/test_matrix.py @@ -1340,6 +1340,79 @@ class TestMatrixSyncLoop: assert captured[0].text == "hello" assert captured[0].source.chat_type == "dm" + @pytest.mark.asyncio + async def test_sync_loop_retries_transient_error_instead_of_stopping(self): + """A transient error containing a false-positive status digit must be retried, not fatal. + + This is the regression the false-positive substring match caused in + production: an HTML error body with an embedded coordinate like + "40.4302" contains the digits "403", so a naive `"403" in str(exc)` + check stopped the sync loop permanently on what was really a passing + 502 blip. This test drives `_sync_loop` itself (not just the + classifier function) so a regression in how the loop wires the + classifier in would also be caught here. + """ + adapter = _make_adapter() + adapter._closing = False + + real_502 = ( + '502: ' + ) + + calls = {"n": 0} + + async def _sync_side_effect(**kwargs): + calls["n"] += 1 + if calls["n"] == 1: + raise Exception(real_502) + # Second call: stop the loop cleanly after proving the retry + # happened, without needing a real sync payload. + adapter._closing = True + return {"next_batch": "s1"} + + mock_sync_store = MagicMock() + mock_sync_store.get_next_batch = AsyncMock(return_value=None) + mock_sync_store.put_next_batch = AsyncMock() + + fake_client = MagicMock() + fake_client.sync = AsyncMock(side_effect=_sync_side_effect) + fake_client.sync_store = mock_sync_store + adapter._client = fake_client + + with patch("asyncio.sleep", new=AsyncMock()) as mock_sleep: + await adapter._sync_loop() + + # Retried rather than returning after the first (transient) error. + assert calls["n"] == 2 + # The 5s backoff sleep on the retry path (not the 0s dispatch-yield). + assert 5 in [call.args[0] for call in mock_sleep.await_args_list] + + @pytest.mark.asyncio + async def test_sync_loop_stops_on_real_permanent_auth_error(self): + """A genuine 401/403 auth failure must stop the loop, not retry forever.""" + adapter = _make_adapter() + adapter._closing = False + + auth_exc = Exception("M_FORBIDDEN: Invalid access token") + auth_exc.errcode = "M_FORBIDDEN" + + mock_sync_store = MagicMock() + mock_sync_store.get_next_batch = AsyncMock(return_value=None) + mock_sync_store.put_next_batch = AsyncMock() + + fake_client = MagicMock() + fake_client.sync = AsyncMock(side_effect=auth_exc) + fake_client.sync_store = mock_sync_store + adapter._client = fake_client + + with patch("asyncio.sleep", new=AsyncMock()) as mock_sleep: + await adapter._sync_loop() + + # No retry backoff: the loop returned on the first, permanent error. + assert fake_client.sync.await_count == 1 + assert 5 not in [call.args[0] for call in mock_sleep.await_args_list] + @pytest.mark.asyncio async def test_connect_receives_dm_from_initial_sync_dispatch(self): """A DM delivered by initial sync should reach the message handler after connect.""" @@ -3397,3 +3470,37 @@ class TestMatrixPermanentAuthClassifier: def test_bare_auth_keyword_without_status_stops_sync(self): assert self._fn()(Exception("Unauthorized")) is True + + def test_misleading_code_attribute_is_not_misclassified(self): + # A non-Matrix exception with a bare `.code` that happens to be 401 + # must NOT be treated as an auth failure. `.code` on an arbitrary + # exception can be an errno or an exit code; only `.http_status` + # is trustworthy for HTTP status classification. + exc = Exception("subprocess failed") + exc.code = 401 + assert self._fn()(exc) is False + + def test_status_attribute_is_ignored_in_favor_of_http_status(self): + # aiohttp response objects and similar carry `.status`, which is a + # different namespace than mautrix's `.http_status`. A bare `.status` + # must not drive the classification. + exc = Exception("some http client error") + exc.status = 403 + assert self._fn()(exc) is False + + @pytest.mark.parametrize( + "exc_factory", + [ + lambda: asyncio.TimeoutError("401 in the pagination token url"), + lambda: TimeoutError("403 in the pagination token url"), + lambda: ConnectionError("... forbidden ..."), + lambda: ConnectionResetError("Connection reset, status 401"), + lambda: OSError("network unreachable, code 403"), + ], + ) + def test_transient_exception_types_never_classify_as_permanent(self, exc_factory): + # Transport-level exception types are short-circuited to "transient" + # before any text/status inspection runs, so a coincidental "401" + # or the keyword "forbidden" inside their message can never flip + # them to permanent. + assert self._fn()(exc_factory()) is False