hub: prevent concurrent duplicate outgoing connections to the same SKI#85
Conversation
|
coverage: 92.972% (+0.02%) from 92.952% — andig:fix/double-connection-outgoing-dedup into enbility:dev |
Fix evcc-io/evcc#31465 Found while chasing an intermittent double-connection related test hang in evcc (evcc-io/evcc#31493) that reproduces on top of #85's dedup fix. `sendSpineData` only calls `CloseConnection` when `IsDataConnectionClosed()` already reports the connection closed: ```go if isClosed, err := c.dataWriter.IsDataConnectionClosed(); isClosed { c.CloseConnection(false, 0, "") return err } ... err = c.dataWriter.WriteMessageToWebsocketConnection(shipMsg) if err != nil { logging.Log().Debug("error sending message: ", err) return err } ``` If the actual websocket write fails for any other reason (e.g. the remote already closed the connection before this side's writer noticed), the error is logged and swallowed, but the connection is never torn down. The `ShipConnection` stays registered as the current writer for that SKI, so every subsequent send silently fails forever and the SKI is never freed for a fresh connection attempt. This surfaced concretely while stress-testing a double-connection race scenario: after one side's connection was implicitly invalidated, the other kept retrying sends against it every couple of seconds indefinitely ("Error sending spine message: connection closed for remote SKI ...", repeating), and no reconnect was ever attempted. Fix: also call `CloseConnection(false, 0, "")` when the write itself errors, mirroring the existing `IsDataConnectionClosed` branch. Updated `TestSendSpineData_WriteError` to expect the resulting `HandleConnectionClosed` call. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
|
Sorry for the churn here — the nil-check removal was on me, I asked for it thinking The latest run against CI history confirms the trigger:
Some test path exercises Two options to unstick this:
Either works for me — happy to defer to your preference. Sorry again for sending you down this path. |
|
I'll push 2, its consistent with the rest of the codebase. |
…ating
ReportMdnsEntries schedules a delayed timer that can reach
connectFoundService asynchronously, which writes to
connectionsInitiating. The bare &Hub{} literals in hub_mdns_test.go
predate that field and left it nil, causing a flaky
"assignment to entry in nil map" panic that took down the whole
package's test binary.
Replaces the ten repeated &Hub{...} literals with a single helper so
future fields (like the connectionsInitiating fix) only need to be
added once.
|
Actually, did best of both ;) |
3 participants
connectFoundServiceonly dedupes outgoing dials viaisSkiConnected, which becomes true afterregisterConnection(i.e. after the websocket dial + SHIP handshake). Two concurrent outgoing dials to the same SKI can both pass that check, both establish, and thenkeepThisConnectiontears one down mid-discovery. Some remotes then stop answering measurement reads on the surviving connection until the whole service is restarted.Reserve the SKI for the duration of the dial via a new
connectionsInitiatingset (guarded bymuxCon), so a concurrent outgoing dial returns early instead of racing to a second handshake. The incoming-vs-outgoing case is still handled bykeepThisConnection.This is a real-live issue observed as a stalled EEBus meter on a Vaillant heatpump in evcc-io/evcc#30889. PR is done by Claude but lgtm.