SEP-1686: Tasks #1041
SEP-1686: Tasks #1041
Conversation
There was a problem hiding this comment.
SEP-1686 currently states that it "introduces a mechanism for requestors (which can be either clients or servers, depending on the direction of communication) to augment their requests with tasks." Is this still the case?
I don't see any tests or examples demonstrating using a TaskStore with an McpClient (it's all McpServer or the Protocol base type), although I suppose it should work considering its shared code. It still might be nice to have an end-to-end test demonstrating that client-side support for stuff like elicitations works using the public APIs.
| * Note: This is not suitable for production use as all data is lost on restart. | ||
| * For production, consider implementing TaskStore with a database or distributed cache. | ||
| */ | ||
| export class InMemoryTaskStore implements TaskStore { |
There was a problem hiding this comment.
Is there a reason we cannot create a default in-memory task store with settable max limits on the total number of tasks?
I like that this PR adds the TaskStore abstraction which opens the door for a distributed implementation and doesn't require the application developer to manually handle tasks/get, tasks/list, etc., but it feels weird to me to not have a production-ready in-memory solution provided by the SDK.
It should still be off by default to make sure people are explicit about any maxTrackedTask limits and the like, but it should be built in and not left to an example that is "not suitable for production." This will prove it's possible to implement a production-quality TaskStore in the easiest case where everything is tracked in-memory and lost on process restart.
- Note: This is not suitable for production use as all data is lost on restart.
- For production, consider implementing TaskStore with a database or distributed cache.
This is true, but I think the impact is overstated. The logic in protocol.ts that calls _taskStore.updateTaskStatus(taskMetadata.taskId, 'input_required'); and then sets the task back to 'working' when the a request completes also breaks if the server restarts. If the process exits before the client provides a response, the task will be permanently left in an 'input_required' state indefinitely without some manual intervention outside of the SDK.
In most cases supported by the SDK, when tasks cannot outlive the MCP server process, an in-memory TaskStore would be better than a distributed one because the client will be informed that the Task is no longer being tracked by the server after it restarts automatically.
There was a problem hiding this comment.
I don't disagree, but precedence was the main reason for leaving this as an example and leaving out any limits, really - it's the same case with InMemoryEventStore and InMemoryOAuthClientProvider. @ihrpr @felixweinberger any input here?
There was a problem hiding this comment.
If the SDK is going to provide a production-grade implementation, it needs to have limits and some form of logging extension points, but if it is not going to provide a production-grade implementation, I don't want to misrepresent this example as one.
src/shared/protocol.ts
Outdated
| .then(async () => { | ||
| // If this request asked for task creation, create the task and send notification | ||
| if (taskMetadata && this._taskStore) { | ||
| const task = await this._taskStore!.getTask(taskMetadata.taskId); |
There was a problem hiding this comment.
Am I correct in reading that you need to pass in a unique TaskStore when constructing each server instance or else risk collisions? This seems like a huge security footgun for Streamable HTTP scenarios, and I don't think this is called out loudly enough considering the simpleStremableHttp example uses a single InMemoryTaskStore for multiple McpServers.
Not having any limits on memory consumption is one problem, but being able to read the results of other people's tool calls is far worse and too much for even example code in my opinion.
Our examples should make it very clear that tasks need to be partitioned by session or at least by user, and that using just the taskId by itself as a DB key is a huge security risk.
There was a problem hiding this comment.
The TaskStore interface does need a parameter for the session ID to perform auth binding, that was an oversight - for very simple use cases or stdio servers there's no way to require this, but it should be partitioned if applicable/possible.
src/shared/protocol.ts
Outdated
| // If this request had a task, mark it as working | ||
| if (taskMetadata && this._taskStore) { | ||
| try { | ||
| await this._taskStore.updateTaskStatus(taskMetadata.taskId, 'working'); |
There was a problem hiding this comment.
Why not start the task immediately in the 'working' state other than the SEP requiring it start in the 'submitted' state?
I think the 'submitted' status should be removed from the SEP. The only reason a Task wouldn't immediately move from 'submitted' to 'working' is if the sending 'notifications/tasks/created' failed or was really slow for some reason. And sending notifications are supposed to be fire-and-forget. Chances are that most clients will never see the Task in the 'submitted' state and it conveys no useful information.
| */ | ||
| export const TaskSchema = z.object({ | ||
| taskId: z.string(), | ||
| status: z.enum(['submitted', 'working', 'input_required', 'completed', 'failed', 'cancelled', 'unknown']), |
There was a problem hiding this comment.
Does anything actually use the 'unknown' status? It seems like we use ErrorCode.InvalidParams everywhere we might have used 'unknown' otherwise. I don't think it makes sense to have multiple ways to represent that the state for a given task is unknown and not even use one of them.
I think we should probably remove the 'unknown' status from the SEP too.
Agreed, will add this. edit: Done |
2 participants
This PR implements the required changes for modelcontextprotocol/modelcontextprotocol#1686, which adds task augmentation to requests.
Motivation and Context
The current MCP specification supports tool calls that execute a request and eventually receive a response, and tool calls can be passed a progress token to integrate with MCP’s progress-tracking functionality, enabling host applications to receive status updates for a tool call via notifications. However, there is no way for a client to explicitly request the status of a tool call, resulting in states where it is possible for a tool call to have been dropped on the server, and it is unknown if a response or a notification may ever arrive. Similarly, there is no way for a client to explicitly retrieve the result of a tool call after it has completed — if the result was dropped, clients must call the tool again, which is undesirable for tools expected to take minutes or more. This is particularly relevant for MCP servers abstracting existing workflow-based APIs, such as AWS Step Functions, Workflows for Google Cloud, or APIs representing CI/CD pipelines, among other applications.
This proposal (and implementation) solves this by introducing the concept of Tasks, which are pending work objects that can augment any other request. Clients generate a task ID and augment their request with it — that task ID is both a reference to the request and an idempotency token. If the server accepts the task augmentation request, clients can then poll for the status and eventual result of the task with the
tasks/getandtasks/resultoperations.How Has This Been Tested?
Unit tests and updated sHTTP example.
Breaking Changes
None.
Types of changes
Checklist
Additional context