diff --git a/.changeset/delete-race-onsessionclosed.md b/.changeset/delete-race-onsessionclosed.md new file mode 100644 index 0000000000..5a8515e689 --- /dev/null +++ b/.changeset/delete-race-onsessionclosed.md @@ -0,0 +1,5 @@ +--- +'@modelcontextprotocol/server': patch +--- + +Fire `onsessionclosed` at most once when DELETE requests for the same session overlap. The first DELETE awaited the callback before `close()` ran, so `_closed` was still `false` and a second DELETE passed the same guards and invoked the callback again for a session already being torn down. The notification is now claimed synchronously before the await. DELETE remains idempotent — a concurrent or repeat request still terminates the session and answers 200. diff --git a/packages/server/src/server/streamableHttp.ts b/packages/server/src/server/streamableHttp.ts index c0f48560a2..a319fc7b92 100644 --- a/packages/server/src/server/streamableHttp.ts +++ b/packages/server/src/server/streamableHttp.ts @@ -241,6 +241,12 @@ export class WebStandardStreamableHTTPServerTransport implements Transport { private sessionIdGenerator: (() => string) | undefined; private _started: boolean = false; private _closed: boolean = false; + /** + * Set synchronously before `onsessionclosed` is awaited, so a DELETE that arrives while the + * callback is in flight does not fire it again. `_closed` cannot serve this purpose: it is set + * by `close()`, which only runs once the callback has already settled. + */ + private _sessionClosedNotified: boolean = false; private _streamMapping: Map = new Map(); private _requestToStreamMapping: Map = new Map(); private _requestResponseMap: Map = new Map(); @@ -984,7 +990,13 @@ export class WebStandardStreamableHTTPServerTransport implements Transport { } try { - await Promise.resolve(this._onsessionclosed?.(this.sessionId!)); + // Claim the notification before awaiting it — see `_sessionClosedNotified`. DELETE stays + // idempotent: a concurrent or repeat request still terminates the session and answers 200, + // it just does not re-run the callback. + if (!this._sessionClosedNotified) { + this._sessionClosedNotified = true; + await Promise.resolve(this._onsessionclosed?.(this.sessionId!)); + } return new Response(null, { status: 200 }); } finally { await this.close(); diff --git a/packages/server/test/server/streamableHttp.test.ts b/packages/server/test/server/streamableHttp.test.ts index 9ec6baf46c..3eb212c79b 100644 --- a/packages/server/test/server/streamableHttp.test.ts +++ b/packages/server/test/server/streamableHttp.test.ts @@ -602,6 +602,36 @@ describe('Zod v4', () => { expect(onClosed).toHaveBeenCalledWith('test-session-456'); }); + + it('fires onsessionclosed once when two DELETEs arrive concurrently', async () => { + // The first DELETE parks on the callback await; `_closed` is only set by close() in the + // finally, so a second DELETE passes the same guards and fires the callback again for a + // session that is already being torn down. + let releaseCallback!: () => void; + const callbackGate = new Promise(resolve => { + releaseCallback = resolve; + }); + const onClosed = vi.fn().mockReturnValue(callbackGate); + + const mcpServer = new McpServer({ name: 'test-server', version: '1.0.0' }, { capabilities: {} }); + const transport = new WebStandardStreamableHTTPServerTransport({ + sessionIdGenerator: () => 'test-session-789', + onsessionclosed: onClosed + }); + + await mcpServer.connect(transport); + await transport.handleRequest(createRequest('POST', TEST_MESSAGES.initialize)); + + const sendDelete = (): Promise => + transport.handleRequest(createRequest('DELETE', undefined, { sessionId: 'test-session-789' })); + + const first = sendDelete(); + const second = sendDelete(); + releaseCallback(); + await Promise.all([first, second]); + + expect(onClosed).toHaveBeenCalledTimes(1); + }); }); describe('HTTPServerTransport - Event Store (Resumability)', () => {