Skip to content

fix(simplex): escalate non-retryable WebSocket handshake rejections instead of looping forever - #35557

Open
lambertian wants to merge 1 commit into
NousResearch:mainfrom
lambertian:fix/simplex-handshake-fatal-escalation
Open

fix(simplex): escalate non-retryable WebSocket handshake rejections instead of looping forever#35557
lambertian wants to merge 1 commit into
NousResearch:mainfrom
lambertian:fix/simplex-handshake-fatal-escalation

Conversation

@lambertian

@lambertian lambertian commented May 30, 2026

Copy link
Copy Markdown

Summary

When the simplex-chat daemon rejects the WebSocket handshake with an HTTP status (e.g. a permanently misconfigured endpoint, wrong path, or auth failure), the websockets client raises InvalidStatus. Because InvalidStatus is a subclass of WebSocketException, the SimpleX adapter's reconnect loop (_ws_listener) caught it in the generic reconnect arm and busy-looped forever with exponential backoff — never surfacing a gateway-visible fatal state, so operators got no signal that the connection could never succeed.

This adds a specific arm that catches the handshake-rejection exception and inspects the HTTP status:

  • 4xx (permanent misconfiguration that can never succeed on retry) → call _set_fatal_error("handshake_rejected", ..., retryable=False) and break out of the reconnect loop, matching the fatal-error convention used by the base adapter and siblings (e.g. IRC, Mattermost's permanent-auth-failure stop).
  • 5xx / unknown → treated as potentially transient and left to the existing reconnect path, so behavior there is unchanged.

Transient errors (ConnectionClosed, other WebSocketExceptions) keep retrying exactly as before.

A small helper, _handshake_status_code, reads the status from either the newer InvalidStatus (.response.status_code) or the older InvalidStatusCode (.status_code), and the import is guarded so the adapter works across websockets versions.

Tests

Added two focused async tests to tests/gateway/test_simplex_plugin.py:

  • test_ws_listener_escalates_4xx_handshake_rejection — patches websockets.connect to raise a real InvalidStatus(403), runs _ws_listener, and asserts _set_fatal_error is called exactly once with retryable=False and the loop exits. It wraps the listener in asyncio.wait_for(..., timeout=5) so the old busy-loop behavior fails fast instead of hanging.
  • test_ws_listener_retries_5xx_handshake_rejection — a 503 rejection is retried (loop does not escalate), guarding against over-broad escalation.

The 4xx test is red on current main (the handshake rejection falls into the generic reconnect arm and the loop never exits, tripping the 5s timeout) and green with this change. The 5xx test passes both before and after, confirming transient behavior is preserved.

(One unrelated test in the file, test_standalone_send_missing_url, is environment-dependent and is not affected by this change; where it fails, it fails identically on clean main.)

Notes

The SimpleX adapter's reconnect loop caught a rejected WebSocket
handshake (websockets InvalidStatus, a WebSocketException subclass) in
the generic reconnect arm and busy-looped forever with no gateway-visible
fatal state — so a permanently misconfigured daemon (bad path/auth/4xx)
never surfaced an error.

Add a specific arm in _ws_listener that catches the handshake-rejection
exception and reads the HTTP status:
- 4xx: permanent, call _set_fatal_error(..., retryable=False) and break.
- 5xx/unknown: left to the existing reconnect path (possibly transient).

A _handshake_status_code helper reads the status from the newer
InvalidStatus (.response.status_code) or the older InvalidStatusCode
(.status_code); the import is guarded for cross-version support.
Transient errors (ConnectionClosed etc.) keep retrying as before.

Adds tests asserting a 4xx escalates to a non-retryable fatal and exits,
while a 5xx is retried.
@alt-glitch alt-glitch added type/bug Something isn't working comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have labels May 30, 2026

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

Verdict: Approved

Review Findings

This PR escalates non-retryable 4xx WebSocket handshake rejections to a fatal error state instead of busy-looping forever. Previously, a permanently misconfigured daemon (wrong path, auth failure) would cause infinite reconnection attempts with no operator signal.

✅ Looks Good

  • Design: Clear distinction between 4xx (permanent — fatal escalation) and 5xx/unknown (transient — keep retrying). This matches the adapter pattern used by IRC and Mattermost.
  • _handshake_status_code helper: Handles both websockets >= 14 (InvalidStatus.response.status_code) and older InvalidStatusCode.status_code — good cross-version compatibility.
  • Import guard: The try/except import for InvalidStatus/InvalidStatusCode is clean and maintainable.
  • Error message: "SimpleX daemon rejected WebSocket handshake (HTTP {status})" — informative and actionable.
  • Tests:
    • test_ws_listener_escalates_4xx_handshake_rejection — patches websockets.connect with a real InvalidStatus(403), verifies _set_fatal_error called exactly once with retryable=False, and uses asyncio.wait_for(timeout=5) to catch the old busy-loop behavior.
    • test_ws_listener_retries_5xx_handshake_rejection — verifies 503 keeps retrying without escalation, guarding against over-broad escalation.
  • Diff: 157 additions, 0 deletions — all new code, no risk of regression.

No Issues Found


Reviewed by Hermes Agent

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

Verdict: Approved

Review

Fixes SimpleX WebSocket handshake rejection causing an infinite reconnect loop. 4xx handshake rejections (permanent misconfiguration) now escalate to fatal error instead of retrying forever.

✅ Looks Good

  • Correct escalation: 4xx → fatal, non-retryable. 5xx → still retried as potentially transient.
  • Version-safe: Handles both newer InvalidStatus and older InvalidStatusCode from websockets library.
  • Good test coverage: Two focused async tests — 4xx escalates, 5xx retries.
  • Prevents operator-blind failures: Gateway-visible fatal state means operators get a signal instead of silent busy-loop.

Reviewed by Hermes Agent (cron job)

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for isolating a real reconnect-loop gap. Current main still catches a rejected InvalidStatus in the generic retry arm at plugins/platforms/simplex/adapter.py:332-344; a controlled 403 repro logged a reconnect and left fatal_error unset.

Problems

  • plugins/platforms/simplex/adapter.py:284 treats every 4xx as permanently fatal. The PR proves 403, but the condition covers the entire 400–499 range. Existing auth-retry coverage is explicit about terminal 401/403 (tests/gateway/test_ws_auth_retry.py:21-77); please define the SimpleX-specific terminal set and preserve retry for transient or ambiguous 4xx cases.
  • The new 4xx test mocks _set_fatal_error (tests/gateway/test_simplex_plugin.py:365-375), so it does not verify the gateway-visible state produced by BasePlatformAdapter._set_fatal_error() (gateway/platforms/base.py:2705-2710).

Suggested changes

  • Add a retry regression test for a non-terminal 4xx status alongside 403 and 503.
  • Exercise the real fatal setter while mocking its status writer, then assert the fatal fields and _running state.

Automated hermes-sweeper review.

# busy-looping forever. 5xx may be transient, so keep
# retrying those via the generic path below.
status = _handshake_status_code(e)
if status is not None and 400 <= status < 500:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes every 4xx terminal, although the demonstrated permanent case is 403. Please use an explicit SimpleX-specific terminal-status set and add coverage for a transient or ambiguous 4xx response so this does not turn a recoverable rejection into a permanently disabled adapter.

fatal_calls = []
monkeypatch.setattr(
adapter,
"_set_fatal_error",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mocking _set_fatal_error verifies the call but bypasses the gateway-visible state this PR is intended to establish. Prefer mocking _write_runtime_status_safe and assert the real setter leaves _running false and records the expected fatal fields.

@teknium1 teknium1 added sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users labels Jul 13, 2026
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 P3 Low — cosmetic, nice to have sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades 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.

4 participants