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?
Use FakeTimeProvider, check the probe doesn't cancel metadata loading.
|
@chrikrah #1719 is in now, so I merged main into the branch and updated the tests. The two old tests are now one theory with the probe armed (5s probe, 20s InitializationTimeout), on FakeTimeProvider and AsyncGate same as DiscoveryTimeoutTests. The test holds the auth server metadata request and moves the clock past the probe budget. Then it checks that the request is still not cancelled, so no fallback to initialize. After that caller cancel gives OperationCanceledException and InitializationTimeout gives TimeoutException. Without the fix both cases still fail with McpException "Failed to find .well-known/...". I also removed the probe suspension from #1719 locally to check, and the probe check fails then, so I think it covers your case. Thanks for catching it. @halter73 should be ready for a look when you have time. |
chrikrah
left a comment
There was a problem hiding this comment.
@Empiree the probe-armed theory closes what I left open on c2d13c5. At 00aaa7f both cases pass, both go back to McpException without the filter, and the probe assertion catches a missing #1719 suspension. Approving.
$ dotnet test tests/ModelContextProtocol.AspNetCore.Tests -f net10.0 --filter "FullyQualifiedName~OAuth" # 00aaa7f, SDK 10.0.401
Passed! - Failed: 0, Passed: 117, Skipped: 0, Total: 117
# ClientOAuthProvider.cs from merge base 3d8af7f, tests kept
Failed ...ObservesCallerAndInitializationCancellation(initializationTimeout: True)
Expected: typeof(System.TimeoutException) Actual: typeof(ModelContextProtocol.McpException)
Failed ...ObservesCallerAndInitializationCancellation(initializationTimeout: False)
Expected: typeof(System.OperationCanceledException) Actual: typeof(ModelContextProtocol.McpException)
Failed! - Failed: 2, Passed: 115, Skipped: 0, Total: 117
# 00aaa7f with RequestTimeout.Suspend() no longer calling Stop()
$ dotnet test ... --filter "FullyQualifiedName~AuthServerMetadataCancellation"
Assert.False() Failure Expected: False Actual: True # AuthServerMetadataCancellationTests.cs:line 40
Failed! - Failed: 2, Passed: 0, Skipped: 0, Total: 2
# not run: net8.0 and net9.0, the rest of the solution, Windows
@halter73 (or @tarekgh, if it's easier to pick up) the change is the five-line when filter plus this test, and #1719 is already in. Is it ready to merge?
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