Skip to content

[mcp] server handler for streamable HTTP transport#59499

Merged
greedy52 merged 4 commits intomasterfrom
STeve/56588_server_handler
Sep 29, 2025
Merged

[mcp] server handler for streamable HTTP transport#59499
greedy52 merged 4 commits intomasterfrom
STeve/56588_server_handler

Conversation

@greedy52
Copy link
Copy Markdown
Contributor

@greedy52 greedy52 commented Sep 23, 2025

Part of:

Run a MCP server in streamable HTTP transport, for example:

npx @modelcontextprotocol/server-everything streamableHttp

App definition:

  - name: "everything-http"
    uri: "mcp+http://localhost:3001/mcp"
    labels:
      env: dev

Start local proxy:

tsh proxy app -p 8888 everything-http

Connect with inspector (npx @modelcontextprotocol/inspector):
Screenshot 2025-09-23 at 2 08 35 PM

Connect with Claude Desktop

{
  "mcpServers": {
    "teleport-mcp-everything-http": {
      "command": "npx",
      "args": [
        "mcp-remote",
        "http://localhost:8888/mcp",
        "--transport",
        "http-only"
      ]
    }
  }
}
Screenshot 2025-09-23 at 2 20 07 PM

@greedy52 greedy52 added the MCP MCP Server related label Sep 23, 2025
@greedy52 greedy52 marked this pull request as ready for review September 25, 2025 18:32
@github-actions github-actions Bot added application-access size/lg tsh tsh - Teleport's command line tool for logging into nodes running Teleport. labels Sep 25, 2025
@github-actions github-actions Bot requested review from avatus and kopiczko September 25, 2025 18:33
Comment thread lib/srv/mcp/session.go Outdated
Comment thread lib/srv/mcp/session.go
Comment thread lib/srv/mcp/http.go
Comment on lines +58 to +61
go func() {
// Make sure handler returns if ctx is canceled.
<-ctx.Done()
waitConn.Close()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This works, but I wonder if we can pass ctx somewhere to have this automated?

Copy link
Copy Markdown
Contributor Author

@greedy52 greedy52 Sep 26, 2025

Choose a reason for hiding this comment

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

this is a tough one. i updated the code to pass the ctx as base context. but even then, it's up to many parts of the handler flow to honor the ctx or not. So i kept the go routine just to be sure. ebdad41

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Given that we're closing the connection after the function returns (due to defer cancel()), wouldn't it make sense to have it all on a single defer call?

defer func() {
	cancel() // relevant to handler.
	waitConn.Close() // Ensure connection is closed
}()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Given that we're closing the connection after the function returns (due to defer cancel()), wouldn't it make sense to have it all on a single defer call?

defer func() {
	cancel() // relevant to handler.
	waitConn.Close() // Ensure connection is closed
}()

the situation we are trying to resolve is when the input ctx is canceled outside this handler func. there is a long chain of functions from http server to mcputils through all the connection handling. when things implemented correctly, everything shuts down when parent ctx is canceled. again, if implemented correctly. so I just don't want to risk that so i explicitly close the connection when input context is canceled.

Comment thread lib/srv/mcp/http_test.go
@greedy52 greedy52 added the no-changelog Indicates that a PR does not require a changelog entry label Sep 26, 2025
Copy link
Copy Markdown
Contributor

@gabrielcorado gabrielcorado left a comment

Choose a reason for hiding this comment

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

LGTM just some minor comments.

Comment thread lib/srv/mcp/http.go
Comment thread lib/srv/mcp/http.go
Comment thread lib/srv/mcp/http_test.go
@greedy52 greedy52 enabled auto-merge September 29, 2025 17:22
@greedy52 greedy52 added this pull request to the merge queue Sep 29, 2025
Merged via the queue into master with commit 8489b48 Sep 29, 2025
41 checks passed
@greedy52 greedy52 deleted the STeve/56588_server_handler branch September 29, 2025 18:04
rosstimothy pushed a commit that referenced this pull request Sep 29, 2025
* [mcp] server handler for streamable hTTP transport

* review comments round 1

* add comments and fix flaky test

Signed-off-by: Tim Ross <tim.ross@goteleport.com>
greedy52 added a commit that referenced this pull request Oct 23, 2025
* [mcp] server handler for streamable hTTP transport

* review comments round 1

* add comments and fix flaky test
github-merge-queue Bot pushed a commit that referenced this pull request Oct 27, 2025
* [MCP] server-side SSE support (#56051)

* MCP access part 12: server-side SSE support

* parse uri for determining transport type

* fix pointer, atomic, and parse error

� Conflicts:
�	lib/srv/mcp/server.go

* fix schema, etc

* switch to golang internal mcp sse parsing

* remove ParentCtx from logging

* fix build and address comments

* [mcp] refactor jwt token and app header rewrite logic (#58601)

* [mcp] mcputils for streamable http (#58764)

* [mcp] mcputils for streamable http

* fix flaky test

* use utils.ReadAtMost and fmt.Appendf

* add a marshal function for event

* fix spell

* [mcp] update audit events for streamable HTTP transport (#59155)

* [mcp] update audit events for streamable HTTP transport

* nolint for unused functions for now, they will be used in next PR

* [mcp] server handler for streamable HTTP transport (#59499)

* [mcp] server handler for streamable hTTP transport

* review comments round 1

* add comments and fix flaky test

* [mcp] bump mcp-go version (#59500)

* [mcp] bump mcp-go version

* fix IO transport by explicit start

* [mcp] add server prometheus metrics (#59773)

* [mcp] add server prometheus metrics

* remove TODO and nolint

* use counter where possible and limit known methods

* move reporting test to individual tests

* nolint for "cancelled"

* Fix an issue docker container launched by MCP commands are not removed sometimes (#59879)

* Fix an issue docker container launched by MCP commands are not removed sometimes

* switch to math/rand/v2

* add "tsh proxy mcp" command (#59968)

* [refactor] client.NewMCPServerDialer (#60020)

* [refactor] client.NewMCPServerDialer

* TestVerifyTLSCertLeafExpiry

* TestMatchResourcesByFilters

* fix typo

* fix lint

* mcputils for streamable HTTP transport conversion (#60024)

* mcputils for streamable HTTP transport conversion

* remove need of context from mcptest functions

* add test for notification

* [mcp] "tsh mcp connect" support for streamable HTTP (#60120)

* implement "tsh mcp connect" for streamable HTTP

* wait for 5s just to be conservative

* [mcp] Web UI and Teleport Connect adjustments for SSE and Streamable HTTP MCP servers (#60281)

* [mcp] Web UI and Teleport Connect adjustments for SSE and Streamable HTTP MCP servers

* review comments

* [mcp] fix some edge cases for streamable HTTP (#60286)

* [mcp] add JWT and rewrite headers support for SSE MCP servers (#60320)

* fix go.mod to match master

* fix lint and ut
rhammonds-teleport pushed a commit that referenced this pull request Nov 6, 2025
* [mcp] server handler for streamable hTTP transport

* review comments round 1

* add comments and fix flaky test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

application-access MCP MCP Server related no-changelog Indicates that a PR does not require a changelog entry size/lg tsh tsh - Teleport's command line tool for logging into nodes running Teleport.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants