fix(client): replace body.cancel() with text() to prevent hanging #1320
+13
−13
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
body.cancel() can hang indefinitely with certain server responses
(e.g., Sentry MCP's OAuth flow). Using text?.().catch(() => {}) is safer:
- Always completes (reads all data)
- Releases connection (body consumed)
- No stream coordination needed
- Uses optional chaining for compatibility with mock responses
Fixes regression from modelcontextprotocol#1173 and modelcontextprotocol#1214.
grimmerk
force-pushed
the
fix/replace-body-cancel-with-text-consume
branch
from
c131704 to
358910d
Compare
Contributor
|
Tests are failing here, reckon you could have a look |
commit: |
Contributor
Author
|
@mattzcarey thanks for the reminder. I've force-pushed it to fix the broken tests. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Replace
response.body.cancel()withresponse.text().catch(() => {})in client package to prevent indefinite hanging with certain server responses.Motivation and Context
PRs #1173 and #1214 added
body.cancel()calls to fix connection leaks in Cloudflare Workers. However,body.cancel()can hang indefinitely with certain MCP servers (e.g., Sentry MCP during OAuth metadata discovery).The hang occurs because
cancel()attempts to abort the stream, which requires coordination with the server that doesn't always complete.Using
text().catch(() => {})instead:In all affected code paths (OAuth metadata discovery, error responses, 202 Accepted, session termination), the response bodies are small, making
text()a safe and efficient choice.This preserves the original fix for Cloudflare Workers connection leaks while eliminating the hanging issue.
How Has This Been Tested?
https://mcp.sentry.dev/mcp) which previously hung indefinitelyBreaking Changes
None. Drop-in replacement that maintains the same behavior.
Types of changes
Checklist
Additional context
Fixes regression introduced in #1173 and #1214.
References: