Fix flaky TestShipPairing: patched ship-go fork for double-connection race#31493
Merged
Conversation
CI intermittently fails TestShipPairing with 'paired device not routed to consumer' after a 1-minute timeout (e.g. run 28740106700). Root cause traced to two bugs in enbility/ship-go's connection handling that compound under concurrent bidirectional pairing: - Two concurrent outgoing dials to the same SKI can both pass the isSkiConnected check before either registers, establish, and then tear each other down (enbility/ship-go#85, open upstream, unmerged). - sendSpineData only tore down the connection when the writer already reported it closed; a write failing for any other reason (e.g. the remote closed it first) left a zombie connection that silently failed every future send and never freed the SKI for reconnect. Both are fixed in evcc-io/ship-go (branch fix/double-connection-outgoing-dedup, cherry-picked onto our exact pinned commit): the first is PR #85 as-is, the second is a new fix alongside it. 10/10 stress runs pass locally with these fixes and the original 1-minute timeouts unchanged; the same run reliably failed before either fix.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Consider adding a short inline comment next to the new
ship-goreplace directive ingo.modsummarizing that it fixes the double-connection race / connection teardown bug, so the rationale for the fork is obvious without having to dig up this PR.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider adding a short inline comment next to the new `ship-go` replace directive in `go.mod` summarizing that it fixes the double-connection race / connection teardown bug, so the rationale for the fork is obvious without having to dig up this PR.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
uhl
added a commit
to enbility/ship-go
that referenced
this pull request
Jul 6, 2026
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)
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.
fixes flaky CI failures in TestShipPairing, e.g. https://github.com/evcc-io/evcc/actions/runs/28740106700. Should fix #31465.
CI intermittently fails with "paired device not routed to consumer" after the test's 1-minute connect timeout. Traced to two compounding bugs in
enbility/ship-go's connection handling, reproduced locally by stress-running the test under a noisier network than CI (100% failure rate before either fix, 10/10 passes after both, with the original timeouts left untouched):isSkiConnectedcheck before either registers as connected, establish, and then abort each other in a loop — this is hub: prevent concurrent duplicate outgoing connections to the same SKI enbility/ship-go#85 (open upstream, unmerged).sendSpineDataonly tore down a connection when the writer already reported it closed. A write failing for any other reason (e.g. the remote closed it first) left the connection registered as alive, so every subsequent send silently failed and the SKI was never freed for reconnect — a second, previously-unreported bug found while investigating the first, now also filed upstream as ship: tear down connection on a failed spine data write enbility/ship-go#86.Both are fixed in
evcc-io/ship-go(fork, branchfix/double-connection-outgoing-dedup, based on our exact currently-pinned commit): the first is enbility/ship-go#85's fix cherry-picked as-is, the second is enbility/ship-go#86 alongside it. This PR only changesgo.mod/go.sumto point at that fork via areplacedirective, mirroring the existingeebus-gofork pattern already in this repo.No timeouts were changed — verified the fix holds under the original 1-minute per-connect timeouts.
🤖 Generated with Claude Code