fix(matrix): tighten sync error classifier
I hit a bug where the Matrix sync loop treated a passing 502 from Umbrel's app proxy as a permanent auth failure and stopped syncing for good. The old check did a naive "403" in str(exc) substring match, and the 502 HTML error body embedded an SVG path with the coordinate 40.4302, which contains the digit sequence 403. I replaced the substring check with a layered classifier. Transport exceptions like TimeoutError, ConnectionError, and OSError are always treated as transient regardless of their message text. Structured signals take priority next: the errcode attribute against a known set of permanent Matrix error codes, then the http_status attribute against 401/403 specifically (not status, status_code, or code, which belong to unrelated exception shapes and risk coincidental integer matches). Only when none of those are present does it fall back to a bounded, word-boundary-safe text scan on the first 200 characters. Added tests covering the attribute narrowing, the transient exception types, and two loop-level tests exercising _sync_loop directly to confirm it retries on a transient error and stops on a genuine 401/403. (cherry picked from commit 96d3363e45a63e08d9f07518ed949df334a33b3c)
This commit is contained in:
Stephen Chin
kshitij
@@ -195,6 +195,55 @@ def _strip_reply_fallback(body: str) -> str:
|
||||
continue
|
||||
stripped.append(line)
|
||||
return "\n".join(stripped) if stripped else body
|
||||
# Auth errcodes that genuinely require re-authentication (never retried).
|
||||
_MATRIX_PERMANENT_ERRCODES = frozenset({
|
||||
"m_unknown_token",
|
||||
"m_missing_token",
|
||||
"m_forbidden",
|
||||
})
|
||||
# Leading HTTP status on error strings formatted as ``"<status>: <body>"``.
|
||||
_MATRIX_LEADING_STATUS_RE = re.compile(r"^\s*(\d{3})\b")
|
||||
|
||||
|
||||
def _is_permanent_matrix_auth_error(exc: object) -> bool:
|
||||
"""Return True only for genuine auth failures that must stop the sync loop.
|
||||
|
||||
A transient homeserver outage surfaces as a 5xx whose body may be an HTML
|
||||
error page (Umbrel's app-proxy returns one). Naive substring checks like
|
||||
``"403" in str(exc)`` false-positive on digits embedded in that HTML (an SVG
|
||||
coordinate such as ``40.4302`` contains ``403``), which previously stopped
|
||||
the sync loop permanently on a passing blip. Classify on the real HTTP
|
||||
status / errcode instead, and retry everything that is not 401/403.
|
||||
"""
|
||||
# Prefer structured attributes when the exception carries them.
|
||||
errcode = getattr(exc, "errcode", None)
|
||||
if (
|
||||
isinstance(errcode, str)
|
||||
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)
|
||||
|
||||
# Fall back to parsing a leading status code off the string form
|
||||
# (e.g. ``"502: <!DOCTYPE html>..."``). 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.
|
||||
head = text[:200].lower()
|
||||
return bool(
|
||||
re.search(
|
||||
r"\b(m_unknown_token|m_missing_token|m_forbidden|unauthorized|forbidden)\b",
|
||||
head,
|
||||
)
|
||||
)
|
||||
|
||||
|
||||
class _MatrixHtmlSanitizer(HTMLParser):
|
||||
@@ -1776,8 +1825,9 @@ class MatrixAdapter(BasePlatformAdapter):
|
||||
except Exception as exc:
|
||||
if self._closing:
|
||||
return
|
||||
if any(k in str(exc).lower() for k in ("401", "403", "unauthorized", "forbidden")):
|
||||
logger.error("Matrix: permanent auth error: %s — stopping sync", exc)
|
||||
# Detect permanent auth/permission failures. Transient 5xx outages must retry.
|
||||
if _is_permanent_matrix_auth_error(exc):
|
||||
logger.error("Matrix: permanent auth error, stopping sync: %s", exc)
|
||||
return
|
||||
logger.warning("Matrix: sync error: %s — retrying in 5s", exc)
|
||||
await asyncio.sleep(5)
|
||||
|
||||
@@ -3342,3 +3342,58 @@ class TestCryptoPickleKeyMigration:
|
||||
# start still sees a legacy-key account and retries the migration.
|
||||
store.put_account.assert_not_awaited()
|
||||
assert "retried on the next start" in caplog.text
|
||||
|
||||
|
||||
class TestMatrixPermanentAuthClassifier:
|
||||
"""Only genuine 401/403 auth failures may stop the sync loop.
|
||||
|
||||
A transient homeserver outage surfaces as a 5xx whose body may be an HTML
|
||||
error page. The Umbrel app-proxy returns one whose embedded SVG contains the
|
||||
coordinate ``40.4302``, which contains the substring ``403``. The old
|
||||
``"403" in str(exc)`` check false-positived on that and permanently halted
|
||||
Matrix sync on a passing blip. The classifier must retry any status that is
|
||||
not 401/403.
|
||||
"""
|
||||
|
||||
# Trimmed excerpt of the actual Umbrel 502 body that caused the outage;
|
||||
# the SVG path coordinate 40.4302 contains the substring "403".
|
||||
_REAL_502 = (
|
||||
'502: <!DOCTYPE html><svg><path d="M17.4517 40.4302C12.7214 40.4302'
|
||||
' 9.82339 41.8182 7.98048 44.0001"/></svg>'
|
||||
)
|
||||
|
||||
def _fn(self):
|
||||
from plugins.platforms.matrix.adapter import _is_permanent_matrix_auth_error
|
||||
|
||||
return _is_permanent_matrix_auth_error
|
||||
|
||||
def test_transient_502_html_body_is_retried(self):
|
||||
# The exact failure mode: a 502 whose HTML body embeds "403".
|
||||
assert self._fn()(Exception(self._REAL_502)) is False
|
||||
|
||||
@pytest.mark.parametrize("status", [500, 502, 503, 504, 429])
|
||||
def test_server_errors_are_retried(self, status):
|
||||
assert self._fn()(Exception(f"{status}: upstream unavailable")) is False
|
||||
|
||||
def test_connection_errors_are_retried(self):
|
||||
fn = self._fn()
|
||||
assert fn(Exception("[Errno 104] Connection reset by peer")) is False
|
||||
assert fn(Exception("Server disconnected")) is False
|
||||
|
||||
@pytest.mark.parametrize("status", [401, 403])
|
||||
def test_real_auth_status_stops_sync(self, status):
|
||||
assert self._fn()(Exception(f"{status}: nope")) is True
|
||||
|
||||
def test_http_status_attribute_beats_body_digits(self):
|
||||
# A structured 502 whose message text also contains "403" must retry.
|
||||
exc = Exception("body 40.4302")
|
||||
exc.http_status = 502
|
||||
assert self._fn()(exc) is False
|
||||
|
||||
def test_errcode_unknown_token_stops_sync(self):
|
||||
exc = Exception("sync failed")
|
||||
exc.errcode = "M_UNKNOWN_TOKEN"
|
||||
assert self._fn()(exc) is True
|
||||
|
||||
def test_bare_auth_keyword_without_status_stops_sync(self):
|
||||
assert self._fn()(Exception("Unauthorized")) is True
|
||||
|
||||
Reference in New Issue
Block a user