fix(client): suppress onerror when SSE disconnect will be handled by reconnect#1827
Open
felixweinberger wants to merge 1 commit intomainfrom
Open
fix(client): suppress onerror when SSE disconnect will be handled by reconnect#1827felixweinberger wants to merge 1 commit intomainfrom
felixweinberger wants to merge 1 commit intomainfrom
Conversation
…reconnect Previously onerror fired unconditionally on every stream disconnect, before the reconnect decision. This produced 'SSE stream disconnected: TypeError: terminated' noise every few minutes on long-lived connections even though the transport recovered transparently. Moved onerror into the else branch of the reconnect check so it only fires when the stream will not recover. Fixes #1211
🦋 Changeset detectedLatest commit: 31beb5b The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@modelcontextprotocol/client
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
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
1 participant
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.
Moves
onerrorinto the else branch of the reconnect check in_handleSseStream's catch block.Motivation and Context
Fixes #1211.
onerrorfired unconditionally on every stream disconnect, before the reconnect decision at streamableHttp.ts:447. On long-lived GET SSE streams this produced"SSE stream disconnected: TypeError: terminated"every few minutes even though the transport recovered transparently via_scheduleReconnection.Three users confirmed on #1211 (original report on 1.24.3, playwright-mcp user with screenshot, status-ask). #1349 attempted a fix via string-matching
'terminated'/'body stream', but that's fragile (undici could change messages, and'body stream'also matches'body stream already read'which is a different bug). This takes the structural approach instead: only fireonerrorwhen reconnection won't happen.How Has This Been Tested?
Updated the existing
"should reconnect a GET-initiated notification stream that fails"test to assertonerroris NOT called when reconnect fires (previously asserted the buggy behavior). Test fails on main (expected not called, but called 1 times), passes with the fix. All 346 client tests pass.Breaking Changes
Behavioral change for users who relied on
onerroras a connection-activity monitor: transient disconnects that will be handled by reconnection no longer fireonerror. Permanent failures ("Failed to reconnect","Maximum reconnection attempts exceeded") still fire.Types of changes
Checklist
Additional context
Credit to @hassan123789 for #1349 which identified the issue and the code location.