Skip to content

Commit 8d130aa

Browse files
ravygclaude
andcommitted
mcp: check ctx.Err() instead of Authorize error for cancellation
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>
1 parent 57e25f4 commit 8d130aa

2 files changed

Lines changed: 18 additions & 5 deletions

File tree

mcp/streamable.go

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1807,7 +1807,14 @@ func (c *streamableClientConn) Write(ctx context.Context, msg jsonrpc.Message) e
18071807
// sends in response to ctx cancellation) short-circuit instead of
18081808
// re-invoking the OAuth handler. Otherwise the user gets prompted
18091809
// to authorize a request they have already abandoned. See #882.
1810-
if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) {
1810+
//
1811+
// We check ctx.Err() rather than the error returned by Authorize,
1812+
// because the handler is user-implemented and may return an error
1813+
// that does not wrap context.Canceled (e.g. a custom sentinel or
1814+
// a fmt.Errorf with %v). The context itself is the authoritative
1815+
// source for whether the caller abandoned the request.
1816+
ctxErr := ctx.Err()
1817+
if errors.Is(ctxErr, context.Canceled) || errors.Is(ctxErr, context.DeadlineExceeded) {
18111818
c.fail(fmt.Errorf("%s: authorization cancelled: %w", requestSummary, err))
18121819
}
18131820
// Wrap with ErrRejected so the jsonrpc2 connection doesn't set writeErr

mcp/streamable_client_test.go

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1017,9 +1017,12 @@ func TestStreamableClientOAuth_401(t *testing.T) {
10171017
}
10181018

10191019
// blockingCountingOAuthHandler is an OAuthHandler that blocks inside
1020-
// Authorize until the caller's context is cancelled, then returns ctx.Err().
1021-
// It records how many times Authorize is invoked. Used by the regression
1022-
// test for #882.
1020+
// Authorize until the caller's context is cancelled, then returns a custom
1021+
// error that does NOT wrap context.Canceled. This mirrors real-world OAuth
1022+
// handlers that catch the cancellation internally and surface their own
1023+
// error type. The fix for #882 checks ctx.Err() directly rather than
1024+
// relying on the error from Authorize, so this must still trigger c.fail().
1025+
// It records how many times Authorize is invoked.
10231026
type blockingCountingOAuthHandler struct {
10241027
mu sync.Mutex
10251028
callCount int
@@ -1036,7 +1039,10 @@ func (h *blockingCountingOAuthHandler) Authorize(ctx context.Context, req *http.
10361039
// Block until the caller's context is cancelled, mirroring an
10371040
// interactive OAuth flow that the user has abandoned.
10381041
<-ctx.Done()
1039-
return ctx.Err()
1042+
// Return a custom error that does not wrap context.Canceled, as a
1043+
// real-world handler might. The code under test must check ctx.Err()
1044+
// to detect the cancellation, not this error.
1045+
return fmt.Errorf("oauth flow interrupted")
10401046
}
10411047

10421048
func (h *blockingCountingOAuthHandler) Calls() int {

0 commit comments

Comments
 (0)