fix: Share a HealthManager instance across all mqtt channels#672
fix: Share a HealthManager instance across all mqtt channels#672allenporter merged 1 commit intoPython-roborock:mainfrom
Conversation
The motiviation is to better isolate single device timeouts from overall session health. I have not seen this as an explicit issue, but seems worth being defensive against. This will ensure that a single inactive device will not unnecessarily restart the MQTT session. Any successful RPCs by other devices will reset the counter. Adds explicit logging when a session is reset by the health manager.
| ), | ||
| decoder=decoder, | ||
| health_manager=self._mqtt_health_manager, | ||
| health_manager=self._mqtt_channel.health_manager, |
There was a problem hiding this comment.
Note to reviewer: The key change is here where the health manager will be shared across devices using the common mqtt session health manger.
There was a problem hiding this comment.
Pull request overview
This PR refactors the MQTT health monitoring to share a single HealthManager instance across all device channels instead of creating one per device. This prevents a single inactive device from unnecessarily restarting the entire MQTT session, since any successful RPC by other devices will reset the timeout counter. The PR also adds explicit debug logging when a session is restarted by the health manager.
Key changes:
- Moved HealthManager instantiation from per-device (V1Channel) to per-session (RoborockMqttSession)
- Added abstract health_manager property to MqttSession interface and implemented it in all concrete classes
- Enhanced logging in HealthManager to track session restarts
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| roborock/mqtt/health_manager.py | Added logging import and debug logging when restarting MQTT session; refactored timeout duration calculation for clarity |
| roborock/mqtt/session.py | Added abstract health_manager property to MqttSession base class |
| roborock/mqtt/roborock_session.py | Instantiated HealthManager in RoborockMqttSession and exposed it via property in both RoborockMqttSession and LazyMqttSession |
| roborock/devices/mqtt_channel.py | Added health_manager property that delegates to the underlying session |
| roborock/devices/v1_channel.py | Removed per-device HealthManager instantiation and updated to use the shared health_manager from mqtt_channel |
| tests/conftest.py | Added health_manager to FakeChannel mock for testing |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Lash-L
left a comment
Lash-L
left a comment
There was a problem hiding this comment.
Good change. sharing a health manager makes a lot of sense to me
2 participants
The motiviation is to better isolate single device timeouts from overall session health. I have not seen this as an explicit issue, but seems worth being defensive against. This will ensure that a single inactive device will not unnecessarily restart the MQTT session. Any successful RPCs by other devices will reset the counter.
Adds explicit logging when a session is reset by the health manager.