diff --git a/plugins/platforms/matrix/adapter.py b/plugins/platforms/matrix/adapter.py index 005ba7e76d..f8b91992f9 100644 --- a/plugins/platforms/matrix/adapter.py +++ b/plugins/platforms/matrix/adapter.py @@ -1837,9 +1837,28 @@ class MatrixAdapter(BasePlatformAdapter): next_batch = await client.sync_store.get_next_batch() # resume from the initial sync while not self._closing: try: +<<<<<<< HEAD # 45s outer cap guards TCP-level hangs the 30s long-poll timeout can't catch. sync_data = await asyncio.wait_for(client.sync(since=next_batch, timeout=30000), timeout=45.0) # Auth failures (M_UNKNOWN_TOKEN) arrive as SyncError objects, not exceptions. +======= + # Wrap in asyncio.wait_for to guard against TCP-level hangs + # that the Matrix long-poll timeout cannot catch. Long-poll + # is 30s, so 45s gives 15s slack for network drain. + sync_data = await asyncio.wait_for( + client.sync( + since=next_batch, + timeout=30000, + ), + timeout=45.0, + ) + + # mautrix's Client.sync() returns a plain dict on success but + # an object carrying a "message" string (not a raised + # exception) for auth failures like M_UNKNOWN_TOKEN. Detect + # and stop immediately rather than falling through to the + # dict-shaped handling below. +>>>>>>> 4e16313582 (fix(matrix): use a genuinely discriminating fixture for the sync-loop test) _sync_msg = getattr(sync_data, "message", None) if isinstance(_sync_msg, str) and "unknown_token" in _sync_msg.lower(): logger.error("Matrix: permanent auth error from sync: %s — stopping", _sync_msg) diff --git a/tests/gateway/test_matrix.py b/tests/gateway/test_matrix.py index e368d1b560..ebb78080d1 100644 --- a/tests/gateway/test_matrix.py +++ b/tests/gateway/test_matrix.py @@ -1342,22 +1342,36 @@ class TestMatrixSyncLoop: @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. + """A timeout whose message text incidentally contains "401" 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. + This is the actual production regression: a plain connection timeout + wraps the sync pagination token in its message, and that token is an + arbitrary digit string that happens to contain "401". The old naive + classifier did `"401" in str(exc).lower()` and stopped the sync loop + permanently on what was really a transient timeout with no auth + failure at all. + + I confirmed this fixture actually discriminates the two classifiers + (see _verify_fixture.py, run manually against both): the OLD + substring check classifies it "permanent" (bug reproduces) and the + NEW layered classifier classifies it "transient" (fix works), since + the digits sit inside a pagination token, not a structured + http_status/errcode and not a whole-word match in the bounded + prefix scan. A fixture that both classifiers agree on (like the + old 502/SVG body used before this rework) proves nothing, since + agreement means nothing was actually being tested. + + 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: ' + transient_timeout_with_incidental_401 = ( + "Connection timeout to host https://matrix.example.org/_matrix/" + "client/v3/sync?timeout=30000&since=s72802_401975_486_12943_11759" + "_12_1514_279_0_1_2_1_1" ) calls = {"n": 0} @@ -1365,7 +1379,7 @@ class TestMatrixSyncLoop: async def _sync_side_effect(**kwargs): calls["n"] += 1 if calls["n"] == 1: - raise Exception(real_502) + raise Exception(transient_timeout_with_incidental_401) # Second call: stop the loop cleanly after proving the retry # happened, without needing a real sync payload. adapter._closing = True @@ -1390,7 +1404,17 @@ class TestMatrixSyncLoop: @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.""" + """A genuine 401/403 auth failure must stop the loop, not retry forever. + + Note this fixture is not a discriminating RED/GREEN test between the + old and new classifier. The message text contains the word + "forbidden", so the old naive substring check also stops the loop + here, just for the wrong reason (a keyword match instead of a + structured errcode). I kept this test because it proves the loop + still stops on a real auth failure after the classifier rewrite, a + straightforward regression check, not proof that the fix changed + behavior on this specific input. + """ adapter = _make_adapter() adapter._closing = False