fix: self-describing error for requests before session initialization (was generic -32602)#3128
Open
MrSampson wants to merge 3 commits into
Open
Conversation
ServerSession._received_request raised a bare RuntimeError for any
request received before the session completed its initialize
handshake. That exception propagated to BaseSession._receive_loop's
blanket except-Exception handler, which discards the exception's
message entirely and always responds with the same hardcoded
ErrorData(code=INVALID_PARAMS, message="Invalid request parameters")
-- indistinguishable from an actually malformed request.
This surfaces most concretely with the SSE transport: connect_sse()
issues a brand-new session_id and a fresh, uninitialized ServerSession
on every GET /sse. A client that reconnects (after an idle-timeout
abort, a network blip, or a proxy restart) without resending
'initialize' on the new stream gets -32602 on every subsequent call,
indefinitely -- which reads as "your parameters are wrong" when the
real condition is "this session was never initialized." We hit this in
production; the -32602 text led to real diagnostic time being spent
looking at the wrong tool's parameter schema before server-side logs
("Received request before initialization was complete") revealed the
actual cause.
Fix: answer the request directly via responder.respond(ErrorData(...))
-- a public API already used by the InitializeRequest branch just
above it -- instead of raising, so the generic catch-all never sees
it. Uses INVALID_REQUEST rather than INVALID_PARAMS: the request's
parameters aren't the problem, the session's initialization state is.
Updated the existing test_other_requests_blocked_before_initialization,
which had asserted error_code == INVALID_PARAMS -- i.e. it encoded the
bug as expected behavior. Now asserts INVALID_REQUEST and checks the
message text.
Based on v1.x per CONTRIBUTING.md's guidance for bug fixes to a
released version (this reproduces against mcp==1.28.1, the latest 1.x
release; main is the in-progress v2.0.0 rewrite and has moved past this
code shape entirely).
… cite precedent
Addresses review feedback on the session-not-initialized error fix:
- The error message asserted a specific cause ("this session_id's
stream was reconnected or lost") that _initialization_state has no
way to actually verify -- a session that was never initialized at
all (e.g. a client bug) hits the same branch and would get told a
story that's simply false in that case. Reworded to state the fact
(an initialize handshake is required) without asserting why this
particular session never completed one.
- Cited the existing INVALID_REQUEST precedent in
streamable_http_manager.py's "Session not found" responses (same
class of session-validity condition) in the inline comment, rather
than relying on JSON-RPC semantics alone.
- This fix has an untested, unmentioned side effect: answering via
responder.respond() means RequestResponder.__exit__ now sees
_completed=True and fires on_complete, popping the request out of
_in_flight. The old bare RuntimeError bypassed the responder's
context-manager cleanup entirely, so every rejected pre-init request
leaked an _in_flight entry for the life of the session -- a client
hammering an uninitialized session could grow it unboundedly. Added
an assertion pinning this (len(session._in_flight) == 0 after the
rejected request), so a future refactor can't silently reintroduce
the leak.
- Also assert on error.data ("session_not_initialized"), the
machine-readable discriminator this fix introduces -- message text
isn't meant to be programmatically parsed, this is.
Verified: tests/server/test_session.py (7/7) and the broader
tests/server/ + tests/shared/test_session.py suite (521 passed, same
pre-existing unrelated websockets-extra collection error as before)
both pass. ruff check / ruff format --check / pyright clean on both
changed files.
Author
|
Pushed a follow-up commit addressing review feedback:
Re-verified: |
handle_post_message's session-lookup failure returned a bare "Could not find session" 404, indistinguishable from any other cause and naming neither the reason nor the remedy. This is the more common sibling of the case fixed in 8058392 (self-describing error for a request before initialization): that fix answers through the ServerSession's responder once a session object exists, but a restarted process has no session object at all for an unknown session_id -- the rejection happens in handle_post_message itself, one layer below where a JSON-RPC error could be shaped. Every redeploy invalidates every previously-connected client's session_id at once, so this 404 is what most stranded clients actually hit, and it reads as a service outage rather than "restart your MCP client." Both 404 branches (unknown session_id, and the credential-mismatch branch that deliberately mimics "session doesn't exist" so it can't be used to probe for valid session IDs under another user) now share one message via a single Response built once, so they can't drift apart and leak which case occurred. Updated test_sse_security_post_valid_content_type, which asserted the old exact bare string -- it's testing content-type handling reaches the 404 branch at all, not the message's wording, so relaxed to a substring check.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
ServerSession._received_requestraises a bareRuntimeErrorfor any request received before the session completes itsinitializehandshake. That exception propagates toBaseSession._receive_loop's blanketexcept Exception as e:handler, which discardseentirely and always responds with the same hardcodedErrorData(code=INVALID_PARAMS, message="Invalid request parameters")-- indistinguishable from an actually malformed request.This surfaces most concretely with the SSE transport:
connect_sse()issues a brand-newsession_idand a fresh, uninitializedServerSessionon everyGET /sse. A client that reconnects (after an idle-timeout abort, a network blip, or a proxy restart) without resendinginitializeon the new stream gets-32602on every subsequent call, indefinitely -- which reads as "your parameters are wrong" when the real condition is "this session was never initialized." We hit this in production behind an MCP server built on this SDK; the-32602text cost real diagnostic time (initially misattributed to a different tool's parameter schema) before server-side logs (Received request before initialization was complete) revealed the actual cause.Fix
Answer the request directly via
responder.respond(ErrorData(...))-- a public API already used by theInitializeRequestbranch immediately above it -- instead of raising, so the generic catch-all inBaseSession._receive_loopnever sees it. UsesINVALID_REQUESTrather thanINVALID_PARAMS: the request's parameters aren't the problem, the session's initialization state is.Based on
v1.xperCONTRIBUTING.md's guidance for bug fixes to a released version -- this reproduces againstmcp==1.28.1(latest 1.x release);mainis the in-progress v2.0.0 rewrite and has moved past this code shape entirely, so I didn't attempt this fix there.Test plan
test_other_requests_blocked_before_initialization, which had assertederror_code == types.INVALID_PARAMS-- i.e. it encoded the bug as expected behavior. Now assertsINVALID_REQUESTand checks the message text.uv run pytest tests/server/ tests/shared/test_session.py: 521 passed (1 pre-existing, unrelated collection error:tests/server/test_websocket_security.pyneeds thewebsocketsextra, not installed by--all-extras --devin my environment; unrelated to this change).uv run ruff check/ruff format --check/uv run pyrighton both changed files: all clean.