fix(transport): stop keeping a session and GET stream at 2026-07-28 - #1256
LizunovSergey wants to merge 4 commits into
Conversation
SEP-2567 removes sessions and the standalone GET endpoint at
2026-07-28, so an Mcp-Session-Id and a GET stream are both artifacts of
a pre-2026-07-28 server shape. The modern startup path already gets
this right: a server/discover bootstrap sets session_id to None
unconditionally. The legacy-shaped paths did not, and all three
spawn_common_stream call sites were guarded only on a session id being
present, never on the negotiated version:
- legacy startup, where a server can answer legacy initialize with
both a session id and protocolVersion 2026-07-28;
- the fallback initialize taken after server/discover fails;
- session re-establishment after an expired-session 404, which
respawned on the new id without reconsulting the version.
Gating happens where a session id is adopted rather than at each
spawn: session_id_for_version drops the id once the negotiated version
has no sessions, which leaves all three call sites with nothing to open
and stops the id being echoed on requests. PeerRequestAssociation
Unassociated then becomes unreachable over streamable HTTP instead of
merely rejected, and the receive-side enforcement stays as the defence
for anything that still slips through.
A volunteered id is dropped with a warning rather than being treated as
fatal, and the id is still recorded for the shutdown DELETE, so a
session the server really did create is torn down.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
github-actions
Bot
added
T-dependencies
The harness reached an unassociated stream via the standalone GET stream, which required the client to keep a session while negotiating 2026-07-28. That combination no longer exists, so the scenario was unreachable and the test timed out waiting for a rejection. Enforcement is strict only at >= 2026-07-28, so dropping to a legacy version would have disabled the very check under test. Use the SSE body of a post-startup notification POST instead: it carries no request id, so the stream is Unassociated at a strict protocol version, with an unrelated request still in flight. The scripted server also stops minting a session it is not entitled to at that version.
|
CI went red on Why it broke. That harness reached an unassociated stream through the standalone GET stream, which required the client to hold a session while negotiating Why not just negotiate a legacy version. That was my first instinct and it is wrong — What I did instead. Kept the protocol at Evidence. Both tests in the file pass. The renamed test is still a witness by mutation — disabling the If you would rather keep that test pinned to the legacy shape and assert the GET stream is gone separately, say the word and I will split it that way instead. |
| auth_header: config.auth_header.clone(), | ||
| protocol_headers: protocol_headers.clone(), | ||
| }); | ||
| session_id = session_id_for_version(session_id, &negotiated_version); |
There was a problem hiding this comment.
This gate runs after perform_reinitialization has already POSTed notifications/initialized with new_session_id. So during recovery, if it negotiates 2026-07-28, that request still echoes the ID. Should the gate be applied inside perform_reinitialization?
There was a problem hiding this comment.
Good catch — you're right, and it was a real hole. Fixed in d36ce25.
perform_reinitialization POSTs notifications/initialized itself, so the handshake is already complete by the time it returns and the caller's gate was too late for that one request. The startup path doesn't have this problem because its gate sits above its own initialized POST; the recovery path just wasn't matched to it.
So I moved the gate inside perform_reinitialization, directly after version negotiation and before the notification goes out. The fallback-initialize path was already fine — its gate precedes the notification, which goes through the normal message loop.
The one wrinkle is session cleanup. The caller builds SessionCleanupInfo from the id before gating on purpose, so the shutdown DELETE still tears down a session the server really created — if the gate simply moved inward and returned None, that teardown would be lost. So the function now returns a small Reinitialized struct carrying both the id as sent (cleanup_session_id, for the DELETE) and the gated one (session_id, for requests and streams). A struct rather than a 4-tuple because two Option<Arc<str>> positions next to each other are easy to swap by accident.
Two new tests in test_streamable_http_sessionless_version.rs, both driven through a real expired-session 404 so the recovery path actually runs:
replacement_handshake_at_a_modern_version_drops_the_session_too— starts on a2025-11-25session, server 404s atools/list, replacement handshake answers with a new session id and2026-07-28. Asserts theinitializednotification carries no session header, that nothing afterwards does, that no GET stream opens on the replacement session, and that the shutdownDELETEstill carries the id.replacement_handshake_at_a_legacy_version_keeps_its_session— same route ending at2025-11-25, pinning that the legacy recovery path is unchanged: the id is echoed and each session gets its GET stream.
Both checked by mutation. Restoring the old ordering fails the first on exactly your scenario (left: Some("session-issued-by-the-replacement-handshake"), right: None) while the other three still pass; separately, building cleanup from the gated id instead fails the DELETE assertion. The handler also records the teardown DELETE now instead of trying to parse its empty body as JSON, which was panicking in the server task.
Green and unchanged: test_streamable_http_stale_session (4), test_client_lifecycle_modes (15), test_server_discover_http (12), test_streamable_http_priming (5), test_discover_http_client_startup (3). cargo +nightly fmt --all --check clean; clippy clean on the lib and the test target.
The gate was applied to the id `perform_reinitialization` returned, but that function has already POSTed `notifications/initialized` on the new session by then. At 2026-07-28 that one request still echoed an Mcp-Session-Id. Move the gate inside `perform_reinitialization`, where the startup path already has it relative to its own `initialized` notification, and return the id as the server sent it alongside the gated one so the shutdown DELETE is unaffected.
Records the shutdown DELETE instead of parsing its empty body as JSON, so the teardown is assertable rather than a panic in the server task.
| auth_header: config.auth_header.clone(), | ||
| protocol_headers: protocol_headers.clone(), | ||
| }); | ||
| session_id = session_id_for_version(session_id, &negotiated_version); |
There was a problem hiding this comment.
Starting the client in ClientLifecycleMode::Auto with the existing recorder takes the fallback path and fails once this line is removed. Would you parameterize the startup tests by lifecycle mode, or add a dedicated pair of fallback tests?
| if inline_version.is_some() { | ||
| negotiated_version = request_version.clone(); |
There was a problem hiding this comment.
With a per-request _meta.protocolVersion, negotiated_version gets updated here without rechecking session_id.
| "-".to_owned(), | ||
| session, | ||
| )); | ||
| // Hang so the client keeps the stream if it opens one; the test cancels it. |
There was a problem hiding this comment.
The comment says the handler hangs so the client keeps the stream open, but this branch returns 405 immediately.
| /// 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. |
There was a problem hiding this comment.
| /// 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. | |
| /// rather than echoed, which also leaves every `spawn_common_stream` call site — each | |
| /// guarded on a session id being present — with no stream to open. |
2 participants
Closes #1108.
What was wrong
SEP-2567 removes sessions and the standalone GET endpoint at
2026-07-28, so anMcp-Session-Idand a GET stream are both artifacts of a pre-2026-07-28server shape. The modern path already gets this right — aserver/discoverbootstrap setssession_idtoNoneunconditionally. The legacy-shaped paths did not: all threespawn_common_streamcall sites were guarded only onif let Some(session_id), never onnegotiated_version.initializewith both a session id andprotocolVersion: 2026-07-28got a GET stream.initializeafterserver/discoverfails, on theinitializednotification.How
The gate goes where a session id is adopted, not at each spawn.
session_id_for_versiondrops the id once the negotiated version has no sessions, and because all three spawn sites are already guarded on the id being present, they are left with nothing to open — no change needed at any of them. The id also stops being echoed on requests.That makes
PeerRequestAssociation::Unassociatedunreachable over streamable HTTP rather than merely rejected, and the receive-side enforcement from #1055 stays as the defence for anything that still slips through.The two open questions from the issue
Both needed a decision to write the change at all, so here is what I picked and why. Happy to move either way.
1. Server returns a session id and negotiates
2026-07-28. The issue weighed dropping it silently against failing startup. I went with dropping it plus atracing::warn!, rather than either extreme. Failing would break clients against servers that work today over a SHOULD-level hardening, which seemed too blunt; dropping in complete silence hides a real server bug from whoever has to debug it. A warning naming the negotiated version puts the evidence in the log without changing whether the client connects.2. Should session cleanup be skipped too? I kept it.
SessionCleanupInfois still built from the id the server sent, so the shutdownDELETEstill fires and a session the server really did create gets torn down. Skipping cleanup would leak server-side state as the price of a client-side conformance fix, which felt like the wrong trade. The ordering in the diff is deliberate: the cleanup record is built first, then the id is dropped for request and stream purposes.I did not touch #863; if there is deliberately no session at
2026-07-28, that accessor is a legacy-only affordance, but saying so belongs in that issue.Tests
New
tests/test_streamable_http_sessionless_version.rs, a scripted server that answers legacyinitializewith anMcp-Session-Idheader and a configurableprotocolVersion, recording every request as(HTTP method, JSON-RPC method, session header):modern_version_drops_the_session_and_opens_no_stream— at2026-07-28, no GET request arrives at all and no post-handshake POST carries the header.legacy_version_keeps_the_session_and_opens_the_stream— at2025-11-25, exactly one GET arrives, it carries the session id, and the id is echoed afterwards.Both are witnesses, checked by mutation. Removing the three gating lines fails
modern_version_drops_the_session_and_opens_no_streamwhilelegacy_version_keeps_the_session_and_opens_the_streamstill passes — so the legacy assertion is genuinely pinning the unchanged path rather than passing by accident.Also green, unchanged:
test_streamable_http_stale_session(4, and it covers the recovery path this touches),test_client_lifecycle_modes(15),test_server_discover_http(12),test_streamable_http_priming(5),test_discover_http_client_startup(3).cargo +nightly fmt --all --checkclean;cargo clippy -- -D warningsclean on the lib and on the new test.