Skip to content

Keep HTTP requests separate during cancellation - #298

Open
EhsanAzish80 wants to merge 6 commits into
modelcontextprotocol:mainfrom
EhsanAzish80:review/http-identity-cancellation
Open

EhsanAzish80 wants to merge 6 commits into
modelcontextprotocol:mainfrom
EhsanAzish80:review/http-identity-cancellation

Conversation

@EhsanAzish80

@EhsanAzish80 EhsanAzish80 commented Sep 26, 2026 •

Copy link
Copy Markdown

This brings together #264, #260, and #270. Stateless HTTP requests get separate internal IDs, while each response keeps the client's original ID. Cancellation reaches the matching handler before the HTTP exchange ends, and a late handler result cannot send a second response.

Following the review discussion, an ambiguous cancellation ID leaves the active requests alone. A duplicate outstanding ID is rejected only when it is a genuine conflict. For the 2025 HTTP path, the cancellation POST returns 202 and the original POST ends as HTTP 200 with an empty SSE stream, without a JSON-RPC response. This wire behavior needs maintainer review before merge. This change does not add a default SDK timeout.

The HTTP adapter owns the result when it cancels an exchange, including timeouts and disconnects. In-flight requests ended by server shutdown now complete with HTTP 503 instead of a generic 500.

Local checks passed: 565 Swift tests in 41 suites, plus a real HTTP loopback check for cancellation, cleanup, and server shutdown. CI is awaiting maintainer approval on this revision.

Tests were written with AI help.

jstar0 and others added 5 commits September 25, 2026 20:48
Signed-off-by: King Star <mcxin.y@gmail.com>
…essHTTPServerTransport

A request cancelled via notifications/cancelled correctly receives no
JSON-RPC response from Server, but nothing resumed the transport's HTTP
response waiter, so the original POST hung until transport termination.

The transport now detects notifications/cancelled, forwards it to the
Server unchanged (its existing cancellation path still cancels the
handler task), and resumes the matching waiter with a synthesized
JSON-RPC error response (code -32002, "Request cancelled"), satisfying
the Streamable HTTP requirement that a request POST receive one JSON
object. Unknown, already-completed, and requestId-less cancellations
are ignored per spec.

Fixes modelcontextprotocol#255

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@EhsanAzish80
EhsanAzish80 marked this pull request as ready for review September 27, 2026 06:01

Copy link
Copy Markdown

The updated head still completes a cancelled MCP request by synthesizing a JSON-RPC error:

AnyMethod.response(
    id: waiter.originalID,
    error: .serverError(code: -32800, message: message)
)

That looks inconsistent with the cancellation contract. For notifications/cancelled, MCP says receivers SHOULD stop processing, free resources, and not send a response for the cancelled request.

I agree the HTTP exchange itself must not hang, but those are separable requirements. The transport can terminate the POST without producing a JSON-RPC response associated with the cancelled request — e.g. 204/no body when nothing has been committed — while still forwarding the cancellation so the handler stops.

Could completeCancelledExchange finish the waiter with an HTTP no-content outcome rather than fabricating -32800, and update the tests to assert an empty response body? That would preserve the transport liveness fix without changing MCP-level cancellation semantics.

AI-assisted review; re-checked this against the updated 2c3e81b head before posting.

@EhsanAzish80

Copy link
Copy Markdown
Author

I pushed a follow-up for the cancellation review: the original POST now ends as a 200 empty SSE stream, without a synthetic JSON-RPC error. This changes our earlier -32800 choice, so I’d like a maintainer call on the wire behavior before merge. All 565 Swift tests passed locally, plus a real HTTP client/server cancellation check. AI helped write the new tests; I ran them. Could a maintainer review this and approve CI?

ianegordon pushed a commit to ianegordon/swift-sdk that referenced this pull request Oct 13, 2026
…ests without a reply (upstream modelcontextprotocol#298, EhsanAzish80)

Supersedes the contributions of modelcontextprotocol#264, modelcontextprotocol#260 and modelcontextprotocol#270 (manifest entries 1-3),
which modelcontextprotocol#298 combines. Conflicts resolved in favour of modelcontextprotocol#298 in
HTTPServerTypes.swift, StatelessHTTPServerTransport.swift, Server.swift,
CancellationTests.swift and HTTPServerTransportTests.swift.

Fork changes re-applied on top:
- modelcontextprotocol#258's Server.init description/websiteUrl/icons parameters
- modelcontextprotocol#221's MockTransport receiveCallCount / finishStream
- entry 1a's collision regression tests and their makeStatelessPOSTRequest
  authorization overload

Behavior: a cancelled original POST now ends as HTTP 200 with an empty
text/event-stream and no JSON-RPC body (previously -32002 from modelcontextprotocol#260).
Follow-up on when to use -32800: fork issue #41.

Full suite: 589 tests in 43 suites passed.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
ianegordon pushed a commit to ianegordon/swift-sdk that referenced this pull request Oct 13, 2026
…ede entries 1-3

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
ianegordon added a commit to ianegordon/swift-sdk that referenced this pull request Oct 13, 2026
…rder

Entry 3a broke two conventions: letter suffixes mark fork additions, and
the Order column claims merge order, in which modelcontextprotocol#298 comes after modelcontextprotocol#258. The
maintenance procedure now says to skip superseded rows.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
EhsanAzish80 added a commit to ianegordon/swift-sdk that referenced this pull request Oct 13, 2026
298-delta: bring stateless HTTP cancellation to upstream modelcontextprotocol#298's behavior
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.

4 participants