Skip to content

fix(matrix): classify sync auth failures by status and errcode, not substring (salvage #66878) - #1

Merged
steveonjava merged 4 commits into
mainfrom
feat/matrix-sync-401-salvage
Aug 20, 2026
Merged

fix(matrix): classify sync auth failures by status and errcode, not substring (salvage #66878)#1
steveonjava merged 4 commits into
mainfrom
feat/matrix-sync-401-salvage

Conversation

@steveonjava

Copy link
Copy Markdown
Owner

This is gmoranxyz's work in #66878. The diagnosis and original patch are theirs. That PR went quiet after the maintainer left review asks open, so this carries it across the finish line.

Problem

MatrixAdapter._sync_loop() decided whether a sync failure was permanent by substring-matching the stringified exception for "401", "403", "unauthorized", "forbidden". The stringified exception embeds the request URL, and the Matrix sync pagination token is an arbitrary digit string:

Connection timeout to host https://.../_matrix/client/v3/sync?timeout=30000&since=s72802_401975_486...
                                                                                    ^^^^^^

"401" in err_str evaluates to True, so a plain network timeout was classified as a permanent auth error and the loop returned. _sync_task is created once at startup and never restarted, so the adapter went permanently deaf on Matrix while the process stayed healthy.

Real-world triggers include an Umbrel app-proxy 502 whose SVG coordinate 40.4302 contains 403, and since-token digits in the sync URL. The symptom is nasty because the bot looks alive: inbound dies, outbound keeps working.

What this salvage adds

Each item maps to teknium1's review asks on NousResearch#66878:

  • Async loop-level regression test exercising _sync_loop's retry-versus-return flow (primary ask; original tests only called the helper directly). The fixture uses a plain connection timeout whose message wraps the real Matrix sync pagination token, which happens to contain 401. RED/GREEN proof: pre-fix classifier fails, fix passes.
  • Explicit rationale for the bounded unstructured-text fallback (deliberate divergence from fix(matrix): stop classifying auth errors by substring match NousResearch/hermes-agent#57375's structured-only approach). Some exceptions lack .http_status or .errcode, so a word-boundary-safe scan on the first 200 characters provides a safety net without re-introducing false positives.
  • M_UNAUTHORIZED considered. Found that M_UNAUTHORIZED is a distinct Matrix error code from M_UNKNOWN_TOKEN and M_FORBIDDEN. The classifier does not currently include it in the permanent errcode set. This is acceptable because the M_UNKNOWN_TOKEN pre-check on the returned sync object already covers the real revocation case, and M_UNAUTHORIZED is rarely emitted by modern homeservers in the sync path.
  • Two extra hardening fixes found during review:
    • Tightened the attribute probe: no more first-int-wins across status/code/http_status. Each attribute is checked independently.
    • Transient exception types (TimeoutError, ConnectionError, OSError) are short-circuited before any keyword logic, so a timeout can never be misread regardless of its message content.

Cluster context

Four PRs address this same root cause: NousResearch#57375, NousResearch#78039, NousResearch#61206, and NousResearch#66878. GottZ's consolidation triage named NousResearch#66878 the survivor and recommended closing NousResearch#57375 as a duplicate. This PR is that consolidation carried out: it takes NousResearch#66878's approach, closes the review gaps, and lands it upstream. Not a fifth competing entry.

Testing

# Own suite (tests/gateway/test_matrix.py)
python -m pytest tests/gateway/test_matrix.py -v
# 133 passed in 3.59s

# Full suite (pre-existing noise, zero overlap with this change)
python -m pytest -x
# 1210 failed, 24328 passed, 324 skipped, 1 xfailed, 17 errors in 2980.32s
# Zero failures in tests/gateway/test_matrix.py

# Lint
ruff check plugins/platforms/matrix/adapter.py tests/gateway/test_matrix.py
# All checks passed

Explicitly out of scope

The sync watchdog (restart a dead or stalled _sync_task). This is a real remaining gap that none of the four PRs in this cluster address. A follow-up is planned.


Diff: 2 files changed, 274 insertions(+), 13 deletions(-)
Commits: 3 (all conventional, all Co-authored-by gmoranxyz)

@steveonjava
steveonjava force-pushed the feat/matrix-sync-401-salvage branch from 43514d4 to 7e503bd Compare August 6, 2026 18:53
steveonjava and others added 3 commits August 7, 2026 05:00
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>
@steveonjava
steveonjava force-pushed the feat/matrix-sync-401-salvage branch from 7e503bd to 4e16313 Compare August 7, 2026 13:01
@steveonjava steveonjava closed this Aug 7, 2026
@steveonjava steveonjava reopened this Aug 7, 2026
…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.
steveonjava pushed a commit that referenced this pull request Aug 13, 2026
…rst run

The first-run provider picker showed Fireworks AI alongside Nous Portal
before the user opened the 'Other providers' disclosure. Only Nous Portal
should be visible up front; Fireworks now lives inside the expanded list
but keeps its #1 position there (Nous -> Fireworks ordering preserved).
@steveonjava
steveonjava marked this pull request as ready for review August 20, 2026 01:52
@steveonjava
steveonjava merged commit ee00568 into main Aug 20, 2026
42 checks passed
steveonjava pushed a commit that referenced this pull request Aug 24, 2026
posix.sh now probes `update --help` before the real update call; the fake
counted the probe as call #1, shifting the exits.N mapping so the retry
gate never fired. Answer the probe out-of-band so counted calls remain
actual update attempts.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant