Conversation
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. |
halter73
marked this pull request as draft
July 27, 2026 23:49
Contributor
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
force-pushed
the
halter73-release-priority-triage
branch
from
September 19, 2026 04:28
596c3f9 to
8b14805
Compare
halter73
marked this pull request as ready for review
September 19, 2026 04:29
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Exclude SDK OAuth token acquisition from
DiscoverProbeTimeoutwithout removing the deadline from the actualserver/discoverrequest.The short probe exists to detect older servers that silently ignore discovery. Today it can instead interrupt metadata discovery or interactive login, then start
initializeand 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 byInitializationTimeout.Cancellation and transport behavior
InitializationTimeoutremain 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.initialize, notserver/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.HttpTransportMode.SsewithProtocolVersion = "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 to2026-07-28still rejects the initialize fallback if SSE is selected; it does not silently downgrade. Server-side acceptance is unchanged.McpSessionHandlerand 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 sendingnotifications/cancelled; this does not generalize disconnect cancellation to legacy resumable sessions.400/404path 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:
The logs show
initializestarting after the probe deadline interrupted the first OAuth exchange: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.ClientIntegrationTests, andDockerEverythingServerTests: 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.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: missingJsonTypeInfoforMcpServerduring 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.