Skip to content

Fix 307 Temporary Redirect when use streamable_http #781

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 36 commits into
base: main
Choose a base branch
from

Conversation

chi2liu
Copy link

@chi2liu chi2liu commented May 22, 2025

The default mount path of Streamable HTTP is /mcp, but in some environments, a 307 Temporary Redirect to /mcp/mcp/ will appear, causing the request to hang or fail. When the streamable_http path is set to /mcp/, everything works fine.

Code Analysis
Mount Path

In the server.py and simple-streamablehttp-stateless examples, the ASGI routes are Mount("/mcp", app=handle_streamable_http), that is, there is no trailing slash.
In the Settings class of server.py, the default value of streamable_http_path is "/mcp" (also without a slash).
Redirection Issue

Mount("/mcp", ...) route of Starlette/FastAPI, if /mcp is requested, will automatically 307 redirect to /mcp/ (with a slash).
But if the client requests /mcp/ and the server only mounts /mcp, a redirect chain of /mcp/mcp/ will appear in some environments, resulting in "unreachable".
This is related to the "strict slash matching" of ASGI routing.
Solution

Recommended practice: Always use /mcp/ as the mounting path (with a slash) and let the client request /mcp/, so there will be no redirection problem.
Or, when mounting /mcp, make sure the client only requests /mcp without a slash.

Motivation and Context

How Has This Been Tested?

Breaking Changes

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

@chi2liu
Copy link
Author

chi2liu commented May 22, 2025

#732

@chi2liu
Copy link
Author

chi2liu commented May 22, 2025

when we set the default /mcp, the log will be, in some env will

INFO:     Started server process [55547]
INFO:     Waiting for application startup.
INFO:     Application startup complete.
INFO:     Uvicorn running on http://0.0.0.0:8080 (Press CTRL+C to quit)
INFO:     10.183.170.230:53048 - "POST /mcp HTTP/1.1" 307 Temporary Redirect
INFO:     10.183.170.230:53054 - "POST /mcp/ HTTP/1.1" 200 OK
INFO:     10.183.170.230:53048 - "POST /mcp HTTP/1.1" 307 Temporary Redirect
INFO:     10.183.170.230:53054 - "POST /mcp/ HTTP/1.1" 202 Accepted
INFO:     10.183.170.230:53048 - "GET /mcp HTTP/1.1" 307 Temporary Redirect
INFO:     10.183.170.230:53054 - "GET /mcp/ HTTP/1.1" 200 OK
INFO:     10.183.170.230:34296 - "POST /mcp HTTP/1.1" 307 Temporary Redirect
INFO:     10.183.170.230:34312 - "POST /mcp/ HTTP/1.1" 200 OK

in some env will be hanging, because 307 Temporary Redirect to /byoa/mcp/mcp/, this is unreachable

INFO:     Started server process [35]
INFO:     Waiting for application startup.
INFO:     Application startup complete.
INFO:     Uvicorn running on http://0.0.0.0:8080 (Press CTRL+C to quit)
INFO:     100.67.31.17:58472 - "POST /mcp HTTP/1.1" 307 Temporary Redirect
INFO:     100.67.30.95:57600 - "GET /mcp HTTP/1.1" 307 Temporary Redirect

when we set the streamable_http to '/mcp/', it will be good, as the log:

[05/22/25 01:52:28] INFO     Starting server "StatelessServer"...                                                                server.py:214
INFO:     Started server process [80154]
INFO:     Waiting for application startup.
INFO:     Application startup complete.
INFO:     Uvicorn running on http://0.0.0.0:8080 (Press CTRL+C to quit)
INFO:     10.183.170.230:36166 - "POST /mcp/ HTTP/1.1" 200 OK
INFO:     10.183.170.230:36166 - "POST /mcp/ HTTP/1.1" 202 Accepted
INFO:     10.183.170.230:36182 - "GET /mcp/ HTTP/1.1" 200 OK
INFO:     10.183.170.230:33648 - "POST /mcp/ HTTP/1.1" 200 OK

@chi2liu
Copy link
Author

chi2liu commented May 22, 2025

@chi2liu
Copy link
Author

chi2liu commented May 22, 2025

@ihrpr

Copy link
Contributor

@ihrpr ihrpr left a comment

Choose a reason for hiding this comment

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

Thanks for identifying and reporting this redirect issue! However, I don't think changing the default
streamable_http_path is the right approach here.

The core issue: This change creates inconsistency across the codebase rather than solving the root problem.

  • All client code constructs URLs with /mcp (no trailing slash) - see examples/clients/simple-auth-client/main.py:344
  • All example servers mount at /mcp (no trailing slash) - see both simple-streamablehttp examples
  • All tests use /mcp (no trailing slash) consistently - see tests/shared/test_streamable_http.py:238
  • Only FastMCP would have a different default with this change

Problems with this approach:

  1. Breaking consistency: Makes FastMCP behave differently from all examples and existing usage patterns
  2. Incomplete fix: The examples and tests still use /mcp without trailing slash, so the inconsistency remains
  3. Wrong direction: The entire ecosystem already standardized on /mcp - we shouldn't change that

Alterative:

  1. Keep the default as /mcp and fix the ASGI mounting logic to handle redirects properly
  2. Add logic to FastMCP to handle both /mcp and /mcp/ transparently at the mount level
  3. Document the trailing slash behavior so users can choose the approach that works for their environment

The redirect issue you've identified is real and needs fixing, but changing the default path creates more problems than
it solves. Let's find a solution that maintains consistency while addressing the underlying ASGI routing behavior.

@chi2liu
Copy link
Author

chi2liu commented May 26, 2025

Thanks for identifying and reporting this redirect issue! However, I don't think changing the default streamable_http_path is the right approach here.

The core issue: This change creates inconsistency across the codebase rather than solving the root problem.

  • All client code constructs URLs with /mcp (no trailing slash) - see examples/clients/simple-auth-client/main.py:344
  • All example servers mount at /mcp (no trailing slash) - see both simple-streamablehttp examples
  • All tests use /mcp (no trailing slash) consistently - see tests/shared/test_streamable_http.py:238
  • Only FastMCP would have a different default with this change

Problems with this approach:

  1. Breaking consistency: Makes FastMCP behave differently from all examples and existing usage patterns
  2. Incomplete fix: The examples and tests still use /mcp without trailing slash, so the inconsistency remains
  3. Wrong direction: The entire ecosystem already standardized on /mcp - we shouldn't change that

Alterative:

  1. Keep the default as /mcp and fix the ASGI mounting logic to handle redirects properly
  2. Add logic to FastMCP to handle both /mcp and /mcp/ transparently at the mount level
  3. Document the trailing slash behavior so users can choose the approach that works for their environment

The redirect issue you've identified is real and needs fixing, but changing the default path creates more problems than it solves. Let's find a solution that maintains consistency while addressing the underlying ASGI routing behavior.

Thank you for your review and helpful suggestions.
@ihrpr The following changes have been made to completely solve the trailing slash compatibility issue:

  1. Keep the default path as /mcp to avoid disrupting the existing ecosystem.
  2. In the ASGI route registration logic of FastMCP, automatically mount handlers for both /mcp and /mcp/, and clients can access them in any form without redirection.

In this way, whether the client, example, or test uses /mcp or /mcp/, it can be seamlessly compatible without modifying the existing code. The problem has been completely solved.

Copy link

@vectorstain vectorstain left a comment

Choose a reason for hiding this comment

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

GG

@chi2liu chi2liu requested review from ihrpr and vectorstain May 27, 2025 01:37
Copy link

@vectorstain vectorstain left a comment

Choose a reason for hiding this comment

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

GG

@chi2liu chi2liu requested a review from vectorstain May 27, 2025 09:09
Copy link

@vectorstain vectorstain left a comment

Choose a reason for hiding this comment

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

GG

@vectorstain
Copy link

@chi2liu please update the branch with master and check again tests results

@chi2liu chi2liu requested a review from vectorstain May 27, 2025 19:34
Copy link
Contributor

@ihrpr ihrpr left a comment

Choose a reason for hiding this comment

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

Thank you!

Please can you add tests to verify all works as expected?

@Kludex
Copy link
Member

Kludex commented May 28, 2025

Why are we even mounting applications? We can have a Router and set redirect_slashes to False.

@chi2liu chi2liu requested a review from ihrpr May 29, 2025 02:52
@chi2liu
Copy link
Author

chi2liu commented May 29, 2025

Why are we even mounting applications? We can have a Router and set redirect_slashes to False.

I have modified it according to your suggestion. Could you please review it? @Kludex

@@ -618,7 +618,7 @@ async def run_streamable_http_async(self) -> None:
import uvicorn

starlette_app = self.streamable_http_app()

starlette_app.router.redirect_slashes = False
Copy link
Member

@Kludex Kludex May 29, 2025

Choose a reason for hiding this comment

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

Hmmm... I think we can add redirect_slashes in the Starlette constructor on starlette's side.


But this part is fine.

Comment on lines 785 to 789
async def handle_streamable_http(request: Request) -> Response:
await self.session_manager.handle_request(
request.scope, request.receive, request._send
)
return Response()
Copy link
Member

Choose a reason for hiding this comment

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

This is not fine. Does handle_request needs to be an ASGI application?

Copy link

@vectorstain vectorstain May 29, 2025

Choose a reason for hiding this comment

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

@Kludex This could be a more suitable solution ? PR

Comment on lines 796 to 798
# Always register both /mcp and /mcp/ for full compatibility
_main_path = self.settings.streamable_http_path.removesuffix("/")
_alt_path = _main_path + "/"
Copy link
Member

Choose a reason for hiding this comment

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

I think the problem was the redirect, but if you have a 404 then it's not an issue anymore, and we shouldn't be needing this.

@chi2liu chi2liu requested a review from Kludex May 29, 2025 09:13
@@ -839,6 +839,7 @@ async def handle_streamable_http(
routes=routes,
middleware=middleware,
lifespan=lambda app: self.session_manager.run(),
redirect_slashes=False,
Copy link

@vectorstain vectorstain May 29, 2025

Choose a reason for hiding this comment

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

Tests failed cause MCP server doesn't start.

return Starlette(
TypeError: Starlette.__init__() got an unexpected keyword argument 'redirect_slashes'

You cant pass redirect_slashes to starlette. Pls review my proposed solution PR

Copy link
Member

Choose a reason for hiding this comment

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

That's what I said, we don't have it yet...

chi2liu and others added 2 commits May 29, 2025 22:18
* Refactor FastMCP routing to use Router and streamline request handling

* Add async support to token validation test and enhance metadata snapshot assertions

* Fix tests

* Fix tests

---------

Co-authored-by: Vincenzo Maria Calandra <vincenzomariacalandra@MacBook-Pro-di-Vincenzo.local>
Co-authored-by: 633WHU <cliu_whu@yeah.net>
@chi2liu chi2liu requested a review from vectorstain May 29, 2025 14:21
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.

4 participants