Skip to content

fix: self-describing error for requests before session initialization (was generic -32602)#3128

Open
MrSampson wants to merge 2 commits into
modelcontextprotocol:v1.xfrom
MrSampson:upstream-session-not-initialized-error-v1x
Open

fix: self-describing error for requests before session initialization (was generic -32602)#3128
MrSampson wants to merge 2 commits into
modelcontextprotocol:v1.xfrom
MrSampson:upstream-session-not-initialized-error-v1x

Conversation

@MrSampson

Copy link
Copy Markdown

Summary

ServerSession._received_request raises a bare RuntimeError for any request received before the session completes its initialize handshake. That exception propagates to BaseSession._receive_loop's blanket except Exception as e: handler, which discards e 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 behind an MCP server built on this SDK; the -32602 text 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 the InitializeRequest branch immediately above it -- instead of raising, so the generic catch-all in BaseSession._receive_loop never sees it. Uses INVALID_REQUEST rather than INVALID_PARAMS: the request's parameters aren't the problem, the session's initialization state is.

Based on v1.x per CONTRIBUTING.md's guidance for bug fixes to a released version -- this reproduces against mcp==1.28.1 (latest 1.x release); main is 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

  • Updated the existing test_other_requests_blocked_before_initialization, which had asserted error_code == types.INVALID_PARAMS -- i.e. it encoded the bug as expected behavior. Now asserts INVALID_REQUEST and 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.py needs the websockets extra, not installed by --all-extras --dev in my environment; unrelated to this change).
  • uv run ruff check / ruff format --check / uv run pyright on both changed files: all clean.

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).

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 2 files

Re-trigger cubic

… 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.
@MrSampson

Copy link
Copy Markdown
Author

Pushed a follow-up commit addressing review feedback:

  • Reworded the error message: it previously 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 isn't true in that case. Now states 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.
  • Found and pinned down an incidental fix worth calling out explicitly: answering via responder.respond() (rather than raising, as before) 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 so a future refactor can't silently reintroduce it.
  • Also assert on error.data ("session_not_initialized"), the machine-readable discriminator this fix introduces.

Re-verified: tests/server/test_session.py (7/7), the broader suite (521 passed, same pre-existing unrelated websockets-extra collection error as before), and ruff/pyright all clean.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant