Terminate the client message loop when the transport stream finishes - #275
Open
robertoscipionecom wants to merge 3 commits into
Open
robertoscipionecom wants to merge 3 commits into
robertoscipionecom wants to merge 3 commits into
Conversation
The message handling loop treats the end of the transport stream as transient: it exits the `for try await`, calls `receive()` again and starts over. No transport behaves that way. Every one of them exposes a single-use stream that finishes only when the connection is over — StdioTransport on EOF, HTTPClientTransport and InMemoryTransport on disconnect, NetworkTransport on disconnect (its internal reconnect reuses the same continuation without ever finishing it). Once the stream has finished, `receive()` hands back the very same finished stream, so the `for` returns immediately and the `while true` starts over immediately: a busy loop that saturates a core for as long as the client object is alive. Measured in a shipping app: six MCP clients left on dead connections, 599% CPU, with only one server process still running. Break out of the loop instead, the same way the error branch already does. The `resourceTemporarilyUnavailable` retry — the only legitimate reason for this loop to repeat — is untouched.
The existing suite never exercised the branch: `MockTransport.receive()` creates a new stream on every call and replaces the stored continuation without finishing the previous one, so in tests the stream never finishes while the client is still connected. `StreamFinishingTransport` hands out one single-use stream and counts how many times the client asks for it. After the stream finishes, a client whose loop has terminated never asks again, so the count stays at 1. Verified against 0.12.1 without the fix: the test fails with a receive count in the thousands after a 100 ms wait.
`Client.send` suspends on a checked continuation that only a matching response can resume. The request task is unstructured, so cancelling the caller never reached it: a caller that gave up — `ping()` under a deadline, a connect wrapped in a timeout — stayed suspended until the server answered, which for a dead server is never. `cancelRequest` already did the right thing (remove the pending request, resume it with `CancellationError`, notify the server); nothing ever called it on cancellation. Wrap the continuation in `withTaskCancellationHandler` and call `cancelRequest` from the handler, and forward cancellation from `RequestContext.value` to the unstructured request task, which awaiting `.value` alone does not do. The registration itself is asynchronous, so a cancellation can land before the continuation is in the pending table, with nothing to resume and the registration arriving right after — the same hang, through a narrower door. Remember those IDs and resume the continuation as soon as it registers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NWmvQ6D9fqVct7mvEZdk5r
This was referenced Sep 10, 2026
ianegordon
added a commit
to ianegordon/swift-sdk
that referenced
this pull request
Sep 12, 2026
…cle as entry 9a StdioTransport.send wrote straight to the descriptor, so concurrent sends could interleave while one retried EAGAIN on a full pipe, splicing one message into the middle of another and putting invalid JSON on the wire. Reproduced with the PR's own regression test before the merge. Entry 9 records what the merge accepts rather than fixes: caller cancellation never reaches the send, which is accidentally protective because naive propagation would throw mid-frame and recreate the corruption; and the trade made is frame corruption exchanged for head-of-line blocking, which is the right direction but is a trade. It also records that modelcontextprotocol#275 does not address the transport's cancellation, and that the upstream PR is a draft whose SHA may move. Entry 9a is the follow-up: re-check isConnected inside the retry loop so a backpressured send terminates on disconnect. Its test caveat is recorded — it passes with the change and does not terminate without it, so it is a positive check whose cleanup is best-effort. Tracking: fork issue #16. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Eb9yGSXH1TVkg5Afsu9phk
ianegordon
added a commit
to ianegordon/swift-sdk
that referenced
this pull request
Sep 12, 2026
modelcontextprotocol#275 makes client requests cancellable by calling cancelRequest from the task cancellation handler, and cancelRequest sends notifications/cancelled. initialize reaches that path — connect() -> _initialize() -> sendAndAwait() -> send() plus RequestContext.value — so cancelling a connect() under a deadline, which the PR names as its motivating case, puts a cancellation for the initialize request on the wire. The spec forbids exactly that: The `initialize` request MUST NOT be cancelled by clients -- 2025-11-25, basic/utilities/cancellation Enforce it in one place. cancelRequest takes an ID rather than a method, so the client records the in-flight initialize request's id and cancelRequest withholds the notification for it. Both the public API and the automatic cancellation handler route through there, so neither can bypass the restriction. The caller is still released with CancellationError and a late response is still ignored; only the notification is withheld. Every other method is unaffected. Two tests, one per path: cancelling a connect() whose initialize is never answered, and calling cancelRequest with the initialize id directly. Both assert the request was actually sent first, normalise JSON's escaped slashes before matching, and bound every wait with a deadline so a regression reports a failure instead of hanging. Verified in both directions — each fails on the unguarded tree quoting the offending frame, and passes with this change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Eb9yGSXH1TVkg5Afsu9phk
ianegordon
added a commit
to ianegordon/swift-sdk
that referenced
this pull request
Sep 12, 2026
…odelcontextprotocol#275 commit 8e36cfa, robertoscipionecom) Manifest entry 10. Upstream commit 8e36cfa extracted from refs/pull/275/head, unmodified: same author, same patch-id (e588896c…), cherry-picked with -x so the commit records its own provenance. Client.send suspended on a checked continuation only a matching response could resume, and the request task was unstructured, so cancelling the caller never reached it: a caller that gave up stayed suspended until the server answered, which for a dead server is never. Three linked fixes — forward cancellation from RequestContext.value to the request task, call cancelRequest from the task cancellation handler, and remember ids cancelled before their continuation registered. Two of modelcontextprotocol#275's three commits are deliberately skipped, not adapted: 40c5951 (loop termination) and fded08a (its test) duplicate manifest entries 5 and 5a from modelcontextprotocol#221. Taking them would conflict in Client.swift and produce a duplicate testMessageLoopStopsWhenStreamFinishes declaration that does not compile. The full PR head stays fetchable as branch pr/275. Two defects in this commit are carried as accepted, not fixed; see FORK.md entry 10. A cancellation landing before the send completes is notified before the request is sent, so the peer can ignore an unknown-id cancellation and then execute the request. And cancelledBeforeRegistration cannot distinguish 'not registered yet' from 'already completed', so cancelling a finished request leaves an entry until disconnect. Both need the request-state redesign that belongs upstream, not a fork patch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Eb9yGSXH1TVkg5Afsu9phk
ianegordon
added a commit
to ianegordon/swift-sdk
that referenced
this pull request
Sep 12, 2026
…ry 10, compliance as 10a Client.send suspended on a checked continuation only a matching response could resume, and the request task was unstructured, so cancelling the caller never reached it — a caller that gave up stayed suspended until the server answered, which for a dead server is never. Entry 10 carries upstream commit 8e36cfa, which fixes that; entry 10a withholds notifications/cancelled for initialize, which the spec forbids clients from cancelling and which the entry 10 path would otherwise put on the wire. Entry 10 is an extraction rather than a merge, the first in the manifest. The inclusion policy now bounds when that is allowed: only when every skipped commit is already carried, only unmodified, cherry-picked with -x, with patch-id equality recorded and the full PR head still pushed as pr/<n>. The branch layout table gains a pr/<n>-cherrypick row. Both note why the bound exists — extraction makes the fork a curated patch set rather than a mirror, and an intermediate commit of an open PR can vanish under a rebase. Entry 10 also records two defects carried as accepted, not fixed: a cancellation landing before the send completes is notified before the request is sent, so a cancelled operation can still be executed by the peer; and cancelledBeforeRegistration cannot tell 'not registered yet' from 'already completed'. The first is flagged as a behavior risk rather than bookkeeping. Both need explicit request-state tracking, which belongs upstream. Tracking: fork issue #15. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Eb9yGSXH1TVkg5Afsu9phk
ianegordon
added a commit
to ianegordon/swift-sdk
that referenced
this pull request
Sep 12, 2026
Client.send registers its continuation from an unstructured task, so a cancellation can arrive first. addPendingRequest already handles that — it resumes the continuation with CancellationError and returns early — but the early return only exits that function, not the enclosing task, which went on to transmit the request anyway. The result is a cancellation on the wire ahead of the request it cancels: notifications/cancelled(id) → ping(id) The peer discards the cancellation as referring to an unknown id, then executes a request whose caller has already been told it was cancelled. For a ping that is harmless; for a tool call with side effects it is not. addPendingRequest now reports whether the request is pending, and send skips transmission when it is not. A cancellation notification may still precede a request that is never sent; receivers may ignore unknown ids, and that is strictly better than executing a cancelled operation. This does not cover cancellation overtaking a send already suspended in connection.send. That needs the request-state tracking (registration, sending, completion) that belongs upstream. The test cancels from inside the same actor hop as send, which lands deterministically in the pre-registration window — cancelling from outside almost never wins that race, so a naive version of this test passes regardless. Technique borrowed from the PR modelcontextprotocol#275 review probes. Verified in both directions: it fails without this change, quoting the inverted wire order, and passes with it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Eb9yGSXH1TVkg5Afsu9phk
ianegordon
added a commit
to ianegordon/swift-sdk
that referenced
this pull request
Sep 12, 2026
Issue titles are now bare 'Upstream PR#<n>'; the merge/investigate/decline label carries the decision, so the title does not duplicate it. Two places named the old convention and are updated. The investigate count was also stale: it said twelve, which was true at triage on 2026-09-10. Four were declined since (modelcontextprotocol#280, modelcontextprotocol#226, modelcontextprotocol#118, modelcontextprotocol#204) and two were promoted and merged (modelcontextprotocol#266 as entries 9/9a, modelcontextprotocol#275 as 10/10a/10b), leaving six — modelcontextprotocol#178, modelcontextprotocol#213, modelcontextprotocol#216, modelcontextprotocol#257, modelcontextprotocol#258, modelcontextprotocol#259 — now listed by number rather than by count, so the sentence cannot drift again. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Eb9yGSXH1TVkg5Afsu9phk
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.
Problem
Client.connect(transport:)runs its message handling loop as arepeat { for try await … } while true, which treats the end of the transport streamas transient: it exits the
for, callsreceive()again and starts over.No transport behaves that way. Every transport in this repository exposes a
single-use stream that finishes only when the connection is over:
finish()sitesStdioTransportreadLoopon EOF/read error,disconnect()HTTPClientTransportdisconnect()onlyInMemoryTransportdisconnect(), peer disconnectNetworkTransportdisconnect()/isStopping— its internal reconnect reuses the same continuation without finishing itOnce the stream has finished,
receive()hands back the very same finished stream, sothe
forreturns immediately and thewhile truestarts over immediately. The result isa busy loop that saturates a core for as long as the
Clientobject is alive — and sincethe loop's
Taskholds a strong reference to theClient, that is forever unless someonecalls
disconnect().Measured in a shipping macOS app: six MCP clients left on dead stdio connections,
599% CPU across six spinning threads, with only one server process still running.
A
sampleof the process shows all six threads insideclosure #1 in Client.connect(transport:)→AsyncThrowingStream.Iterator.next().The loop already has the right exit (
break, in the genericcatch), but it is onlyreachable when the stream throws something other than
Errno.resourceTemporarilyUnavailable. A stream that finishes cleanly never gets there.Fix
Break out of the loop when the stream finishes, the same way the error branch already
does. The
resourceTemporarilyUnavailableretry — the only legitimate reason for thisloop to repeat — is untouched. The
repeatis labelled so thebreakinside thedoblock is unambiguous to the reader.
Tests
A regression test is included:
Message loop stops when the transport stream finishes.StreamFinishingTransporthands out one single-use stream and counts how many times theclient asks for it. Once the stream finishes, a client whose loop has terminated never
asks again, so the count stays at 1.
0.12.1without the fix: fails — the receive count is in the thousandsafter a 100 ms wait.
Full suite: 552 tests in 40 suites passing (551 before this PR, all unchanged).
The branch was not covered before, which is why the bug survived:
MockTransport.receive()creates a new stream on every call and replaces the stored continuation without finishing
the previous one, so in tests the stream never finishes while the client is still
connected.