[Claude/Opus] Detect stuck WebSocket connections via heartbeat and read timeout#12
Conversation
Previously listen() blocked in ReadMessage() with no read deadline, so a half-open connection (peer stops sending data but no TCP FIN/RST arrives) was never detected: onDisconnected never fired and the reconnection loop never engaged. This adds two complementary mechanisms: - Heartbeat: a goroutine sends a Home Assistant ping every PingInterval (default 30s); HA replies with pong. This generates traffic during quiet periods so a healthy-but-idle connection is not mistaken for a dead one. - Staleness detection: listen() sets a read deadline (ReadTimeout, default 60s) that resets on every received frame. If nothing arrives within the window, ReadMessage errors out and flows into the existing reconnection path. Both are configurable via hal.yaml (pingInterval, readTimeout). Also adds a writeMutex around all socket writes, since the heartbeat introduces a concurrent writer and gorilla/websocket forbids concurrent writes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
dansimau
changed the title
![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: d570c5f961
ℹ️ 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".
| return | ||
| } | ||
|
|
||
| _, msgBytes, err := conn.ReadMessage() |
There was a problem hiding this comment.
When ReadMessage returns because the newly-added read deadline expires, this branch logs and returns without closing the captured conn; the deferred cleanup stops the heartbeat and opens a replacement connection, but the old Home Assistant websocket can remain open server-side until HA times it out. In repeated stale/quiet periods this leaks websocket sessions while reconnecting, so close the captured connection before returning on read errors caused by the deadline.
Useful? React with 👍 / 👎.
| if config.ReadTimeout == 0 { | ||
| config.ReadTimeout = defaultReadTimeout |
There was a problem hiding this comment.
If a user configures only PingInterval to a value above 60s and leaves ReadTimeout unset, this default still sets the read timeout to 60s, so a quiet connection hits the read deadline before the first heartbeat ping is sent and will reconnect continuously. Since these fields are exposed independently through Config, either validate ReadTimeout > PingInterval or derive the default read timeout from the configured ping interval.
Useful? React with 👍 / 👎.
| c.writeMutex.Lock() | ||
| defer c.writeMutex.Unlock() | ||
|
|
||
| return conn.WriteMessage(websocket.TextMessage, msgBytes) |
There was a problem hiding this comment.
When the network path is half-open or Home Assistant stops reading, this new heartbeat WriteMessage can block indefinitely while holding writeMutex; the reconnect path then authenticates/subscribes on a fresh socket via send(), but that also needs the same mutex, so recovery and even Close() can hang behind the stuck heartbeat write. Set a write deadline before heartbeat writes so a blackholed connection cannot wedge the client.
Useful? React with 👍 / 👎.
|
|
||
| // PingInterval is how often to send a heartbeat ping to Home Assistant to | ||
| // keep the connection active. Defaults to 30s if unset. | ||
| PingInterval time.Duration `yaml:"pingInterval"` |
There was a problem hiding this comment.
This exposes pingInterval/readTimeout as YAML knobs, but runEventsCommand builds its separate websocket client with only Host and Token (cmd/hal/commands/events.go:53-56), so hal events ignores the custom heartbeat settings. In environments where these values are tuned for a proxy or Home Assistant deployment, the main daemon uses the override while the long-running event stream still uses the hard-coded defaults; pass these fields into that client too.
Useful? React with 👍 / 👎.
…eout - Close the captured socket in listen()'s deferred cleanup so a stale/ half-open connection is not leaked server-side across reconnects. - Add a write deadline to all websocket writes via a shared writeMessage helper, so a blackholed connection cannot wedge a write while holding writeMutex and block reconnection or shutdown. - Derive the default ReadTimeout from PingInterval (2x) and guard against ReadTimeout <= PingInterval, which would otherwise reconnect continuously on quiet connections when a large custom PingInterval is set. - Pass PingInterval/ReadTimeout to the `hal events` websocket client so it honours the configured heartbeat settings. - Add unit tests for the read-timeout derivation and guard logic. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Addressed the Codex review feedback in 54e2524:
Build, |
1 participant
Problem
Occasionally the connection to Home Assistant gets "stuck" — the WebSocket stays open but stops delivering data, so automations stop firing (e.g. zero events for an hour while the socket appears healthy).
Root cause:
listen()blocked inconn.ReadMessage()with no read deadline. When a connection goes half-open (the peer stops sending data but no TCP FIN/RST arrives), that read never returns, soonDisconnectednever fires and the existing reconnection loop never engages.Changes
Implements both mechanisms suggested — they work together, since neither alone is fully reliable:
Heartbeat (
hassws/client.go) — Aheartbeat()goroutine sends a Home AssistantpingeveryPingInterval(default 30s); HA replies withpong. This generates traffic during quiet periods so a healthy-but-idle connection isn't mistaken for a dead one. If the ping write itself fails, it closes the connection to trigger reconnect immediately.Staleness detection (
hassws/client.go) —listen()now sets a read deadline (ReadTimeout, default 60s) that resets on every received frame (including pongs). If nothing arrives within the window,ReadMessageerrors out and flows into the existing reconnection path. This is the reliable detector, since a half-open socket may keep accepting writes for a while.Write serialization — Added a
writeMutexaround all socket writes. The heartbeat introduces a concurrent writer, and gorilla/websocket forbids concurrent writes (this also fixes a latent race between concurrentCallServicecalls).Config (
config.go,connection.go) — Configurable viahal.yaml(pingInterval,readTimeout), defaulting to 30s/60s.ReadTimeoutshould exceedPingIntervalso pongs keep the deadline alive.Tests
Extended the mock server to answer pings (with a
SetRespondToPings(false)toggle to simulate a stuck connection), plus two new tests:TestHeartbeatKeepsConnectionAlive— no events for 2× the read timeout with pings answered → no reconnect.TestStaleConnectionTriggersReconnect— server stops answering pings → staleness detected → reconnects → recovers.Verification
go build ./...,go vet ./...,gofmt— clean🤖 Generated with Claude Code