mcp: do not re-prompt OAuth after cancelled Authorize by ravyg · Pull Request #885 · modelcontextprotocol/go-sdk

ravyg · 2026-04-08T04:52:45Z

When OAuthHandler.Authorize returns a context-cancelled error, the cancellation notification that the call layer sends in response to the same ctx cancellation is routed back through the streamable client transport, re-triggering the 401 → Authorize path and prompting the user a second time for a request they have already abandoned.

Fix in two layers:

mcp/transport.go — bound the cancellation Notify with a fresh context.Background()-derived timeout instead of reusing the cancelled caller context. This guarantees the caller never blocks on a doomed notify against a degraded connection.
mcp/streamable.go — when Authorize returns a context-cancelled error, mark the streamable client connection as failed via c.fail(). The existing failure check in Write() then short-circuits the cancellation notification without re-invoking the OAuth handler, matching the bug reporter's expectation that cancelling Authorize abandons the connection.

Includes a regression test (TestStreamableClientOAuth_CancelledAuthorize_NoReprompt) with a blocking OAuthHandler that asserts Authorize is invoked exactly once. The test was verified to fail on unfixed code (got 2 calls) and pass with the fix.

Fixes #882

jba · 2026-04-08T20:30:54Z

+		// attempt to deliver but never blocks the caller indefinitely.
+		notifyCtx, cancelNotify := context.WithTimeout(context.Background(), notifyCancellationTimeout)
+		defer cancelNotify()
+		err := conn.Notify(notifyCtx, notificationCancelled, &CancelledParams{


Won't this still trigger a second auth flow?

On its own, yes, it would, but the companion change in mcp/streamable.go (calling c.fail() when Authorize returns a cancelled error) is what actually prevents the second auth flow.

Doesn't xcontext.Detach accomplish the same thing?

Please use xcontext.Detach instead of context.Background when building the context. This will preserve the context values.

Or event better: use https://pkg.go.dev/context#WithoutCancel.

smlx · 2026-04-09T02:51:03Z

+			// sends in response to ctx cancellation) short-circuit instead of
+			// re-invoking the OAuth handler. Otherwise the user gets prompted
+			// to authorize a request they have already abandoned. See #882.
+			if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) {


This PR doesn't fix the bug I'm seeing and I think it is because this logic is not quite right.

You shouldn't check the error returned from Authorize() because it could be anything. The error to inspect is the one returned from the context passed into Authorize()

Suggested change

if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) {

ctxErr := ctx.Err()

if errors.Is(ctxErr, context.Canceled) || errors.Is(ctxErr, context.DeadlineExceeded) {

I tested this patch on top of your PR changes and it fixes the bug for me :-)

ravyg

Good catch @smlx — you're right, the handler is user-implemented and may not wrap context.Canceled. Pushed the fix in 8d130aa checking ctx.Err() directly per your suggestion, and updated the test mock to return a non-wrapping error so it exercises the real-world path. Thanks for testing against your setup!

Going to spend some more time today (PST) diving deeper into this area as well.

ravyg · 2026-04-09T16:14:11Z

Good catch @smlx — you're right, the handler is user-implemented and may not wrap context.Canceled. Pushed the fix in 8d130aa checking ctx.Err() directly per your suggestion, and updated the test mock to return a non-wrapping error so it exercises the real-world path. Thanks for testing against your setup!

Going to spend some more time today (PST) diving deeper into this area as well.

smlx

Fixes the bug I was seeing. Thanks!

maciej-kisiel · 2026-04-24T10:29:25Z

@ravyg We're one small adjustment from accepting and merging this. Will you have time to take a look in the nearest future?

ravyg · 2026-04-24T13:46:11Z

Yes, just saw your comments above, adding those, I totally missed those earlier, thanks for pointing out.

When OAuthHandler.Authorize returned a context-cancelled error, the cancellation notification the call layer sent in response to the same ctx cancellation was routed back through the streamable client transport, re-triggering the 401 -> Authorize path and prompting the user a second time for a request they had already abandoned. Fix in two layers: 1. mcp/transport.go: bound the cancellation Notify with a fresh context.Background()-derived timeout instead of reusing the cancelled caller context. This guarantees the caller never blocks on a doomed notify against a degraded connection. 2. mcp/streamable.go: when Authorize returns a context-cancelled error, mark the streamable client connection as failed via c.fail(). The existing failure check in Write() then short-circuits the cancellation notification without re-invoking the OAuth handler, matching the bug reporter's expectation that cancelling Authorize abandons the connection. Includes a regression test with a blocking OAuth handler that asserts Authorize is invoked exactly once. The test was verified to fail on unfixed code (got 2 calls) and pass with the fix. Fixes modelcontextprotocol#882

@smlx

The cancellation check in streamableClientConn.Write inspected the error returned by OAuthHandler.Authorize to decide whether the caller abandoned the request. This failed for real-world handlers that return a custom error rather than wrapping context.Canceled (e.g. fmt.Errorf("oauth flow interrupted")). Check ctx.Err() directly — it is the authoritative source for whether the caller's context was cancelled, regardless of what Authorize returns. Update the regression test mock to return a non-wrapping error so it exercises the real-world path. Verified: test fails (got 2 Authorize calls) with the old errors.Is(err, ...) check, passes with ctx.Err(). Thanks to @smlx for identifying the bug and suggesting the fix. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

…ellation notify Preserves context values (e.g. auth credentials, tracing) while still detaching from the parent's cancellation signal.

ravyg marked this pull request as ready for review April 8, 2026 05:05

jba reviewed Apr 8, 2026

View reviewed changes

smlx suggested changes Apr 9, 2026

View reviewed changes

ravyg requested a review from smlx April 9, 2026 16:11

ravyg commented Apr 9, 2026

View reviewed changes

smlx approved these changes Apr 10, 2026

View reviewed changes

ravyg and others added 3 commits April 24, 2026 07:04

mcp: use context.WithoutCancel instead of context.Background for canc…

f3ed4f3

…ellation notify Preserves context values (e.g. auth credentials, tracing) while still detaching from the parent's cancellation signal.

ravyg force-pushed the fix/882-auth-cancel-no-renotify branch from 301a654 to f3ed4f3 Compare April 24, 2026 14:16

maciej-kisiel approved these changes Apr 24, 2026

View reviewed changes

maciej-kisiel merged commit db50910 into modelcontextprotocol:main Apr 24, 2026
9 checks passed

inhuman mentioned this pull request Aug 6, 2026

mcp: best-effort cancellation notification blocks the caller's return past its deadline #1150

Closed

Navigation Menu

Uh oh!

mcp: do not re-prompt OAuth after cancelled Authorize - #885

mcp: do not re-prompt OAuth after cancelled Authorize#885
maciej-kisiel merged 3 commits into
modelcontextprotocol:mainfrom
ravyg:fix/882-auth-cancel-no-renotify

ravyg commented Apr 8, 2026

Uh oh!

jba Apr 8, 2026

Uh oh!

ravyg Apr 9, 2026

Uh oh!

jba Apr 9, 2026

Uh oh!

maciej-kisiel Apr 21, 2026

Uh oh!

maciej-kisiel Apr 21, 2026

Uh oh!

smlx Apr 9, 2026

Uh oh!

ravyg left a comment

Uh oh!

ravyg commented Apr 9, 2026

Uh oh!

smlx left a comment

Uh oh!

maciej-kisiel commented Apr 24, 2026

Uh oh!

ravyg commented Apr 24, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	if errors.Is(err, context.Canceled) \|\| errors.Is(err, context.DeadlineExceeded) {
	ctxErr := ctx.Err()
	if errors.Is(ctxErr, context.Canceled) \|\| errors.Is(ctxErr, context.DeadlineExceeded) {

Uh oh!

Conversation

ravyg commented Apr 8, 2026

Uh oh!

jba Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

ravyg Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

jba Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

maciej-kisiel Apr 21, 2026

Choose a reason for hiding this comment

Uh oh!

maciej-kisiel Apr 21, 2026

Choose a reason for hiding this comment

Uh oh!

smlx Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

ravyg left a comment

Choose a reason for hiding this comment

Uh oh!

ravyg commented Apr 9, 2026

Uh oh!

smlx left a comment

Choose a reason for hiding this comment

Uh oh!

maciej-kisiel commented Apr 24, 2026

Uh oh!

ravyg commented Apr 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ravyg commented Apr 24, 2026 •

edited

Loading