Skip to content

Commit 1c760e9

Browse files
committed
address MCP server review feedback
- McpHttpServer: cap POST body read via ReadBlockAsync so chunked requests can't bypass the Content-Length limit and exhaust memory - McpHttpServer: capture the listener into a local in the accept loop to avoid an NRE race with a concurrent Dispose() - McpHttpServer: document the deliberate stateless single-session design (unconditional, unvalidated Mcp-Session-Id) - McpToolsRegistry: hand out a DeepClone of the tools/list payload so concurrent dispatches don't race on the shared JObject - McpJsonRpcDispatcher: return -32600 (Invalid Request) for well-formed JSON that isn't an object, keeping -32700 for unparseable input - McpJsonRpcDispatcher: log a warning when the client's initialize protocolVersion differs from the server's - McpServerPlugin: store the log entry bus in a field and unsubscribe in Dispose()
1 parent 0a2861f commit 1c760e9

3 files changed

Lines changed: 44 additions & 9 deletions

File tree

Explorer/Assets/DCL/McpServer/Core/McpHttpServer.cs

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,13 @@ public class McpHttpServer : IDisposable
2424

2525
private readonly McpJsonRpcDispatcher dispatcher;
2626
private readonly int port;
27+
28+
// Stateless single-session design: this localhost, single-user automation server exposes one logical MCP
29+
// session for the whole process lifetime. It therefore uses one id for its entire life and echoes it on
30+
// every response (including errors and before initialize) instead of minting one per handshake, and it does
31+
// not validate the incoming Mcp-Session-Id — there is no per-session state to bind a request to. This
32+
// deliberately departs from the Streamable HTTP session lifecycle; revisit if the server ever serves
33+
// multiple concurrent clients.
2734
private readonly string sessionId = Guid.NewGuid().ToString("N");
2835

2936
private HttpListener? listener;
@@ -73,11 +80,16 @@ public async UniTaskVoid RunAsync(CancellationToken ct)
7380
{
7481
await DCLTask.SwitchToThreadPool();
7582

76-
while (!ct.IsCancellationRequested && listener is { IsListening: true })
83+
// Capture once: a concurrent Dispose() on the main thread nulls the field, and re-reading it between
84+
// the guard and GetContextAsync would throw an unfiltered NRE. Dispose still stops/closes this same
85+
// instance, so a parked GetContextAsync surfaces as one of the caught exceptions below.
86+
HttpListener? local = listener;
87+
88+
while (!ct.IsCancellationRequested && local is { IsListening: true })
7789
{
7890
HttpListenerContext context;
7991

80-
try { context = await listener.GetContextAsync(); }
92+
try { context = await local.GetContextAsync(); }
8193
catch (Exception e) when (e is HttpListenerException or ObjectDisposedException or InvalidOperationException)
8294
{
8395
// The listener was stopped or disposed; end the accept loop.

Explorer/Assets/DCL/McpServer/Core/McpJsonRpcDispatcher.cs

Lines changed: 23 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ public McpJsonRpcDispatcher(McpToolsRegistry tools, string serverVersion)
4949

5050
return routable.Value.method switch
5151
{
52-
"initialize" => JsonRpcEnvelope.Result(id, InitializeResult()),
52+
"initialize" => JsonRpcEnvelope.Result(id, InitializeResult(routable.Value.callParams)),
5353
"ping" => JsonRpcEnvelope.Result(id, new JObject()),
5454
"tools/list" => JsonRpcEnvelope.Result(id, tools),
5555
"tools/call" => await CallToolAsync(id,
@@ -69,14 +69,22 @@ private static (JToken id, string method, JObject? callParams)? ParseRoutableReq
6969
{
7070
earlyResponse = null;
7171

72-
JObject request;
73-
try { request = JObject.Parse(requestJson); }
72+
JToken parsed;
73+
try { parsed = JToken.Parse(requestJson); }
7474
catch (JsonException)
7575
{
7676
earlyResponse = JsonRpcEnvelope.Error(null, PARSE_ERROR, "Parse error");
7777
return null;
7878
}
7979

80+
// -32700 is reserved for unparseable JSON; well-formed JSON that is not a JSON-RPC object
81+
// (a bare array, number or string) is a valid document but an invalid request: -32600.
82+
if (parsed is not JObject request)
83+
{
84+
earlyResponse = JsonRpcEnvelope.Error(null, INVALID_REQUEST, "Invalid request: expected a JSON-RPC object");
85+
return null;
86+
}
87+
8088
JToken? id = request["id"];
8189
string? method = request["method"]?.Value<string>();
8290

@@ -93,8 +101,17 @@ private static (JToken id, string method, JObject? callParams)? ParseRoutableReq
93101
return (id, method, request["params"] as JObject);
94102
}
95103

96-
private JObject InitializeResult() =>
97-
new ()
104+
private JObject InitializeResult(JObject? initializeParams)
105+
{
106+
// We implement a single MCP revision. Per the handshake rules we answer with our version regardless,
107+
// but a client pinned to a different revision may abort on a strict mismatch — surface it so the
108+
// otherwise-silent interop failure is diagnosable from the server logs.
109+
string? requestedVersion = initializeParams?["protocolVersion"]?.Value<string>();
110+
111+
if (!string.IsNullOrEmpty(requestedVersion) && requestedVersion != PROTOCOL_VERSION)
112+
ReportHub.LogWarning(ReportCategory.MCP, $"MCP client requested protocol version '{requestedVersion}', server responding with '{PROTOCOL_VERSION}'");
113+
114+
return new JObject
98115
{
99116
["protocolVersion"] = PROTOCOL_VERSION,
100117
["capabilities"] = new JObject { ["tools"] = new JObject() },
@@ -105,6 +122,7 @@ private JObject InitializeResult() =>
105122
["pid"] = processId,
106123
},
107124
};
125+
}
108126

109127
private async UniTask<string?> CallToolAsync(JToken id, string? toolName, JObject arguments, CancellationToken ct)
110128
{

Explorer/Assets/DCL/McpServer/Core/McpToolsRegistry.cs

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,9 +10,14 @@ public class McpToolsRegistry
1010
private readonly Dictionary<string, IMcpTool> tools = new ();
1111
private JObject toolsList = null!;
1212

13-
/// <summary>Lets the built registry stand in directly for its tools/list payload.</summary>
13+
/// <summary>
14+
/// Lets the built registry stand in directly for its tools/list payload. Returns a detached clone:
15+
/// dispatched requests run concurrently on the thread pool, and attaching the shared instance to a
16+
/// response envelope re-parents it in place, so handing out the same object would race on its parent/
17+
/// sibling pointers during serialization.
18+
/// </summary>
1419
public static implicit operator JObject(McpToolsRegistry registry) =>
15-
registry.toolsList;
20+
(JObject)registry.toolsList.DeepClone();
1621

1722
public McpToolsRegistry Add(IMcpTool tool)
1823
{

0 commit comments

Comments
 (0)