modelcontextprotocol / modelcontextprotocol/csharp-sdk

ProcessMessagesAsync's internal fire-and-forget task duplicates Completion's failure signal via TaskScheduler.UnobservedTaskException

Open
#1,851 2 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
C#
Stars
4.5k
Forks
814
Avg merge
9d 19h
Merged PRs (30d)
4

Description

Description

McpClientImpl.ConnectAsync starts the client's message-processing loop as a discarded task:

// src/ModelContextProtocol.Core/Client/McpClientImpl.cs:285
_ = _sessionHandler.ProcessMessagesAsync(CancellationToken.None);

That task runs McpSessionHandler.ProcessMessagesCoreAsync, which reads from _transport.MessageReader.ReadAllAsync(...) and only catches OperationCanceledException:

// src/ModelContextProtocol.Core/McpSessionHandler.cs:352
catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested)
{
    // Normal shutdown
    LogEndpointMessageProcessingCanceled(EndpointName);
}

When the transport disconnects unexpectedly (e.g. the server 401s or otherwise closes the message channel mid-session), the transport already records this correctly — SetDisconnected(exception) completes the channel, and that same exception is exposed to callers through the documented, opt-in public API:

// McpClient.cs:64-75
/// For unexpected closure (e.g., process crash, network failure), it may contain
/// an exception that caused or that represents the failure.
public abstract Task<ClientCompletionDetails> Completion { get; }

ReadAllAsync then rethrows that same exception when it observes the completed-with-fault channel, and because ProcessMessagesCoreAsync's only catch clause is for cancellation, it propagates out of the discarded task. Since that task is never awaited or observed (_ =, CancellationToken.None), it becomes eligible for TaskScheduler.UnobservedTaskException, firing later on the GC finalizer thread with no request/caller context.

Why this is a duplication, not a missing feature

Completion already gives callers full control over how to react to this exact failure — await it, log it, retry, ignore it. That's the right design for something as opinionated as "what should happen when the transport dies." The problem is that the internal read loop independently re-surfaces the same failure through an unrelated, unobservable channel (TaskScheduler.UnobservedTaskException), which no caller asked for and can't configure. It's not that the SDK is missing a way to hand this back to the caller — it already has one — it's that this internal implementation detail leaks the same information a second time, through the worst possible channel (fires late, on the finalizer thread, disconnected from ConnectAsync's call site).

Impact

Consumers who wire up a global TaskScheduler.UnobservedTaskException handler (a common .NET practice for exactly this class of leak) get a contextless, delayed error report for a condition their code may already be aware of and had a designed way to observe via Completion. It reads as an SDK-internal unhandled bug rather than the already-surfaced session-completion condition it actually is.

Suggested fix

ProcessMessagesCoreAsync's read loop doesn't need to re-throw on transport disconnect — the transport already recorded why it disconnected, and that's retrievable via Completion. The loop exiting for that reason isn't a new failure to report, just a symptom of one already exposed elsewhere. A minimal fix: catch and swallow non-cancellation exceptions from the await foreach at the point the loop exits, since Completion (backed by the same SetDisconnected/channel-fault state) remains the sanctioned way for a caller to learn about and react to the disconnect:

catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested)
{
    // Normal shutdown
}
catch (Exception)
{
    // Already recorded via SetDisconnected and exposed through Completion;
    // the loop exiting is a symptom, not a new failure to surface here.
}

Happy to send a PR if this framing looks right — wanted to open the issue first per CONTRIBUTING.md and get agreement on the fix's direction before writing code, since it touches how the SDK's failure-reporting is supposed to work.

Version

ModelContextProtocol.Core 2.0.0

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in src/ModelContextProtocol.Core/Client/McpClientImpl.cs around ConnectAsync and follow the discarded ProcessMessagesAsync task into src/ModelContextProtocol.Core/McpSessionHandler.cs and its ReadAllAsync loop. Check the existing SetDisconnected and McpClient.Completion paths, then verify that transport failures remain available through Completion without producing a later TaskScheduler.UnobservedTaskException.

Written by the indexing model from the issue text.

Assessment

Tech stack
csharp
Domain
api
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
70/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.