Skip to content

Terminate the client message loop when the transport stream finishes - #275

Open
robertoscipionecom wants to merge 3 commits into
modelcontextprotocol:mainfrom
robertoscipionecom:coders-client-loop-fix
Open

robertoscipionecom wants to merge 3 commits into
modelcontextprotocol:mainfrom
robertoscipionecom:coders-client-loop-fix

Conversation

@robertoscipionecom

@robertoscipionecom robertoscipionecom commented Aug 16, 2026

Copy link
Copy Markdown

Problem

Client.connect(transport:) runs its message handling loop as a
repeat { for try await … } while true, which treats the end of the transport stream
as transient: it exits the for, calls receive() 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:

Transport finish() sites Reopens?
StdioTransport readLoop on EOF/read error, disconnect() no
HTTPClientTransport disconnect() only no
InMemoryTransport disconnect(), peer disconnect no
NetworkTransport disconnect() / isStopping — its internal reconnect reuses the same continuation without finishing it no

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. The result is
a busy loop that saturates a core for as long as the Client object is alive — and since
the loop's Task holds a strong reference to the Client, that is forever unless someone
calls 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 sample of the process shows all six threads inside
closure #1 in Client.connect(transport:)AsyncThrowingStream.Iterator.next().

The loop already has the right exit (break, in the generic catch), but it is only
reachable 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 resourceTemporarilyUnavailable retry — the only legitimate reason for this
loop to repeat — is untouched. The repeat is labelled so the break inside the do
block is unambiguous to the reader.

Tests

A regression test is included: Message loop stops when the transport stream finishes.

StreamFinishingTransport hands out one single-use stream and counts how many times the
client asks for it. Once the stream finishes, a client whose loop has terminated never
asks again, so the count stays at 1.

  • Against 0.12.1 without the fix: fails — the receive count is in the thousands
    after a 100 ms wait.
  • With the fix: passes.

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.

robertoscipionecom and others added 3 commits August 16, 2026 14:03
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
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
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.

1 participant