-
Notifications
You must be signed in to change notification settings - Fork 641
fix(transport): stop keeping a session and GET stream at 2026-07-28 #1256
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
LizunovSergey
wants to merge
4
commits into
modelcontextprotocol:main
Choose a base branch
from
LizunovSergey:fix/1108-sessionless-at-modern-version
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+564
−47
Open
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
06b54aa
fix(transport): stop keeping a session and GET stream at 2026-07-28
LizunovSergey ac58bf5
test(sep-2260): drive enforcement from a stream that survives the fix
LizunovSergey 7566a73
fix(transport): drop the session before the recovery handshake completes
LizunovSergey d36ce25
test(sep-2567): pin the replacement handshake and its teardown
LizunovSergey File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -90,6 +90,51 @@ fn request_version_headers( | |
| (version, headers) | ||
| } | ||
|
|
||
| /// Decides whether a session id survives version negotiation. | ||
| /// | ||
| /// SEP-2567 removes sessions and the standalone GET endpoint at | ||
| /// [`ProtocolVersion::STANDARD_HEADERS`], so at that version an `Mcp-Session-Id` and a GET | ||
| /// stream are both artifacts of a pre-`2026-07-28` server shape. A legacy-shaped handshake | ||
| /// can still answer with a session id while negotiating that version; the id is dropped | ||
| /// rather than echoed, which also leaves every `spawn_common_stream` call site — all three | ||
| /// of which are guarded on a session id being present — with no stream to open. | ||
| /// | ||
| /// Dropping is deliberate rather than fatal: refusing to start would break clients against | ||
| /// servers that work today, and the receive-side enforcement added for SEP-2260 still | ||
| /// rejects anything that reaches the client over a stream it should not have. The caller | ||
| /// keeps the original id for the shutdown `DELETE`, so a session the server really did | ||
| /// create is still torn down. | ||
| fn session_id_for_version( | ||
| session_id: Option<Arc<str>>, | ||
| negotiated_version: &ProtocolVersion, | ||
| ) -> Option<Arc<str>> { | ||
| if negotiated_version < &ProtocolVersion::STANDARD_HEADERS { | ||
| return session_id; | ||
| } | ||
| if session_id.is_some() { | ||
| tracing::warn!( | ||
| version = negotiated_version.as_str(), | ||
| "server returned an Mcp-Session-Id while negotiating a version that has no sessions; \ | ||
| the id will not be sent on requests and no standalone GET stream will be opened" | ||
| ); | ||
| } | ||
| None | ||
| } | ||
|
|
||
| /// The session established by [`StreamableHttpClientWorker::perform_reinitialization`]. | ||
| /// | ||
| /// The two ids are the same id at different stages of [`session_id_for_version`], and they | ||
| /// are deliberately not interchangeable: `cleanup_session_id` is what the server sent, kept | ||
| /// so the shutdown `DELETE` still tears down a session the server really created, while | ||
| /// `session_id` is what may be echoed on requests and used to open a standalone GET stream. | ||
| /// At [`ProtocolVersion::STANDARD_HEADERS`] the latter is always `None`. | ||
| struct Reinitialized { | ||
| session_id: Option<Arc<str>>, | ||
| cleanup_session_id: Option<Arc<str>>, | ||
| negotiated_version: ProtocolVersion, | ||
| protocol_headers: HashMap<HeaderName, HeaderValue>, | ||
| } | ||
|
|
||
| fn cache_tools_from_response( | ||
| cache: &mut HashMap<String, Arc<JsonObject>>, | ||
| message: &mut ServerJsonRpcMessage, | ||
|
|
@@ -950,24 +995,21 @@ impl<C: StreamableHttpClient> StreamableHttpClientWorker<C> { | |
| /// future remains `Send` without requiring `C: Sync`). POSTs the saved | ||
| /// initialize request without a session ID, extracts the new session ID and | ||
| /// protocol version, sends `notifications/initialized`, and returns the new | ||
| /// `(session_id, protocol_headers)` pair. The init result message is **not** | ||
| /// session in a [`Reinitialized`]. The init result message is **not** | ||
| /// forwarded to the handler because the handler already processed the original | ||
| /// initialization. | ||
| /// | ||
| /// The handshake completes here, so [`session_id_for_version`] is applied here too: | ||
| /// the `initialized` notification is part of the new session and must not echo an id | ||
| /// the negotiated version has no sessions for. | ||
| async fn perform_reinitialization( | ||
| client: C, | ||
| saved_init_request: ClientJsonRpcMessage, | ||
| uri: Arc<str>, | ||
| auth_header: Option<String>, | ||
| custom_headers: HashMap<HeaderName, HeaderValue>, | ||
| max_sse_event_size: usize, | ||
| ) -> Result< | ||
| ( | ||
| Option<Arc<str>>, | ||
| ProtocolVersion, | ||
| HashMap<HeaderName, HeaderValue>, | ||
| ), | ||
| StreamableHttpError<C::Error>, | ||
| > { | ||
| ) -> Result<Reinitialized, StreamableHttpError<C::Error>> { | ||
| let (init_msg, new_session_id_str) = client | ||
| .post_message_with_max_sse_event_size( | ||
| uri.clone(), | ||
|
|
@@ -981,10 +1023,13 @@ impl<C: StreamableHttpClient> StreamableHttpClientWorker<C> { | |
| .expect_initialized::<C::Error>() | ||
| .await?; | ||
|
|
||
| let new_session_id: Option<Arc<str>> = new_session_id_str.map(|s| Arc::from(s.as_str())); | ||
| let cleanup_session_id: Option<Arc<str>> = | ||
| new_session_id_str.map(|s| Arc::from(s.as_str())); | ||
|
|
||
| let (negotiated_version, new_protocol_headers) = | ||
| negotiate_version_headers(&init_msg, custom_headers); | ||
| let new_session_id = | ||
| session_id_for_version(cleanup_session_id.clone(), &negotiated_version); | ||
|
|
||
| let initialized_notification = ClientJsonRpcMessage::notification( | ||
| ClientNotification::InitializedNotification(InitializedNotification { | ||
|
|
@@ -1011,7 +1056,12 @@ impl<C: StreamableHttpClient> StreamableHttpClientWorker<C> { | |
| .await? | ||
| .expect_accepted_or_json::<C::Error>()?; | ||
|
|
||
| Ok((new_session_id, negotiated_version, new_protocol_headers)) | ||
| Ok(Reinitialized { | ||
| session_id: new_session_id, | ||
| cleanup_session_id, | ||
| negotiated_version, | ||
| protocol_headers: new_protocol_headers, | ||
| }) | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -1133,6 +1183,7 @@ impl<C: StreamableHttpClient> Worker for StreamableHttpClientWorker<C> { | |
| auth_header: config.auth_header.clone(), | ||
| protocol_headers: protocol_headers.clone(), | ||
| }); | ||
| session_id = session_id_for_version(session_id, &negotiated_version); | ||
|
|
||
| context.send_to_handler(message).await?; | ||
| if is_legacy_startup { | ||
|
|
@@ -1235,7 +1286,12 @@ impl<C: StreamableHttpClient> Worker for StreamableHttpClientWorker<C> { | |
| ) => result.unwrap_or(Err(StreamableHttpError::SessionRecoveryTimeout)), | ||
| }; | ||
| match recovery { | ||
| Ok((new_session_id, new_version, new_headers)) => { | ||
| Ok(Reinitialized { | ||
| session_id: new_session_id, | ||
| cleanup_session_id, | ||
| negotiated_version: new_version, | ||
| protocol_headers: new_headers, | ||
| }) => { | ||
| streams.abort_all(); | ||
| while streams.join_next().await.is_some() {} | ||
| request_stream_cancellations.clear(); | ||
|
|
@@ -1254,10 +1310,12 @@ impl<C: StreamableHttpClient> Worker for StreamableHttpClientWorker<C> { | |
| session_id = new_session_id; | ||
| negotiated_version = new_version; | ||
| protocol_headers = new_headers; | ||
| session_cleanup_info = session_id.as_ref().map(|sid| SessionCleanupInfo { | ||
| // Built from the id as sent, not the gated one, so a session the | ||
| // server really created is still torn down at shutdown. | ||
| session_cleanup_info = cleanup_session_id.map(|sid| SessionCleanupInfo { | ||
| client: self.client.clone(), | ||
| uri: config.uri.clone(), | ||
| session_id: sid.clone(), | ||
| session_id: sid, | ||
| auth_header: config.auth_header.clone(), | ||
| protocol_headers: protocol_headers.clone(), | ||
| }); | ||
|
|
@@ -1517,6 +1575,7 @@ impl<C: StreamableHttpClient> Worker for StreamableHttpClientWorker<C> { | |
| auth_header: config.auth_header.clone(), | ||
| protocol_headers: protocol_headers.clone(), | ||
| }); | ||
| session_id = session_id_for_version(session_id, &negotiated_version); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Starting the client in |
||
| context.send_to_handler(initialize_response).await?; | ||
| awaiting_fallback_initialized = true; | ||
| continue; | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.