Skip to content

Commit 389ab1f

Browse files
authored
fix(core): use sessionId for MCP transport correlation (#19172)
## Summary Fixes MCP server instrumentation not recording events for wrapper transport patterns (like `NodeStreamableHTTPServerTransport` which wraps `WebStandardStreamableHTTPServerTransport`). The root cause: wrapper transports proxy `onmessage`/`send` via getters/setters, causing different `this` values. The previous `WeakMap<transport, ...>` correlation couldn't match requests to responses. This changes to `Map<sessionId, ...>` correlation which works regardless of transport object reference. ## Changes - Changed `correlation.ts` to use sessionId-based Maps instead of transport-based WeakMaps - Changed `sessionManagement.ts` to use sessionId-based session lookup - Added unit tests with mock wrapper transport utility - Added E2E tests for StreamableHTTP transport across node-express, node-express-v5, and tsx-express ## Test Plan - Unit tests: `cd packages/core && yarn test` (2010 tests pass) - E2E tests: All pass - node-express: 14/14 - node-express-v5: 12/12 - tsx-express: 10/10 --- - [x] If you've added code that should be tested, please add tests. - [x] Ensure your code lints and the test suite passes (`yarn lint`) & (`yarn test`). - [x] Link an issue if there is one related to your pull request. If no issue is linked, one will be auto-generated and linked. Closes #19233 Thanks @gotenxds for reporting and debugging this issue!
1 parent 422f159 commit 389ab1f

13 files changed

Lines changed: 1149 additions & 32 deletions

File tree

dev-packages/e2e-tests/test-applications/node-express-v5/package.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@
1111
"test:assert": "pnpm test"
1212
},
1313
"dependencies": {
14-
"@modelcontextprotocol/sdk": "^1.10.2",
14+
"@modelcontextprotocol/sdk": "^1.26.0",
1515
"@sentry/node": "latest || *",
1616
"@trpc/server": "10.45.4",
1717
"@trpc/client": "10.45.4",

dev-packages/e2e-tests/test-applications/node-express-v5/src/mcp.ts

Lines changed: 137 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,16 @@
1+
import { randomUUID } from 'node:crypto';
12
import express from 'express';
23
import { McpServer, ResourceTemplate } from '@modelcontextprotocol/sdk/server/mcp.js';
34
import { SSEServerTransport } from '@modelcontextprotocol/sdk/server/sse.js';
5+
import { StreamableHTTPServerTransport } from '@modelcontextprotocol/sdk/server/streamableHttp.js';
46
import { z } from 'zod';
57
import { wrapMcpServerWithSentry } from '@sentry/node';
68

9+
// Helper to check if request is an initialize request (compatible with all MCP SDK versions)
10+
function isInitializeRequest(body: unknown): boolean {
11+
return typeof body === 'object' && body !== null && (body as { method?: string }).method === 'initialize';
12+
}
13+
714
const mcpRouter = express.Router();
815

916
const server = wrapMcpServerWithSentry(
@@ -61,4 +68,134 @@ mcpRouter.post('/messages', async (req, res) => {
6168
}
6269
});
6370

71+
// =============================================================================
72+
// Streamable HTTP Transport Endpoints
73+
// This uses StreamableHTTPServerTransport which wraps WebStandardStreamableHTTPServerTransport
74+
// and exercises the wrapper transport pattern that was fixed in the sessionId-based correlation
75+
// See: https://github.com/getsentry/sentry-mcp/issues/767
76+
// =============================================================================
77+
78+
// Create a separate wrapped server for streamable HTTP (to test independent of SSE)
79+
const streamableServer = wrapMcpServerWithSentry(
80+
new McpServer({
81+
name: 'Echo-Streamable',
82+
version: '1.0.0',
83+
}),
84+
);
85+
86+
// Register the same handlers on the streamable server
87+
streamableServer.resource(
88+
'echo',
89+
new ResourceTemplate('echo://{message}', { list: undefined }),
90+
async (uri, { message }) => ({
91+
contents: [
92+
{
93+
uri: uri.href,
94+
text: `Resource echo: ${message}`,
95+
},
96+
],
97+
}),
98+
);
99+
100+
streamableServer.tool('echo', { message: z.string() }, async ({ message }) => {
101+
return {
102+
content: [{ type: 'text', text: `Tool echo: ${message}` }],
103+
};
104+
});
105+
106+
streamableServer.prompt('echo', { message: z.string() }, ({ message }) => ({
107+
messages: [
108+
{
109+
role: 'user',
110+
content: {
111+
type: 'text',
112+
text: `Please process this message: ${message}`,
113+
},
114+
},
115+
],
116+
}));
117+
118+
// Map to store streamable transports by session ID
119+
const streamableTransports: Record<string, StreamableHTTPServerTransport> = {};
120+
121+
// POST endpoint for streamable HTTP (handles both initialization and subsequent requests)
122+
mcpRouter.post('/mcp', express.json(), async (req, res) => {
123+
const sessionId = req.headers['mcp-session-id'] as string | undefined;
124+
125+
try {
126+
let transport: StreamableHTTPServerTransport;
127+
128+
if (sessionId && streamableTransports[sessionId]) {
129+
// Reuse existing transport for session
130+
transport = streamableTransports[sessionId];
131+
} else if (!sessionId && isInitializeRequest(req.body)) {
132+
// New initialization request - create new transport
133+
transport = new StreamableHTTPServerTransport({
134+
sessionIdGenerator: () => randomUUID(),
135+
onsessioninitialized: sid => {
136+
// Store transport when session is initialized
137+
streamableTransports[sid] = transport;
138+
},
139+
});
140+
141+
// Clean up on close
142+
transport.onclose = () => {
143+
const sid = transport.sessionId;
144+
if (sid && streamableTransports[sid]) {
145+
delete streamableTransports[sid];
146+
}
147+
};
148+
149+
// Connect to server before handling request
150+
await streamableServer.connect(transport);
151+
await transport.handleRequest(req, res, req.body);
152+
return;
153+
} else {
154+
// Invalid request
155+
res.status(400).json({
156+
jsonrpc: '2.0',
157+
error: { code: -32000, message: 'Bad Request: No valid session ID provided' },
158+
id: null,
159+
});
160+
return;
161+
}
162+
163+
// Handle request with existing transport
164+
await transport.handleRequest(req, res, req.body);
165+
} catch (error) {
166+
console.error('Error handling streamable HTTP request:', error);
167+
if (!res.headersSent) {
168+
res.status(500).json({
169+
jsonrpc: '2.0',
170+
error: { code: -32603, message: 'Internal server error' },
171+
id: null,
172+
});
173+
}
174+
}
175+
});
176+
177+
// GET endpoint for SSE streams (server-initiated messages)
178+
mcpRouter.get('/mcp', async (req, res) => {
179+
const sessionId = req.headers['mcp-session-id'] as string | undefined;
180+
if (!sessionId || !streamableTransports[sessionId]) {
181+
res.status(400).send('Invalid or missing session ID');
182+
return;
183+
}
184+
185+
const transport = streamableTransports[sessionId];
186+
await transport.handleRequest(req, res);
187+
});
188+
189+
// DELETE endpoint for session termination
190+
mcpRouter.delete('/mcp', async (req, res) => {
191+
const sessionId = req.headers['mcp-session-id'] as string | undefined;
192+
if (!sessionId || !streamableTransports[sessionId]) {
193+
res.status(400).send('Invalid or missing session ID');
194+
return;
195+
}
196+
197+
const transport = streamableTransports[sessionId];
198+
await transport.handleRequest(req, res);
199+
});
200+
64201
export { mcpRouter };

dev-packages/e2e-tests/test-applications/node-express-v5/tests/mcp.test.ts

Lines changed: 133 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import { expect, test } from '@playwright/test';
22
import { waitForTransaction } from '@sentry-internal/test-utils';
33
import { Client } from '@modelcontextprotocol/sdk/client/index.js';
44
import { SSEClientTransport } from '@modelcontextprotocol/sdk/client/sse.js';
5+
import { StreamableHTTPClientTransport } from '@modelcontextprotocol/sdk/client/streamableHttp.js';
56

67
test('Should record transactions for mcp handlers', async ({ baseURL }) => {
78
const transport = new SSEClientTransport(new URL(`${baseURL}/sse`));
@@ -120,3 +121,135 @@ test('Should record transactions for mcp handlers', async ({ baseURL }) => {
120121
// TODO: When https://github.com/modelcontextprotocol/typescript-sdk/pull/358 is released check for trace id equality between the post transaction and the handler transaction
121122
});
122123
});
124+
125+
/**
126+
* Tests for StreamableHTTPServerTransport (wrapper transport pattern)
127+
*
128+
* StreamableHTTPServerTransport wraps WebStandardStreamableHTTPServerTransport via getters/setters.
129+
* This causes different `this` values in onmessage vs send, which was breaking span correlation.
130+
*
131+
* The fix uses sessionId as the correlation key instead of transport object reference.
132+
* This test verifies that spans are correctly recorded when using the wrapper transport.
133+
*
134+
* @see https://github.com/getsentry/sentry-mcp/issues/767
135+
*/
136+
test('Should record transactions for streamable HTTP transport (wrapper transport pattern)', async ({ baseURL }) => {
137+
const transport = new StreamableHTTPClientTransport(new URL(`${baseURL}/mcp`));
138+
139+
const client = new Client({
140+
name: 'test-client-streamable',
141+
version: '1.0.0',
142+
});
143+
144+
const initializeTransactionPromise = waitForTransaction('node-express-v5', transactionEvent => {
145+
return (
146+
transactionEvent.transaction === 'initialize' &&
147+
transactionEvent.contexts?.trace?.data?.['mcp.server.name'] === 'Echo-Streamable'
148+
);
149+
});
150+
151+
await client.connect(transport);
152+
153+
await test.step('initialize handshake', async () => {
154+
const initializeTransaction = await initializeTransactionPromise;
155+
expect(initializeTransaction).toBeDefined();
156+
expect(initializeTransaction.contexts?.trace?.op).toEqual('mcp.server');
157+
expect(initializeTransaction.contexts?.trace?.data?.['mcp.method.name']).toEqual('initialize');
158+
expect(initializeTransaction.contexts?.trace?.data?.['mcp.client.name']).toEqual('test-client-streamable');
159+
expect(initializeTransaction.contexts?.trace?.data?.['mcp.server.name']).toEqual('Echo-Streamable');
160+
// Verify it's using a StreamableHTTP transport (may be wrapper or inner depending on environment)
161+
expect(initializeTransaction.contexts?.trace?.data?.['mcp.transport']).toMatch(/StreamableHTTPServerTransport/);
162+
});
163+
164+
await test.step('tool handler (tests wrapper transport correlation)', async () => {
165+
// This is the critical test - without the sessionId fix, the span would not be completed
166+
// because onmessage and send see different transport instances (wrapper vs inner)
167+
const toolTransactionPromise = waitForTransaction('node-express-v5', transactionEvent => {
168+
const transport = transactionEvent.contexts?.trace?.data?.['mcp.transport'] as string | undefined;
169+
return transactionEvent.transaction === 'tools/call echo' && transport?.includes('StreamableHTTPServerTransport');
170+
});
171+
172+
const toolResult = await client.callTool({
173+
name: 'echo',
174+
arguments: {
175+
message: 'wrapper-transport-test',
176+
},
177+
});
178+
179+
expect(toolResult).toMatchObject({
180+
content: [
181+
{
182+
text: 'Tool echo: wrapper-transport-test',
183+
type: 'text',
184+
},
185+
],
186+
});
187+
188+
const toolTransaction = await toolTransactionPromise;
189+
expect(toolTransaction).toBeDefined();
190+
expect(toolTransaction.contexts?.trace?.op).toEqual('mcp.server');
191+
expect(toolTransaction.contexts?.trace?.data?.['mcp.method.name']).toEqual('tools/call');
192+
expect(toolTransaction.contexts?.trace?.data?.['mcp.tool.name']).toEqual('echo');
193+
// This attribute proves the span was completed with results (sessionId correlation worked)
194+
expect(toolTransaction.contexts?.trace?.data?.['mcp.tool.result.content_count']).toEqual(1);
195+
});
196+
197+
await test.step('resource handler', async () => {
198+
const resourceTransactionPromise = waitForTransaction('node-express-v5', transactionEvent => {
199+
const transport = transactionEvent.contexts?.trace?.data?.['mcp.transport'] as string | undefined;
200+
return (
201+
transactionEvent.transaction === 'resources/read echo://streamable-test' &&
202+
transport?.includes('StreamableHTTPServerTransport')
203+
);
204+
});
205+
206+
const resourceResult = await client.readResource({
207+
uri: 'echo://streamable-test',
208+
});
209+
210+
expect(resourceResult).toMatchObject({
211+
contents: [{ text: 'Resource echo: streamable-test', uri: 'echo://streamable-test' }],
212+
});
213+
214+
const resourceTransaction = await resourceTransactionPromise;
215+
expect(resourceTransaction).toBeDefined();
216+
expect(resourceTransaction.contexts?.trace?.op).toEqual('mcp.server');
217+
expect(resourceTransaction.contexts?.trace?.data?.['mcp.method.name']).toEqual('resources/read');
218+
});
219+
220+
await test.step('prompt handler', async () => {
221+
const promptTransactionPromise = waitForTransaction('node-express-v5', transactionEvent => {
222+
const transport = transactionEvent.contexts?.trace?.data?.['mcp.transport'] as string | undefined;
223+
return (
224+
transactionEvent.transaction === 'prompts/get echo' && transport?.includes('StreamableHTTPServerTransport')
225+
);
226+
});
227+
228+
const promptResult = await client.getPrompt({
229+
name: 'echo',
230+
arguments: {
231+
message: 'streamable-prompt',
232+
},
233+
});
234+
235+
expect(promptResult).toMatchObject({
236+
messages: [
237+
{
238+
content: {
239+
text: 'Please process this message: streamable-prompt',
240+
type: 'text',
241+
},
242+
role: 'user',
243+
},
244+
],
245+
});
246+
247+
const promptTransaction = await promptTransactionPromise;
248+
expect(promptTransaction).toBeDefined();
249+
expect(promptTransaction.contexts?.trace?.op).toEqual('mcp.server');
250+
expect(promptTransaction.contexts?.trace?.data?.['mcp.method.name']).toEqual('prompts/get');
251+
});
252+
253+
// Clean up - close the client connection
254+
await client.close();
255+
});

dev-packages/e2e-tests/test-applications/node-express/package.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@
1111
"test:assert": "pnpm test"
1212
},
1313
"dependencies": {
14-
"@modelcontextprotocol/sdk": "^1.10.2",
14+
"@modelcontextprotocol/sdk": "^1.26.0",
1515
"@sentry/node": "latest || *",
1616
"@trpc/server": "10.45.4",
1717
"@trpc/client": "10.45.4",

0 commit comments

Comments
 (0)