Skip to content

fix(client): suppress onerror when SSE disconnect will be handled by reconnect #1827

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.

Sign up for GitHub

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
felixweinberger wants to merge 5 commits into
base: main
Choose a base branch
from
+49 −13
Open

fix(client): suppress onerror when SSE disconnect will be handled by reconnect #1827

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
31beb5b
fix(client): suppress onerror when SSE disconnect will be handled by …
felixweinberger Mar 31, 2026
dc7d763
Merge branch 'main' into fweinberger/fix-sse-disconnect-noise
felixweinberger Apr 1, 2026
e64d9ca
Merge remote-tracking branch 'origin/main' into fweinberger/fix-sse-d…
felixweinberger Apr 2, 2026
7ce0a21
fix(client): preserve triggering error when reconnection bails at max…
felixweinberger Apr 2, 2026
bb9eef8
fix(client): suppress SSE-disconnect onerror when abort came from int…
felixweinberger Apr 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/fix-sse-disconnect-noise.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@modelcontextprotocol/client': patch
---

Suppress `onerror` when an SSE stream disconnects but reconnection will be scheduled. Previously `onerror` fired unconditionally on every stream disconnect, producing `"SSE stream disconnected: TypeError: terminated"` noise every few minutes on long-lived connections even though the transport recovered transparently.
22 changes: 15 additions & 7 deletions packages/client/src/client/streamableHttp.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,16 +325,23 @@
/**
* Schedule a reconnection attempt using server-provided retry interval or backoff
*
* @param lastEventId The ID of the last received event for resumability
* @param attemptCount Current reconnection attempt count for this specific stream
* @param lastError The error that triggered this reconnection attempt, surfaced if max retries is exceeded
*/
private _scheduleReconnection(options: StartSSEOptions, attemptCount = 0): void {
private _scheduleReconnection(options: StartSSEOptions, attemptCount = 0, lastError?: unknown): void {

Check warning on line 332 in packages/client/src/client/streamableHttp.ts

View check run for this annotation

Claude / Claude Code Review

Stale @param lastEventId in _scheduleReconnection JSDoc 

The JSDoc for `_scheduleReconnection` contains a stale `@param lastEventId` that no longer matches any parameter — the actual first parameter is `options: StartSSEOptions`. This is a pre-existing inaccuracy, but this PR directly modified the same JSDoc block by adding `@param lastError` without removing or correcting the stale entry. IDE hover-text and future contributors will see incorrect parameter documentation for this method.
Comment on lines 328 to +332
Copy link
Copy Markdown

@claude claude bot Apr 9, 2026

Choose a reason for hiding this comment

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

🟡 The JSDoc for _scheduleReconnection contains a stale @param lastEventId that no longer matches any parameter — the actual first parameter is options: StartSSEOptions. This is a pre-existing inaccuracy, but this PR directly modified the same JSDoc block by adding @param lastError without removing or correcting the stale entry. IDE hover-text and future contributors will see incorrect parameter documentation for this method.

Extended reasoning...

What the bug is and how it manifests

The JSDoc comment block for _scheduleReconnection contains @param lastEventId The ID of the last received event for resumability, but the actual first parameter of the method is options: StartSSEOptions. There is no lastEventId parameter in the current signature. IDEs will surface this stale tag as hover-text whenever a developer inspects a call site, and any generated documentation tooling that processes private methods will also show the incorrect name.

The specific code path that triggers it

The method signature after this PR is:

private _scheduleReconnection(options: StartSSEOptions, attemptCount = 0, lastError?: unknown): void

The JSDoc still documents three params: lastEventId (wrong), attemptCount (correct), and the newly added lastError (correct). The first tag is a fossil from an earlier version of the method when the first argument may have been a lastEventId string.

Why existing code does not prevent it

TypeScript does not enforce that JSDoc @param names match the actual parameter names for private methods (and even for public ones, only some strict-doc linters catch this). There is nothing in the build or CI pipeline that would flag the mismatch.

What the impact would be

Developers reading the code or hovering over a call to _scheduleReconnection in an IDE will see lastEventId listed as the first parameter, which no longer exists. This is misleading and increases cognitive overhead when tracing the reconnection logic — exactly the part of the codebase this PR is modifying. The impact is purely documentation; runtime behavior is unaffected.

How to fix it

Remove the stale @param lastEventId ... line and replace it with the correct tag:

@param options SSE start options (resumption token, callbacks) forwarded to _startOrAuthSse on each attempt

Step-by-step proof

  1. Before this PR, _scheduleReconnection accepted options: StartSSEOptions as its first parameter — the @param lastEventId tag was already wrong.
  2. This PR adds @param lastError to the same block (the diff shows + * @param lastError ...) without touching or removing @param lastEventId.
  3. The resulting JSDoc block now documents three @param tags — lastEventId, attemptCount, lastError — while the actual signature has options, attemptCount, lastError.
  4. Any developer hovering over this._scheduleReconnection({...}, 0, error) in VSCode will see lastEventId as the first parameter description, which is incorrect.

// Use provided options or default options
const maxRetries = this._reconnectionOptions.maxRetries;

// Check if we've exceeded maximum retry attempts
if (attemptCount >= maxRetries) {
this.onerror?.(new Error(`Maximum reconnection attempts (${maxRetries}) exceeded.`));
const reason = lastError === undefined ? undefined : lastError instanceof Error ? lastError.message : String(lastError);
this.onerror?.(
new Error(
`Maximum reconnection attempts (${maxRetries}) exceeded` + (reason ? `: ${reason}` : '.'),
lastError === undefined ? undefined : { cause: lastError }
)
);
Comment on lines +338 to +344
Copy link
Copy Markdown

@claude claude bot Apr 2, 2026

Choose a reason for hiding this comment

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

🟡 When lastError is an Error with an empty message (e.g. new Error("")), the triggering error is silently omitted from the "Maximum reconnection attempts exceeded" message due to a falsy check on reason. Change reason ? to reason !== undefined ? so that empty-string reasons still produce a colon suffix that signals a cause exists.

Extended reasoning...

What the bug is and how it manifests

In _scheduleReconnection, the PR introduces logic to surface the triggering error in the max-retries message at lines 338-344. When lastError is new Error(""), reason evaluates to "" — an empty string, which is falsy in JavaScript. The ternary (reason ? ': ' + reason : '.') then falls through to the period branch, producing the exact same message as the no-error case: "Maximum reconnection attempts (N) exceeded." The error.cause property is still set correctly, but callers who only inspect error.message (the common pattern) receive no indication that a triggering error was present.

The specific code path that triggers it

The relevant lines are 338-344 in packages/client/src/client/streamableHttp.ts. The reason variable is computed as: lastError === undefined ? undefined : lastError instanceof Error ? lastError.message : String(lastError). For new Error(""), this yields "". The ternary check (reason ? ...) treats this identically to undefined. The catch block in _handleSseStream passes error directly to _scheduleReconnection as the third argument, so any network throw producing an empty-message Error would trigger the silent drop.

Why existing code doesn't prevent it

The truthiness check reason ? is designed to detect "no reason" (i.e., lastError === undefined), but conflates two distinct states: (1) reason === undefined meaning no error was provided, and (2) reason === "" meaning an error was provided but has an empty message. Both are falsy, so both produce the same output. A strict undefined check (reason !== undefined) would correctly distinguish them.

What the impact would be

Users who only read error.message — the common pattern in logging, error display, and monitoring — would see "Maximum reconnection attempts (N) exceeded." for an empty-message error, identical to the case where no error occurred at all. They lose all indication that a root-cause error exists and must inspect error.cause to find it. In practice, real network errors (AbortError, TypeError: terminated, etc.) all have non-empty messages, so the practical impact is near-zero. The cause is preserved for programmatic inspection.

Step-by-step proof

  1. lastError = new Error("") is passed to _scheduleReconnection.
  2. reason = lastError instanceof Error ? lastError.message : String(lastError) evaluates to "".
  3. (reason ? ': ' + reason : '.') — "" is falsy — evaluates to ".".
  4. Final message: "Maximum reconnection attempts (0) exceeded."
  5. Meanwhile, lastError === undefined is false, so { cause: lastError } is passed and error.cause is correctly set.
  6. The message is indistinguishable from the no-error case; cause exists but is invisible to message-only consumers.

How to fix it

Replace the truthiness check with an explicit undefined check: (reason !== undefined ? ': ' + reason : '.'). This produces "Maximum reconnection attempts (0) exceeded: " for empty-message errors — the trailing colon at minimum signals to readers that a cause was present — while still appending "." only when no error was provided at all.

Copy link
Copy Markdown
Contributor Author

@felixweinberger felixweinberger Apr 9, 2026

Choose a reason for hiding this comment

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

Empty-message Errors don't occur on the network paths feeding this, and cause is preserved for programmatic access. The suggested output (exceeded: with trailing colon-space) reads as a formatting bug.

return;
}

Expand All @@ -347,7 +354,7 @@
this._startOrAuthSse(options).catch(error => {
this.onerror?.(new Error(`Failed to reconnect SSE stream: ${error instanceof Error ? error.message : String(error)}`));
try {
this._scheduleReconnection(options, attemptCount + 1);
this._scheduleReconnection(options, attemptCount + 1, error);
} catch (scheduleError) {
this.onerror?.(scheduleError instanceof Error ? scheduleError : new Error(String(scheduleError)));
}
Expand Down Expand Up @@ -447,9 +454,6 @@
);
}
} catch (error) {
// Handle stream errors - likely a network disconnect
this.onerror?.(new Error(`SSE stream disconnected: ${error}`));

// Attempt to reconnect if the stream disconnects unexpectedly and we aren't closing
// Reconnect if: already reconnectable (GET stream) OR received a priming event (POST stream with event ID)
// BUT don't reconnect if we already received a response - the request is complete
Expand All @@ -464,11 +468,15 @@
onresumptiontoken,
replayMessageId
},
0
0,
error
);
} catch (error) {
this.onerror?.(new Error(`Failed to reconnect: ${error instanceof Error ? error.message : String(error)}`));
}
} else if (!this._abortController?.signal.aborted) {
// Stream disconnected and reconnection will not happen; surface the error
this.onerror?.(new Error(`SSE stream disconnected: ${error instanceof Error ? error.message : String(error)}`));
}
}
};
Expand Down
35 changes: 29 additions & 6 deletions packages/client/test/client/streamableHttp.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -925,12 +925,9 @@ describe('StreamableHTTPClientTransport', () => {
await vi.advanceTimersByTimeAsync(20); // Trigger reconnection timeout

// ASSERT
expect(errorSpy).toHaveBeenCalledWith(
expect.objectContaining({
message: expect.stringContaining('SSE stream disconnected: Error: Network failure')
})
);
// THE KEY ASSERTION: A second fetch call proves reconnection was attempted.
// onerror is NOT called: reconnection will handle the disconnect transparently
expect(errorSpy).not.toHaveBeenCalled();
// A second fetch call proves reconnection was attempted.
expect(fetchMock).toHaveBeenCalledTimes(2);
expect(fetchMock.mock.calls[0]![1]?.method).toBe('GET');
expect(fetchMock.mock.calls[1]![1]?.method).toBe('GET');
Expand Down Expand Up @@ -1786,6 +1783,32 @@ describe('StreamableHTTPClientTransport', () => {
expect(transport['_cancelReconnection']).toBeUndefined();
});

it('should surface the triggering error when maxRetries is 0', async () => {
transport = new StreamableHTTPClientTransport(new URL('http://localhost:1234/mcp'), {
reconnectionOptions: {
initialReconnectionDelay: 10,
maxRetries: 0,
maxReconnectionDelay: 1000,
reconnectionDelayGrowFactor: 1
}
});

const errorSpy = vi.fn();
transport.onerror = errorSpy;

const triggeringError = new Error('socket hang up');
transport['_scheduleReconnection']({}, 0, triggeringError);

expect(errorSpy).toHaveBeenCalledTimes(1);
expect(errorSpy).toHaveBeenCalledWith(
expect.objectContaining({
message: 'Maximum reconnection attempts (0) exceeded: socket hang up',
cause: triggeringError
})
);
expect(transport['_cancelReconnection']).toBeUndefined();
});

it('should schedule reconnection when maxRetries is greater than 0', async () => {
// ARRANGE
transport = new StreamableHTTPClientTransport(new URL('http://localhost:1234/mcp'), {
Expand Down
Loading