fix(tasks): avoid dropping completed task results during collection by coinmoles · Pull Request #639 · modelcontextprotocol/rust-sdk

coinmoles · 2026-02-02T09:40:33Z

Remove the std::mem::take from OperationProcessor::collect_completed_results

Motivation and Context

std::mem::take would clear OperationProcessor::completed_results, causing completed tasks to be dropped. This made tasks/get and tasks/result to fail for subsequent requests.

How Has This Been Tested?

All existing tests passed
Tested with a custom task server/client implementation (Will add this to examples if requested)

Breaking Changes

This is technically a breaking change that changes the return type of OperationProcessor::collect_completed_results from Vec<TaskResult> to (). However, I doubt there was anyone calling the method directly without using the #[tool_handler] macro.

I did this to avoid introducing an unnecessary clone call. If this is a problem, I can change the method so it returns either:

self.completed_results.clone()
An owned vector of newly collected items, which I think was the intended return value in the first place.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation update

Checklist

I have read the MCP Documentation
My code follows the repository's style guidelines
New and existing tests pass locally
I have added appropriate error handling
I have added or updated documentation as needed

Additional context

Fixes#638

alexhancock · 2026-02-04T00:31:27Z


 /// Collect completed results from running tasks and remove them from the running tasks map.
- pub fn collect_completed_results(&mut self) -> Vec<TaskResult> {
+ pub fn collect_completed_results(&mut self) {


thoughts on this being pub in the first place?

Right now, the function has to be public because it is called from the output of #[task_handler] macro.
However, it would make sense to make this private and have OperationProcessor::peek_completed and OperationProcessor::take_completed_result call this before returning. Do you think that would be better?

Yes, something like that makes sense to me. Can you make the change?

Pushed the change.
One thing that is slightly awkward is that peek_completed (and a few other functions whose names suggest non-mutating behavior) now requires &mut self. I think it is fine, but it could be slightly confusing.
Also, while I was doing that I realized that task_result_receiver has no real reason to be an Option (The channel is initialized on startup and not set again) and made it required.

coinmoles · 2026-02-02T09:40:33Z

Remove the std::mem::take from OperationProcessor::collect_completed_results

Motivation and Context

std::mem::take would clear OperationProcessor::completed_results, causing completed tasks to be dropped. This made tasks/get and tasks/result to fail for subsequent requests.

How Has This Been Tested?

All existing tests passed
Tested with a custom task server/client implementation (Will add this to examples if requested)

Breaking Changes

This is technically a breaking change that changes the return type of OperationProcessor::collect_completed_results from Vec<TaskResult> to (). However, I doubt there was anyone calling the method directly without using the #[tool_handler] macro.

I did this to avoid introducing an unnecessary clone call. If this is a problem, I can change the method so it returns either:

self.completed_results.clone()
An owned vector of newly collected items, which I think was the intended return value in the first place.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation update

Checklist

I have read the MCP Documentation
My code follows the repository's style guidelines
New and existing tests pass locally
I have added appropriate error handling
I have added or updated documentation as needed

Additional context

Fixes#638

alexhancock · 2026-02-04T00:31:27Z


 /// Collect completed results from running tasks and remove them from the running tasks map.
- pub fn collect_completed_results(&mut self) -> Vec<TaskResult> {
+ pub fn collect_completed_results(&mut self) {


thoughts on this being pub in the first place?

Right now, the function has to be public because it is called from the output of #[task_handler] macro.
However, it would make sense to make this private and have OperationProcessor::peek_completed and OperationProcessor::take_completed_result call this before returning. Do you think that would be better?

Yes, something like that makes sense to me. Can you make the change?

Pushed the change.
One thing that is slightly awkward is that peek_completed (and a few other functions whose names suggest non-mutating behavior) now requires &mut self. I think it is fine, but it could be slightly confusing.
Also, while I was doing that I realized that task_result_receiver has no real reason to be an Option (The channel is initialized on startup and not set again) and made it required.

coinmoles · 2026-02-02T09:40:33Z

Remove the std::mem::take from OperationProcessor::collect_completed_results

Motivation and Context

std::mem::take would clear OperationProcessor::completed_results, causing completed tasks to be dropped. This made tasks/get and tasks/result to fail for subsequent requests.

How Has This Been Tested?

All existing tests passed
Tested with a custom task server/client implementation (Will add this to examples if requested)

Breaking Changes

This is technically a breaking change that changes the return type of OperationProcessor::collect_completed_results from Vec<TaskResult> to (). However, I doubt there was anyone calling the method directly without using the #[tool_handler] macro.

I did this to avoid introducing an unnecessary clone call. If this is a problem, I can change the method so it returns either:

self.completed_results.clone()
An owned vector of newly collected items, which I think was the intended return value in the first place.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation update

Checklist

I have read the MCP Documentation
My code follows the repository's style guidelines
New and existing tests pass locally
I have added appropriate error handling
I have added or updated documentation as needed

Additional context

Fixes#638

alexhancock · 2026-02-04T00:31:27Z


 /// Collect completed results from running tasks and remove them from the running tasks map.
- pub fn collect_completed_results(&mut self) -> Vec<TaskResult> {
+ pub fn collect_completed_results(&mut self) {


thoughts on this being pub in the first place?

Right now, the function has to be public because it is called from the output of #[task_handler] macro.
However, it would make sense to make this private and have OperationProcessor::peek_completed and OperationProcessor::take_completed_result call this before returning. Do you think that would be better?

Yes, something like that makes sense to me. Can you make the change?

Pushed the change.
One thing that is slightly awkward is that peek_completed (and a few other functions whose names suggest non-mutating behavior) now requires &mut self. I think it is fine, but it could be slightly confusing.
Also, while I was doing that I realized that task_result_receiver has no real reason to be an Option (The channel is initialized on startup and not set again) and made it required.

coinmoles · 2026-02-02T09:40:33Z

Remove the std::mem::take from OperationProcessor::collect_completed_results

Motivation and Context

std::mem::take would clear OperationProcessor::completed_results, causing completed tasks to be dropped. This made tasks/get and tasks/result to fail for subsequent requests.

How Has This Been Tested?

All existing tests passed
Tested with a custom task server/client implementation (Will add this to examples if requested)

Breaking Changes

This is technically a breaking change that changes the return type of OperationProcessor::collect_completed_results from Vec<TaskResult> to (). However, I doubt there was anyone calling the method directly without using the #[tool_handler] macro.

I did this to avoid introducing an unnecessary clone call. If this is a problem, I can change the method so it returns either:

self.completed_results.clone()
An owned vector of newly collected items, which I think was the intended return value in the first place.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation update

Checklist

I have read the MCP Documentation
My code follows the repository's style guidelines
New and existing tests pass locally
I have added appropriate error handling
I have added or updated documentation as needed

Additional context

Fixes#638

alexhancock · 2026-02-04T00:31:27Z


 /// Collect completed results from running tasks and remove them from the running tasks map.
- pub fn collect_completed_results(&mut self) -> Vec<TaskResult> {
+ pub fn collect_completed_results(&mut self) {


thoughts on this being pub in the first place?

Right now, the function has to be public because it is called from the output of #[task_handler] macro.
However, it would make sense to make this private and have OperationProcessor::peek_completed and OperationProcessor::take_completed_result call this before returning. Do you think that would be better?

Yes, something like that makes sense to me. Can you make the change?

Pushed the change.
One thing that is slightly awkward is that peek_completed (and a few other functions whose names suggest non-mutating behavior) now requires &mut self. I think it is fine, but it could be slightly confusing.
Also, while I was doing that I realized that task_result_receiver has no real reason to be an Option (The channel is initialized on startup and not set again) and made it required.

coinmoles · 2026-02-02T09:40:33Z

Remove the std::mem::take from OperationProcessor::collect_completed_results

Motivation and Context

std::mem::take would clear OperationProcessor::completed_results, causing completed tasks to be dropped. This made tasks/get and tasks/result to fail for subsequent requests.

How Has This Been Tested?

All existing tests passed
Tested with a custom task server/client implementation (Will add this to examples if requested)

Breaking Changes

This is technically a breaking change that changes the return type of OperationProcessor::collect_completed_results from Vec<TaskResult> to (). However, I doubt there was anyone calling the method directly without using the #[tool_handler] macro.

I did this to avoid introducing an unnecessary clone call. If this is a problem, I can change the method so it returns either:

self.completed_results.clone()
An owned vector of newly collected items, which I think was the intended return value in the first place.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation update

Checklist

I have read the MCP Documentation
My code follows the repository's style guidelines
New and existing tests pass locally
I have added appropriate error handling
I have added or updated documentation as needed

Additional context

Fixes#638

alexhancock · 2026-02-04T00:31:27Z


 /// Collect completed results from running tasks and remove them from the running tasks map.
- pub fn collect_completed_results(&mut self) -> Vec<TaskResult> {
+ pub fn collect_completed_results(&mut self) {


thoughts on this being pub in the first place?

Right now, the function has to be public because it is called from the output of #[task_handler] macro.
However, it would make sense to make this private and have OperationProcessor::peek_completed and OperationProcessor::take_completed_result call this before returning. Do you think that would be better?

Yes, something like that makes sense to me. Can you make the change?

Pushed the change.
One thing that is slightly awkward is that peek_completed (and a few other functions whose names suggest non-mutating behavior) now requires &mut self. I think it is fine, but it could be slightly confusing.
Also, while I was doing that I realized that task_result_receiver has no real reason to be an Option (The channel is initialized on startup and not set again) and made it required.

coinmoles · 2026-02-02T09:40:33Z

Remove the std::mem::take from OperationProcessor::collect_completed_results

Motivation and Context

std::mem::take would clear OperationProcessor::completed_results, causing completed tasks to be dropped. This made tasks/get and tasks/result to fail for subsequent requests.

How Has This Been Tested?

All existing tests passed
Tested with a custom task server/client implementation (Will add this to examples if requested)

Breaking Changes

This is technically a breaking change that changes the return type of OperationProcessor::collect_completed_results from Vec<TaskResult> to (). However, I doubt there was anyone calling the method directly without using the #[tool_handler] macro.

I did this to avoid introducing an unnecessary clone call. If this is a problem, I can change the method so it returns either:

self.completed_results.clone()
An owned vector of newly collected items, which I think was the intended return value in the first place.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation update

Checklist

I have read the MCP Documentation
My code follows the repository's style guidelines
New and existing tests pass locally
I have added appropriate error handling
I have added or updated documentation as needed

Additional context

Fixes#638

alexhancock · 2026-02-04T00:31:27Z


 /// Collect completed results from running tasks and remove them from the running tasks map.
- pub fn collect_completed_results(&mut self) -> Vec<TaskResult> {
+ pub fn collect_completed_results(&mut self) {


thoughts on this being pub in the first place?

Right now, the function has to be public because it is called from the output of #[task_handler] macro.
However, it would make sense to make this private and have OperationProcessor::peek_completed and OperationProcessor::take_completed_result call this before returning. Do you think that would be better?

Yes, something like that makes sense to me. Can you make the change?

Pushed the change.
One thing that is slightly awkward is that peek_completed (and a few other functions whose names suggest non-mutating behavior) now requires &mut self. I think it is fine, but it could be slightly confusing.
Also, while I was doing that I realized that task_result_receiver has no real reason to be an Option (The channel is initialized on startup and not set again) and made it required.

coinmoles · 2026-02-02T09:40:33Z

Remove the std::mem::take from OperationProcessor::collect_completed_results

Motivation and Context

std::mem::take would clear OperationProcessor::completed_results, causing completed tasks to be dropped. This made tasks/get and tasks/result to fail for subsequent requests.

How Has This Been Tested?

All existing tests passed
Tested with a custom task server/client implementation (Will add this to examples if requested)

Breaking Changes

This is technically a breaking change that changes the return type of OperationProcessor::collect_completed_results from Vec<TaskResult> to (). However, I doubt there was anyone calling the method directly without using the #[tool_handler] macro.

I did this to avoid introducing an unnecessary clone call. If this is a problem, I can change the method so it returns either:

self.completed_results.clone()
An owned vector of newly collected items, which I think was the intended return value in the first place.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation update

Checklist

I have read the MCP Documentation
My code follows the repository's style guidelines
New and existing tests pass locally
I have added appropriate error handling
I have added or updated documentation as needed

Additional context

Fixes#638

alexhancock · 2026-02-04T00:31:27Z


 /// Collect completed results from running tasks and remove them from the running tasks map.
- pub fn collect_completed_results(&mut self) -> Vec<TaskResult> {
+ pub fn collect_completed_results(&mut self) {


thoughts on this being pub in the first place?

Right now, the function has to be public because it is called from the output of #[task_handler] macro.
However, it would make sense to make this private and have OperationProcessor::peek_completed and OperationProcessor::take_completed_result call this before returning. Do you think that would be better?

Yes, something like that makes sense to me. Can you make the change?

Pushed the change.
One thing that is slightly awkward is that peek_completed (and a few other functions whose names suggest non-mutating behavior) now requires &mut self. I think it is fine, but it could be slightly confusing.
Also, while I was doing that I realized that task_result_receiver has no real reason to be an Option (The channel is initialized on startup and not set again) and made it required.

coinmoles · 2026-02-02T09:40:33Z

Remove the std::mem::take from OperationProcessor::collect_completed_results

Motivation and Context

std::mem::take would clear OperationProcessor::completed_results, causing completed tasks to be dropped. This made tasks/get and tasks/result to fail for subsequent requests.

How Has This Been Tested?

All existing tests passed
Tested with a custom task server/client implementation (Will add this to examples if requested)

Breaking Changes

This is technically a breaking change that changes the return type of OperationProcessor::collect_completed_results from Vec<TaskResult> to (). However, I doubt there was anyone calling the method directly without using the #[tool_handler] macro.

I did this to avoid introducing an unnecessary clone call. If this is a problem, I can change the method so it returns either:

self.completed_results.clone()
An owned vector of newly collected items, which I think was the intended return value in the first place.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation update

Checklist

I have read the MCP Documentation
My code follows the repository's style guidelines
New and existing tests pass locally
I have added appropriate error handling
I have added or updated documentation as needed

Additional context

Fixes#638

alexhancock · 2026-02-04T00:31:27Z


 /// Collect completed results from running tasks and remove them from the running tasks map.
- pub fn collect_completed_results(&mut self) -> Vec<TaskResult> {
+ pub fn collect_completed_results(&mut self) {


thoughts on this being pub in the first place?

Right now, the function has to be public because it is called from the output of #[task_handler] macro.
However, it would make sense to make this private and have OperationProcessor::peek_completed and OperationProcessor::take_completed_result call this before returning. Do you think that would be better?

Yes, something like that makes sense to me. Can you make the change?

Pushed the change.
One thing that is slightly awkward is that peek_completed (and a few other functions whose names suggest non-mutating behavior) now requires &mut self. I think it is fine, but it could be slightly confusing.
Also, while I was doing that I realized that task_result_receiver has no real reason to be an Option (The channel is initialized on startup and not set again) and made it required.

fix(tasks): avoid dropping completed task results during collection
0d1d077

github-actionsBot added T-test Testing related changes T-core Core library changes labels Feb 2, 2026

coinmoles mentioned this pull request Feb 2, 2026
tasks/result response do not match the MCP spec #640
Closed

alexhancock reviewed Feb 4, 2026
View reviewed changes

coinmoles added 2 commits February 5, 2026 03:45

chore(tasks): make task_result_receiver required
163a880

refactor(tasks): make collect_completed_results private
6d77ad4

github-actionsBot added the T-macros Macro changes label Feb 5, 2026

alexhancock approved these changes Feb 5, 2026
View reviewed changes

alexhancock merged commit be23334 into modelcontextprotocol:mainFeb 5, 2026
11 checks passed

github-actionsBot mentioned this pull request Feb 4, 2026
chore: release v0.15.0 #636
Merged