-
Notifications
You must be signed in to change notification settings - Fork 752
Fix server/discover timeout interrupting OAuth #1719
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -104,6 +104,10 @@ internal static bool SupportsNaturalOutputSchemas(string? protocolVersion) | |
| private CancellationTokenSource? _messageProcessingCts; | ||
| private Task? _messageProcessingTask; | ||
|
|
||
| internal sealed class McpResponseTimeoutException(string message, Exception innerException) : TimeoutException(message, innerException) | ||
| { | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Initializes a new instance of the <see cref="McpSessionHandler"/> class. | ||
| /// </summary> | ||
|
|
@@ -700,7 +704,13 @@ public IAsyncDisposable RegisterNotificationHandler(string method, Func<JsonRpcN | |
| /// <param name="request">The JSON-RPC request to send.</param> | ||
| /// <param name="cancellationToken">The <see cref="CancellationToken"/> to monitor for cancellation requests. The default is <see cref="CancellationToken.None"/>.</param> | ||
| /// <returns>A task containing the server's response.</returns> | ||
| public async Task<JsonRpcResponse> SendRequestAsync(JsonRpcRequest request, CancellationToken cancellationToken) | ||
| public Task<JsonRpcResponse> SendRequestAsync(JsonRpcRequest request, CancellationToken cancellationToken) => | ||
| SendRequestAsync(request, cancellationToken, Timeout.InfiniteTimeSpan); | ||
|
|
||
| internal async Task<JsonRpcResponse> SendRequestAsync( | ||
| JsonRpcRequest request, | ||
| CancellationToken cancellationToken, | ||
| TimeSpan responseTimeout) | ||
| { | ||
| Throw.IfNull(request); | ||
|
|
||
|
|
@@ -752,15 +762,32 @@ public async Task<JsonRpcResponse> SendRequestAsync(JsonRpcRequest request, Canc | |
|
|
||
| await SendToRelatedTransportAsync(request, cancellationToken).ConfigureAwait(false); | ||
|
|
||
| // Start the response timeout after sending completes so transport setup and authentication remain | ||
| // governed by the caller's cancellation token rather than consuming the response budget. | ||
| using var responseTimeoutCts = responseTimeout == Timeout.InfiniteTimeSpan | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This fixes Passing the probe token into the entire send would reintroduce the OAuth bug because that operation may receive a 401, perform separate OAuth requests, and retry the POST internally. The HTTP/OAuth layer likely needs to apply the probe timeout while awaiting each MCP POST response, exclude the OAuth requests, and use a fresh timeout for the authenticated retry, while AI-assisted review comment. |
||
| ? null | ||
| : CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); | ||
| responseTimeoutCts?.CancelAfter(responseTimeout); | ||
| var responseCancellationToken = responseTimeoutCts?.Token ?? cancellationToken; | ||
|
|
||
| // Now that the request has been sent, register for cancellation. If we registered before, | ||
| // a cancellation request could arrive before the server knew about that request ID, in which | ||
| // case the server could ignore it. | ||
| // Per spec, "The initialize request MUST NOT be cancelled by clients", so skip registration for initialize. | ||
| LogRequestSentAwaitingResponse(EndpointName, request.Method, request.Id); | ||
| JsonRpcMessage? response; | ||
| using (var registration = method != RequestMethods.Initialize ? RegisterCancellation(cancellationToken, request) : default) | ||
| using (var registration = method != RequestMethods.Initialize ? RegisterCancellation(responseCancellationToken, request) : default) | ||
| { | ||
| response = await tcs.Task.WaitAsync(cancellationToken).ConfigureAwait(false); | ||
| try | ||
| { | ||
| response = await tcs.Task.WaitAsync(responseCancellationToken).ConfigureAwait(false); | ||
| } | ||
| catch (OperationCanceledException ex) when ( | ||
| responseTimeoutCts?.IsCancellationRequested is true && | ||
| !cancellationToken.IsCancellationRequested) | ||
| { | ||
| throw new McpResponseTimeoutException($"Timed out waiting for a response to method '{request.Method}'.", ex); | ||
| } | ||
| } | ||
|
|
||
| if (response is JsonRpcError error) | ||
|
|
||
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.
Can we move these closer to the use site in the local function?