Improve websocket reliability per Mist best practices#26
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves the reliability and operability of the Mist WebSocket client by introducing a callback worker queue (decoupling receive from user processing), adding subscription acknowledgement tracking with a watchdog-driven reconnect strategy, and updating ping/pong handling and reconnect backoff behavior. It also updates defaults/docs (ping interval/timeout) and extends unit tests to cover the new behavior.
Changes:
- Add callback worker queue, subscription watchdog + subscribe-failure close/reconnect, ping/pong hooks, and 429-aware reconnect delay logic in the core WebSocket client.
- Update default ping settings to
ping_interval=60andping_timeout=45across docs and wrappers. - Expand unit tests to validate callback-worker behavior and new validations (e.g., ping interval/timeout constraint, channel limit).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
src/mistapi/websockets/__ws_client.py |
Implements callback worker queue, subscription watchdog/ack tracking, ping/pong hooks, rate-limit-aware backoff, and channel safeguards. |
src/mistapi/websockets/sites.py |
Updates wrapper defaults/docs for ping interval/timeout. |
src/mistapi/websockets/orgs.py |
Updates wrapper defaults/docs for ping interval/timeout. |
src/mistapi/websockets/location.py |
Updates wrapper defaults/docs for ping interval/timeout. |
src/mistapi/websockets/session.py |
Updates wrapper defaults/docs for ping interval/timeout. |
tests/unit/test_websocket_client.py |
Adjusts tests for callback-worker behavior, ping defaults/validation, and channel limit enforcement. |
README.md |
Updates documented defaults and documents new WebSocket kwargs and callbacks. |
Comments suppressed due to low confidence (4)
src/mistapi/websockets/sites.py:84
- This public channel
__init__doesn’t accept/forward the new base-class kwargs (subscription_watchdog_timeout,rate_limit_backoff,throughput_log_interval). Given the README now documents them as supported for channel classes, this class (and the other channel wrappers) should accept and pass them through tosuper().__init__(or accept**kwargs).
def __init__(
self,
mist_session: APISession,
site_ids: list[str],
ping_interval: int = 60,
ping_timeout: int = 45,
auto_reconnect: bool = False,
max_reconnect_attempts: int = 5,
reconnect_backoff: float = 2.0,
max_reconnect_backoff: float | None = None,
queue_maxsize: int = 0,
) -> None:
src/mistapi/websockets/orgs.py:84
- This channel wrapper
__init__doesn’t accept/forward the new base-class kwargs (subscription_watchdog_timeout,rate_limit_backoff,throughput_log_interval). If these options are intended to be part of the public API (per README), they need to be accepted here and passed tosuper().__init__(or use a**kwargspassthrough).
def __init__(
self,
mist_session: APISession,
org_id: str,
ping_interval: int = 60,
ping_timeout: int = 45,
auto_reconnect: bool = False,
max_reconnect_attempts: int = 5,
reconnect_backoff: float = 2.0,
max_reconnect_backoff: float | None = None,
queue_maxsize: int = 0,
) -> None:
src/mistapi/websockets/location.py:87
- This channel wrapper
__init__doesn’t accept/forward the new base-class kwargs (subscription_watchdog_timeout,rate_limit_backoff,throughput_log_interval). If these options are intended to be publicly configurable, add them here and forward tosuper().__init__(or accept**kwargs).
def __init__(
self,
mist_session: APISession,
site_id: str,
map_ids: list[str],
ping_interval: int = 60,
ping_timeout: int = 45,
auto_reconnect: bool = False,
max_reconnect_attempts: int = 5,
reconnect_backoff: float = 2.0,
max_reconnect_backoff: float | None = None,
queue_maxsize: int = 0,
) -> None:
src/mistapi/websockets/session.py:93
SessionWithUrl.__init__doesn’t accept/forward the new base-class kwargs (subscription_watchdog_timeout,rate_limit_backoff,throughput_log_interval). If the README intends these to be supported across channel classes, this wrapper should accept and pass them through tosuper().__init__(or allow**kwargs).
def __init__(
self,
mist_session: APISession,
url: str,
ping_interval: int = 60,
ping_timeout: int = 45,
auto_reconnect: bool = False,
max_reconnect_attempts: int = 5,
reconnect_backoff: float = 2.0,
max_reconnect_backoff: float | None = None,
queue_maxsize: int = 0,
) -> None:
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Addressed the review feedback in follow-up commits.\n\nWhat was fixed:\n- Public channel wrappers now accept and forward subscription_watchdog_timeout, rate_limit_backoff, and throughput_log_interval (session/orgs/sites/location).\n- _handle_error now always refreshes _last_http_status (prevents stale 429 state).\n- _handle_close now preserves previously-set close reason details unless explicit non-empty close values are provided.\n- _handle_message now normalizes non-object JSON payloads into a dict wrapper to match callback/receive typing expectations.\n- Test cleanup now uses disconnect(wait=True, timeout=1) for callback worker shutdown in affected tests.\n- README clarified ping_timeout constraint wording (applies when ping_interval > 0) and queue_maxsize semantics (receive + callback queues).\n- Additional hardening: reduced noisy ack/ping/pong logging and made throughput counters synchronized across threads.\n\nValidation:\n- tests/unit/test_websocket_client.py: 141 passed\n- full unit suite: 484 passed |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (4)
src/mistapi/websockets/location.py:48
- The PARAMS section here doesn’t mention the newly added reliability kwargs (subscription_watchdog_timeout, rate_limit_backoff, throughput_log_interval) and the queue_maxsize description is now incomplete since queue_maxsize also applies to callback delivery. Updating the docstring would keep per-class docs aligned with the updated initializer.
ping_interval : int, default 60
Interval in seconds to send WebSocket ping frames (keep-alive).
ping_timeout : int, default 45
Time in seconds to wait for a ping response before considering the connection dead.
auto_reconnect : bool, default False
Automatically reconnect on unexpected disconnections using exponential backoff.
max_reconnect_attempts : int, default 5
Maximum number of reconnect attempts before giving up.
reconnect_backoff : float, default 2.0
Base backoff delay in seconds. Doubles after each failed attempt.
max_reconnect_backoff : float | None, default None
Maximum backoff delay in seconds. If None, backoff grows indefinitely.
queue_maxsize : int, default 0
Maximum number of messages buffered in the internal queue for the
``receive()`` generator. ``0`` means unbounded. When set,
incoming messages are dropped with a warning when the queue is
full, preventing memory growth on high-frequency streams.
src/mistapi/websockets/session.py:55
- SessionWithUrl now accepts subscription_watchdog_timeout, rate_limit_backoff, and throughput_log_interval, but the PARAMS docstring doesn’t list them and still frames queue_maxsize as only for receive(). Please update the docstring to reflect the actual initializer kwargs/behavior.
ping_interval : int, default 60
Interval in seconds to send WebSocket ping frames (keep-alive).
ping_timeout : int, default 45
Time in seconds to wait for a ping response before considering the connection dead.
auto_reconnect : bool, default False
Automatically reconnect on unexpected disconnections using exponential backoff.
max_reconnect_attempts : int, default 5
Maximum number of reconnect attempts before giving up.
reconnect_backoff : float, default 2.0
Base backoff delay in seconds. Doubles after each failed attempt.
max_reconnect_backoff : float | None, default None
Maximum backoff delay in seconds. If None, backoff grows indefinitely.
queue_maxsize : int, default 0
Maximum number of messages buffered in the internal queue for the
``receive()`` generator. ``0`` means unbounded. When set,
incoming messages are dropped with a warning when the queue is
full, preventing memory growth on high-frequency streams.
src/mistapi/websockets/sites.py:46
- This class’ PARAMS docstring still describes queue_maxsize as only affecting the receive() queue and doesn’t document the newly added reliability kwargs (subscription_watchdog_timeout, rate_limit_backoff, throughput_log_interval). Since these kwargs are now part of the public initializer, the docstring should be updated to match the actual behavior/parameters.
ping_interval : int, default 60
Interval in seconds to send WebSocket ping frames (keep-alive).
ping_timeout : int, default 45
Time in seconds to wait for a ping response before considering the connection dead.
auto_reconnect : bool, default False
Automatically reconnect on unexpected disconnections using exponential backoff.
max_reconnect_attempts : int, default 5
Maximum number of reconnect attempts before giving up.
reconnect_backoff : float, default 2.0
Base backoff delay in seconds. Doubles after each failed attempt.
max_reconnect_backoff : float | None, default None
Maximum backoff delay in seconds. If None, backoff grows indefinitely.
queue_maxsize : int, default 0
Maximum number of messages buffered in the internal queue for the
``receive()`` generator. ``0`` means unbounded. When set,
incoming messages are dropped with a warning when the queue is
full, preventing memory growth on high-frequency streams.
src/mistapi/websockets/orgs.py:46
- This class’ PARAMS docstring doesn’t document the newly added reliability kwargs (subscription_watchdog_timeout, rate_limit_backoff, throughput_log_interval) and still describes queue_maxsize as only buffering receive() messages. Please update the docstring so the public API docs match the initializer signature/behavior.
ping_interval : int, default 60
Interval in seconds to send WebSocket ping frames (keep-alive).
ping_timeout : int, default 45
Time in seconds to wait for a ping response before considering the connection dead.
auto_reconnect : bool, default False
Automatically reconnect on unexpected disconnections using exponential backoff.
max_reconnect_attempts : int, default 5
Maximum number of reconnect attempts before giving up.
reconnect_backoff : float, default 2.0
Base backoff delay in seconds. Doubles after each failed attempt.
max_reconnect_backoff : float | None, default None
Maximum backoff delay in seconds. If None, backoff grows indefinitely.
queue_maxsize : int, default 0
Maximum number of messages buffered in the internal queue for the
``receive()`` generator. ``0`` means unbounded. When set,
incoming messages are dropped with a warning when the queue is
full, preventing memory growth on high-frequency streams.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- derive ping_timeout as min(45, ping_interval - 1) when not supplied, so existing callers passing only ping_interval keep working - drain the callback queue on shutdown instead of dropping pending messages; the None sentinel now marks end-of-stream - report subscription watchdog timeouts and subscribe_failed events through on_error so callers without auto_reconnect get a signal - don't resurrect the callback worker after disconnect (stop flag is now only cleared by connect()) - cite the Mist rate-limits doc for the channel-count constants - document the new reliability kwargs in all channel class docstrings - add tests: ping derivation, worker drain, worker stop guard, watchdog expiry/cancel, subscribe_failed error reporting Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…cope - note that ping_timeout falls back to 45 when ping_interval=0 (pings disabled, value unused) in the constants comment, README, and all channel class docstrings - document that queue_maxsize bounds both the receive() and callback queues in the channel class docstrings Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Summary
Validation