[Pi/Opus] Detect stuck websocket connections via heartbeat and read timeout#11
Closed
dansimau wants to merge 3 commits into
Closed
[Pi/Opus] Detect stuck websocket connections via heartbeat and read timeout#11dansimau wants to merge 3 commits into
dansimau wants to merge 3 commits into
Conversation
The websocket connection could get into a state where it was still open (so no disconnect callback fired) but no longer receiving data. The existing reconnection logic only triggered on a read error or close message, so a silently-dead connection would hang indefinitely. This adds two detection mechanisms in the hassws client: - Heartbeat: a keepAlive goroutine sends a websocket ping every PingInterval (default 30s). A pong resets the read deadline. - Read/idle timeout: every connection has a read deadline (PongTimeout, default 60s) that is extended on any received data or pong. If neither arrives in time, ReadMessage errors and the existing reconnect path runs. Both are configurable via hal.yaml (pingInterval, readTimeout). Also adds a writeMutex to serialize concurrent writes (gorilla/websocket does not allow concurrent writers) and scopes the listen/keepAlive goroutines per-connection. Adds Server.SimulateStuckConnection() test helper and TestStuckConnectionTriggersReconnect covering the new behaviour.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 844a10a10d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| defer s.lock.Unlock() | ||
|
|
||
| if s.websocket != nil { | ||
| s.websocket.SetPingHandler(func(string) error { return nil }) |
There was a problem hiding this comment.
When SimulateStuckConnection is called after the test server is already in listen, this writes the gorilla websocket ping handler while ReadMessage is reading it; go test -race -run TestStuckConnectionTriggersReconnect reports a race at this line. That makes the new test fail under race-enabled CI, so prefer installing a handler once before the read loop and toggling the stuck behavior with an atomic flag.
Useful? React with 👍 / 👎.
The mock server shared a single `s.websocket` and `s.subscribers` across connections. When a stuck connection was later abandoned, the old listen() goroutine's deferred cleanup wiped the subscriber list that the reconnected client had just registered, so events sent after reconnect went nowhere (the CI failure). - Scope the server listen loop to its own connection and only clear subscribers if it is still the active connection. - Reset subscribers when a new connection is accepted. - Simulate a stuck connection via an atomic flag checked in a ping handler installed once at connect time, instead of racily calling SetPingHandler concurrently with ReadMessage. - Close abandoned connections client-side on disconnect to avoid leaking the socket and to let the remote end observe the closure.
- Ensure PongTimeout is always greater than PingInterval. A misconfigured readTimeout <= pingInterval could otherwise expire the read deadline on a quiet-but-healthy connection before a ping/pong refreshes it, causing a reconnect loop. Bump it to 2x the ping interval and warn. - On a ping write failure, close the connection so the read loop errors out and the reconnect path runs. Previously an asymmetric failure (writes failing, reads still arriving) left the client in stateConnected with silently failing service calls. - Add unit tests for NewClient timeout defaulting/validation. The earlier feedback about closing timed-out sockets before reconnecting and installing the ping handler before the read loop (avoiding the SetPingHandler data race) was already addressed in 32310a4.
Owner
Author
|
Thanks for the review! Addressed all four points:
|
dansimau
changed the title
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
1 participant
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.
Problem
Occasionally the connection gets stuck: the WebSocket stays open (so no disconnect callback fires) but no longer receives data. The existing reconnection logic only triggered on a read error or close message, so a silently-dead connection would hang indefinitely — e.g. an hour with zero automations triggered despite an apparently healthy connection.
Fix
Two complementary detection mechanisms in the
hasswsclient:Heartbeat (ping/pong) — a
keepAlivegoroutine sends a WebSocket ping frame everyPingInterval(default 30s). Home Assistant replies with a pong, which extends the read deadline. If the ping write itself fails, the goroutine exits and a reconnect follows.Read/idle timeout — every connection has a read deadline (
PongTimeout, default 60s), extended whenever any data or a pong arrives. If neither arrives within the window,ReadMessagereturns a timeout error,listenexits, and the existingonDisconnected→ reconnect path runs.Together these cover both a healthy-but-idle connection (kept alive by pong) and a truly stuck one (forced to reconnect by the read deadline).
Other changes
pingIntervalandreadTimeoutinhal.yaml(both optional; sensible defaults,readTimeoutmust be >pingInterval).writeMutexsince gorilla/websocket forbids concurrent writers and the ping goroutine now writes alongside normal senders.listen/keepAlivegoroutines per-connection so stale goroutines can't interfere after a reconnect.Tests
Server.SimulateStuckConnection()test helper (stops replying to pings, sends no data).TestStuckConnectionTriggersReconnectverifying the client detects a stuck connection via the read timeout, reconnects, and resumes receiving events.All existing tests pass.