Repository navigation
Keep HTTP requests separate during cancellation - #298
EhsanAzish80 wants to merge 12 commits into
Conversation
|
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 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 AI-assisted review; re-checked this against the updated 2c3e81b head before posting. |
|
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? |
…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>
…ede entries 1-3 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…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>
298-delta: bring stateless HTTP cancellation to upstream modelcontextprotocol#298's behavior
25a7a6f to
1c619a8
Compare
There was a problem hiding this comment.
Thanks @EhsanAzish80. Consolidating #260/#264/#270 into one design is the right call, and the routing
looks solid. The suite passes locally, and I ran the cancellation and HTTP server suites 10× with no flakes.
One required change:
Only change the id, not the whole body. routingRequest/restoringResponseID re-serialize every
request and response with JSONSerialization, which rewrites payload data. With the exact options in the PR:
"id": trueis accepted asnumber(1)(JSON-RPC ids must be strings or numbers)- tool arguments
0.1→0.10000000000000001,19.99→19.989999999999998,0.3→0.29999999999999999
The response side would rewrite numbers in every tool result sent to clients. A Codable round trip
(JSONDecoder→ JSON value →JSONEncoder) keeps these exactly. Could you patch just theidthat way
(or splice the top-level"id"), and reject ids that aren't strings or integers?
On the wire-format question: an empty SSE stream is the cleanest "no JSON-RPC response", but this
transport acceptsAccept: application/jsonalone (AcceptHeaderValidator(mode: .jsonOnly)). I'd return
the empty SSE stream whenAcceptincludestext/event-streamand fall back to a JSON-RPC error otherwise.
Smaller points:
- Rejecting a duplicate outstanding id sends an error with that same id, so the client matches it to the
original request and drops the real response later. Since duplicates violate the spec, logging and
ignoring may be safer; either way, worth documenting. - When the HTTP caller goes away,
cancelWaitingExchangefrees the exchange but the handler keeps running;
callingcancelRequest(id:)for the routed id would stop it. - The
.responseand.notificationcases have identical bodies now. - Release-note material: cancelled requests no longer get a response on any transport.
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>
1c619a8 to
206bc64
Compare
|
Updated ID routing to change only the top-level |
varun06
left a comment
There was a problem hiding this comment.
Thanks @EhsanAzish80, this addresses everything blocking.
One optional nit: JSONEncoder escapes slashes, so a string id "a/b" comes back as "a\/b" (same value)..withoutEscapingSlashes would make it byte-exact.
The duplicate-id and disconnect-cancellation points from my earlier review can be follow-ups.
|
Thanks @varun06. I added |
akhilcb
left a comment
There was a problem hiding this comment.
Thanks @EhsanAzish80, the blocking items are all fixed. I fetched f617597 and the full suite passes locally. Deferring the duplicate-id and disconnect points sounds good.
A few small suggestions, none blocking:
handlePoststill has.notificationand.responseas separate cases with identical bodies. Combining them again, as onmain, would close out that earlier nit.- When the JSON parses but the
idis invalid,-32600Invalid Request may fit better than-32700Parse error. - It might be worth a line in the PR description that
Server.stop()now cancels in-flight handlers.
|
Thanks, I took care of these in |
|
RouteObjects is building a local stdio These results are from The tested revision's unchanged cancellation tests and HTTP/cancellation/context tests pass: 66 tests / 5 suites on each platform. An independent fixture that holds Two additional lifecycle behaviors were observable at the tested revision: Linux qualification of that revision is limited to the selected tests: its complete upstream test target was blocked by existing sending-closure diagnostics in the unrelated Independent fixtures (early cancellation passes; cleanup/EOF expose the follow-up behaviors)Place these two files in the SDK test target. Each fixture is bounded; unchanged fixtures were run against each pinned revision with production SDK sources unmodified. import Foundation
import Logging
import Testing
@testable import MCP
// CHANGE: deterministic gates exercise admission and lifecycle boundaries without real services.
actor ResourceTestGate {
private var opened = false
private var waiters: [CheckedContinuation<Void, Never>] = []
func wait() async {
if opened { return }
await withCheckedContinuation { waiters.append($0) }
}
func open() {
opened = true
let pending = waiters
waiters.removeAll()
for waiter in pending { waiter.resume() }
}
}
actor ResourceTestProbe {
var started: [Int] = []
var completed: [Int] = []
var cancelled: [Int] = []
var marked = false
func start(_ id: Int) { started.append(id) }
func complete(_ id: Int) { completed.append(id) }
func cancel(_ id: Int) { cancelled.append(id) }
func mark() { marked = true }
}
actor ResourceTestTransport: Transport, HTTPContextProviding {
nonisolated let logger = Logger(label: "resource-tests", factory: { _ in SwiftLogNoOpLogHandler() })
let stream: AsyncThrowingStream<Data, Error>
let continuation: AsyncThrowingStream<Data, Error>.Continuation
var sent: [Data] = []
var contextEntered = false
var blockedContextID: ID?
var contextGate: ResourceTestGate?
var sendGate: ResourceTestGate?
var sendEntered = false
init() {
(stream, continuation) = AsyncThrowingStream.makeStream()
}
func connect() async throws {}
func disconnect() async {
continuation.finish()
await sendGate?.open()
}
func send(_ data: Data) async throws {
sendEntered = true
await sendGate?.wait()
try Task.checkCancellation()
sent.append(data)
}
func receive() -> AsyncThrowingStream<Data, Error> { stream }
func queue(_ data: Data) { continuation.yield(data) }
func queue<M: MCP.Method>(_ request: Request<M>) throws {
continuation.yield(try JSONEncoder().encode(request))
}
func cancel(_ id: ID) throws {
continuation.yield(try JSONEncoder().encode(CancelledNotification.message(.init(requestId: id))))
}
func eof() { continuation.finish() }
func blockContext(id: ID, gate: ResourceTestGate) {
blockedContextID = id
contextGate = gate
}
func blockSend(_ gate: ResourceTestGate) { sendGate = gate }
func httpRequestContext(for id: ID) async -> HTTPRequest? {
if id == blockedContextID {
contextEntered = true
await contextGate?.wait()
}
return nil
}
}
enum ResourceTestWork: MCP.Method {
static let name = "test/resource-work"
typealias Parameters = Int
typealias Result = String
}
struct ResourceTestTimeout: Error {}
func resourceEventually(_ predicate: @Sendable () async -> Bool) async throws {
let deadline = ContinuousClock.now + .seconds(3)
while !(await predicate()) {
guard ContinuousClock.now < deadline else { throw ResourceTestTimeout() }
try await Task.sleep(for: .milliseconds(1))
}
}import Foundation
import Testing
@testable import MCP
@Suite("Server lifecycle regressions")
struct ServerLifecycleRegressionTests {
@Test("Cancellation during HTTP context lookup prevents handler execution")
func cancellationBeforeHandlerRegistration() async throws {
let transport = ResourceTestTransport()
let gate = ResourceTestGate()
await transport.blockContext(id: 1, gate: gate)
let probe = ResourceTestProbe()
let server = Server(name: "early-cancel", version: "1")
await server.withMethodHandler(ResourceTestWork.self) { id in
await probe.start(id)
return "ok"
}
try await server.start(transport: transport)
// CHANGE: the observer runs after the SDK cancellation handler, proving delivery before release.
await server.onNotification(CancelledNotification.self) { _ in await probe.mark() }
try await transport.queue(ResourceTestWork.request(id: 1, 1))
try await resourceEventually { await transport.contextEntered }
try await transport.cancel(1)
try await resourceEventually { await probe.marked }
await gate.open()
try await transport.queue(ResourceTestWork.request(id: 2, 2))
try await resourceEventually { await probe.started.contains(2) }
// A negative observation needs a bounded settling interval after the deterministic race barrier.
try await Task.sleep(for: .milliseconds(50))
#expect(await probe.started.contains(1) == false)
for data in await transport.sent {
let response = try JSONDecoder().decode(Response<ResourceTestWork>.self, from: data)
#expect(response.id != .number(1))
}
await server.stop()
}
@Test("Stop awaits cooperative handler cleanup")
func stopAwaitsCleanup() async throws {
let transport = ResourceTestTransport()
let cleanupGate = ResourceTestGate()
let probe = ResourceTestProbe()
let stopProbe = ResourceTestProbe()
let server = Server(name: "cleanup", version: "1")
await server.withMethodHandler(ResourceTestWork.self) { id in
await probe.start(id)
do {
try await Task.sleep(for: .seconds(10))
return "unexpected"
} catch is CancellationError {
await probe.cancel(id)
await cleanupGate.wait()
await probe.complete(id)
throw CancellationError()
}
}
try await server.start(transport: transport)
try await transport.queue(ResourceTestWork.request(id: 1, 1))
try await resourceEventually { await probe.started == [1] }
let stopping = Task { await server.stop(); await stopProbe.mark() }
do {
try await resourceEventually { await probe.cancelled == [1] }
#expect(await stopProbe.marked == false)
} catch {
Issue.record("Stop did not cancel the active handler")
}
await cleanupGate.open()
await stopping.value
#expect(await probe.completed == [1])
#expect(await transport.sent.isEmpty)
}
@Test("Input EOF cancels active handlers and awaits their cleanup")
func eofRetiresWork() async throws {
let transport = ResourceTestTransport()
let probe = ResourceTestProbe()
let server = Server(name: "EOF", version: "1")
await server.withMethodHandler(ResourceTestWork.self) { id in
await probe.start(id)
do { try await Task.sleep(for: .seconds(10)) }
catch is CancellationError { await probe.complete(id); throw CancellationError() }
return "unexpected"
}
try await server.start(transport: transport)
try await transport.queue(ResourceTestWork.request(id: 1, 1))
try await resourceEventually { await probe.started == [1] }
await transport.eof()
await server.waitUntilCompleted()
#expect(await probe.completed == [1])
#expect(await transport.sent.isEmpty)
await server.stop()
}
} |
Combines #264, #260, and #270. Stateless HTTP requests get private IDs, and each response restores the client's original ID without changing the rest of the JSON. Invalid IDs return 400. Valid JSON with an invalid request ID now returns
-32600Invalid Request; malformed JSON returns-32700Parse error. Ambiguous cancellation IDs leave active requests alone.The cancellation POST returns 202. The original POST ends with an empty SSE response when the client accepts SSE, or a
-32800JSON-RPC error with its original ID for JSON-only clients. The adapter owns disconnect and timeout outcomes; server shutdown returns 503. No default SDK timeout is added.Server.stop()also cancels in-flight handlers for all transports.The restored ID keeps slashes unescaped, so a string ID such as
"a/b"returns with the same bytes. The branch pins the Swift 6.1 test job to Ubuntu 24.04 and makes existing client tests wait for initialize instead of a fixed 10 ms sleep. Those CI fixes are shared with #307 and #295.Tests: the slash-ID test failed before the fix and passed afterward. All 19 stateless HTTP transport tests and all 597 Swift tests passed locally on
bc2e1cf. All six CI jobs passed on the same commit, including Ubuntu, both macOS jobs, conformance, documentation, and the Linux static build. AI helped write the tests; I ran them.The duplicate-ID and disconnect-cancellation refinements can be follow-ups, as discussed in review.