feat: add streamable http [MCP-42] #361

fmenezes · 2025-07-14T15:57:54Z

Proposed changes

Introduces Streamable HTTP transport; No authentication or authorization was implemented.

Note: this is a feature branch, every single individual change was approved on each PR.

Checklist

I have signed the MongoDB CLA

coveralls · 2025-07-14T16:06:04Z

Pull Request Test Coverage Report for Build 16422846497

Details

238 of 402 (59.2%) changed or added relevant lines in 10 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-1.6%) to 80.243%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/server.ts	24	36	66.67%
src/transports/stdio.ts	8	21	38.1%
src/index.ts	0	28	0.0%
src/common/sessionStore.ts	51	92	55.43%
src/transports/streamableHttp.ts	88	158	55.7%

Totals
Change from base Build 16373544917:	-1.6%
Covered Lines:	3099
Relevant Lines:	3826

💛 - Coveralls

Copilot

Pull Request Overview

This PR introduces streamable HTTP transport functionality to the MongoDB MCP Server, allowing it to run as an HTTP server in addition to the existing stdio transport. The implementation includes session management with timeout handling and configurable logging options.

Adds StreamableHttpRunner with Express-based HTTP server and session management
Refactors transport layer with base class and configurable runner selection
Introduces comprehensive timeout management for HTTP sessions with cleanup

Reviewed Changes

Copilot reviewed 24 out of 25 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/transports/streamableHttp.ts	New HTTP transport implementation with session management and Express server
src/transports/stdio.ts	Refactored stdio transport into runner pattern with base class
src/transports/base.ts	Abstract base class for transport runners
src/common/timeoutManager.ts	New timeout management utility for session cleanup
src/common/sessionStore.ts	HTTP session storage and lifecycle management
src/common/config.ts	Enhanced configuration with HTTP and logging options
src/server.ts	Refactored server initialization and validation logic
src/index.ts	Simplified main entry point with transport runner selection
tests/integration/transports/streamableHttp.test.ts	Integration tests for HTTP transport
tests/integration/transports/stdio.test.ts	Integration tests for stdio transport
tests/unit/common/timeoutManager.test.ts	Unit tests for timeout manager

Copilot · 2025-07-21T11:00:30Z

src/common/timeoutManager.ts

+        if (this.callback) {
+            try {
+                await this.callback();
+            } catch (error: unknown) {
+                this.onerror?.(error);
+            }


The runCallback method checks if 'this.callback' exists, but the callback is marked as 'readonly' and provided in the constructor, so this check is unnecessary and adds complexity.

Suggested change

if (this.callback) {

try {

await this.callback();

} catch (error: unknown) {

this.onerror?.(error);

}

try {

await this.callback();

} catch (error: unknown) {

this.onerror?.(error);

Copilot · 2025-07-21T11:00:30Z

src/common/sessionStore.ts

+        const logger = new McpLogger(session.mcpServer);
+        logger.info(


Creating a new McpLogger instance for each notification is inefficient. Consider creating the logger once and reusing it, or passing it as a dependency.

Suggested change

const logger = new McpLogger(session.mcpServer);

logger.info(

session.logger.info(

agreed, we should create a logger and store it inside the our session map

Copilot · 2025-07-21T11:00:30Z

src/common/sessionStore.ts

+            const logger = new McpLogger(mcpServer);
+            logger.info(


Creating a new McpLogger instance inside the timeout callback is inefficient and could be problematic if called multiple times. Consider creating the logger once and reusing it.

Suggested change

const logger = new McpLogger(mcpServer);

logger.info(

this.sessions[sessionId].logger.info(

gagik

Going to be focused on some technical writing today but just some initial review on first part of the changes

src/common/sessionStore.ts

gagik · 2025-07-21T11:17:17Z

src/common/sessionStore.ts

+        const logger = new McpLogger(session.mcpServer);
+        logger.info(


agreed, we should create a logger and store it inside the our session map

gagik · 2025-07-21T11:19:57Z

src/common/sessionStore.ts

+            const logger = new McpLogger(mcpServer);
+            logger.info(


gagik · 2025-07-21T11:25:52Z

src/common/timeoutManager.ts

+     * Resets the timeout.
+     */
+    reset() {


Suggested change

* Resets the timeout.

*/

reset() {

* Restarts the timeout.

*/

restart() {

Just to better differentiate from clear as it's not clear that the initial state has the timeout started already

gagik · 2025-07-21T11:26:43Z

src/common/timeoutManager.ts

+     * Runs the callback function.
+     */
+    private async runCallback() {
+        if (this.callback) {


this.callback is always defined right? this is not needed

src/common/timeoutManager.ts

gagik · 2025-07-21T11:45:23Z

src/common/timeoutManager.ts

+        if (timeoutMS <= 0) {
+            throw new Error("timeoutMS must be greater than 0");
+        }
+        this.reset();


we should not have side-effects in constructors. I think this whole helper can be better represented as a single function.

type ManagedTimeout = { cancel: () => void; restart: () => void; }; export function setManagedTimeout( callback: () => Promise<void> | void, timeoutMS: number, { onError }: { onError?: (error: unknown) => void } ) { const wrappedCallback = () => { try { await this.callback(); } catch (error: unknown) { onError?.(error); } }; let timeout = setTimeout(wrappedCallback, timeoutMS); const cancel = () => { cancelTimeout(timeout); timeout = undefined; }; const restart = () => { cancel(); timeout = setTimeout(wrappedCallback, timeoutMS); } return { cancel, restart }; }

gagik · 2025-07-21T11:49:01Z

src/server.ts

@@ -186,6 +189,35 @@ export class Server {
    }

    private async validateConfig(): Promise<void> {
+        const transport = this.userConfig.transport as string;


are these type casts needed?

That is the way I've found to validate it.

The problem here is as far as we are concerned, the possible values are predefined, so the literal types make sense, but the inputs are cli args or env vars, which are basically strings that can contain any values.

ah okay I see

if (transport !== "http" && transport !== "stdio") { throw new Error(`Invalid transport: ${String(transport)}`); }

could prevent it but yeah honestly isn't much better.

We can also use zod for assertions here. We can define a schema for the config and just let it assert, it'd do the error messages well. But can be done later too

fmenezes · 2025-07-21T13:41:48Z

@gagik I'm addressing all of these at #386, if you're happy with the changes, would you approve it?

thanks!

fmenezes · 2025-07-21T16:45:54Z

@gagik I've merged the PR according to your comments, would take another look?

gagik

Looks good, nice work. Just a couple questions and structure / style ideas.

src/index.ts

gagik · 2025-07-22T08:15:39Z

src/server.ts

@@ -66,15 +67,17 @@ export class Server {
            return existingHandler(request, extra);
        });

-        const containerEnv = await detectContainerEnv();
-
-        if (containerEnv) {


do we not need to do this setting anymore?

yes, for our telemetry

I meant I don't see it in the new code, is this being done somewhere else?

gagik · 2025-07-22T08:20:36Z

src/server.ts

@@ -186,6 +189,35 @@ export class Server {
    }

    private async validateConfig(): Promise<void> {
+        const transport = this.userConfig.transport as string;


ah okay I see

if (transport !== "http" && transport !== "stdio") { throw new Error(`Invalid transport: ${String(transport)}`); }

could prevent it but yeah honestly isn't much better.

We can also use zod for assertions here. We can define a schema for the config and just let it assert, it'd do the error messages well. But can be done later too

gagik · 2025-07-22T08:27:37Z

src/transports/streamableHttp.ts

+    return (req: express.Request, res: express.Response, next: express.NextFunction) => {
+        fn(req, res, next).catch((error) => {
+            logger.error(
+                LogId.streamableHttpTransportRequestFailure,
+                "streamableHttpTransport",
+                `Error handling request: ${error instanceof Error ? error.message : String(error)}`
+            );
+            res.status(400).json({
+                jsonrpc: "2.0",
+                error: {
+                    code: JSON_RPC_ERROR_CODE_PROCESSING_REQUEST_FAILED,
+                    message: `failed to handle request`,
+                    data: error instanceof Error ? error.message : String(error),
+                },
+            });
+        });
+    };


Suggested change

return (req: express.Request, res: express.Response, next: express.NextFunction) => {

fn(req, res, next).catch((error) => {

logger.error(

LogId.streamableHttpTransportRequestFailure,

"streamableHttpTransport",

`Error handling request: ${error instanceof Error ? error.message : String(error)}`

);

res.status(400).json({

jsonrpc: "2.0",

error: {

code: JSON_RPC_ERROR_CODE_PROCESSING_REQUEST_FAILED,

message: `failed to handle request`,

data: error instanceof Error ? error.message : String(error),

},

});

});

};

return async (req: express.Request, res: express.Response, next: express.NextFunction) {

try {

await fn(req, res, next);

} catch (error) {

logger.error(

LogId.streamableHttpTransportRequestFailure,

"streamableHttpTransport",

`Error handling request: ${error instanceof Error ? error.message : String(error)}`

);

res.status(400).json({

jsonrpc: "2.0",

error: {

code: JSON_RPC_ERROR_CODE_PROCESSING_REQUEST_FAILED,

message: `failed to handle request`,

data: error instanceof Error ? error.message : String(error),

},

});

};

};

nit: just prefer async syntax wherever possible

The problem here is that Express.js does not work well with async middlewares. This is about making sure errors are still caught in Express.js.

but this is still an async middleware right? it returns a promise

it does not return a promise it acts as a callback, if fn() which returns a promise fails I use .catch to treat it.

if I change to async function ... there is a chance of it throwing and we never respond.

in essence, this converts promise back to callback to make Express.js happy

gagik · 2025-07-22T08:28:48Z

src/transports/streamableHttp.ts

+const JSON_RPC_ERROR_CODE_SESSION_NOT_FOUND = -32003;
+const JSON_RPC_ERROR_CODE_INVALID_REQUEST = -32004;
+
+function promiseHandler(


Suggested change

function promiseHandler(

function withErrorHandling(

nit: I assume that's the purpose of it?

gagik · 2025-07-22T08:53:16Z

src/transports/streamableHttp.ts

+        app.enable("trust proxy"); // needed for reverse proxy support
+        app.use(express.json());
+
+        const handleRequest = async (req: express.Request, res: express.Response) => {


Suggested change

const handleRequest = async (req: express.Request, res: express.Response) => {

const handleSessionRequest = async (req: express.Request, res: express.Response) => {

just to make it clearer this is for session-specific requests.
maybe we should move this to a private class method along with the general request handler?

gagik · 2025-07-22T09:04:28Z

src/transports/streamableHttp.ts

+                const sessionId = req.headers["mcp-session-id"];
+                if (sessionId) {
+                    await handleRequest(req, res);
+                    return;
+                }
+
+                if (!isInitializeRequest(req.body)) {
+                    res.status(400).json({
+                        jsonrpc: "2.0",
+                        error: {
+                            code: JSON_RPC_ERROR_CODE_INVALID_REQUEST,
+                            message: `invalid request`,
+                        },
+                    });
+                    return;
+                }
+
+                const server = this.setupServer(this.userConfig);
+                const transport = new StreamableHTTPServerTransport({
+                    sessionIdGenerator: () => randomUUID().toString(),
+                    onsessioninitialized: (sessionId) => {
+                        this.sessionStore.setSession(sessionId, transport, server.mcpServer);
+                    },
+                    onsessionclosed: async (sessionId) => {
+                        try {
+                            await this.sessionStore.closeSession(sessionId, false);
+                        } catch (error) {
+                            logger.error(
+                                LogId.streamableHttpTransportSessionCloseFailure,
+                                "streamableHttpTransport",
+                                `Error closing session: ${error instanceof Error ? error.message : String(error)}`
+                            );
+                        }
+                    },
+                });
+
+                transport.onclose = () => {
+                    server.close().catch((error) => {
+                        logger.error(


nit: maybe worth moving this to a private class method? not sure what would be a good name but something to differnetiate from handleSessionRequest for example. Just to make this start function define the paths and individual methods to handle them

I don't think I follow? you want to move the logger.error or server.close? I want the close of http transport to trigger the server to stop listening on port and gracefully shutdown if possible.

ah no this isn't about this line, I just meant we can consider moving the handler into its own named function as well to make the start function not nest too much. purely stylistic nit. I don't mind it like this either

fmenezes · 2025-07-22T15:09:02Z

@gagik I'm merging as is and will address points in a follow up pr

feat: add streamable http [MCP-55] (#359)

ff982e0

test: add streamable http tests [MCP-60] (#362)

2c21af3

fmenezes changed the title ~~feat: add streamable http [MCP-43]~~ feat: add streamable http [MCP-42] Jul 15, 2025

Merge branch 'main' into MCP-42

17b149c

fmenezes force-pushed the MCP-42 branch from c86dac0 to 17b149c Compare July 15, 2025 14:17

fmenezes added 5 commits July 15, 2025 18:24

refactor: move http server (#377)

db23253

Merge branch 'main' into MCP-42

b7c616a

feat: add session management for streamableHttp [MCP-52] (#379)

ac40e8a

chore: add transport to telemetry [MCP-62] (#382)

427663f

Merge branch 'main' into MCP-42

5058fa6

fmenezes mentioned this pull request Jul 18, 2025

feat: add HTTP/SSE transport support and document usage in Quick Start #257

Closed

feat: add client idle timeout [MCP-57] (#383)

1297ed9

fmenezes marked this pull request as ready for review July 21, 2025 10:59

Copilot AI review requested due to automatic review settings July 21, 2025 10:59

fmenezes requested a review from a team as a code owner July 21, 2025 10:59

Copilot AI reviewed Jul 21, 2025

View reviewed changes

blva requested a review from gagik July 21, 2025 11:02

gagik requested changes Jul 21, 2025

View reviewed changes

fmenezes added a commit that referenced this pull request Jul 21, 2025

chore: address comments from #361

7d3acf7

fmenezes mentioned this pull request Jul 21, 2025

chore: address comments from #361 #386

Merged

1 task

chore: address comments from #361 (#386)

3ba8a4a

gagik approved these changes Jul 22, 2025

View reviewed changes

fmenezes merged commit f8e5000 into main Jul 22, 2025
16 of 17 checks passed

fmenezes deleted the MCP-42 branch July 22, 2025 15:09

fmenezes added a commit that referenced this pull request Jul 22, 2025

chore: address more comments from #361

502ddf7

fmenezes mentioned this pull request Jul 22, 2025

chore: address more comments from #361 #389

Merged

1 task

fmenezes added a commit that referenced this pull request Jul 22, 2025

chore: address more comments from #361 (#389)

fdea0df

		const logger = new McpLogger(session.mcpServer);
		logger.info(

	const logger = new McpLogger(session.mcpServer);
	logger.info(
	session.logger.info(

	const logger = new McpLogger(mcpServer);
	logger.info(
	this.sessions[sessionId].logger.info(

	const handleRequest = async (req: express.Request, res: express.Response) => {
	const handleSessionRequest = async (req: express.Request, res: express.Response) => {

feat: add streamable http [MCP-42] #361

feat: add streamable http [MCP-42] #361

Uh oh!

Conversation

fmenezes commented Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed changes

Checklist

Uh oh!

coveralls commented Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 16422846497

Details

💛 - Coveralls

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gagik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fmenezes commented Jul 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fmenezes commented Jul 21, 2025

Uh oh!

gagik left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gagik Jul 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

fmenezes commented Jul 14, 2025 •

edited

Loading

coveralls commented Jul 14, 2025 •

edited

Loading

fmenezes commented Jul 21, 2025 •

edited

Loading

gagik left a comment •

edited

Loading

gagik Jul 22, 2025 •

edited

Loading

gagik Jul 22, 2025 •

edited

Loading