fix: self-describing error for requests before session initialization (was generic -32602)#3128
Open
MrSampson wants to merge 2 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: |
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.