Skip to content

fix(server): fire onsessionclosed once when DELETEs overlap - #2583

Draft
axits-lab wants to merge 1 commit into
modelcontextprotocol:mainfrom
axits-lab:fix/delete-race-onsessionclosed
Draft

fix(server): fire onsessionclosed once when DELETEs overlap#2583
axits-lab wants to merge 1 commit into
modelcontextprotocol:mainfrom
axits-lab:fix/delete-race-onsessionclosed

Conversation

@axits-lab

Copy link
Copy Markdown

Refs #2562 — item 2 of three. Items 1 and 3 are not touched here; item 1 will follow separately, and item 3 looks like it needs a call on the EventStore contract first.

Opened as a draft: non-write-access users are capped at one open PR on this repo, and #2582 is currently using it. Ready for review as-is — happy to mark it ready whenever that one lands or on your say-so.

The race

handleDeleteRequest awaits onsessionclosed and only then runs close() from the finally:

try {
    await Promise.resolve(this._onsessionclosed?.(this.sessionId!));
    return new Response(null, { status: 200 });
} finally {
    await this.close();
}

_closed is set by close(), so it is still false for the whole time the callback is in flight. Neither validateSession nor validateProtocolVersion consults _closed — they check sessionIdGenerator, _initialized, and the session-id match — so a second DELETE arriving in that window passes every guard and fires the callback a second time for a session already being torn down.

Fix

Claim the notification synchronously, before the await. _closed cannot serve this purpose, since by construction it is only set after the callback has settled — hence the separate flag.

DELETE stays idempotent: a concurrent or repeat request still terminates the session and answers 200, it just does not re-run the callback.

Test

Added to the existing Session Callbacks block. It holds the callback open on a promise the test controls, issues the second DELETE while the first is parked on it, then releases:

× fires onsessionclosed once when two DELETEs arrive concurrently
  AssertionError: expected "vi.fn()" to be called 1 times, but got 2 times

That is the failure on main without the src change; it passes with it.

Verification

  • 469/469 tests pass in @modelcontextprotocol/server (53 in streamableHttp.test.ts)
  • pnpm lint and pnpm typecheck clean
  • Grepped the workspace: onsessionclosed is implemented only in packages/server/src/server/streamableHttp.ts, so no sibling transport carries the same window

Changeset included.

You noted the item applies to v1.x as well — happy to put up the equivalent there if you'd like it as a separate PR.

`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.

Refs modelcontextprotocol#2562 (item 2)

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@changeset-bot

changeset-bot Bot commented Jul 30, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: ac6312f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 6 packages
Name Type
@modelcontextprotocol/server Patch
@modelcontextprotocol/core Patch
@modelcontextprotocol/client Patch
@modelcontextprotocol/server-legacy Patch
@modelcontextprotocol/codemod Patch
@modelcontextprotocol/core-internal Patch

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

@pkg-pr-new

pkg-pr-new Bot commented Jul 30, 2026

Copy link
Copy Markdown

Open in StackBlitz

@modelcontextprotocol/client

npm i https://pkg.pr.new/@modelcontextprotocol/client@2583

@modelcontextprotocol/codemod

npm i https://pkg.pr.new/@modelcontextprotocol/codemod@2583

@modelcontextprotocol/core

npm i https://pkg.pr.new/@modelcontextprotocol/core@2583

@modelcontextprotocol/server

npm i https://pkg.pr.new/@modelcontextprotocol/server@2583

@modelcontextprotocol/server-legacy

npm i https://pkg.pr.new/@modelcontextprotocol/server-legacy@2583

@modelcontextprotocol/express

npm i https://pkg.pr.new/@modelcontextprotocol/express@2583

@modelcontextprotocol/fastify

npm i https://pkg.pr.new/@modelcontextprotocol/fastify@2583

@modelcontextprotocol/hono

npm i https://pkg.pr.new/@modelcontextprotocol/hono@2583

@modelcontextprotocol/node

npm i https://pkg.pr.new/@modelcontextprotocol/node@2583

commit: ac6312f

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant