Skip to content

fix(mcp-server): Add defensive patches for Transport edge cases #17291

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 15 commits into
base: develop
Choose a base branch
from

Conversation

codyde
Copy link

@codyde codyde commented Aug 2, 2025

Problem

The Sentry MCP server instrumentation has several edge cases that can cause runtime errors when working with StreamableHTTPServerTransport and similar transport implementations. These issues occur during:

  1. Transport initialization - when transport constructors are undefined/null
  2. Session establishment - when sessionId is undefined during MCP initialization phases
  3. Span correlation - when transport objects cannot be used as WeakMap keys
  4. Type validation - when invalid transport objects are passed to correlation functions

Solution

This PR adds defensive patches to handle these edge cases gracefully while preserving existing functionality and maintaining backward compatibility.

Changes Made

1. Transport Constructor Null Checks (attributeExtraction.ts)

export function getTransportTypes(transport: MCPTransport): { mcpTransport: string; networkTransport: string } {
  // Handle undefined transport gracefully while preserving type detection
  if (\!transport || \!transport.constructor) {
    return { mcpTransport: 'unknown', networkTransport: 'unknown' };
  }
  const transportName = transport.constructor.name?.toLowerCase() || '';
  // ... rest of function
}

2. Graceful sessionId Handling (attributeExtraction.ts)

export function buildTransportAttributes(
  transport: MCPTransport,
  extra?: ExtraHandlerData,
): Record<string, string | number> {
  // Gracefully handle undefined sessionId during MCP initialization
  // Respects client-provided sessions and waits for proper session establishment
  const sessionId = transport && 'sessionId' in transport ? transport.sessionId : undefined;
  // ... rest of function
}

3. WeakMap Correlation Fallback System (correlation.ts)

const transportToSpanMap = new WeakMap<MCPTransport, Map<RequestId, RequestSpanMapValue>>();

/**
 * Fallback span map for invalid transport objects
 * @internal Used when transport objects cannot be used as WeakMap keys
 */
const fallbackSpanMap = new Map<RequestId, RequestSpanMapValue>();

function getOrCreateSpanMap(transport: MCPTransport): Map<RequestId, RequestSpanMapValue> {
  // Handle invalid transport values for WeakMap while preserving correlation
  if (\!transport || typeof transport \!== 'object') {
    // Return persistent fallback Map to maintain correlation across calls
    return fallbackSpanMap;
  }
  // ... rest of function
}

Testing

Added comprehensive test coverage for all edge cases:

  • attributeExtraction.test.ts: Tests undefined/null transports, constructor edge cases, sessionId handling
  • correlation.test.ts: Tests WeakMap fallback scenarios, invalid transport objects, correlation isolation

All tests pass and verify that the patches handle edge cases without breaking existing functionality.

Compatibility

  • Backward compatible - No breaking changes to existing APIs
  • Non-invasive - Only adds defensive checks, doesn't modify core logic
  • Performance neutral - Minimal overhead with early returns for invalid cases
  • Type safe - Maintains existing TypeScript contracts

Impact

This fix prevents runtime errors in production environments when:

  • MCP transports are not fully initialized
  • Session establishment is in progress
  • Invalid transport objects are passed to instrumentation functions
  • Edge case transport implementations are used

The patches ensure reliable instrumentation while maintaining full compatibility with existing MCP server implementations.

Testing Instructions

  1. The existing test suite continues to pass
  2. New edge case tests verify defensive behavior
  3. Real-world StreamableHTTPServerTransport usage is now more robust

🤖 Generated with Claude Code

…ort edge cases

This patch adds defensive handling for edge cases in MCP server instrumentation
when working with StreamableHTTPServerTransport and similar transport implementations.

Changes:
- Add transport constructor null checks in getTransportTypes()
- Graceful sessionId undefined handling in buildTransportAttributes()
- WeakMap correlation fallback system for invalid transport objects
- Type validation for WeakMap operations in correlation.ts

The patches prevent runtime errors during MCP initialization and session establishment
while maintaining backward compatibility and proper instrumentation functionality.

Includes comprehensive test coverage for all edge cases and transport scenarios.

Fixes edge cases where:
- Transport objects have undefined/null constructors
- sessionId is undefined during initialization phases
- Transport objects cannot be used as WeakMap keys
- Invalid transport objects are passed to correlation functions

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
@codyde codyde requested a review from betegon August 2, 2025 08:38
codyde and others added 6 commits August 2, 2025 09:39
…patterns

- Streamline sessionId handling with cleaner undefined checks
- Use optional chaining for transport constructor validation
- Maintain WeakMap fallback system for invalid transport objects
- Align TypeScript patterns with working JavaScript build output

These changes ensure compatibility with StreamableHTTPServerTransport
while following current Sentry v10.0.0 defensive programming patterns.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
…ation

The defensive programming patterns are now the correct baseline
implementation, not patches to be applied.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
@betegon
Copy link
Member

betegon commented Aug 4, 2025

I've changed a couple of thing, mostly agreed with @codyde . The description in his PR lines 5 items:

  1. Transport initialization - when transport constructors are undefined/null.
  2. Session establishment - when sessionId is undefined during MCP initialization phases.

These two first items are the main addition in this PR. So I haven't touch them much, just did a small refactor so it was easier to follow.

Now about these:

  1. Span correlation - when transport objects cannot be used as WeakMap keys
  2. Type validation - when invalid transport objects are passed to correlation functions

i've remove them as there aren't any code path that could put us in those situation as we have 1.) and 2.) in place.

I also added better transport name handling following @codyde addition of using

const transportName = transport.constructor.name?.toLowerCase() || 'unknown';

So now we get the name of the transport object used, which is handy when there could be more than one http and people can add their own transport and will be instrumenting those too.

@betegon betegon requested review from AbhiPrasad and removed request for betegon August 4, 2025 07:36
@betegon betegon marked this pull request as ready for review August 4, 2025 07:36
cursor[bot]

This comment was marked as outdated.

cursor[bot]

This comment was marked as outdated.

cursor[bot]

This comment was marked as outdated.

@betegon betegon changed the title fix(mcp-server): Add defensive patches for StreamableHTTPServerTransport edge cases fix(mcp-server): Add defensive patches for Transport edge cases Aug 4, 2025
cursor[bot]

This comment was marked as outdated.

cursor[bot]

This comment was marked as outdated.

@@ -61,7 +61,7 @@ describe('MCP Server Semantic Conventions', () => {
'mcp.session.id': 'test-session-123',
'client.address': '192.168.1.100',
'client.port': 54321,
'mcp.transport': 'http',
'mcp.transport': 'streamablehttpservertransport',
Copy link
Member

Choose a reason for hiding this comment

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

hmm, should we maybe no lower-case the transport? I suppose this would be easier to read in camel case? 😅

Copy link
Member

Choose a reason for hiding this comment

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

totally right. fixing it

Copy link
Member

@mydea mydea left a comment

Choose a reason for hiding this comment

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

approved, LGTM - only one idea (but feel free to ignore) left, which is maybe we should not lowercase the transport name? but will happily defer to your perspective on this! nice work!

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