fix(matrix): classify sync auth failures by status and errcode, not substring (salvage #66878) - #80532
fix(matrix): classify sync auth failures by status and errcode, not substring (salvage #66878)#80532steveonjava wants to merge 4 commits into
Conversation
alt-glitch
added
type/bug
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.
I added two more classifier unit tests for the attribute narrowing: a bare .code attribute that happens to be 401, and a bare .status attribute that happens to be 403, both must stay classified as transient since only .http_status is trustworthy. I also added a parametrized test for the five transient exception types the sync loop now short-circuits on. On top of that I added two tests that exercise _sync_loop directly instead of just the classifier function in isolation. One replays the real 502 Umbrel repro string through a mocked client.sync and confirms the loop retries with the 5s backoff. The other raises a genuine M_FORBIDDEN error and confirms the loop stops on the first call with no retry sleep. These catch a regression in how the loop wires the classifier in, not just a regression in the classifier itself.
… 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>
7e503bd to
4e16313
Compare
|
This was generated by AI during triage. Summary: Problems:
Solution: Checked against |
|
Thanks for digging into the call sites. Three of your structural facts are correct and I want to confirm them before I get to where I think the conclusion breaks. Where the conclusion does not hold is the premise that a 403 or That branch is dead code inherited from the earlier On the remedy, your two options are not equivalent and I would rather not take the second one. Routing the object through the classifier works, because the classifier is typed What I would like to do is fix the incorrect comment, route the branch through the classifier with the message text as a secondary fallback so the code is correct under either library, and add a test that pins it. That is defence in depth against a future library swap rather than a live 403 bypass, so I do not think it should gate this PR, which fixes a failure that is happening now: a 502 HTML body containing the digits |
…the classifier The comment above the result-object branch in _sync_loop claimed mautrix's Client.sync() returns an object carrying a message string for auth failures. That is wrong. In the pinned mautrix 0.21.0, HTTPAPI._send raises make_request_error() for any non-2xx and otherwise returns parsed JSON, so a real M_FORBIDDEN arrives as an exception and is handled by the except branch. The claim was introduced by this PR, which rewrote an accurate comment about the earlier matrix-nio client (whose SyncError result objects were genuine). The branch itself is kept as defense in depth against a future client swap, but it now classifies with the same errcode/http_status logic as the exception path instead of a lone "unknown_token" substring test, which silently missed M_MISSING_TOKEN and M_FORBIDDEN and resynced forever against a credential that can never succeed. A structured errcode/http_status is authoritative; the message text is only consulted when the object exposes neither, since str(object) is an opaque repr. The text scan deliberately cannot override a structured verdict, so a transient 502 whose HTML body contains "Forbidden" is still retried. Adds four tests. Three are discriminating RED/GREEN cases that fail against the old substring branch (M_MISSING_TOKEN errcode, http_status=401 with no keyword in the message, and an unstructured object whose only signal is .message). The fourth pins the precedence rule and passes either way. Verified: 136 passed / 1 failed in tests/gateway/test_matrix.py; the single failure (test_password_login_uses_device_id) fails identically at the pristine PR head and is unrelated.
|
Thanks for the review. The requested cleanup is addressed in
The focused compatibility tests pass |
3 participants
What does this PR do?
The Matrix sync loop used a naive substring match on
str(exc)to detect permanent auth failures. Any401or403in an exception message stopped the loop permanently. In production, an Umbrel app-proxy 502 response included an SVG coordinate,40.4302, whose embedded403triggered the old check. Outbound messages still worked, but inbound sync was dead.The fix classifies real authentication failures from structured
errcodeandhttp_statusvalues, retries transient transport errors, and uses a bounded whole-word message scan only when structured signals are absent. It also keeps the result-object path defensive for compatibility with a future client swap.Related Issue
This PR carries forward
gmoranxyz's work from #66878, including the original diagnosis and patch. It consolidates the related work from #57375, #78039, #61206, and #66878.GottZ's consolidation triage identified #66878 as the surviving implementation and recommended closing #57375 as a duplicate.The sync watchdog that would restart a dead or stalled
_sync_taskremains out of scope and should be handled by a separate follow-up.Type of Change
Changes Made
plugins/platforms/matrix/adapter.py: replace substring-based auth detection with a layered classifier. Transport exceptions are always transient, structurederrcodeandhttp_statusvalues take precedence, and unstructured messages use a bounded word-boundary fallback.plugins/platforms/matrix/adapter.py: route compatibility result objects through the same classifier, preserving structured checks and using.messageonly when structured fields are absent.tests/gateway/test_matrix.py: add loop-level regression tests for the pagination-token false positive, real permanent auth failures, result-object errcodes and HTTP status, message-only compatibility results, transient structured 502 responses, attribute narrowing, and transient exception types.gmoranxyz. The PR retains that contribution with aCo-authored-bytrailer.How to Test
git diff --check. Result: passed.Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passpython -m pytest -x, but it recorded pre-existing or order-dependent failures. The Matrix suite itself had 133 passed.Documentation & Housekeeping
docs/, docstrings) — N/A (no user-facing documentation change was needed)cli-config.yaml.exampleif I added/changed config keys — N/A (no config keys changed)CONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — N/A (no contributor workflow changed)Screenshots / Logs
N/A. The regression is covered by automated tests and the verification results are listed above.