Add double-connect guard to prevent protocol message handling errors#450
Open
Add double-connect guard to prevent protocol message handling errors#450
Conversation
Calling connect() twice on the same transport instance caused the MCP SDK's Protocol class to chain onmessage handlers, so each incoming message was processed twice. This made incoming responses get consumed by the first processing (resolving the handler) and then trigger a spurious "Received a response for an unknown message ID" error on the second processing pass, since the handler was already removed from the response map. Add an explicit guard in both App.connect() and AppBridge.connect() that throws if the instance is already connected. This converts the silent message-corruption into a clear, actionable error and prevents downstream confusion. Adds three regression tests: - AppBridge.connect() throws when already connected - App.connect() throws when already connected - Demonstrates the double-processing root cause and verifies it is fixed Fixes #429 https://claude.ai/code/session_01XU3FW1TpjBkyRbkBUH5gqR
@modelcontextprotocol/ext-apps
@modelcontextprotocol/server-basic-react
@modelcontextprotocol/server-basic-vanillajs
@modelcontextprotocol/server-budget-allocator
@modelcontextprotocol/server-cohort-heatmap
@modelcontextprotocol/server-customer-segmentation
@modelcontextprotocol/server-map
@modelcontextprotocol/server-pdf
@modelcontextprotocol/server-scenario-modeler
@modelcontextprotocol/server-shadertoy
@modelcontextprotocol/server-sheet-music
@modelcontextprotocol/server-system-monitor
@modelcontextprotocol/server-threejs
@modelcontextprotocol/server-transcript
@modelcontextprotocol/server-video-resource
@modelcontextprotocol/server-wiki-explorer
commit: |
src/app-bridge.test.ts
Outdated
Comment on lines
642
to
671
| // Regression test: before the double-connect guard was added, calling | ||
| // connect() twice on the same transport caused the MCP Protocol to chain | ||
| // onmessage handlers, processing each incoming message twice. This caused | ||
| // the second processing of a response to fail with "unknown message ID" | ||
| // because the first processing already consumed the response handler. | ||
| // | ||
| // This test verifies that the guard prevents this scenario entirely. | ||
| bridge.onupdatemodelcontext = async () => ({}); | ||
|
|
||
| await bridge.connect(bridgeTransport); | ||
| await app.connect(appTransport); | ||
|
|
||
| // After close(), reconnection should be allowed | ||
| await bridgeTransport.close(); | ||
| const [newAppTransport, newBridgeTransport] = | ||
| InMemoryTransport.createLinkedPair(); | ||
| const newBridge = new AppBridge( | ||
| createMockClient() as Client, | ||
| testHostInfo, | ||
| testHostCapabilities, | ||
| ); | ||
| const newApp = new App(testAppInfo, {}, { autoResize: false }); | ||
|
|
||
| const errors: Error[] = []; | ||
| newBridge.onerror = (e) => errors.push(e); | ||
| newApp.onerror = (e) => errors.push(e); | ||
| newBridge.onupdatemodelcontext = async () => ({}); | ||
|
|
||
| await newBridge.connect(newBridgeTransport); | ||
| await newApp.connect(newAppTransport); |
Member
There was a problem hiding this comment.
I'm not sure I understand what this test is doing... newBridge and newApp use newBridgeTransport and newAppTransport. Should they use bridgeTransport and appTransport instead?
…uard check The previous third test in "double-connect guard" never actually attempted a double-connect — it created fresh instances and verified normal operation, which every other test already covers. The stray bridge.onupdatemodelcontext and bridgeTransport.close() at the top were leftover noise. Replace it with a focused test that verifies the guard fires even when the caller passes the *same* transport object (not just a different one), which is the exact scenario described in #429. https://claude.ai/code/session_01XU3FW1TpjBkyRbkBUH5gqR
jonathanhefner
approved these changes
Feb 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR adds validation to prevent calling
connect()multiple times on the sameAppBridgeorAppinstance without first callingclose(). This could prevents a subtle bug where an overwrittenonmessagehandler wouldn't know about earlier requests, leading to "unknown message ID" errors.This could be what's happening in #429 but worth adding regardless.
Key Changes
"AppBridge is already connected"error ifconnect()is called when already connected"App is already connected"error ifconnect()is called when already connectedclose()Implementation Details
The guard checks if
this.transportis already set before allowing a new connection. This is a simple but effective prevention mechanism that forces users to explicitly callclose()before reconnecting, ensuring clean state management and preventing the message handler chaining issue that would otherwise occur.