fix(server): fire onsessionclosed once when DELETEs overlap (v1.x) - #2587
Draft
axits-lab wants to merge 2 commits into
Draft
fix(server): fire onsessionclosed once when DELETEs overlap (v1.x)#2587axits-lab wants to merge 2 commits into
axits-lab wants to merge 2 commits into
Conversation
Backport of the main-branch fix (modelcontextprotocol#2583) to v1.x, where the same window exists in `WebStandardStreamableHTTPServerTransport`. `handleDeleteRequest` awaited `onsessionclosed` before `close()` ran, so `_closed` was still false while the callback was in flight. A second DELETE for the same session passed `validateSession`/`validateProtocolVersion` unchanged — neither consults `_closed` — and invoked the callback again for a session already being torn down. Claim the notification synchronously before the await. `_closed` cannot serve this purpose: it is set by `close()`, which only runs once the callback has settled. DELETE stays idempotent — a concurrent or repeat request still terminates the session and answers 200, it just does not re-run the callback. The regression test drives `handleRequest()` directly rather than a real socket: over HTTP the callback release wins the race, the two DELETEs serialize, and the bug does not reproduce. Refs modelcontextprotocol#2562 (item 2) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 1312370 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
commit: |
`prettier --check .` covers .changeset/*.md on this branch; the file was written by hand and never formatted, which failed the build job. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
v1.xbackport of #2583. Refs #2562 (item 2), which notes the item applies to both branches — I offered this on the issue and confirmed the window is present here before writing anything.The race, on this branch
WebStandardStreamableHTTPServerTransport.handleDeleteRequestis byte-identical to main's pre-fix code:_closedis set byclose(), so it staysfalsefor as long as the callback is in flight. NeithervalidateSessionnorvalidateProtocolVersionconsults it, so a second DELETE arriving in that window passes every guard and fires the callback again for a session already being torn down.Fix
Claim the notification synchronously, before the await —
_closedcannot serve the purpose since by construction it is only set after the callback settles. DELETE stays idempotent: a concurrent or repeat request still terminates the session and answers200, it just does not re-run the callback.Test — one note worth flagging
The regression test drives
transport.handleRequest()directly rather than going through a real socket.My first attempt used
fetchagainstcreateTestServer, matching the surroundingonsessionclosedtests, and it passed without the fix — over HTTP the callback release wins the race, so the two DELETEs simply serialize and the bug never surfaces. Driving the transport in-process makes the second DELETE provably reach the handler while the first is parked. It follows the existingWebStandardStreamableHTTPServerTransport - onerror callbackblock, which already tests this way.Verified it fails without the src change:
Verification
1640/1640tests pass across 52 filesnpm run lintandnpm run typecheckcleanChangeset included.