Track Mcp-Session-Id and protocol version in HTTP client#325
Conversation
7cb64a4 to
cc9b8b4
Compare
| # respond with 404 to requests containing an expired session ID. | ||
| # https://modelcontextprotocol.io/specification/2025-11-25/basic/transports#session-management | ||
| clear_session | ||
| raise SessionExpiredError.new( |
There was a problem hiding this comment.
This implementation currently raises SessionExpiredError for every 404, including when @session_id is nil (e.g., the very first request before initialize, or a stateless server where the URL is misconfigured).
The spec wording is:
When a client receives HTTP 404 in response to a request containing an
Mcp-Session-Id, it MUST start a new session...
So strictly speaking, "session expired" would apply only when a session ID was actually attached.
Not a blocker, and it's fine to keep the current behavior if that's intentional, but one alternative would be:
if @session_id
clear_session
raise SessionExpiredError.new(...)
else
raise RequestHandlerError.new(..., error_type: :not_found, ...)
endThat way a 404 from a wrong URL or a server that's simply not there surfaces as a generic :not_found rather than as a potentially misleading "session expired".
Would this make sense?
There was a problem hiding this comment.
Good catch, the spec scopes the "start a new session" MUST to 404s carrying an MCP-Session-Id, so the session-expired interpretation shouldn't apply to plain 404s. Updated per your suggestion: branch on @session_id and only raise SessionExpiredError (and clear session state) when a session was attached; otherwise raise a generic RequestHandlerError with :not_found. Split the test into pre-session and post-session cases.
| # https://modelcontextprotocol.io/specification/2025-11-25/basic/transports#resumability-and-redelivery | ||
| class HTTP | ||
| ACCEPT_HEADER = "application/json, text/event-stream" | ||
| SESSION_ID_HEADER = "Mcp-Session-Id" |
There was a problem hiding this comment.
The spec has this as "MCP-Session-Id". Will standardize casing for existing server code + this new client code in a separate PR.
There was a problem hiding this comment.
Good catch, it looks like this was updated in the spec at some point.
modelcontextprotocol/modelcontextprotocol#1325
cc9b8b4 to
90f91d4
Compare
Per the Streamable HTTP spec, if the server returns an Mcp-Session-Id header during `initialize`, the client MUST include it on subsequent requests. Capture the session ID and protocol version from the `initialize` response and attach them automatically on subsequent POSTs. Per the Protocol Version Header section of the spec, the client MUST also include `MCP-Protocol-Version` on all subsequent requests so the server can respond based on the negotiated protocol version. Map 404 responses to a new `SessionExpiredError` (a subclass of `RequestHandlerError` for backward compatibility) and clear local session state so callers can start a fresh session with a new `initialize` request, as required by the spec. - https://modelcontextprotocol.io/specification/2025-11-25/basic/transports#session-management - https://modelcontextprotocol.io/specification/2025-11-25/basic/transports#protocol-version-header Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
90f91d4 to
07226b7
Compare
|
Thanks! |
Summary
Mcp-Session-IdandprotocolVersionfrom theinitializeresponse and expose them on the transport assession_id/protocol_versionMcp-Session-Idon subsequent requests once capturedMCP::Client::SessionExpiredError(subclass ofRequestHandlerError) and clear local session state so callers can start a new sessionPer the Streamable HTTP spec:
https://modelcontextprotocol.io/specification/2025-11-25/basic/transports#session-management
This is part of a series of PRs splitting up #321. Previous: #322 (SSE parsing) and #323 (202 Accepted). Next: session termination via DELETE (
close), then client handshake ergonomics and example rewrite.Test plan
initializeMcp-Session-Idheader) work fineSessionExpiredErrorand clears session statebundle exec rake test)🤖 Generated with Claude Code