Skip to content

Fix cancellation being reported as missing OAuth metadata - #1891

Open
Empiree wants to merge 1 commit into
modelcontextprotocol:mainfrom
Empiree:fix/oauth-metadata-cancellation
Open

Empiree wants to merge 1 commit into
modelcontextprotocol:mainfrom
Empiree:fix/oauth-metadata-cancellation

Conversation

@Empiree

@Empiree Empiree commented Sep 26, 2026

Copy link
Copy Markdown

If CreateAsync is cancelled or InitializationTimeout hits while the client loads auth server metadata, the catch in GetAuthServerMetadataAsync treats it like failed endpoint and tries the next one. So the user gets "Failed to find .well-known/... metadata" instead of OperationCanceledException / TimeoutException.

Now OperationCanceledException is rethrown when our token is cancelled. Other errors (also HttpClient timeout) still go to the next endpoint like before.

Added 2 tests, they fail without the fix.

Related to #1806

@chrikrah chrikrah left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@Empiree the filter fixes what #1806 reports: with ClientOAuthProvider.cs at the merge base both new tests fail on the exception type, and at c2d13c5 the OAuth tests pass. One non-blocking finding below, for halter73 to weigh before merge.

$ dotnet test tests/ModelContextProtocol.AspNetCore.Tests -f net10.0 --filter "FullyQualifiedName~OAuth"   # c2d13c5, SDK 10.0.401
Passed!  - Failed:     0, Passed:   100, Skipped:     0, Total:   100
$ # same command, ClientOAuthProvider.cs at c40ee04 (merge base), tests kept
Failed AuthServerMetadataCancellationTests.InitializationTimeout_DuringAuthServerMetadataDiscovery_ThrowsTimeoutException
  Expected: typeof(System.TimeoutException)  Actual: typeof(ModelContextProtocol.McpException)
Failed AuthServerMetadataCancellationTests.CallerCancellation_DuringAuthServerMetadataDiscovery_ThrowsOperationCanceledException
  Expected: typeof(System.OperationCanceledException)  Actual: typeof(ModelContextProtocol.McpException)
Failed!  - Failed:     2, Passed:    98, Skipped:     0, Total:   100
$ # scratch test: default McpClientOptions, first auth-server metadata request hangs, request log from a DelegatingHandler
# c2d13c5:  connected after 5.7s, requests: server/discover | ...oauth-authorization-server (hanging) | initialize | ...oauth-authorization-server | /token | initialize | notifications/initialized
# c40ee04:  McpException after 5.0s: Failed to find .well-known/openid-configuration or .well-known/oauth-authorization-server metadata
# not run: net8.0 and net9.0, the rest of the solution, Windows

non-blocking: both new tests set InitializationTimeout to 2 s, so the 5 s DiscoverProbeTimeout is never armed. With defaults it is, and when it fires during metadata discovery the filter now hands McpClientImpl.cs:394 a probe timeout, so the client moves to initialize without retrying server/discover. Connecting beats failing, but that is the fallback #1719 is written to avoid.

#1833 by jstar0 widens the test probe budget for #1806. @halter73 you made the latest changes to src/ModelContextProtocol.Core/Authentication/ and opened #1719. Should this land after #1719, or with a probe-armed test?

@Empiree

Empiree commented Oct 10, 2026

Copy link
Copy Markdown
Author

@chrikrah thanks for checking. Right, with default options the probe timeout now ends in the initialize fallback instead of the error. #1719 pauses the probe during oauth, so this goes away after it. I am fine to wait for #1719, then rebase and add test with the probe armed.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants