Skip to content

fix(ship): resolve double connections by SKI and recency (TC_SHIP_CONN_001) - #106

Open
kirollosnct wants to merge 2 commits into
devfrom
fix/ship-double-connection
Open

fix(ship): resolve double connections by SKI and recency (TC_SHIP_CONN_001)#106
kirollosnct wants to merge 2 commits into
devfrom
fix/ship-double-connection

Conversation

@kirollosnct

Copy link
Copy Markdown
Member

fix(ship): resolve double connections by SKI and recency (TC_SHIP_CONN_001)

Fixes #96.

The problem

SHIP 12.2.2 says the node with the bigger 160-bit SKI keeps the most recent
connection to a peer and closes all others. keepThisConnection implemented something
else — keep whichever connection the higher-SKI node initiated — and said so in its own
comment:

// This is hard to implement without any flaws. Therefore I chose a
// different approach: The connection initiated by the higher SKI will be kept

That rule is symmetric, so it always converged on one connection, but it is not the
spec's rule, and TC_SHIP_CONN_001 fails on it. With the DUT holding the larger SKI the
test tool opens connection B as soon as it sees the TCP stream of the DUT's connection A;
the DUT is required to keep B and close A, and instead keeps A and tears down B after B
has completed a handshake.

Getting the criterion right is necessary but not sufficient. The old check ran after the
TLS handshake and the websocket upgrade, while the test expects A closed "within 30s
(latest before the TLS handshake of connection B completes)" — and at that point A is not
even a connection yet, it is an in-flight dialer.Dial with no handle to cancel.

What changed

The criterion. keepThisConnection is replaced by resolveDoubleConnection. The
direction a connection was opened in is no longer an input — that parameter was the bug.

The decision moved into the TLS handshake. verifyPeerCertificate now performs the
arbiter check, which is what SHIP 12.2.2 asks for: "The SHIP node with the greater SKI
SHOULD check for double connections directly during the TLS handshake."
It never rejects
the incoming connection; it only retires the older one.

The outgoing dial became cancellable. connectionsInitiating carries the raw socket
(dialState), so that check can abort an in-flight dial synchronously instead of waiting
for it to return. This is the case TC_SHIP_CONN_001 actually produces, and the one that
has to beat the handshake deadline.

The registry owns the swap. registerConnection installs the new connection and
retires what it displaced in one step, so the map never passes through a state where the
SKI has no connection. A displaced connection can therefore tell it was replaced rather
than disconnected.

Two smaller spec items. A superseded connection that had reached data exchange is now
closed with the SHIP 13.4.7 termination announce rather than a socket close, and the
double close that raced its own close frame is gone — a rejected connection reports
4001 "double connection" instead of dying at 1006 "unexpected EOF".

A retired dial no longer reports success unconditionally. It is retired during the
incoming connection's TLS handshake, before that connection has shown it can get past
ServeHTTP at all. If it never establishes, the SKI would be left with nothing connected
and nothing scheduled to fix it, so the dial now waits briefly and then reports a failure
onto the existing retry path.

The first commit (fix(ws)) is a prerequisite: every websocket write now sets its own
deadline. A deadline stays armed on the socket after the write it was set for, and only a
SHIP message or the 50 s keepalive refreshed it, so an established connection idle for
more than writeWait failed its next write instantly — and gorilla latches write errors.
The write most exposed to that is the close frame, which is exactly the 4001 this PR
relies on.

Testing

New scenarios drive a real Hub over real TLS sockets against a scripted SHIP peer, so
the behaviour is observed the way the certification tool observes it — which socket died,
with which close code, and in which order relative to the TLS handshake:

Test Covers
TestDoubleConn_TC_SHIP_CONN_001_IncomingDuringInflightDial the ordering the test spec describes: B arrives while A is still an in-flight dial
TestDoubleConn_TC_SHIP_CONN_001_IncomingAfterEstablished A already registered when B arrives (the 30 s bound)
TestDoubleConn_TC_SHIP_CONN_001_ClosesBeforeHandshakeCompletes A closed before B's TLS handshake completes
TestDoubleConn_ReplacementDoesNotReportDisconnect no disconnect callback when a connection is replaced
TestDoubleConn_SupersededConnectionInDataExchangeIsAnnounced SHIP 13.4.7 announce, reason unspecific per 13.4.7.1.1
TestDoubleConn_SupersededDialFailsIfIncomingNeverEstablishes a retired dial that leads nowhere is retried
TestDoubleConn_TLS13PeerResolvesLateButCorrectly documents the TLS 1.3 split above
TestDoubleConn_DistinctPeersBothSurvive TC_SHIP_ROLE_003 guard: two distinct SKIs must both survive
TestWriteOnIdleConnection, TestCloseFrameOnIdleConnection the write-deadline fix

Each new test was verified to fail without its fix. Existing tests that encoded the old
criterion were rewritten rather than deleted — Test_KeepThisConnection_DirectTest
Test_DoubleConnectionResolution_DirectTest, TestKeepThisConnectionBasics
TestDoubleConnectionActionBasics, plus TestDoubleConnectionPreventionEdgeCases,
connection_race_fix_test.go and connection_delay_safety_test.go.

🤖 Generated with Claude Code

A write deadline stays armed on the socket after the write it was set for.
Only writeShipPump and handlePing set one, so a connection that has been
idle for longer than writeWait (10s) sits with an elapsed deadline - which
is the normal state of an established connection between SHIP messages,
since the keepalive ping only refreshes it every 50s.

Writing under an elapsed deadline fails instantly with "i/o timeout", and
gorilla latches write errors, so the first such write takes the connection
down for good.

The write most exposed to this is the close frame: CloseDataConnection
writes it on a connection that has usually been quiet for a while, and it
never set a deadline at all. When it is lost, a clean close degrades into
an abnormal one and the peer sees 1006 "unexpected EOF" instead of the code
and reason it was sent.

Move the deadline into writeMessageWithoutErrorHandling so every write gets
a fresh one, rather than repeating it at each call site and missing some.
SHIP 12.2.2 requires the node with the bigger SKI to keep the most recent
connection to a peer and close all others. keepThisConnection instead kept
"the connection initiated by the higher SKI", derived from whether the
connection was incoming. That is a different rule, and TC_SHIP_CONN_001
fails on it: the DUT keeps the older connection it dialled and tears down
the one the test tool opened.

Replace it with resolveDoubleConnection. The direction a connection was
opened in is no longer an input - that parameter was the bug - only the SKI
comparison is.

Getting the criterion right is not enough on its own, because the old check
ran after the TLS handshake and the websocket upgrade, while
TC_SHIP_CONN_001 expects the older connection closed "latest before the TLS
handshake of connection B completes". Three structural changes make that
reachable:

- verifyPeerCertificate now performs the arbiter check, per SHIP 12.2.2
  "The SHIP node with the greater SKI SHOULD check for double connections
  directly during the TLS handshake". It never rejects the incoming
  connection, it only retires the older one.
- connectionsInitiating carries the raw socket (dialState), so that check
  can abort an in-flight outgoing dial synchronously rather than waiting
  for it to return on its own. This is the case the test actually produces:
  the tool opens its connection as soon as it sees the TCP stream of ours,
  so ours is still an unfinished dial.
- registerConnection owns the swap. The registry never passes through a
  state where the SKI has no connection, so a displaced connection can tell
  it was replaced rather than disconnected, and HandleConnectionClosed
  stops reporting a peer as gone while it is still connected. Without that,
  resolving a double connection tears down the application state belonging
  to the connection that just won.

Two smaller spec items come with it: a superseded connection that had
reached data exchange is closed with the SHIP 13.4.7 termination announce
rather than a socket close, and the double close that raced its own close
frame is gone, so a rejected connection reports 4001 "double connection"
instead of dying at 1006.

A dial retired for an incoming connection also waits briefly for that
connection to register before reporting success. It is retired during the
TLS handshake, before it is known whether the incoming connection can get
past ServeHTTP at all; if it cannot, the SKI would be left with nothing
connected and nothing scheduled to fix it.

Adds scenarios driving a real Hub over TLS against a scripted SHIP peer,
so the behaviour is observed the way the certification tool observes it:
which socket died, with which close code, and in which order relative to
the TLS handshake.
@coveralls

coveralls commented Aug 2, 2026

Copy link
Copy Markdown

Coverage Status

coverage: 93.086% (+0.3%) from 92.773% — fix/ship-double-connection into dev

@kirollosnct
kirollosnct force-pushed the fix/ship-double-connection branch from 88f14b2 to 490d2f2 Compare August 2, 2026 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Test Compliance: TC_SHIP_CONN_001 — Double-connection resolution uses wrong criterion

2 participants