Cover dispatcher-thread commit inside McpClient::initializeProtocol#217
Merged
Merged
Conversation
The worker thread that blocks on the initialize response parses it, then posts the actual mutation of client state -- server_capabilities_, initialized_, and the protocol state machine's INITIALIZED transition -- back to the main dispatcher, and only resolves the externally-visible std::future<InitializeResult> from inside that post. Writing those fields off the dispatcher would be a data race with dispatcher readers. Drive the full path end-to-end against a real McpServer: pick a free loopback port, stand the server up on a background thread, connect a real McpClient at http://127.0.0.1:<port>/rpc, run initializeProtocol().get(), and fire a follow-up ping on the same connection. Observable asserts: - connect() succeeds against a listening server - the initialize future resolves (no deadlock / lost post) - protocolVersion is populated, proving the dispatcher-thread commit post executed before the future was released - a subsequent ping round-trips, proving the protocol state machine actually advanced into INITIALIZED Finer-grained capability / serverInfo asserts are deliberately omitted: the current client parser only recognizes flat dotted-key metadata while the server emits nested JSON objects. Bridging that deserialization gap is out of scope here -- this test covers the dispatcher-routing contract, not the response-parsing schema. Teardown leaks the McpServer on purpose. McpServer::shutdown() flips server_running_ = false on the caller thread before posting cleanup, and the shutdown post itself never drains active_connections_, so connections outlive the dispatcher and get destroyed on the test thread inside ~McpServer, which trips the dispatcher-thread assert inside onConnectionLifecycleEvent. That bug belongs in the server-lifecycle test's scope; here we just need the client-side contract verified and a clean exit, and the process is about to die anyway.
caleb2h
approved these changes
Apr 20, 2026
gophergogo
pushed a commit
that referenced
this pull request
Apr 20, 2026
…217) The worker thread that blocks on the initialize response parses it, then posts the actual mutation of client state -- server_capabilities_, initialized_, and the protocol state machine's INITIALIZED transition -- back to the main dispatcher, and only resolves the externally-visible std::future<InitializeResult> from inside that post. Writing those fields off the dispatcher would be a data race with dispatcher readers. Drive the full path end-to-end against a real McpServer: pick a free loopback port, stand the server up on a background thread, connect a real McpClient at http://127.0.0.1:<port>/rpc, run initializeProtocol().get(), and fire a follow-up ping on the same connection. Observable asserts: - connect() succeeds against a listening server - the initialize future resolves (no deadlock / lost post) - protocolVersion is populated, proving the dispatcher-thread commit post executed before the future was released - a subsequent ping round-trips, proving the protocol state machine actually advanced into INITIALIZED Finer-grained capability / serverInfo asserts are deliberately omitted: the current client parser only recognizes flat dotted-key metadata while the server emits nested JSON objects. Bridging that deserialization gap is out of scope here -- this test covers the dispatcher-routing contract, not the response-parsing schema. Teardown leaks the McpServer on purpose. McpServer::shutdown() flips server_running_ = false on the caller thread before posting cleanup, and the shutdown post itself never drains active_connections_, so connections outlive the dispatcher and get destroyed on the test thread inside ~McpServer, which trips the dispatcher-thread assert inside onConnectionLifecycleEvent. That bug belongs in the server-lifecycle test's scope; here we just need the client-side contract verified and a clean exit, and the process is about to die anyway.
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
2 participants
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
End-to-end integration test for the contract that worker-parsed
initializeresults are committed toMcpClientstate on the main dispatcher, not on the worker thread.McpServeron a loopback ephemeral port and drives a realMcpClientthroughconnect → initializeProtocol → ping.protocolVersioncomes back populated (observable proof that the dispatcher-thread commit post ran before the future was released).pinground-trips, proving the protocol state machine actually advanced intoINITIALIZED.Finer-grained capability /
serverInfofield asserts are intentionally skipped: the current client parser only recognizes flat dotted-key metadata while the server emits nested JSON objects. That deserialization gap is out of scope for this test.Known pre-existing teardown bug (flagged, not fixed here)
McpServer::shutdown()flipsserver_running_ = falseon the caller thread before posting any cleanup, and the shutdown post itself never drainsactive_connections_. Connections therefore outlive the dispatcher and get destroyed on the test thread inside~McpServer, whoseConnectionImpl::~ConnectionImpl() → closeSocket(LocalClose)forwards through the lifecycle adapter intoMcpServer::onConnectionLifecycleEvent, which asserts it is on the dispatcher thread (mcp_server.cc:804).That fix belongs in the server-lifecycle test's scope (tracked under the next PR). Here the teardown deliberately
release()s the server to keep this test focused on the client-side routing contract. The test process is about to exit; the heap is reclaimed by the OS; the dispatcher thread has already joined so nothing else touches the object.Test plan
cmake --build . --target test_mcp_client_initialize_routing./tests/test_mcp_client_initialize_routing— passes