Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 2.1k
fix(client): remove redundant onerror in _startOrAuthSse catch#1826
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
Closed
+122
−59
Closed
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
570c642
fix(client): remove redundant onerror in _startOrAuthSse catch
felixweinberger 4041340
fix(client): remove redundant onerror in _startOrAuthSse catch
felixweinberger 91dd658
Merge branch 'main' into fweinberger/fix-double-onerror-868
felixweinberger 8f20442
Merge remote-tracking branch 'origin/main' into fweinberger/fix-doubl…
felixweinberger 0e2cdc9
fix(client): restore onerror in resumeStream and remove dead try/catch
felixweinberger File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Jump to file
Failed to load files.
Loading
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| '@modelcontextprotocol/client': patch | ||
| --- | ||
| Fix double `onerror` invocation when `_startOrAuthSse` fails. The internal catch block fired `onerror` then threw, and all callers already `.catch(onerror)`, causing every failure to fire twice. Removed the redundant internal call. | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -233,72 +233,66 @@ export class StreamableHTTPClientTransport implements Transport { | ||
| private async _startOrAuthSse(options: StartSSEOptions, isAuthRetry = false): Promise<void> { | ||
| const { resumptionToken } = options; | ||
| try { | ||
| // Try to open an initial SSE stream with GET to listen for server messages | ||
| // This is optional according to the spec - server may not support it | ||
| const headers = await this._commonHeaders(); | ||
| const userAccept = headers.get('accept'); | ||
| const types = [...(userAccept?.split(',').map(s => s.trim().toLowerCase()) ?? []), 'text/event-stream']; | ||
| headers.set('accept', [...new Set(types)].join(', ')); | ||
| // Include Last-Event-ID header for resumable streams if provided | ||
| if (resumptionToken) { | ||
| headers.set('last-event-id', resumptionToken); | ||
| } | ||
| // Try to open an initial SSE stream with GET to listen for server messages | ||
| // This is optional according to the spec - server may not support it | ||
| const headers = await this._commonHeaders(); | ||
| const userAccept = headers.get('accept'); | ||
| const types = [...(userAccept?.split(',').map(s => s.trim().toLowerCase()) ?? []), 'text/event-stream']; | ||
| headers.set('accept', [...new Set(types)].join(', ')); | ||
| // Include Last-Event-ID header for resumable streams if provided | ||
| if (resumptionToken) { | ||
| headers.set('last-event-id', resumptionToken); | ||
| } | ||
| const response = await (this._fetch ?? fetch)(this._url, { | ||
| ...this._requestInit, | ||
| method: 'GET', | ||
| headers, | ||
| signal: this._abortController?.signal | ||
| }); | ||
| const response = await (this._fetch ?? fetch)(this._url, { | ||
| ...this._requestInit, | ||
| method: 'GET', | ||
| headers, | ||
| signal: this._abortController?.signal | ||
| }); | ||
| if (!response.ok) { | ||
| if (response.status === 401 && this._authProvider) { | ||
| if (response.headers.has('www-authenticate')) { | ||
| const { resourceMetadataUrl, scope } = extractWWWAuthenticateParams(response); | ||
| this._resourceMetadataUrl = resourceMetadataUrl; | ||
| this._scope = scope; | ||
| } | ||
| if (!response.ok) { | ||
| if (response.status === 401 && this._authProvider) { | ||
| if (response.headers.has('www-authenticate')) { | ||
| const { resourceMetadataUrl, scope } = extractWWWAuthenticateParams(response); | ||
| this._resourceMetadataUrl = resourceMetadataUrl; | ||
| this._scope = scope; | ||
| } | ||
| if (this._authProvider.onUnauthorized && !isAuthRetry) { | ||
| await this._authProvider.onUnauthorized({ | ||
| response, | ||
| serverUrl: this._url, | ||
| fetchFn: this._fetchWithInit | ||
| }); | ||
| await response.text?.().catch(() => {}); | ||
| // Purposely _not_ awaited, so we don't call onerror twice | ||
| return this._startOrAuthSse(options, true); | ||
| } | ||
| if (this._authProvider.onUnauthorized && !isAuthRetry) { | ||
| await this._authProvider.onUnauthorized({ | ||
| response, | ||
| serverUrl: this._url, | ||
| fetchFn: this._fetchWithInit | ||
| }); | ||
| await response.text?.().catch(() => {}); | ||
| if (isAuthRetry) { | ||
| throw new SdkError(SdkErrorCode.ClientHttpAuthentication, 'Server returned 401 after re-authentication', { | ||
| status: 401 | ||
| }); | ||
| } | ||
| throw new UnauthorizedError(); | ||
| return this._startOrAuthSse(options, true); | ||
| } | ||
| await response.text?.().catch(() => {}); | ||
| // 405 indicates that the server does not offer an SSE stream at GET endpoint | ||
| // This is an expected case that should not trigger an error | ||
| if (response.status === 405) { | ||
| return; | ||
| if (isAuthRetry) { | ||
| throw new SdkError(SdkErrorCode.ClientHttpAuthentication, 'Server returned 401 after re-authentication', { | ||
| status: 401 | ||
| }); | ||
| } | ||
| throw new UnauthorizedError(); | ||
| } | ||
| throw new SdkError(SdkErrorCode.ClientHttpFailedToOpenStream, `Failed to open SSE stream: ${response.statusText}`, { | ||
| status: response.status, | ||
| statusText: response.statusText | ||
| }); | ||
| await response.text?.().catch(() => {}); | ||
| // 405 indicates that the server does not offer an SSE stream at GET endpoint | ||
| // This is an expected case that should not trigger an error | ||
| if (response.status === 405) { | ||
| return; | ||
| } | ||
| this._handleSseStream(response.body, options, true); | ||
| } catch (error) { | ||
| this.onerror?.(error as Error); | ||
| throw error; | ||
| throw new SdkError(SdkErrorCode.ClientHttpFailedToOpenStream, `Failed to open SSE stream: ${response.statusText}`, { | ||
| status: response.status, | ||
| statusText: response.statusText | ||
| }); | ||
| } | ||
claude[bot] marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| this._handleSseStream(response.body, options, true); | ||
| } | ||
| /** | ||
| @@ -753,9 +747,14 @@ export class StreamableHTTPClientTransport implements Transport { | ||
| * @param options Optional callback to receive new resumption tokens | ||
| */ | ||
| async resumeStream(lastEventId: string, options?: { onresumptiontoken?: (token: string) => void }): Promise<void> { | ||
| await this._startOrAuthSse({ | ||
| resumptionToken: lastEventId, | ||
| onresumptiontoken: options?.onresumptiontoken | ||
| }); | ||
| try { | ||
| await this._startOrAuthSse({ | ||
| resumptionToken: lastEventId, | ||
| onresumptiontoken: options?.onresumptiontoken | ||
| }); | ||
| } catch (error) { | ||
| this.onerror?.(error as Error); | ||
| throw error; | ||
| } | ||
| } | ||
| } | ||
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
Oops, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 The changeset description inaccurately states "all callers already
.catch(onerror)" --resumeStreamdid NOT have.catch(onerror)before this PR and was silently dropping errors. The PR makes two distinct changes (removing double-fire for three fire-and-forget callers AND adding new error handling toresumeStream), but the changeset only documents the first, leaving the behavioral change toresumeStreamundocumented.Extended reasoning...
What the bug is
The changeset at
.changeset/fix-double-onerror-startOrAuthSse.mdline 5 reads:The claim "all callers already
.catch(onerror)" is factually incorrect. There are four callers of_startOrAuthSse, not three: lines 344, 531/532, 642, andresumeStream(~line 750). The first three are fire-and-forget callers that each chain.catch(error => this.onerror?.(error)). ButresumeStreamwas different -- it simply didawait this._startOrAuthSse({...})with no try-catch and no.catch(onerror)at all.The specific code path
Before this PR, when
resumeStreamfailed:_startOrAuthSsethrew (after its internal catch calledonerrorand rethrew)resumeStreamhad no handler, so the exception propagated to the calleronerrorwas called exactly once -- by the internal catch in_startOrAuthSseAfter this PR removes the internal catch from
_startOrAuthSse, the PR correctly compensates by adding a try-catch toresumeStreamitself (lines 747-758 in the diff). So the code is correct. But the changeset description omits this second change entirely.Why existing documentation does not cover it
The changeset is the primary artifact consumers read to understand what changed between versions. By describing the fix purely as "removing the redundant internal call" and saying "all callers already
.catch(onerror)", a reader would not know thatresumeStreamnow has onerror-reporting behavior it lacked before. Prior to this PR, aresumeStreamfailure would callonerroronce (via the internal catch). After this PR, it still callsonerroronce -- but via a different mechanism, and the behavior is preserved only because the PR added a new try-catch. The changeset implies no behavioral change occurred for any caller.Step-by-step proof
resumeStreamcallsawait this._startOrAuthSse({resumptionToken: lastEventId})_startOrAuthSsethrowsSdkError(ClientHttpFailedToOpenStream, ...)_startOrAuthSseinternal catch fires: callsthis.onerror?.(error), then rethrowsresumeStreamhas no handler -- exception propagates to calleronerrorfires once (from step 3)onerrorinternallyresumeStreamcatches the error, callsthis.onerror?.(error), rethrowsonerrorfires once (now from step 7 instead of step 3)Impact and fix
The impact is documentation-only -- the code behavior is correct. A changelog reader consulting this entry would not know
resumeStream's error-reporting pathway changed. The fix is to update the changeset to accurately describe both changes: (1) removing the redundant double-fire for the three fire-and-forget callers, and (2) adding explicitonerrorhandling toresumeStreamwhich previously relied on the internal catch that was removed.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The trace in this comment shows resumeStream fired onerror exactly once before and exactly once after, with the error still propagating. Observable behavior is identical, so there's nothing consumer-facing to document. The 'all callers' phrasing describes a private method consumers can't call.