Skip to content

Fix socket leak (CLOSE_WAIT) in NetworkTransport when reconnection is disabled - #283

Open
skirrellyjones wants to merge 2 commits into
modelcontextprotocol:mainfrom
skirrellyjones:fix/network-transport-close-wait-leak
Open

skirrellyjones wants to merge 2 commits into
modelcontextprotocol:mainfrom
skirrellyjones:fix/network-transport-close-wait-leak

Conversation

@skirrellyjones

Copy link
Copy Markdown

Fixes #282.

Problem

When reconnectionConfig.enabled == false and the receive loop's
NWConnection terminates for any reason — including the peer closing
gracefully — the loop finishes the message stream but never calls
connection.cancel(). NWConnection.state never transitions to
.cancelled or .failed on its own after a peer-initiated FIN, so
nothing downstream (e.g. code watching connection.state to decide when
to clean up) fires either. The socket is left stuck in CLOSE_WAIT
until 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" branches
of the receive loop, mirroring what the reconnecting branches already do
a few lines above (they call self.connection.cancel() before
retrying).

Testing

  • swift build succeeds.
  • swift test --filter NetworkTransportTests — all 16 existing tests
    pass, including "Resource Cleanup" and "Disconnect During Receive".
  • Verified against a real server (mattt/iMCP,
    which sets reconnectionConfig: .disabled for its server-side
    connections): before this fix, a burst of ~250 client connect/disconnect
    cycles left 247+ sockets in CLOSE_WAIT within 10 seconds and had
    already 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/call over one and got a real response
    back).

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

@varun06 varun06 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. The NWError branch can't be reached. receiveData() wraps every NWError in
    MCPError.transportError (line ~765), so catch let error as NWError never matches. Errors and graceful
    peer closes (MCPError.connectionClosed) all go through the generic catch, which is where your second
    change does the work. Fine to keep the first one for safety, but worth knowing.
  2. One more exit leaks. The break after repeated empty reads when connection.state != .ready
    (line ~613) leaves the connection uncancelled and never finishes the message stream, so receive()
    callers hang. Could you cancel and messageContinuation.finish(throwing: MCPError.connectionClosed) there
    too (when not reconnecting)?
  3. A regression test. This fails on main and 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.

skirrellyjones and others added 2 commits October 24, 2026 10:17
… 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
skirrellyjones force-pushed the fix/network-transport-close-wait-leak branch from c0ce9d6 to 318ff45 Compare October 24, 2026 14:21
@skirrellyjones
skirrellyjones requested a review from a team as a code owner October 24, 2026 14:21
@skirrellyjones

Copy link
Copy Markdown
Author

Thanks for the review! Addressed in 318ff45:

  • Empty-read exit: when the loop gives up because the connection is no longer ready, it now cancels the connection and finishes the stream with MCPError.connectionClosed, so receive() callers don't hang.
  • Regression test: added "Receive failure with reconnection disabled cancels the connection" using MockNetworkConnection + ECONNRESET. Confirmed it fails on main and passes with the fix.
  • Rebased onto current main (includes Fix macOS CI: use the Xcode toolchain instead of standalone Swift 6.1 #306).
  • Left the NWError catch in place as a safety net, per your note.

Full suite passes locally (559 tests), and the docs build is clean with --warnings-as-errors. CI is awaiting workflow approval on the fork run.

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.

NetworkTransport leaks the underlying socket (CLOSE_WAIT) when reconnection is disabled and the receive loop terminates

2 participants