Add ASP.NET Core per-user MCP client sample - #1788
Conversation
b990e79 to
6a69512
Compare
|
Thanks, @luisangelrod! It will probably take a week or so for us to review this adequately. Stay tuned though! |
|
Thanks, Jeff. I appreciate the update. I will stay available for feedback and will be happy to make any requested revisions. |
Disposing SessionClientRegistry waits for each session's in-flight operation to complete. A tool call blocked in ElicitationBroker.RequestAsync never completes until a frontend posts a response, so an unanswered elicitation would stall application shutdown until the host's shutdown timeout forced the process down. - Add ElicitationBroker.CancelAll() to release every pending elicitation. - Register an ApplicationStopping callback that cancels pending elicitations before the singleton registry is disposed. - Collect progress notifications in a ConcurrentQueue, since they are reported from the MCP session's message loop while the request thread serializes the response. - Document the disposal behavior and both cancellation paths in the README. - Cover CancelAll() with a test across multiple sessions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
jeffhandley
left a comment
There was a problem hiding this comment.
Thanks for putting this together, @luisangelrod — this is a genuinely useful sample. The separation of the application session from the MCP protocol session is the right framing, and SessionClientRegistry<TClient> handles the tricky parts carefully: the removal races, cleaning up when the client factory throws, and deterministic async disposal all check out. The "Production considerations" section is a good touch, especially calling out that a distributed cache can't hold a live connection.
I pushed one commit for preventing shutdown hangs on unanswered elicitations.
Because disposing the registry waits for each session's in-flight operation, a tool call blocked in ElicitationBroker.RequestAsync never completes until a frontend posts a response — so an unanswered elicitation would stall application shutdown until the host's shutdown timeout forced the process down. DELETE /session already avoided this by cancelling the session's pending elicitation first, but nothing covered the shutdown path. The commit adds ElicitationBroker.CancelAll() and wires it to ApplicationStopping, so pending elicitations are released before the singleton registry is disposed.
It also collects progress notifications in a ConcurrentQueue instead of a List, since those are reported from the MCP session's message loop while the request thread serializes the response, plus a test for CancelAll() and a README note on the disposal behavior.
Approving — feel free to push back on any of it if you'd rather handle it differently.
This review comment was drafted with AI assistance (GitHub Copilot).
|
Thanks @jeffhandley! I reviewed the changes, and they look great to me. I really appreciate you catching that issue, improving the sample, and taking the time to review my contribution. |
Summary
Why
ASP.NET Core applications need an application-lifetime owner for stateful MCP clients. The SDK can correlate concurrent protocol messages, but the application still needs to decide who owns each live connection, how frontend interaction reaches an elicitation handler, and when the connection is disposed. This sample makes those responsibilities explicit without presenting the in-memory registry as a distributed production store.
Validation
dotnet test tests/AspNetCoreMcpClient.Tests/AspNetCoreMcpClient.Tests.csproj --no-restoredotnet format tests/AspNetCoreMcpClient.Tests/AspNetCoreMcpClient.Tests.csproj --verify-no-changes --no-restoregit diff --checkCloses #1126