Skip to content

Commit ddd79f0

Browse files
committed
Address review feedback on middleware reshape
- OpenTelemetryMiddleware: nest under the ambient span when no traceparent is present. Passing an explicit empty Context to start_as_current_span orphans the span; extract_trace_context now returns None for an absent or malformed carrier so callers fall through to ambient parenting. Adds a regression test with both span tiers installed. - Rename 'mw' -> 'middleware' across runner.py; drop redundant CallNext annotation on the compose-loop accumulator. - Document that 'initialize' is observed by ServerMiddleware but not rewritable: the post-chain handshake commit reads the wire params, so a rewritten ctx.params on initialize does not reach connection state. TODO at the commit site names the desync triggers; resolves when initialize becomes a built-in handler.
1 parent 2522d4c commit ddd79f0

5 files changed

Lines changed: 58 additions & 11 deletions

File tree

src/mcp/server/_otel.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ async def __call__(self, ctx: ServerRequestContext[Any, Any], call_next: CallNex
3232
name=f"MCP handle {ctx.method}{f' {target}' if target else ''}",
3333
kind=SpanKind.SERVER,
3434
attributes=attributes,
35-
context=extract_trace_context(ctx.meta or {}),
35+
context=extract_trace_context(ctx.meta),
3636
record_exception=False,
3737
set_status_on_exception=False,
3838
) as span:

src/mcp/server/context.py

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -150,6 +150,9 @@ class ServerMiddleware(Protocol[_MwLifespanT]):
150150
server-to-client request (`ctx.session.send_request`, `send_ping`, ...)
151151
while handling `initialize` therefore deadlocks the connection: the
152152
response can never be dequeued. Send-and-forget notifications are safe.
153+
`initialize` is observed but not rewritable: the post-chain handshake
154+
commit reads the wire params, so to veto the handshake raise *before*
155+
`call_next()`.
153156
154157
`Server[L].middleware` holds `ServerMiddleware[L]`, so an app-specific
155158
middleware sees `ctx.lifespan_context: L`. While the context is the

src/mcp/server/runner.py

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -201,11 +201,11 @@ async def to_jsonrpc_response(request_id: RequestId, coro: Awaitable[dict[str, A
201201

202202

203203
def _apply_middleware(
204-
mw: ServerMiddleware[Any], call_next: CallNext, ctx: ServerRequestContext[Any, Any]
204+
middleware: ServerMiddleware[Any], call_next: CallNext, ctx: ServerRequestContext[Any, Any]
205205
) -> Awaitable[HandlerResult]:
206206
"""Adapt one middleware to the `CallNext` shape: bind `call_next`, take
207207
`ctx` at call time so a rewritten context flows down the chain."""
208-
return mw(ctx, call_next)
208+
return middleware(ctx, call_next)
209209

210210

211211
@dataclass
@@ -228,7 +228,9 @@ def on_request(self) -> OnRequest:
228228
wraps everything - initialize, METHOD_NOT_FOUND, validation failures
229229
included.
230230
"""
231-
return reduce(lambda h, mw: mw(h), reversed(self.dispatch_middleware), self._on_request)
231+
return reduce(
232+
lambda handler, middleware: middleware(handler), reversed(self.dispatch_middleware), self._on_request
233+
)
232234

233235
@cached_property
234236
def on_notify(self) -> OnNotify:
@@ -303,6 +305,11 @@ async def _inner(ctx: ServerRequestContext[LifespanT, Any]) -> HandlerResult:
303305
if method == "initialize":
304306
# Commit only on chain success, so a middleware veto leaves no state.
305307
# Race-free: the read loop is parked until this call returns.
308+
# TODO: this re-reads the wire `params`, so a middleware that rewrote
309+
# `ctx.params` (or `ctx.method`, or short-circuited without `call_next`)
310+
# can leave `connection.protocol_version` out of step with the
311+
# `InitializeResult` `_inner` produced. Resolve when `initialize` becomes
312+
# a built-in handler so commit and result derive from one negotiation.
306313
self.connection.client_params, self.connection.protocol_version = self._negotiate_initialize(params)
307314
return result
308315

@@ -362,9 +369,9 @@ def _compose_server_middleware(self, inner: CallNext) -> CallNext:
362369
observes every inbound message. The composed callable takes the `ctx`
363370
at call time, so a middleware can rewrite it for the rest of the chain.
364371
"""
365-
call: CallNext = inner
366-
for mw in reversed(self.server.middleware):
367-
call = partial(_apply_middleware, mw, call)
372+
call = inner
373+
for middleware in reversed(self.server.middleware):
374+
call = partial(_apply_middleware, middleware, call)
368375
return call
369376

370377
def _make_context(

src/mcp/shared/_otel.py

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -41,10 +41,16 @@ def inject_trace_context(meta: dict[str, Any]) -> None:
4141
inject(meta)
4242

4343

44-
def extract_trace_context(meta: Mapping[str, Any]) -> Context:
45-
"""Extract W3C trace context from a `_meta` dict."""
44+
def extract_trace_context(meta: Mapping[str, Any] | None) -> Context | None:
45+
"""Extract W3C trace context from a `_meta` dict.
46+
47+
Returns `None` when the carrier is absent or malformed so callers fall
48+
through to ambient parenting; an explicit empty `Context` would orphan
49+
the span instead of nesting under the current one.
50+
"""
51+
if not meta:
52+
return None
4653
try:
4754
return extract(meta)
4855
except (ValueError, TypeError):
49-
# If the traceparent is malformed, degrade to no parent rather than failing the request.
50-
return Context()
56+
return None

tests/server/test_otel.py

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
from mcp.server._otel import OpenTelemetryMiddleware
1111
from mcp.server.context import CallNext
1212
from mcp.server.lowlevel.server import Server
13+
from mcp.server.runner import otel_middleware
1314
from mcp.shared._otel import inject_trace_context
1415
from mcp.shared.exceptions import MCPError
1516
from mcp.types import CallToolRequestParams, ListToolsResult, NotificationParams, PaginatedRequestParams, Tool
@@ -64,6 +65,36 @@ async def on_roots(ctx: Ctx, params: NotificationParams | None) -> None:
6465
assert "jsonrpc.request.id" not in span.attributes
6566

6667

68+
@pytest.mark.anyio
69+
async def test_nests_under_ambient_span_when_no_traceparent(server: SrvT, spans: SpanCapture):
70+
"""With no `_meta` on the inbound message (a non-SDK client), the
71+
context-tier span must parent to the ambient current span (here, the
72+
dispatch-tier `otel_middleware` span) rather than become an orphan root.
73+
SDK-defined: SEP-414 only covers the traceparent-present case."""
74+
75+
def strip_meta(call_next: Any) -> Any:
76+
# The in-process client always injects `_meta.traceparent`; strip it so
77+
# both server tiers see the no-carrier path.
78+
async def wrapped(dctx: Any, method: str, params: dict[str, Any] | None) -> Any:
79+
stripped = {k: v for k, v in (params or {}).items() if k != "_meta"}
80+
return await call_next(dctx, method, stripped or None)
81+
82+
return wrapped
83+
84+
server.middleware.append(OpenTelemetryMiddleware())
85+
async with connected_runner(server, dispatch_middleware=[strip_meta, otel_middleware]) as (client, _):
86+
spans.clear()
87+
await client.send_raw_request("tools/list", None)
88+
server_spans = [s for s in spans.finished() if s.kind == SpanKind.SERVER]
89+
assert len(server_spans) == 2
90+
[outer] = [s for s in server_spans if s.parent is None]
91+
[inner] = [s for s in server_spans if s.parent is not None]
92+
assert inner.context is not None and outer.context is not None
93+
assert inner.parent is not None
94+
assert inner.context.trace_id == outer.context.trace_id
95+
assert inner.parent.span_id == outer.context.span_id
96+
97+
6798
@pytest.mark.anyio
6899
async def test_extracts_trace_context_from_meta(server: SrvT, spans: SpanCapture):
69100
meta: dict[str, Any] = {}

0 commit comments

Comments
 (0)