Skip to content

Commit f7c8353

Browse files
author
oliver
committed
fix: don't overclaim cause in error message, test the _in_flight fix, 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.
1 parent 8058392 commit f7c8353

2 files changed

Lines changed: 33 additions & 7 deletions

File tree

src/mcp/server/session.py

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -211,16 +211,27 @@ async def _received_request(self, responder: RequestResponder[types.ClientReques
211211
# parameters") -- indistinguishable from an actually
212212
# malformed request. INVALID_REQUEST (not INVALID_PARAMS)
213213
# is used because the request's parameters aren't the
214-
# problem; the session's state is.
214+
# problem; the session's state is -- matching the existing
215+
# "Session not found" usage in streamable_http_manager.py
216+
# for the same class of session-validity condition.
217+
#
218+
# The message states the fact (an initialize handshake is
219+
# required) without asserting *why* this particular
220+
# session never completed one -- this branch can't tell a
221+
# genuinely uninitialized session (e.g. a client bug) from
222+
# a stream that reconnected without reinitializing, so it
223+
# doesn't claim either.
215224
with responder:
216225
await responder.respond(
217226
types.ErrorData(
218227
code=types.INVALID_REQUEST,
219228
message=(
220-
"MCP session not initialized: this session_id's "
221-
"stream was reconnected or lost without a fresh "
222-
"'initialize' handshake. Reconnect and initialize "
223-
"a new session."
229+
"MCP session not initialized: an 'initialize' "
230+
"request must complete successfully before "
231+
"other requests are handled. If this session "
232+
"was previously initialized, its stream may "
233+
"have been reconnected without a fresh "
234+
"handshake."
224235
),
225236
data="session_not_initialized",
226237
)

tests/server/test_session.py

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -473,6 +473,8 @@ async def test_other_requests_blocked_before_initialization():
473473
error_response_received = False
474474
error_code = None
475475
error_message_text = None
476+
error_data = None
477+
session_ref: list[ServerSession] = []
476478

477479
async def run_server():
478480
async with ServerSession(
@@ -483,13 +485,14 @@ async def run_server():
483485
server_version="0.1.0",
484486
capabilities=ServerCapabilities(),
485487
),
486-
):
488+
) as session:
489+
session_ref.append(session)
487490
# Server should handle the request and send an error response
488491
# No need to process incoming_messages since the error is handled automatically
489492
await anyio.sleep(0.1) # Give time for the request to be processed
490493

491494
async def mock_client():
492-
nonlocal error_response_received, error_code, error_message_text
495+
nonlocal error_response_received, error_code, error_message_text, error_data
493496

494497
# Try to send a non-ping request before initialization
495498
await client_to_server_send.send(
@@ -510,6 +513,7 @@ async def mock_client():
510513
error_response_received = True
511514
error_code = error_message.message.root.error.code
512515
error_message_text = error_message.message.root.error.message
516+
error_data = error_message.message.root.error.data
513517

514518
async with (
515519
client_to_server_send,
@@ -530,3 +534,14 @@ async def mock_client():
530534
# from a genuinely malformed request.
531535
assert error_code == types.INVALID_REQUEST
532536
assert error_message_text is not None and "session not initialized" in error_message_text.lower()
537+
# data is the machine-readable discriminator -- message text isn't meant
538+
# to be programmatically parsed, this is.
539+
assert error_data == "session_not_initialized"
540+
# Answering via responder.respond() (rather than raising, as before this
541+
# fix) means RequestResponder.__exit__ sees _completed=True and fires
542+
# on_complete, which pops the request out of _in_flight. Before this fix,
543+
# the bare RuntimeError bypassed the responder's context-manager cleanup
544+
# entirely, so every rejected pre-init request leaked an _in_flight
545+
# entry for the life of the session.
546+
assert session_ref and len(session_ref[0]._in_flight) == 0
547+
assert error_message_text is not None and "session not initialized" in error_message_text.lower()

0 commit comments

Comments
 (0)