Skip to content

HTTPClientTransport shares one lastEventID between all SSE streams, so the standalone GET can resume a POST stream #281

Description

@wballard

Summary

HTTPClientTransport keeps one lastEventID field for all SSE streams. It writes
this field from processSSE(_:), which reads two different kinds of stream. As a
result, the standalone GET can send the event id of a POST response stream in
its Last-Event-ID header. The server then connects that GET to the wrong
stream, and the client receives no server-initiated message.

All line numbers below are at a0ae212, which is main and also tag 0.12.1.

The client code

In Sources/MCP/Base/Transports/HTTPClientTransport.swift:

Line 90 declares one field for all streams.

/// The last event ID received from the server for SSE stream resumability
private var lastEventID: String?

Line 645 declares processSSE(_:). Line 667 writes the field.

if let eventID = event.id, !eventID.isEmpty {
    self.lastEventID = eventID

processSSE(_:) reads two different kinds of stream:

  • line 361, the SSE body of a POST response;
  • line 641, the standalone GET stream.

Line 602 puts that one field on the standalone GET.

if let lastEventID = lastEventID {
    request.addValue(lastEventID, forHTTPHeaderField: HTTPHeaderName.lastEventID)

So the header on the standalone GET holds the id of the last event from any
stream, and not the id of the last event from the GET stream.

The effect on the server

Sources/MCP/Base/Transports/HTTPServer/StatefulHTTPServerTransport.swift shows
the result. handleGet (line 314) reads the header at line 329 and calls
handleResumeRequest (line 432). That function resolves the id to the stream
that owns it with replayEventsAfter (line 481). Then it connects the new GET
to that stream.

// Re-register the stream for future messages
if replay.streamID == standaloneStreamID {
    standaloneSSEContinuation = sseContinuation
} else {
    requestSSEContinuations[replay.streamID] = sseContinuation
}

If the id came from a POST stream, the second branch runs. The session then
holds no standalone stream. routeServerInitiatedMessage (line 418) finds
standaloneSSEContinuation empty at line 421. It writes "No standalone GET
stream connected, message stored for replay" and it delivers nothing. Each
elicitation/create request and each notification stops there.

Why the failure is intermittent

This is a race.

processResponse sets the session id from the response HEADERS at line 349 and
calls triggerInitialSessionIDSignal() at line 351. The standalone GET starts
as soon as that signal comes. But processSSE parses the response BODY at line
361, which is after the signal.

  • If the GET wins the race, lastEventID is still empty. The GET sends no
    Last-Event-ID header, and it is correct.
  • If the GET loses the race, lastEventID holds an event id of the POST
    stream. The GET then resumes the wrong stream.

How I measured it

A downstream package drives a real MCP client against a real
StatefulHTTPServerTransport over an in-process HTTP loopback. About one full
test run in six failed, because the client did not receive the elicitation
request. A trace of the loopback shows the difference:

  • a good run: the standalone GET carries id: _GET_stream_2;
  • a bad run: the standalone GET carries the event id of a POST request
    stream.

After the change below, 20 full test runs of 20 were clean.

Why one field can not be correct

The specification gives resumability a meaning for each stream. A client resumes
the stream that it read last on that connection. One shared field can not hold
that meaning. Each stream needs its own last event id.

A correction

A correction is public at https://github.com/swissarmyhammer/swift-sdk on
main. You can cherry-pick these two commits. If you prefer a pull request on
this repository, tell me and I will open one.

9aa6fd0 — fix(transport): scope SSE lastEventID per stream in HTTPClientTransport

It replaces the one field with an SSEStreamKind enum (postResponse and
standaloneGET) and a lastEventIDs dictionary with that enum as the key.

/// Identifies which SSE stream `processSSE(_:from:)` reads, so each
/// stream's last event ID is stored in its own slot.
private enum SSEStreamKind {
    /// The SSE body of a POST response.
    case postResponse
    /// The standalone GET SSE stream.
    case standaloneGET
}

/// The last event ID received on each SSE stream kind, kept per stream so
/// a POST response stream's id never reaches the standalone GET's
/// `Last-Event-ID` header.
private var lastEventIDs: [SSEStreamKind: String] = [:]

processSSE takes the stream kind as a parameter and writes only its own slot.
The standalone GET reads lastEventIDs[.standaloneGET] alone.

The commit adds a regression test in
Tests/MCPTests/HTTPClientTransportTests.swift: "Standalone GET carries no
Last-Event-ID from a POST response stream". The test controls the order of the
two streams, so it does not wait on the race. I watched the test fail before I
made the change.

The commit also removes a Swift 6 concurrency capture warning in
NetworkTransport.swift and warnings in HTTPServerTransportTests.swift.

0a82e31 — fix(transport): release session ID signal on SSE init timeout

This corrects a second defect that I found in the same work.
sseInitializationTimeout now really releases the streaming task when no
session id comes. The standalone GET then starts after the timeout, and not
never.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions