Skip to content

Fix Matrix sync halting permanently on transient 5xx outages - #66878

Open
gmoranxyz wants to merge 1 commit into
NousResearch:mainfrom
gmoranxyz:fix/matrix-502-clean
Open

Fix Matrix sync halting permanently on transient 5xx outages#66878
gmoranxyz wants to merge 1 commit into
NousResearch:mainfrom
gmoranxyz:fix/matrix-502-clean

Conversation

@gmoranxyz

Copy link
Copy Markdown

Problem

The Matrix sync loop classified any exception whose string form contained 401/403 as a permanent auth failure and stopped for good, requiring a manual gateway restart.

A homeserver blip (e.g. Synapse restarting behind a reverse proxy) surfaces as a 502 whose HTML body embeds an SVG — and that SVG's path coordinate 40.4302 contains the substring 403. So "403" in str(exc) matched, and a transient outage permanently killed Matrix sync.

Observed in production: sync stopped at a 502 and never retried; the process kept running but went silent on Matrix until restarted.

Fix

Replace the substring check with _is_permanent_matrix_auth_error(), which classifies on the real HTTP status / errcode:

  • Prefers structured attributes (http_status/status/errcode) when present.
  • Else parses the leading status code off the "<status>: <body>" string form — a known status is authoritative.
  • Only 401/403 (or auth errcodes like M_UNKNOWN_TOKEN/M_MISSING_TOKEN/M_FORBIDDEN) stop the loop; every 5xx/429/connection error retries.
  • Falls back to whole-word keyword matching in a bounded prefix only when no status is available, so a large HTML body can't smuggle a false positive.

Tests

Adds TestMatrixPermanentAuthClassifier, including the exact 502-with-403-coordinate payload that triggered the outage, plus 5xx/connection-error retry cases and genuine 401/403/errcode stop cases.

🤖 Generated with Claude Code

The sync loop classified any exception whose string contained "401"/"403"
as a permanent auth failure and stopped for good. A homeserver blip returns
a 502 whose HTML body embeds an SVG with the path coordinate "40.4302" — the
substring "403" — so a passing outage permanently killed Matrix sync until a
manual gateway restart.

Classify on the real HTTP status / errcode instead: only genuine 401/403 (or
auth errcodes like M_UNKNOWN_TOKEN) stop the loop; every 5xx/429/connection
error retries. Add regression coverage including the exact 502-with-"403"-
coordinate payload.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XcKQjcEM1oEamxUmiZ9nAY
@alt-glitch alt-glitch added type/bug Something isn't working comp/plugins Plugin system and bundled plugins platform/matrix Matrix adapter (E2EE) P3 Low — cosmetic, nice to have needs-decision Awaiting maintainer decision before any implementation sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages labels Jul 18, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.
Related to #57375 and the Matrix auth-classifier cluster: this patch also fixes the substring false-positive, but keeps bounded parsing for unstructured status/error text that #57375 deliberately drops. It is a broader competing repair; maintainer choice is needed.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for isolating a real Matrix sync failure mode. Current main still uses substring matching in plugins/platforms/matrix/adapter.py:2420-2430, so the reported transient-502 false positive is present.

Problems

  • The added tests invoke the helper directly. They do not exercise _sync_loop's retry-versus-return control flow at plugins/platforms/matrix/adapter.py:2416-2432.

Suggested changes

  • Add an async loop-level regression test that makes client.sync raise the 502/SVG payload and verifies the retry path is selected.
  • The member triage comment links competing PR fix(matrix): stop classifying auth errors by substring match #57375, whose classifier accepts only structured http_status/errcode. This PR deliberately retains bounded parsing of unstructured text; please make that scope choice explicit during salvage.

Automated hermes-sweeper review.

@teknium1 teknium1 added the sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform label Jul 19, 2026
@GottZ

GottZ commented Aug 3, 2026

Copy link
Copy Markdown

This was generated by AI during triage.

Summary

Two open PRs address the Matrix sync-loop false positive: #57375 replaces exception-text substring matching with structured mautrix status/errcode fields, while #66878 adds a broader classifier with structured fields, bounded textual fallbacks, and direct tests for the reported 502/SVG payload.

Related pull requests

  • fix(matrix): stop classifying auth errors by substring match #57375 duplicate — (+11/-7) — n/a: Replaces the unsafe substring classifier with structured http_status/errcode checks, directly preventing proxy-body digits from terminating sync. Its keep_open review identifies missing regression coverage, incompatibility with the existing plain-RuntimeError test, and omission of M_UNAUTHORIZED.
  • Fix Matrix sync halting permanently on transient 5xx outages #66878 related — (+104/-8) — n/a: Fixes the same root cause through a dedicated classifier covering structured statuses, leading textual statuses, bounded auth keywords, and direct tests for the 502/SVG false positive. Consistent with its automated keep_open verdict, salvage still requires an async _sync_loop test proving retry-versus-return behavior and an explicit decision on retaining unstructured-text fallback.

Duplicates

#57375 and #66878 substantially duplicate the replacement of broad str(exc) substring classification; #66878 is the broader, test-bearing implementation, while #57375 takes the stricter structured-data-only approach.

Suggested consolidation

Keep #66878 open with a salvage path: add the requested loop-level regression, explicitly resolve the bounded unstructured-text fallback policy, and consider the competing review's M_UNAUTHORIZED finding. Close #57375 as a duplicate of #66878 despite its keep_open review: its diff addresses the same classifier but is narrower, lacks regression tests, and has the specifically documented test-contract and errcode omissions; preserve its structured-data-only rationale during #66878 salvage.

Complex graph

flowchart LR
    classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
    classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
    classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
    classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
    classDef best stroke-width:3px,stroke:#b45309
    classDef target stroke-width:3px,stroke:#4338ca
    subgraph Dup57375 ["PRs duplicating each other"]
        P57375["PR #57375 (open)"]
        P66878["PR #66878 (open)"]
    end
    class P57375 open
    class P66878 open
    class P66878 target
    click P57375 "https://github.com/NousResearch/hermes-agent/pull/57375"
    click P66878 "https://github.com/NousResearch/hermes-agent/pull/66878"
Loading

Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label).

Cross-PR triage: Reviewed 2 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 8 kB of PR diffs, 3 kB of issue/PR text, 2 kB of discussion (3 comments), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

@steveonjava

Copy link
Copy Markdown
Contributor

I hit this issue and it is quite frustrating that small network blips intermittently take out the matrix communication. To help move things along, I created a new PR that addresses @teknium1's feedback and credits @gmoranxyz for the great work on the original fix. #80532

steveonjava added a commit to steveonjava/hermes-agent that referenced this pull request Aug 20, 2026
fix(matrix): classify sync auth failures by status and errcode, not substring (salvage NousResearch#66878)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/plugins Plugin system and bundled plugins needs-decision Awaiting maintainer decision before any implementation P3 Low — cosmetic, nice to have platform/matrix Matrix adapter (E2EE) sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants