Skip to content

Fix server/discover timeout interrupting OAuth - #1719

Open
halter73 wants to merge 1 commit into
mainfrom
halter73-release-priority-triage
Open

halter73 wants to merge 1 commit into
mainfrom
halter73-release-priority-triage

Conversation

@halter73

@halter73 halter73 commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Summary

Exclude SDK OAuth token acquisition from DiscoverProbeTimeout without removing the deadline from the actual server/discover request.

The short probe exists to detect older servers that silently ignore discovery. Today it can instead interrupt metadata discovery or interactive login, then start initialize and a second OAuth flow with different state and PKCE values. This addresses the probe-triggered failure reported in #1830.

The revised implementation keeps one request-local, non-serialized timeout controller in the discovery message context, passed through the existing typed request helper. The OAuth provider suspends only that timer around its existing silent and challenged token-acquisition calls. This includes cache access, lock waits, metadata, registration, interactive authorization, refresh, and token exchange. Completing acquisition restarts the full probe budget before the initial or authenticated MCP POST; response headers, JSON/error/SSE bodies, and the pending JSON-RPC response remain bounded.

This replaces the original implementation of this PR, which started the timer after SendMessageAsync. Streamable HTTP sends the request and reads its response inside that method, so a post-send timer leaves silent headers and stalled response bodies bounded only by InitializationTimeout.

Cancellation and transport behavior

  • Caller cancellation and InitializationTimeout remain active during OAuth. Initialization still defaults to 60 seconds; hosts needing a longer login window must configure it. No new public option, detached authorization task, or provider-owned pending flow is introduced. The broader cross-request authorization-lifetime change proposed in Keep interactive OAuth flows alive when the triggering request is canceled #1831 is deliberately separate from this fix.
  • HTTP+SSE now defaults to the client's existing initialize path. AutoDetect can send its initial discovery POST to test Streamable HTTP, but once its GET selects SSE, the client sends initialize, not server/discover, to the SSE message endpoint. When discovery is abandoned to attempt SSE establishment, its probe timer is stopped permanently rather than suspended and restarted. GET establishment uses the existing connection, initialization where applicable, and caller deadlines, not the modern probe deadline. Only OAuth uses the resumable suspension scope.
  • Explicit choices are preserved: selecting HttpTransportMode.Sse with ProtocolVersion = "2026-07-28" still attempts discovery and supports modern MRTR when the peer supports it. This unusual configuration remains bounded by the probe timeout if the peer ignores discovery. With no version specified, explicit SSE initializes instead. An AutoDetect client pinned to 2026-07-28 still rejects the initialize fallback if SSE is selected; it does not silently downgrade. Server-side acceptance is unchanged.
  • A small internal exception transfers control from AutoDetect to the client's connection logic after SSE is selected, without extending the generic session API with optional send delegates or nullable responses. McpSessionHandler and the SSE transport's ownership/disconnection handling are unchanged. This PR does not fix the pre-existing request-after-disconnect lifecycle issue. Modern HTTP response-body timeout coverage verifies that aborting the request cancels the server handler without sending notifications/cancelled; this does not generalize disconnect cancellation to legacy resumable sessions.
  • This builds on independently merged fix(client): skip the SSE fallback when the server/discover probe is rejected with 400/404 #1855: its early unstructured discovery 400/404 path still retries initialize before attempting SSE. That contribution and its tests are in the base, not part of this PR. Other HTTP status classification is unchanged; this does not implement fix: fall back to SSE when AutoDetect probe gets 405 with JSON-RPC error body #1849 or fix: propagate auth errors (401/403) immediately instead of falling b… #1444.

Original CI failure

The regression was observed in Build and Test run 29739945453, Ubuntu Debug job 88344129696:

Assert.Equal() Failure: Collections differ
Expected: ["/.well-known/oauth-protected-resource/mcp", "/.well-known/oauth-protected-resource"]
Actual:   ["/.well-known/oauth-protected-resource/mcp", "/.well-known/oauth-protected-resource", "/.well-known/oauth-protected-resource/mcp", "/.well-known/oauth-protected-resource"]

The logs show initialize starting after the probe deadline interrupted the first OAuth exchange:

[11:53:26] ... sending method 'server/discover' request.
[11:53:26] ... POST https://localhost:7029/token
[11:53:31] ... sending method 'initialize' request.
[11:53:32] ... POST https://localhost:7029/token - 499 ... 5830.9035ms

The original metadata-path regression now deliberately holds metadata work beyond the probe budget and asserts one token exchange, each fallback metadata path requested once, and retained modern negotiation.

Validation

Validated locally on Windows against main c40ee044 (includes #1855), with normal audit settings and warnings treated as errors:

  • dotnet build --no-restore -v:q: zero warnings/errors; includes Core targets .NET 8/9/10 and .NET Standard 2.0.
  • Core suite excluding manual tests, ClientIntegrationTests, and DockerEverythingServerTests: 2,338 passed, 2 skipped on each of .NET 8/9/10; 2,029 passed, 284 skipped on .NET Framework 4.7.2. Includes local stdio tests, version/timeout handling, and fix(client): skip the SSE fallback when the server/discover probe is rejected with 400/404 #1855's AutoDetect regressions.
  • ASP.NET Core selection OAuth|July2026|Sse|StreamableHttp|MapMcp|RequestAbortCancellationTests: 429 passed, 30 skipped on each of .NET 8/9/10. The five explicit-SSE modern MRTR cases run again; no additional MRTR skip conditions remain.

An initial .NET 9 run hit a schema-generation failure in Mrtr_MixedExceptionAndAwaitStyle: missing JsonTypeInfo for McpServer during tool construction, before any client connection. The same failure reproduced on unmodified main. The individual SSE case and the full .NET 9 selection passed on rerun. No code or test changes were made to work around it.

New and refined tests reuse the real SDK OAuth and SSE fixtures, with gates for ordering. They cover slow metadata/callback/silent acquisition, caller/initialization cancellation, fresh retry budget, two-client timer isolation, initial/retry headers and stalled response bodies, actual modern HTTP abort, default SSE initialization, explicit modern SSE negotiation/MRTR and silent-discovery timeout, AutoDetect pinning/deadlines, and finite/infinite/out-of-range timeout combinations. The timeout helper is covered through client behavior rather than a shared-source helper test assembly.

Overlapping slow-auth cases and waits were consolidated: callback survival uses another request's observed probe expiry as a positive control, pinning rows do not each repeat the same slow login, and response-header/body tests authenticate immediately. In representative focused .NET 10 runs, the added/strengthened cases fell from 29.4 to 20.7 seconds summed across tests; the HTTP subset's wall time, including its runner, fell from 13.7 to 9.9 seconds. These measurements are not extrapolated to other frameworks.

The external npm/TestServer integration class did not finish during the earlier prototype validation and was not rerun for this refinement; its cause was not established. No claim is made for that class, Docker/manual tests, the entire ASP.NET Core suite, conformance, or NativeAOT publishing.

@halter73

Copy link
Copy Markdown
Contributor Author

@PranavSenthilnathan I'm also supportive of increasing the test timeouts, but I think it's a functional issue to have the probe timeout affected by the OAuth flow. I think it makes sense for the initialize timeout to still be in force, but how long the OAuth flow takes shouldn't affect protocol negotiation.

Comment thread src/ModelContextProtocol.Core/Client/McpClientImpl.cs Outdated
Comment thread src/ModelContextProtocol.Core/McpSessionHandler.cs Outdated
@jeffhandley jeffhandley added this to the 2.x milestone Jul 27, 2026
@halter73
halter73 marked this pull request as draft July 27, 2026 23:49
@halllo

halllo commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Looking forward for this. My current workaround is options.DiscoverProbeTimeout = Timeout.InfiniteTimeSpan;.

Similar:
#1830
#1831

Pause the request-local discovery timer during SDK token acquisition while keeping response waits, initialization, and caller cancellation bounded. Stop the abandoned probe when AutoDetect selects SSE and initialize by default, while honoring explicit modern SSE configuration.

Preserve the existing transport lifecycle and add focused authentication, deadline, and protocol fallback regression coverage on top of #1855.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@halter73
halter73 force-pushed the halter73-release-priority-triage branch from 596c3f9 to 8b14805 Compare September 19, 2026 04:28
@halter73
halter73 marked this pull request as ready for review September 19, 2026 04:29
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.

4 participants