fix: silence respond() when concurrent cancellation already completed request#2418
Open
manjunathgujjar wants to merge 2 commits intomodelcontextprotocol:mainfrom
Open
Conversation
… request When a CancelledNotification arrives after a handler returns its result but before respond() is called, cancel() sets _completed = True and sends an error response. The subsequent respond() call was crashing with AssertionError because the assert guarded against double-respond but didn't account for this legitimate concurrent-cancellation window. Replace the assert with an early return so that respond() becomes a no-op when _completed is already True, matching the intended semantics: the request is already handled, so the late response is safely discarded. Fixes modelcontextprotocol#2416
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.
Problem
When a
CancelledNotificationarrives after a request handler returns its result but beforerespond()is called, a race window exists in_handle_request:cancel()sets_completed = Trueand sends an error response to the clientrespond()is then called — and crashes withAssertionError: Request already responded toThis manifests as an unhandled exception in the server's task group, reported in #2416.
Root Cause
respond()used anassertto guard against double-respond:This is correct for programming errors (calling
respond()twice) but wrong for the concurrent-cancellation case, where_completed = Truewas legitimately set by a racingcancel()call. The assertion doesn't distinguish between the two scenarios.Fix
Replace the assert with an early return:
When
_completedis alreadyTruedue to a prior cancellation,respond()silently discards the late result — the request is already handled, so there is nothing useful to send.Test
Added
tests/server/test_respond_after_cancellation.pywhich simulates the race by directly setting_completed = Truebefore callingrespond(), verifying that no exception is raised.