Skip to content

Conversation

@cliffhall
Copy link
Member

@cliffhall cliffhall commented May 22, 2025

  • In server/src/index.ts
    • add serverTransports map

    • remove backingServerTransport var

    • in /mcp POST handler, when a new connection is being made

      • create a server transport
      • map the server transport using the session id from the web app connection
      • pass serverTransport to the mcpProxy instead of backingServerTransport
    • in /stdio GET handler, when a new connection is being made

      • create a server transport
      • map the server transport using the session id from the web app connection
      • pass serverTransport to the mcpProxy instead of backingServerTransport
    • in /sse GET handler, when a new connection is being made

      • create a server transport
      • map the server transport using the session id from the web app connection
      • pass serverTransport to the mcpProxy instead of backingServerTransport

Motivation and Context

Some servers are meant to share resources among multiple clients. The current version of the Inspector cannot manage this.

While it does require an appropriate implementation on the server itself, the proxy server for the Inspector also requires a few changes to support this properly. Currently, a single server transport is bound to every client transport, and that causes problems when new connections are made ore broken. Instead, each client transport needs to be paired with its own server transport.

How Has This Been Tested?

This video shows me first connecting to a properly implemented server with multiple instances of the current Inspector to demonstrate the problem, then with the changes in this PR to show how it should work.

In the video, I show connections using SSE, but I've also fixed the StreamableHttp handling as well.

Note that I have another PR for server-everything (now merged), which allows multiple connections, so that this capability is testable entirely with in-project assets.

Breaking Changes

Nope.

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

…ue server tranpsort for each web app transport.

* In server/src/index.ts
  - add serverTransports map

  - remove backingServerTransport var

  - in /mcp POST handler, when a new connection is being made
    - create a server transport
    - map the server transport using the session id from the web app connection
    - pass serverTransport to the mcpProxy instead of backingServerTransport

  - in /stdio GET handler, when a new connection is being made
    - create a server transport
    - map the server transport using the session id from the web app connection
    - pass serverTransport to the mcpProxy instead of backingServerTransport

  - in /sse GET handler, when a new connection is being made
    - create a server transport
    - map the server transport using the session id from the web app connection
    - pass serverTransport to the mcpProxy instead of backingServerTransport
Copy link
Member

@olaservo olaservo left a comment

Choose a reason for hiding this comment

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

Had a couple clarifying questions and suggestions. TBH the terminology and architecture sometimes throws me for a loop here, since almost everything involved is a 'server' or 'client' of some kind but not always referring to a MCP Server or Client.

@cliffhall
Copy link
Member Author

cliffhall commented May 29, 2025

@tadasant @evalstate - @olaservo has approved but if one or both of you could also give this a once over, it'd be nice.

The companion PR for the Everything server is merged, so it can be used to test against since it now supports multiple connections properly.

@evalstate
Copy link
Member

@tadasant @evalstate - @olaservo has approved but if one or both of you could also give this a once over, it'd be nice.

The companion PR for the Everything server is merged, so it can be used to test against since it now supports multiple connections properly.

I'll have a look this evening. This is exactly the scenario I have been working with with 2 separate Servers this week. qq @cliffhall - does this one include the /delete on disconnect?

@evalstate
Copy link
Member

cliff.mp4

Hi Cliff -- initial impressions look good, this is the inspector connected 2* to to a Server which requests sampling and you can see the request pops up in the right inspector window 👍. I'll carry on testing.

@cliffhall
Copy link
Member Author

Hi Cliff -- initial impressions look good, this is the inspector connected 2* to to a Server which requests sampling and you can see the request pops up in the right inspector window 👍. I'll carry on testing.

Excellent test, @evalstate

@cliffhall
Copy link
Member Author

cliffhall commented May 29, 2025

qq @cliffhall - does this one include the /delete on disconnect?

@evalstate No, it doesn't, but after looking into it, I think it would be better handled in a separate PR. The client should send the DELETE when terminateSession is called on the transport. As I try to make that happen I realize it is a separate can of worms. Working on another branch to handle it now.

@cliffhall
Copy link
Member Author

qq @cliffhall - does this one include the /delete on disconnect?

@evalstate There is now a draft PR #470 for handling the DELETE on disconnect, but since it is somewhat tangential to what's in this one, separating them seemed the wisest course. That one is a branch of this one, so after this one is merged, I'll take that one off draft status and we can get it reviewed and merged.

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.

3 participants