fix(matrix): use a genuinely discriminating fixture for the sync-loop test

The independent-verifier caught that my first loop-level test did not
actually prove anything. The 502/SVG coordinate fixture I reused from
gmoranxyz's unit-level test does not contain the substring 403 once
case-folded, so the old naive substring classifier already treated it
as transient. A test that passes under both the buggy code and the
fix proves nothing about the fix.

I replaced the fixture with a plain connection timeout whose message
wraps the real Matrix sync pagination token, an arbitrary digit
string that happens to contain 401. I verified this directly: with
the pre-fix classifier restored, the retry test now fails (the old
code stops the loop on this fixture), and with the fix in place it
passes (the loop retries as it should). That is the RED/GREEN proof
the maintainer originally asked for.

I also documented in the stop test's docstring that it does not
discriminate old from new, since the word forbidden in its message
trips the old naive check too. It is still worth keeping as a
regression test proving genuine auth errors stop the loop, just not
as proof of this specific fix.

While I was in there I also fixed a stale comment above the
M_UNKNOWN_TOKEN sync-object pre-check. It said nio returns SyncError
objects, but the dependency here is mautrix, not matrix-nio, and
importing nio raises ModuleNotFoundError in this codebase. The
pre-check logic itself was already correct and untouched.

Co-authored-by: gmoranxyz <gmoranxyz@users.noreply.github.com>
(cherry picked from commit ad3aad579a675a5aae544a50f88a82717c0ac3b6)

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)

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: <!DOCTYPE html><svg><path d="M17.4517 40.4302'
-            'C12.7214 40.4302 9.82339 41.8182 7.98048 44.0001"/></svg>'
+        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