Skip to content

Conversation

@eiriktsarpalis
Copy link
Member

@eiriktsarpalis eiriktsarpalis commented Jan 20, 2026

Implements MCP task support. See the included tasks.md for a high-level introduction to the new APIs.

Fix #943.

/// The server handles all polling logic internally.
/// </remarks>
[Experimental(Experimentals.Tasks_DiagnosticId, UrlFormat = Experimentals.Tasks_Url)]
public ValueTask<JsonElement> GetTaskResultAsync(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that the GetTaskResultAsync method returns a raw JsonElement instead of, say, a CallToolResult. This is because a task could correspond to requests beyond tool calls. The typescript SDK returns Result in this case, however today we have no implementation of polymorphic deserialization in Result.

/// </remarks>
[DebuggerDisplay("{DebuggerDisplay,nq}")]
[Experimental(Experimentals.Tasks_DiagnosticId, UrlFormat = Experimentals.Tasks_Url)]
public sealed class McpTask
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that unlike other SDKs we adopt the "McpTask" naming convention to distinguish this feature from regular TPL tasks.

/// </para>
/// </remarks>
[Experimental(Experimentals.Tasks_DiagnosticId, UrlFormat = Experimentals.Tasks_Url)]
public async ValueTask<(McpTask Task, JsonElement Result)> WaitForTaskResultAsync(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note the peculiar return signature on this method. This is intentionally copying the signature used by the TS sdk. Alternatives include returning just the JsonElement or using a named envelope type containing both the task and its result. I think this is the simpler solution also due to the similarity with existing SDKs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did we choose a tuple over a named type only because it's more similar to the TS SDK? If that's the only reason, I'd just as soon create a named McpTaskResult type or something.

It also seems a bit unusual to want a McpTask and result together anyway after you've already got the result. And if that's something you really care about, you can call PollTaskUntilCompleteAsync and GetTaskResultAsync yourself without much effort.

My vote would be first just to remove this method, but if we have to keep it, we should name the return type.

// If neither is done, resolving IMcpTaskStore will throw.
services.TryAddSingleton<IMcpTaskStore>(sp =>
{
var options = sp.GetRequiredService<IOptions<McpServerOptions>>().Value;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like some feedback from @halter73 on this one. This is automatically registering the IMcpTaskStore if one has been configured in the server options. We could of course skip this and have users rely on just the server options being available.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The normal way to do this would be add a IMcpTaskStore? taskStore = null parameter to McpServerOptionsSetup and then set options.TaskStore = taskStore in the Configure method.

As far as I can tell, this is the only line of code that currently resolves IMcpTaskStore from the service provider, so another option would be to just undo the AddMcpServer changes and leave it to the app developer to resolve their IMcpTaskStore and set McpServerOptions.TaskStore themselves. This is what @MackinnonBuck did in #1077 for the ISseEventStreamStore.

However, I do like the convenience of just being able to add a singleton to DI and have it light up, so I think we should probably also add an HttpServerTransportOptionsSetup that initializes the EventStreamStore from DI.

/// </para>
/// </remarks>
[Experimental(Experimentals.Tasks_DiagnosticId, UrlFormat = Experimentals.Tasks_Url)]
public async ValueTask<(McpTask Task, JsonElement Result)> WaitForTaskResultAsync(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did we choose a tuple over a named type only because it's more similar to the TS SDK? If that's the only reason, I'd just as soon create a named McpTaskResult type or something.

It also seems a bit unusual to want a McpTask and result together anyway after you've already got the result. And if that's something you really care about, you can call PollTaskUntilCompleteAsync and GetTaskResultAsync yourself without much effort.

My vote would be first just to remove this method, but if we have to keep it, we should name the return type.

Task support levels:
- `Forbidden` (default for sync methods): Tool cannot be called with task augmentation
- `Optional` (default for async methods): Tool can be called with or without task augmentation
- `Required`: Tool must be called with task augmentation
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to add support to configure this via attributes? Can we add an experimental property to [McpServerTool]?

Comment on lines +219 to +222
if (sessionId != null && entry.SessionId != sessionId)
{
throw new InvalidOperationException($"Task not found: {taskId}");
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only the developer would see this, right? Might as well make the error more useful. Also, if an entry was stored with a session ID, we should probably require you retrieve it with the same session ID meaning we should adjust the if condition. I assume we'd never pass in a null session Id when sessions are enabled in practice, but there's no reason not to make the condition more strict.

Suggested change
if (sessionId != null && entry.SessionId != sessionId)
{
throw new InvalidOperationException($"Task not found: {taskId}");
}
if (sessionId != entry.SessionId)
{
throw new InvalidOperationException($"Invalid sessionId: {sessionId} provided for {taskId}");
}

The new exception message still doesn't reveal the expected session ID. It does reveal that the taskId was valid for some other session, but I think that's fine if it's pointed at the developer since it is most likely caused by some sort of logic bug in the IMcpTaskStore consumer in the rare event this exception gets thrown.

I think we should make the same change for StoreTaskResultAsync, UpdateTaskStatusAsync, CancelTaskAsync, etc.


// Stream enumeration - filter by session, exclude expired, apply keyset pagination
var query = _tasks.Values
.Where(e => sessionId == null || e.SessionId == sessionId)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.Where(e => sessionId == null || e.SessionId == sessionId)
.Where(e => sessionId == e.SessionId)

TimeSpan? maxTtl = null,
TimeSpan? pollInterval = null,
TimeSpan? cleanupInterval = null,
int pageSize = 100)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that this is public. Do we want to add the ability to configure a max number of Task stored globally and/or per-session?

using System.Diagnostics.CodeAnalysis;
using System.Text.Json;

namespace ModelContextProtocol.Server;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
namespace ModelContextProtocol.Server;
namespace ModelContextProtocol;

Nit: We should probably move the folder and InMemoryMcpTaskStore too since it's supported on the client.

}

// Retrieve the stored result - already stored as JsonElement
return await taskStore.GetTaskResultAsync(taskId, SessionId, cancellationToken).ConfigureAwait(false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the IMcpTaskStore offer a version of this method that asynchronously blocks until a terminal result is ready? We could have a default interface implementation that polls like this getTaskResultHandler does or switch to an abstract class with a default implementation.

It could save a lot of unnecessary polling in the in-memory case and could even be useful in distributed settings where you have some sort of subscription or push notification.

};

// Register handlers
SetHandler(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do we think about adding McpServerHandlers and/or McpServerFilters for "tasks/get", "tasks/result", "tasks/list" and "tasks/cancel"? It feels like it would be appropriate to add both for all of these.

I can kinda see the argument that the "IMcpTaskStore" is low level enough you don't need to be able to specify direct handlers, but I'm not sure. Take the forced polling for example "tasks/result". It might not be worth the effort to expand the interface, but a low-level handler would let power users avoid polling. And filters seem useful for logging and the like.

/// </remarks>
public bool Cancel(string taskId)
{
if (_disposed)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's weird to check _disposed here but not in Complete. It also doesn't look like the return value is used for anything anyway. Does the McpTaskCancellationTokenProvider even need to be disposable at all?

The finally bock should ensure everything gets removed without increasing the risk of ODEs. and if for some reason the continuation that was supposed to run the finally gets GC'd because a custom task store did something really weird, the GC should take care of all of this too.

Comment on lines +282 to +285
var inputRequiredTask = await store.UpdateTaskStatusAsync(
task.TaskId,
McpTaskStatus.InputRequired,
"Waiting for user confirmation",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the server set the input_required state automatically when the tool call handler has a server-to-client request like elicitation or sampling pending?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SEP-1686: Tasks

2 participants