Skip to content

add streamableHttp server support for everything server #1496

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

Merged
merged 8 commits into from
Apr 24, 2025

Conversation

shivdeepak
Copy link
Contributor

Description

Since Streamable HTTP transport landed on typescript-sdk a few days back, this PR uses that and adds Streamable HTTP server support to everything server. This directly addresses the current open issue: #1247

Server Details

  • Server: everything
  • Changes to: transport

Motivation and Context

I am also trying to add support for Streamable HTTP on inspector, and everything server reference implementation would help testing it out.

How Has This Been Tested?

The server can be started with the new entry point npm run start:streamableHttp.

I tested it with the development version inspector with Streamable HTTP support to ensure functionality works.

Screenshot 2025-04-18 at 6 32 55 PM

Breaking Changes

No. This is an additional server, separate from the existing SSE implementation.

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 Protocol Documentation
  • My changes follows MCP security best practices
  • I have updated the server's README accordingly
  • I have tested this with an LLM client
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have documented all environment variables and configuration options

Additional context

@shivdeepak
Copy link
Contributor Author

@cliffhall @Dogacel, please take a look.

@olaservo olaservo added enhancement New feature or request server-everything Reference implementation for the Everything MCP server - src/everything labels Apr 19, 2025
@Dogacel
Copy link

Dogacel commented Apr 19, 2025

The server can be started with the new entry point npm run start:streamableHttp.

Can we document this in code? That was one thing that gave me a hard time to get started.

@shivdeepak
Copy link
Contributor Author

@Dogacel updated the docs. I am unsure if this could be achieved with npx after publishing the package. Thoughts?


// Handle the request with existing transport - no need to reconnect
// The existing transport is already connected to the server
await transport.handleRequest(req, res, req.body);
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you find it necessary to pass the req.body argument?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@cliffhall cliffhall left a comment

Choose a reason for hiding this comment

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

Hey @shivdeepak, thanks for this. I tested it with your Inspector changes and it worked flawlessly. Just a few requests on message shape.

@shivdeepak
Copy link
Contributor Author

@cliffhall thanks for the review. cleaned it all up, and tested it.

@cliffhall
Copy link
Contributor

@cliffhall thanks for the review. cleaned it all up, and tested it.

Not working now.

Error from MCP server: Error: Error POSTing to endpoint (HTTP 400): {"jsonrpc":"2.0","error":{"code":-32700,"message":"Parse error","data":"InternalServerError: stream is not readable"},"id":null}
    at StreamableHTTPClientTransport.send (file:///Users/cliffhall/Projects/mcp-inspector/node_modules/@modelcontextprotocol/sdk/dist/esm/client/streamableHttp.js:263:23)

Failing because id is null I believe.
Screenshot 2025-04-19 at 5 02 35 PM

Copy link
Contributor

@cliffhall cliffhall left a comment

Choose a reason for hiding this comment

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

Getting errors because of message ids being null.

@shivdeepak
Copy link
Contributor Author

shivdeepak commented Apr 19, 2025

@cliffhall thanks for the review. cleaned it all up, and tested it.

Not working now.

Error from MCP server: Error: Error POSTing to endpoint (HTTP 400): {"jsonrpc":"2.0","error":{"code":-32700,"message":"Parse error","data":"InternalServerError: stream is not readable"},"id":null}
    at StreamableHTTPClientTransport.send (file:///Users/cliffhall/Projects/mcp-inspector/node_modules/@modelcontextprotocol/sdk/dist/esm/client/streamableHttp.js:263:23)

Failing because id is null I believe. Screenshot 2025-04-19 at 5 02 35 PM

~~Hi @cliffhall ~~

A few questions:
1. What triggered this error, i.e. how did you test this from MCP Inspector? Did it happen during DELETE /mcp call?
2. When I click "Disconnect" on MCP Inspector, it doesn't make an API call to the MCP Inspector backend. Is that an expected behavior? Maybe disconnect should be making a DELETE /mcp call?

@shivdeepak
Copy link
Contributor Author

I was able to reproduce the issue. it happens on connect. and it's happening after I removed req.body from transport.handleRequest(req, res, req.body); in post request handler.

i didn't catch it the last time because i forgot to run npm run build on the everything server. mea culpa.

I am currently looking into why req.body is needed.

@shivdeepak
Copy link
Contributor Author

@cliffhall I have addressed the two issues you raised.

  1. req.body is passed because StreamableHTTPServerTransport expects it.
  2. based on the MCP transport spec, response message id can be null if it's an error. I am using request.id if it exists.

PR is now ready for review.

@shivdeepak
Copy link
Contributor Author

@cliffhall
Copy link
Contributor

cliffhall commented Apr 20, 2025

@cliffhall I have addressed the two issues you raised.

  1. req.body is passed because StreamableHTTPServerTransport expects it.
  2. based on the MCP transport spec, response message id can be null if it's an error. I am using request.id if it exists.
  1. It accepts req.body as parsedBody and passes it on for a POST, but it isn't required.

The problem with doing this is that it always bypasses parsing, which includes a adding a maximum message size and encoding taken from the content type parameters.

What happens when we pass in the parsedBody param

      if (parsedBody !== undefined) {
        rawMessage = parsedBody;
      } else {
        const parsedCt = contentType.parse(ct);
        const body = await getRawBody(req, {
          limit: MAXIMUM_MESSAGE_SIZE,
          encoding: parsedCt.parameters.charset ?? "utf-8",
        });
        rawMessage = JSON.parse(body.toString());
      }

IMO, this should not be the default behavior of the client. I know the SDK has a comment that shows how you can pass in a pre-parsed body, but I think there has to be a specific reason why we would do that, not just always-on behavior.

  1. More troubleshooting reveals it isn't the returned error messages with null ids that are the problem. It's the response to the initialize request.

The error I see echoed on the inspector's STDOUT is:

Error from MCP server: Error: Error POSTing to endpoint (HTTP 400): {"jsonrpc":"2.0","error":{"code":-32700,"message":"Parse error","data":"InternalServerError: stream is not readable"},"id":null}

AHA!

In order to continue troubleshooting this, I removed your third param to transport.handleRequest so I would get the errors.

Server received a well-formed initialize request

Screenshot 2025-04-20 at 4 39 14 PM

But, opening the console in the browser, I see the client isn't hitting the /mcp endpoint. It's hitting /sse and /messages like the old SSE server, even though Streamable HTTP is selected and the url field points to /mcp. I tried clearing my local storage, opening in an incognito, etc. Same behavior.

When it fails

Screenshot 2025-04-20 at 4 27 11 PM

Putting your third param (parsed body) back in the mix

It succeeds, but is still talking to an SSE server.

Screenshot 2025-04-20 at 4 43 42 PM

Questions

  • Can you go back to the Inspector and see what's going on there? It should be hitting only an /mcp endpoint with GET and POST.
  • Why would this app, which is clearly only using /mcp endpoints even be responding to /sse and /messages?

@cliffhall
Copy link
Contributor

Why would this app, which is clearly only using /mcp endpoints even be responding to /sse and /messages?

This is a twist.

  • It's because the inspector's proxy server exposes /sse and /message endpoints.

Presumably we need an /mcp endpoint on the proxy as well.

We still don't know why the client is hitting the /sse endpoint to begin with, and that would be good to sort out.

@shivdeepak
Copy link
Contributor Author

Yes. The Inspector frontend talks to the Inspector backend over /sse and /messages endpoints and uses SSE transport. And the backend acts as a proxy for stdio, SSE and Streamable HTTP. This behavior was pre-existing for stdio, and SSE transport modes.

Given how stdio works, it makes sense for it to be implemented that way. But I do agree that for Streamable HTTP, the client side implementation should also use the /mcp endpoint.

I will spend some time today get that working. I will also look into why initialize is failing for this PR.

@cliffhall
Copy link
Contributor

cliffhall commented Apr 21, 2025

I will spend some time today get that working. I will also look into why initialize is failing for this PR.


const app = express();

app.use(express.json());
Copy link
Contributor

@cliffhall cliffhall Apr 21, 2025

Choose a reason for hiding this comment

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

@shivdeepak Remove this app.use(express.json()). It wasn't necessary in sse.ts and it is what's causing the Parse error problem. If you remove this line, you can also remove the third argument to handleRequest and get past problem the with the initialize message.

That moves us on to the next error:

Error from MCP server: Error: Error POSTing to endpoint (HTTP 400): {"jsonrpc":"2.0","error":{"code":-32000,"message":"Bad Request: No valid session ID provided"}}

Copy link
Contributor

Choose a reason for hiding this comment

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

More testing, and I have an idea that it's the inspector sending things incorrectly.

  • SSE wanted a GET request to /sse followed POSTS to /message.
  • Streamable wants a POST to /mcp followed by more POSTS to /mcp with GET used to fetch SSE messages from the server.
  • I'm seeing us send to /mcp with my changes, but it's doing a GET first, like with SSE.
Screenshot 2025-04-21 at 3 28 35 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm tweaking the Inspector PR now.

Copy link
Contributor Author

@shivdeepak shivdeepak Apr 22, 2025

Choose a reason for hiding this comment

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

@shivdeepak Remove this app.use(express.json()). It wasn't necessary in sse.ts and it is what's causing the Parse error problem. If you remove this line, you can also remove the third argument to handleRequest and get past problem the with the initialize message.

That moves us on to the next error:

Error from MCP server: Error: Error POSTing to endpoint (HTTP 400): {"jsonrpc":"2.0","error":{"code":-32000,"message":"Bad Request: No valid session ID provided"}}

@cliffhall I figured it out.

When we remove the express.json() middleware, the req.body becomes undefined. And therefore the following condition doesn't pass and the code execution enters the final else block, which returns http 400 error response. (the error you are referring to); it never reaches transport.handleRequest(req, res).

    } else if (!sessionId && isInitializeRequest(req.body)) {

But if we don't remove express.json() middleware, the req payload is consumed by the middleware, therefore
transport.handleRequest(req, res) doesn't provide request payload to the transport anymore.

I see two approaches to solve this problem

  1. new approach: remove express.json() middleware and also remove isInitializeRequest(req.body) check, and let the transport server handle this check. This way we can remove req.body from transport.handleRequest(req, res).
  2. current approach: or, go with the express.json() middleware, keep the isInitializeRequest(req.body) check, and then keep the req.body in transport.handleRequest(req, res, req.body)

I went with option 2 because the original author of StreamableHTTPServerTransport implemented his examples this way.

I can modify the source code to go with option 1, and that way we don't have to pass req.body to transport.handleRequest(req, res).

IMHO, either approach is fine. Option 1 may be a little cleaner. Let me know what you would like me to do.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the new approach could make sense. I still don't think the second (original) approach is good because, as I mentioned before, it always bypasses parsing, which includes a adding a maximum message size and encoding taken from the content type parameters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cliffhall made the fixes, and tested them against your changes from yesterday -- https://github.com/cliffhall/mcp-inspector/tree/3a2e2485273a4dd1bfb5186ceee73fab8f8b8b9a

  • removed express.json() middleware
  • removed isInitializeRequest(req.body) check
  • removed req.body from transport.handleRequest(req, res, req.body)

@shivdeepak shivdeepak reopened this Apr 23, 2025
@shivdeepak shivdeepak force-pushed the main branch 2 times, most recently from 9570904 to c25d1bc Compare April 23, 2025 02:34
Copy link
Contributor

@cliffhall cliffhall left a comment

Choose a reason for hiding this comment

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

All good. Working fine with the newly improved Inspector, and nothing changed that affects previous functionality.

}

// Check for Last-Event-ID header for resumability
const lastEventId = req.headers['last-event-id'] as string | undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not 100% certain how to test this part. I think we need to beef up the logic in the Inspector to make the Reconnect button send the last-event-id header.

But this endpoint is working correctly for initiating and maintaining the SSE stream, and the only thing we're doing here is logging the fact that this proxy saw the header. The SDK deals with recognizing that header and doing a replay from it, so I think this server is just fine.

@cliffhall cliffhall merged commit f5dac6d into modelcontextprotocol:main Apr 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request server-everything Reference implementation for the Everything MCP server - src/everything
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants