Repository navigation
Fix socket leak (CLOSE_WAIT) in NetworkTransport when reconnection is disabled - #283
Open
skirrellyjones wants to merge 2 commits into
Open
skirrellyjones wants to merge 2 commits into
skirrellyjones wants to merge 2 commits into
Conversation
ianegordon
added a commit
to ianegordon/swift-sdk
that referenced
this pull request
Sep 10, 2026
…s disabled (upstream modelcontextprotocol#283, skirrellyjones) Manifest entry 4. Upstream refs/pull/283/head, unmodified. Fixes modelcontextprotocol#282.
ianegordon
added a commit
to ianegordon/swift-sdk
that referenced
this pull request
Sep 10, 2026
This was referenced Oct 20, 2026
varun06
reviewed
Oct 24, 2026
varun06
left a comment
Contributor
There was a problem hiding this comment.
Thanks @skirrellyjones, this is a real leak and the iMCP numbers are convincing. The change in the generic
catch is the one that fixes it. Three small things:
- The
NWErrorbranch can't be reached.receiveData()wraps everyNWErrorin
MCPError.transportError(line ~765), socatch let error as NWErrornever matches. Errors and graceful
peer closes (MCPError.connectionClosed) all go through the genericcatch, which is where your second
change does the work. Fine to keep the first one for safety, but worth knowing. - One more exit leaks. The
breakafter repeated empty reads whenconnection.state != .ready
(line ~613) leaves the connection uncancelled and never finishes the message stream, soreceive()
callers hang. Could you cancel andmessageContinuation.finish(throwing: MCPError.connectionClosed)there
too (when not reconnecting)? - A regression test. This fails on
mainand passes with your change:@Test("Receive failure with reconnection disabled cancels the connection") func receiveFailureCancelsConnection() async throws { let mockConnection = MockNetworkConnection() let transport = NetworkTransport( mockConnection, heartbeatConfig: .disabled, reconnectionConfig: .disabled) try await transport.connect() let stream = await transport.receive() mockConnection.simulateFailure(error: NWError.posix(.ECONNRESET)) var streamError: Swift.Error? do { for try await _ in stream {} } catch { streamError = error } #expect(streamError != nil) var cancelled = false for _ in 0..<50 { if case .cancelled = mockConnection.state { cancelled = true; break } try await Task.sleep(for: .milliseconds(20)) } #expect(cancelled) await transport.disconnect() }
Also, please rebase onto main to pick up the CI fix (#306). Happy to merge after that.
… disabled When reconnectionConfig.enabled is false and the receive loop's connection terminates for any reason -- including the peer closing gracefully -- the loop finishes the message stream without calling connection.cancel(). NWConnection.state never transitions to .cancelled or .failed on its own after a peer-initiated FIN, so nothing else cleans up the connection either. The socket is left stuck in CLOSE_WAIT until the process exits. The reconnecting branches already call connection.cancel() before retrying; this adds the same call to the two non-reconnecting "give up" branches so the underlying socket is always released. Verified against a server using reconnectionConfig: .disabled for per-client connections (mattt/iMCP): before this fix, a burst of ~250 client connect/disconnect cycles left 247+ sockets in CLOSE_WAIT within 10 seconds; after the fix, 0. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
When the receive loop gives up after repeated empty reads on a connection that is no longer ready, cancel the connection and finish the message stream with MCPError.connectionClosed so the socket is released and receive() callers don't hang. Add a test that a receive failure with reconnection disabled finishes the stream with an error and cancels the connection. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
skirrellyjones
force-pushed
the
fix/network-transport-close-wait-leak
branch
from
October 24, 2026 14:21
c0ce9d6 to
318ff45
Compare
Author
|
Thanks for the review! Addressed in 318ff45:
Full suite passes locally (559 tests), and the docs build is clean with |
1 task done
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
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 #282.
Problem
When
reconnectionConfig.enabled == falseand the receive loop'sNWConnectionterminates for any reason — including the peer closinggracefully — the loop finishes the message stream but never calls
connection.cancel().NWConnection.statenever transitions to.cancelledor.failedon its own after a peer-initiated FIN, sonothing downstream (e.g. code watching
connection.stateto decide whento clean up) fires either. The socket is left stuck in
CLOSE_WAITuntil the process itself exits.
This hits hardest for servers that intentionally disable reconnection
for their per-client connections (the correct choice — a server
shouldn't try to reconnect to a client). Every client disconnect leaks
one file descriptor permanently.
Fix
Call
connection.cancel()in both non-reconnecting "give up" branchesof the receive loop, mirroring what the reconnecting branches already do
a few lines above (they call
self.connection.cancel()beforeretrying).
Testing
swift buildsucceeds.swift test --filter NetworkTransportTests— all 16 existing testspass, including "Resource Cleanup" and "Disconnect During Receive".
which sets
reconnectionConfig: .disabledfor its server-sideconnections): before this fix, a burst of ~250 client connect/disconnect
cycles left 247+ sockets in
CLOSE_WAITwithin 10 seconds and hadalready crashed the process once from file-descriptor exhaustion
(
SIGABRT, "Too many open files"). After the fix: 0 leaked sockets,and the connections that live through the burst work correctly (I
exercised a live MCP
tools/callover one and got a real responseback).