diff --git a/dev-packages/node-integration-tests/suites/tracing/mcp-handler-exact-once/instrument.mjs b/dev-packages/node-integration-tests/suites/tracing/mcp-handler-exact-once/instrument.mjs new file mode 100644 index 000000000000..0b914a70f157 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/mcp-handler-exact-once/instrument.mjs @@ -0,0 +1,8 @@ +import * as Sentry from '@sentry/node'; +import { loggingTransport } from '@sentry-internal/node-integration-tests'; + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + tracesSampleRate: 1, + transport: loggingTransport, +}); diff --git a/dev-packages/node-integration-tests/suites/tracing/mcp-handler-exact-once/run.cjs b/dev-packages/node-integration-tests/suites/tracing/mcp-handler-exact-once/run.cjs new file mode 100644 index 000000000000..75a4f7c73efa --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/mcp-handler-exact-once/run.cjs @@ -0,0 +1,52 @@ +const assert = require('node:assert/strict'); + +module.exports = async function run({ Client, InMemoryTransport, McpServer, Sentry }) { + await Sentry.startSpan({ name: 'handler-regression' }, async span => { + const calls = { tool: 0, resource: 0, prompt: 0, existingResource: 0 }; + const failOnce = (name, result) => () => { + calls[name] += 1; + if (calls[name] === 1) { + throw new Error(`${name} failed`); + } + return result; + }; + const server = new McpServer({ name: 'handler-test-server', version: '1.0.0' }); + server.registerResource( + 'existingResource', + 'test://existing-resource', + {}, + failOnce('existingResource', { contents: [{ uri: 'test://existing-resource', text: 'unexpected retry' }] }), + ); + Sentry.wrapMcpServerWithSentry(server); + server.registerTool('tool', {}, failOnce('tool', { content: [{ type: 'text', text: 'unexpected retry' }] })); + server.registerResource( + 'resource', + 'test://resource', + {}, + failOnce('resource', { contents: [{ uri: 'test://resource', text: 'unexpected retry' }] }), + ); + server.registerPrompt( + 'prompt', + {}, + failOnce('prompt', { messages: [{ role: 'user', content: { type: 'text', text: 'unexpected retry' } }] }), + ); + const client = new Client({ name: 'handler-test-client', version: '1.0.0' }); + const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair(); + + try { + await Promise.all([server.connect(serverTransport), client.connect(clientTransport)]); + assert.deepEqual(await client.callTool({ name: 'tool', arguments: {} }), { + content: [{ type: 'text', text: 'tool failed' }], + isError: true, + }); + await assert.rejects(client.readResource({ uri: 'test://resource' }), /resource failed$/); + await assert.rejects(client.getPrompt({ name: 'prompt' }), /prompt failed$/); + await assert.rejects(client.readResource({ uri: 'test://existing-resource' }), /existingResource failed$/); + assert.deepEqual(calls, { tool: 1, resource: 1, prompt: 1, existingResource: 1 }); + } finally { + await client.close(); + await server.close(); + } + span.setAttribute('test.mcp.handlers_verified', 4); + }); +}; diff --git a/dev-packages/node-integration-tests/suites/tracing/mcp-handler-exact-once/scenario-sdk-v1.mjs b/dev-packages/node-integration-tests/suites/tracing/mcp-handler-exact-once/scenario-sdk-v1.mjs new file mode 100644 index 000000000000..8f4c1cc94aa6 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/mcp-handler-exact-once/scenario-sdk-v1.mjs @@ -0,0 +1,7 @@ +import { Client } from '@modelcontextprotocol/sdk/client/index.js'; +import { InMemoryTransport } from '@modelcontextprotocol/sdk/inMemory.js'; +import { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js'; +import * as Sentry from '@sentry/node'; +import run from './run.cjs'; + +run({ Client, InMemoryTransport, McpServer, Sentry }); diff --git a/dev-packages/node-integration-tests/suites/tracing/mcp-handler-exact-once/scenario-sdk-v2.mjs b/dev-packages/node-integration-tests/suites/tracing/mcp-handler-exact-once/scenario-sdk-v2.mjs new file mode 100644 index 000000000000..b3e311806320 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/mcp-handler-exact-once/scenario-sdk-v2.mjs @@ -0,0 +1,6 @@ +import { Client } from '@modelcontextprotocol/client'; +import { InMemoryTransport, McpServer } from '@modelcontextprotocol/server'; +import * as Sentry from '@sentry/node'; +import run from './run.cjs'; + +run({ Client, InMemoryTransport, McpServer, Sentry }); diff --git a/dev-packages/node-integration-tests/suites/tracing/mcp-handler-exact-once/test.ts b/dev-packages/node-integration-tests/suites/tracing/mcp-handler-exact-once/test.ts new file mode 100644 index 000000000000..61c403f602ce --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/mcp-handler-exact-once/test.ts @@ -0,0 +1,42 @@ +import type { SerializedStreamedSpanContainer } from '@sentry/core'; +import { describe, expect } from 'vitest'; +import { createEsmAndCjsTests } from '../../../utils/runner'; + +describe.each(['v1', 'v2'])('MCP TypeScript SDK %s', sdk => { + createEsmAndCjsTests( + __dirname, + `scenario-sdk-${sdk}.mjs`, + 'instrument.mjs', + (createTestRunner, test) => { + test('preserves handler errors without repeating their side effects', async () => { + let root: SerializedStreamedSpanContainer['items'][number] | undefined; + + await createTestRunner() + .unordered() + .expect({ + event: event => { + expect(event.exception?.values).toHaveLength(1); + expect(event.exception?.values?.[0]?.value).toBe('tool failed'); + expect(event.exception?.values?.[0]?.mechanism?.type).toBe('auto.ai.mcp_server'); + }, + }) + .expect({ + span: container => { + const segment = container.items.find(item => item.is_segment && item.name === 'handler-regression'); + expect(segment?.name).toBe('handler-regression'); + root = segment; + }, + }) + .start() + .completed(); + + expect(root?.status).toBe('ok'); + expect(root?.attributes['test.mcp.handlers_verified']).toEqual({ type: 'integer', value: 4 }); + }); + }, + { + additionalDependencies: sdk === 'v1' ? { '@modelcontextprotocol/sdk': '1.30.0' } : undefined, + copyPaths: ['run.cjs'], + }, + ); +}); diff --git a/packages/core/src/integrations/mcp-server/handlers.ts b/packages/core/src/integrations/mcp-server/handlers.ts index 0f6bf191aa38..88fc5819ddab 100644 --- a/packages/core/src/integrations/mcp-server/handlers.ts +++ b/packages/core/src/integrations/mcp-server/handlers.ts @@ -5,8 +5,6 @@ * and prompt handlers. */ -import { DEBUG_BUILD } from '../../debug-build'; -import { debug } from '../../utils/debug-logger'; import { isObjectLike } from '../../utils/is'; import { fill } from '../../utils/object'; import { captureError } from './errorCapture'; @@ -44,46 +42,21 @@ function wrapMethodHandler(serverInstance: MCPServerInstance, methodName: keyof function createWrappedHandler(originalHandler: MCPHandler, methodName: keyof MCPServerInstance, handlerName: string) { return function (this: unknown, ...handlerArgs: unknown[]): unknown { try { - return createErrorCapturingHandler.call(this, originalHandler, methodName, handlerName, handlerArgs); - } catch (error) { - DEBUG_BUILD && debug.warn('MCP handler wrapping failed:', error); - return originalHandler.apply(this, handlerArgs); - } - }; -} + const result = originalHandler.apply(this, handlerArgs); -/** - * Creates an error-capturing wrapper for handler execution - * @internal - * @param originalHandler - Original handler function - * @param methodName - MCP method name - * @param handlerName - Handler identifier - * @param handlerArgs - Handler arguments - * @param extraHandlerData - Additional handler context - * @returns Handler execution result - */ -function createErrorCapturingHandler( - this: MCPServerInstance, - originalHandler: MCPHandler, - methodName: keyof MCPServerInstance, - handlerName: string, - handlerArgs: unknown[], -): unknown { - try { - const result = originalHandler.apply(this, handlerArgs); + if (isObjectLike(result) && typeof (result as { then?: unknown }).then === 'function') { + return Promise.resolve(result).catch(error => { + captureHandlerError(error, methodName, handlerName); + throw error; + }); + } - if (isObjectLike(result) && typeof (result as { then?: unknown }).then === 'function') { - return Promise.resolve(result).catch(error => { - captureHandlerError(error, methodName, handlerName); - throw error; - }); + return result; + } catch (error) { + captureHandlerError(error as Error, methodName, handlerName); + throw error; } - - return result; - } catch (error) { - captureHandlerError(error as Error, methodName, handlerName); - throw error; - } + }; } /** diff --git a/packages/core/test/lib/integrations/mcp-server/mcpServerErrorCapture.test.ts b/packages/core/test/lib/integrations/mcp-server/mcpServerErrorCapture.test.ts index df3544be9148..8fc9ca8b87fc 100644 --- a/packages/core/test/lib/integrations/mcp-server/mcpServerErrorCapture.test.ts +++ b/packages/core/test/lib/integrations/mcp-server/mcpServerErrorCapture.test.ts @@ -3,6 +3,7 @@ import * as currentScopes from '../../../../src/currentScopes'; import * as exports from '../../../../src/exports'; import { wrapMcpServerWithSentry } from '../../../../src/integrations/mcp-server'; import { captureError } from '../../../../src/integrations/mcp-server/errorCapture'; +import type { MCPHandler } from '../../../../src/integrations/mcp-server/types'; import { createMockClient, createMockMcpServer } from './testUtils'; describe('MCP Server Error Capture', () => { @@ -145,41 +146,86 @@ describe('MCP Server Error Capture', () => { }); describe('Error Capture Integration', () => { - let mockMcpServer: ReturnType; let wrappedMcpServer: ReturnType; + let registeredHandler: MCPHandler; beforeEach(() => { - mockMcpServer = createMockMcpServer(); + captureExceptionSpy.mockReturnValue('event-id'); + const mockMcpServer = createMockMcpServer(); + mockMcpServer.tool.mockImplementation((_name: string, handler: MCPHandler) => { + registeredHandler = handler; + }); wrappedMcpServer = wrapMcpServerWithSentry(mockMcpServer); }); - it('should capture tool execution errors and continue normal flow', async () => { + it('should not retry a handler after a synchronous error', () => { const toolError = new Error('Tool execution failed'); - const mockToolHandler = vi.fn().mockRejectedValue(toolError); + const mockToolHandler = vi + .fn() + .mockImplementationOnce(() => { + throw toolError; + }) + .mockReturnValue({ content: [] }); + wrappedMcpServer.tool('failing-tool', mockToolHandler); + + expect(() => registeredHandler()).toThrow(toolError); + + expect(mockToolHandler).toHaveBeenCalledTimes(1); + expect(captureExceptionSpy).toHaveBeenCalledExactlyOnceWith(toolError, { + mechanism: { + type: 'auto.ai.mcp_server', + handled: false, + data: { error_type: 'tool_execution', tool_name: 'failing-tool' }, + }, + }); + }); + it('should capture and rethrow asynchronous errors without retrying', async () => { + const toolError = new Error('Tool execution failed'); + const mockToolHandler = vi.fn().mockRejectedValue(toolError); wrappedMcpServer.tool('failing-tool', mockToolHandler); - await expect(mockToolHandler({ input: 'test' }, { requestId: 'req-123', sessionId: 'sess-456' })).rejects.toThrow( - 'Tool execution failed', - ); + await expect(registeredHandler()).rejects.toBe(toolError); - // The capture should be set up correctly - expect(captureExceptionSpy).toHaveBeenCalledTimes(0); // No capture yet since we didn't call the wrapped handler + expect(mockToolHandler).toHaveBeenCalledTimes(1); + expect(captureExceptionSpy).toHaveBeenCalledExactlyOnceWith(toolError, { + mechanism: { + type: 'auto.ai.mcp_server', + handled: false, + data: { error_type: 'tool_execution', tool_name: 'failing-tool' }, + }, + }); }); - it('should handle Sentry capture errors gracefully', async () => { + it('should not retry a failing handler when Sentry capture also throws', () => { captureExceptionSpy.mockImplementation(() => { throw new Error('Sentry error'); }); - - // Test that the capture function itself doesn't throw const toolError = new Error('Tool execution failed'); - const mockToolHandler = vi.fn().mockRejectedValue(toolError); - + const mockToolHandler = vi.fn(() => { + throw toolError; + }); wrappedMcpServer.tool('failing-tool', mockToolHandler); - // The error capture should be resilient to Sentry errors - expect(captureExceptionSpy).toHaveBeenCalledTimes(0); + expect(() => registeredHandler()).toThrow(toolError); + + expect(mockToolHandler).toHaveBeenCalledTimes(1); + expect(captureExceptionSpy).toHaveBeenCalledTimes(1); + }); + + it('should preserve the handler receiver, arguments, and return value', () => { + const result = { content: [] }; + const mockToolHandler = vi.fn().mockReturnValue(result); + const receiver = {}; + const args = { input: 'test' }; + const extra = { requestId: 'req-123', sessionId: 'sess-456' }; + wrappedMcpServer.tool('successful-tool', mockToolHandler); + + expect(registeredHandler.call(receiver, args, extra)).toBe(result); + + expect(mockToolHandler).toHaveBeenCalledExactlyOnceWith(args, extra); + expect(mockToolHandler.mock.contexts).toEqual([receiver]); + expect(captureExceptionSpy).not.toHaveBeenCalled(); }); }); });