Repository navigation
Conversation
chrikrah
left a comment
There was a problem hiding this comment.
@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?
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