fix: MQTT reconnection and reliability hardening#91
Merged
Conversation
- Widen reconnection backoff loop exception handling: auth and MQTT errors (Nwp500Error) now count as failed attempts and are retried instead of silently killing the reconnect task; only InvalidCredentialsError is fatal and emits reconnection_failed - Make disconnect() effective while the connection is interrupted: always disable automatic reconnection and stop periodic tasks, and close the SDK connection even when not connected - Flush the command queue after active/deep reconnection; the SDK's on_connection_resumed callback never fires for a rebuilt connection, so queued commands were silently lost - Log exceptions from thread-scheduled coroutines instead of dropping the run_coroutine_threadsafe future - Widen periodic request loop exception handling so MQTT publish failures no longer permanently kill polling tasks - Re-raise CancelledError in reconnection and periodic loops so cancelled tasks are actually marked cancelled - Add operation_timeout (default 30s) for MQTT connect/publish/ subscribe/unsubscribe/disconnect acknowledgements to prevent hangs on half-open TCP connections - Add +/-50% jitter to reconnect backoff to avoid synchronized fleet-wide reconnect storms
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the MQTT client’s long-running reliability by making reconnection/periodic loops resilient to auth/MQTT exceptions, ensuring disconnect fully shuts down background activity, and adding bounded operation ACK timeouts to avoid indefinite hangs on half-open connections.
Changes:
- Make reconnection and periodic loops retry on broader library errors (and propagate
CancelledErrorcorrectly), with bounded backoff jitter. - Add broker acknowledgement timeouts for connect/publish/subscribe/unsubscribe/disconnect via
MqttConnectionConfig.operation_timeout. - Add a comprehensive regression test suite covering the above reliability scenarios and document changes in the changelog.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_mqtt_reliability.py | New regression tests covering reconnection, disconnect semantics, periodic loop resilience, scheduled coroutine logging, and ACK timeouts. |
| src/nwp500/mqtt/utils.py | Adds operation_timeout to MQTT connection configuration. |
| src/nwp500/mqtt/subscriptions.py | Applies subscribe/unsubscribe ACK timeouts and stores an operation_timeout setting. |
| src/nwp500/mqtt/reconnection.py | Broadens retryable exception set, adds jittered backoff, and fixes cancellation propagation/fatal credential handling. |
| src/nwp500/mqtt/periodic.py | Ensures CancelledError propagates and periodic polling survives broader MQTT/library errors. |
| src/nwp500/mqtt/connection.py | Introduces a shared _await_ack() helper to await SDK ACK futures with timeout/shielding. |
| src/nwp500/mqtt/client.py | Ensures queued commands flush after active/deep reconnect, hardens disconnect during interruption, and logs thread-scheduled coroutine failures. |
| CHANGELOG.rst | Documents the reliability fixes and the new operation timeout / jitter behavior. |
Comments suppressed due to low confidence (1)
src/nwp500/mqtt/utils.py:250
operation_timeoutis a user-facing config value, but__post_init__doesn’t validate it.asyncio.wait_for(..., timeout<=0)will time out immediately, which is surprising and hard to debug. Consider enforcing a positive value (or explicitly supportingNoneto disable timeouts).
# MQTT operation acknowledgement timeout
operation_timeout: float = 30.0 # seconds
# Command queue settings
enable_command_queue: bool = True
max_queued_commands: int = 100
def __post_init__(self) -> None:
"""Generate client ID if not provided and validate settings."""
if not self.client_id:
object.__setattr__(
self, "client_id", f"navien-client-{uuid.uuid4().hex[:8]}"
)
if self.deep_reconnect_threshold < 1:
object.__setattr__(self, "deep_reconnect_threshold", 1)
added 2 commits
July 5, 2026 09:29
…liability # Conflicts: # CHANGELOG.rst
- Pass explicit (type, value, traceback) tuple to exc_info in the scheduled-coroutine done-callback; a bare exception instance is treated as a truthy flag and logs the current (empty) exception context instead of the coroutine's traceback - Close the coroutine object when scheduling fails (no running loop or loop closed) to avoid 'coroutine was never awaited' warnings and resource leaks - Accept both concurrent.futures.Future and asyncio.Future in _await_ack, and drop the misleading asyncio.Future casts at call sites (the AWS CRT SDK returns concurrent futures) - Add tests: coroutine cleanup on both failure paths, traceback in the done-callback log record, _await_ack with each future type
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Second batch from the comprehensive repo evaluation: fixes the reliability cluster that could leave a long-running client permanently offline with all failures silently swallowed.
Bugs fixed
mqtt/reconnection.py): the backoff loop caught only(AwsCrtError, RuntimeError), but the quick/deep reconnect paths raiseTokenRefreshError,AuthenticationError, andMqttCredentialsError— none of which areRuntimeError. A network outage coinciding with token expiry killed the reconnect task with an unretrieved exception; the client never reconnected despite unlimited retries. AllNwp500Errors (andTimeoutError) now count as failed attempts and are retried. OnlyInvalidCredentialsErroris fatal: it stops the loop and emitsreconnection_failed.disconnect()was a no-op while interrupted (mqtt/client.py): callingdisconnect()while_connectedwas False returned early without disabling reconnection or stopping periodic tasks — the backoff loop would resurrect the connection after shutdown (the CLI'sfinally: await mqtt.disconnect()hit exactly this path after a drop). Now reconnection is always disabled, periodic tasks stopped, and the SDK connection torn down viaclose()even when not connected.mqtt/client.py): the queue was only flushed from the SDK'son_connection_resumedcallback, which never fires for the brand-new connection built by active/deep reconnection. Both paths now flush afterresubscribe_all().mqtt/periodic.py):MqttNotConnectedError/MqttPublishErrorraised by a publish racing a disconnect escaped the loop's catch and permanently ended polling while the task still looked alive.run_coroutine_threadsafefutures (mqtt/client.py): exceptions from scheduled coroutines (e.g. failed resubscribe after clean-session resume, leaving the client connected but deaf) vanished into discarded futures. A done-callback now logs them.CancelledErrorswallowed withbreak(reconnection + periodic loops): cancelled tasks ended "successfully" instead of cancelled, and the reconnection loop could continue past the loop and emitreconnection_failedduring a manual disconnect. Cancellation now propagates.Improvements
mqtt/connection.py,mqtt/subscriptions.py): connect/publish/subscribe/unsubscribe/disconnect acknowledgements are awaited withMqttConnectionConfig.operation_timeout(default 30s). Previously a half-open TCP connection could hang callers until the 20-minute keep-alive expired. The awscrt future stays shielded, so timeouts/cancellation never corrupt SDK callbacks.max_reconnect_delay) to prevent synchronized fleet-wide reconnect storms after AWS IoT's 24-hour disconnect.Tests
New
tests/test_mqtt_reliability.py(17 tests): auth/MQTT errors retried,InvalidCredentialsErrorfatal + event, cancellation marks tasks cancelled, disconnect-while-interrupted shuts everything down, queue flush after active and deep reconnect, periodic loop survivesMqttError, scheduled-coroutine exception logging, publish/subscribe ack timeouts, jitter bounded bymax_reconnect_delay.Validation
pytest: 525 passed (3 consecutive runs, no flakiness)ruff check/ruff format --check: cleanpyright src/nwp500: 0 errorsmypy src/nwp500: no issues