Add SmartHomeMonitorWatcherTask#59
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a scheduler-based watchdog to keep the Home Assistant smart home WebSocket monitor running, improving resilience when the monitor stops unexpectedly.
Changes:
- Extend
ISmartHomeConnectorwith anEnsureMonitorIsRunninghealth-check/start method. - Implement monitor “ensure running” logic in
HomeAssistantConnectorand enhance monitor-close logging. - Add
SmartHomeMonitorWatcherTaskplus example configuration and documentation.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| TgHomeBot.SmartHome.HomeAssistant/HomeAssistantMonitor.cs | Adds reconnect state to the “socket closed” warning log. |
| TgHomeBot.SmartHome.HomeAssistant/HomeAssistantConnector.cs | Adds logger injection + implements EnsureMonitorIsRunning. |
| TgHomeBot.SmartHome.Contract/ISmartHomeConnector.cs | Adds new connector API for ensuring the monitor is running. |
| TgHomeBot.Scheduling/Tasks/SmartHomeMonitorWatcherTask.cs | New scheduled task that periodically checks/starts the monitor. |
| TgHomeBot.Scheduling/README.md | Documents the new scheduled task and its JSON config. |
| Task<IReadOnlyList<SmartDevice>> GetDevices(IReadOnlyList<MonitoredDevice> requestedDevices); | ||
| Task<ISmartHomeMonitor> CreateMonitorAsync(IReadOnlyList<MonitoredDevice> devices, | ||
| CancellationToken cancellationToken); | ||
| Task<bool> EnsureMonitorIsRunning(); |
There was a problem hiding this comment.
EnsureMonitorIsRunning has no CancellationToken parameter, which makes it hard to avoid starting/restarting the monitor during shutdown and prevents callers (like scheduled tasks) from propagating cancellation. Consider changing the contract to EnsureMonitorIsRunning(CancellationToken cancellationToken) and using it throughout the call chain.
| Task<bool> EnsureMonitorIsRunning(); | |
| Task<bool> EnsureMonitorIsRunning(CancellationToken cancellationToken); |
| public async Task<bool> EnsureMonitorIsRunning() | ||
| { | ||
| try | ||
| { | ||
| switch (_smartHomeMonitor?.State) | ||
| { | ||
| case MonitorState.Idle: | ||
| await _smartHomeMonitor.StartMonitoringAsync(CancellationToken.None); | ||
| return true; | ||
| case MonitorState.Listening: | ||
| return true; | ||
| default: | ||
| return false; | ||
| } |
There was a problem hiding this comment.
EnsureMonitorIsRunning won’t attempt a restart when the monitor state is Unknown (e.g., after creation when _webSocket is null), so the watcher may report false without actually trying to start monitoring. Consider treating Unknown like Idle (attempt StartMonitoringAsync) and propagating a caller CancellationToken instead of using CancellationToken.None.
| try | ||
| { | ||
| switch (_smartHomeMonitor?.State) | ||
| { | ||
| case MonitorState.Idle: | ||
| await _smartHomeMonitor.StartMonitoringAsync(CancellationToken.None); | ||
| return true; |
There was a problem hiding this comment.
EnsureMonitorIsRunning reads _smartHomeMonitor?.State and may call StartMonitoringAsync without any synchronization. Since the monitor is created under _semaphore in CreateMonitorAsync, consider also using the same semaphore here to avoid races (e.g., scheduled watcher running while the hosted service is creating/starting the monitor).
| var isRunning = await smartHomeConnector.EnsureMonitorIsRunning(); | ||
| logger.LogInformation("Smart Home Monitor is running: {IsRunning}", isRunning); |
There was a problem hiding this comment.
ExecuteAsync ignores the provided cancellationToken (and the connector method currently can’t accept one), so the task may start/restart the monitor even while the scheduler is stopping. Also, the scheduler already logs task execution at Information level; logging the running status every 5 minutes can add unnecessary noise—consider logging only on state change/failure or using a lower level.
| var isRunning = await smartHomeConnector.EnsureMonitorIsRunning(); | |
| logger.LogInformation("Smart Home Monitor is running: {IsRunning}", isRunning); | |
| cancellationToken.ThrowIfCancellationRequested(); | |
| var isRunning = await smartHomeConnector.EnsureMonitorIsRunning().ConfigureAwait(false); | |
| if (!isRunning) | |
| { | |
| logger.LogWarning("Smart Home Monitor is not running and could not be started."); | |
| } | |
| else | |
| { | |
| logger.LogDebug("Smart Home Monitor is running."); | |
| } |
|
@copilot open a new pull request to apply changes based on the comments in this thread |
|
@thomasneuberger I've opened a new pull request, #60, to work on those changes. Once the pull request is ready, I'll request review from you. |
…ync, and logging (#60) * Initial plan * Apply PR review feedback: CancellationToken, Unknown state, semaphore, logging improvements Co-authored-by: thomasneuberger <23504477+thomasneuberger@users.noreply.github.com> Agent-Logs-Url: https://github.com/thomasneuberger/TgHomeBot/sessions/02041d09-0799-491d-ac3a-ccd36acdea63 --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: thomasneuberger <23504477+thomasneuberger@users.noreply.github.com>
No description provided.