fix: send HTTP DELETE to terminate server sessions after each scenario (#316)
* fix: send HTTP DELETE to terminate server sessions after each scenario The conformance test harness opened sessions against servers under test but never issued an HTTP DELETE to terminate them, leaving a dangling session on the server after each scenario. This violates the "hermetic" expectation called out in the Streamable HTTP transport spec. - `connection/sdk-client.connectToServer` now calls `transport.terminateSession()` before `client.close()`. A 405 response (DELETE not supported) is tolerated because the spec allows the server to refuse. - A new `terminateSessionRaw` helper handles scenarios that bypass the SDK client and open sessions via raw `fetch` (`lifecycle.ts`, `http-standard-headers.ts`). - `sse-polling.ts` and `sse-multiple-streams.ts`, which manage their own transport, now terminate the session in their existing `finally` block before closing the client. - `lifecycle.test.ts` gains two cases asserting that DELETE is sent with the issued session ID and that no DELETE is sent when the server did not assign one. Verified manually against `examples/servers/typescript/everything-server.ts` that running a server scenario now causes the server to log `Received session termination request for session <id>`. Fixes #79 Signed-off-by: Satoshi Ito <satoshi.ito.tf@hitachi.com> * Use negotiated protocol version for teardown DELETE and bound it Review follow-ups on the session-termination change: - terminateSessionRaw: abort the DELETE after 5s so a wedged server cannot stall suite teardown. - terminateSessionBestEffort: shared SDK-path teardown helper that delegates to terminateSessionRaw with the transport's negotiated protocol version, so the socket is truly released at the bound and the version-mismatch 400 on strict servers (the failure mode of issue #79) goes away. Used by connectToServer close() and both SSE scenarios, replacing their inline try/catch blocks. - lifecycle.ts: the raw session-id probe and its teardown DELETE now use ctx.specVersion instead of a hardcoded 2025-11-25, matching the negotiated-version convention from #415/#416. - Add sdk-client.test.ts regression tests asserting close() sends a DELETE with the issued session ID (and none without one), so a bad future merge cannot silently drop the fix. --------- Signed-off-by: Satoshi Ito <satoshi.ito.tf@hitachi.com> Co-authored-by: Paul Carleton <paulc@anthropic.com>
This commit is contained in:
parent
49103de6ed
commit
c5af5673ba
6 changed files with 196 additions and 8 deletions
77
src/connection/sdk-client.test.ts
Normal file
77
src/connection/sdk-client.test.ts
Normal file
|
|
@ -0,0 +1,77 @@
|
|||
import { describe, it, expect, afterEach } from 'vitest';
|
||||
import express from 'express';
|
||||
import type { Server as HttpServer } from 'http';
|
||||
import { connectToServer } from './sdk-client';
|
||||
|
||||
// Regression guard for the session-termination behavior of close(): a bad
|
||||
// merge of connectToServer must not silently drop the DELETE (issue #79).
|
||||
describe('connectToServer close()', () => {
|
||||
let httpServer: HttpServer | undefined;
|
||||
|
||||
afterEach(
|
||||
() =>
|
||||
new Promise<void>((resolve) => {
|
||||
if (!httpServer) return resolve();
|
||||
httpServer.closeAllConnections?.();
|
||||
httpServer.close(() => resolve());
|
||||
httpServer = undefined;
|
||||
})
|
||||
);
|
||||
|
||||
function startServer(sessionId?: string): Promise<{
|
||||
url: string;
|
||||
deletes: Array<string | undefined>;
|
||||
}> {
|
||||
const deletes: Array<string | undefined> = [];
|
||||
const app = express();
|
||||
app.use(express.json());
|
||||
app.post('/mcp', (req, res) => {
|
||||
if (req.body?.method === 'initialize') {
|
||||
if (sessionId) res.set('mcp-session-id', sessionId);
|
||||
res.json({
|
||||
jsonrpc: '2.0',
|
||||
id: req.body.id,
|
||||
result: {
|
||||
protocolVersion: '2025-11-25',
|
||||
capabilities: {},
|
||||
serverInfo: { name: 'delete-probe', version: '1.0.0' }
|
||||
}
|
||||
});
|
||||
} else {
|
||||
res.status(202).end();
|
||||
}
|
||||
});
|
||||
app.delete('/mcp', (req, res) => {
|
||||
deletes.push(req.headers['mcp-session-id'] as string | undefined);
|
||||
res.status(200).end();
|
||||
});
|
||||
return new Promise((resolve, reject) => {
|
||||
const server = app.listen(0);
|
||||
httpServer = server;
|
||||
server.on('error', reject);
|
||||
server.on('listening', () => {
|
||||
const addr = server.address();
|
||||
const port = typeof addr === 'object' && addr ? addr.port : 0;
|
||||
resolve({ url: `http://localhost:${port}/mcp`, deletes });
|
||||
});
|
||||
});
|
||||
}
|
||||
|
||||
it('sends an HTTP DELETE with the session ID to terminate the session', async () => {
|
||||
const { url, deletes } = await startServer('session-abc-123');
|
||||
|
||||
const connection = await connectToServer(url, {}, '2025-11-25');
|
||||
await connection.close();
|
||||
|
||||
expect(deletes).toEqual(['session-abc-123']);
|
||||
});
|
||||
|
||||
it('does not send DELETE when the server assigned no session ID', async () => {
|
||||
const { url, deletes } = await startServer();
|
||||
|
||||
const connection = await connectToServer(url, {}, '2025-11-25');
|
||||
await connection.close();
|
||||
|
||||
expect(deletes).toEqual([]);
|
||||
});
|
||||
});
|
||||
|
|
@ -121,6 +121,11 @@ function instrumentTransport(
|
|||
* conformant `initialize`. The transport is instrumented so every wire
|
||||
* message in both directions is validated against `specVersion`'s spec
|
||||
* JSON schema; scenarios that call this directly pass `ctx.specVersion`.
|
||||
*
|
||||
* The returned `close()` sends an HTTP DELETE to terminate the server-side
|
||||
* session (per the Streamable HTTP transport spec) before closing the client,
|
||||
* so each scenario leaves the server hermetic. A server that responds 405
|
||||
* (DELETE not supported) is tolerated, since the spec allows that.
|
||||
*/
|
||||
export async function connectToServer(
|
||||
serverUrl: string,
|
||||
|
|
@ -139,11 +144,60 @@ export async function connectToServer(
|
|||
return {
|
||||
client,
|
||||
close: async () => {
|
||||
await terminateSessionBestEffort(transport, serverUrl);
|
||||
await client.close();
|
||||
}
|
||||
};
|
||||
}
|
||||
|
||||
// Shared teardown bound so a wedged server-under-test cannot stall the suite.
|
||||
const SESSION_TERMINATE_TIMEOUT_MS = 5000;
|
||||
|
||||
/**
|
||||
* Best-effort session termination for an SDK transport. Delegates to
|
||||
* terminateSessionRaw (with the transport's negotiated protocol version) so
|
||||
* the DELETE is truly bounded — the socket is aborted at the timeout rather
|
||||
* than left pending against a wedged server. No-op when the server never
|
||||
* issued a session.
|
||||
*/
|
||||
export async function terminateSessionBestEffort(
|
||||
transport: StreamableHTTPClientTransport,
|
||||
serverUrl: string
|
||||
): Promise<void> {
|
||||
if (!transport.sessionId || !transport.protocolVersion) return;
|
||||
await terminateSessionRaw(
|
||||
serverUrl,
|
||||
transport.sessionId,
|
||||
transport.protocolVersion
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* Best-effort HTTP DELETE to terminate a raw (non-SDK) session.
|
||||
*
|
||||
* Used by scenarios that open sessions via raw `fetch` instead of going through
|
||||
* the SDK's StreamableHTTPClientTransport. Failures are swallowed because the
|
||||
* spec allows servers to respond 405, and cleanup must not derail the scenario.
|
||||
*/
|
||||
export async function terminateSessionRaw(
|
||||
serverUrl: string,
|
||||
sessionId: string,
|
||||
protocolVersion: string
|
||||
): Promise<void> {
|
||||
try {
|
||||
await fetch(serverUrl, {
|
||||
method: 'DELETE',
|
||||
headers: {
|
||||
'mcp-session-id': sessionId,
|
||||
'MCP-Protocol-Version': protocolVersion
|
||||
},
|
||||
signal: AbortSignal.timeout(SESSION_TERMINATE_TIMEOUT_MS)
|
||||
});
|
||||
} catch {
|
||||
// best-effort cleanup
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Helper to collect notifications (logging and progress)
|
||||
*/
|
||||
|
|
|
|||
|
|
@ -2,9 +2,14 @@ import { testContext } from '../../connection/testing';
|
|||
import { ServerInitializeScenario } from './lifecycle';
|
||||
import { connectToServer } from '../../connection/sdk-client';
|
||||
|
||||
vi.mock('../../connection/sdk-client', () => ({
|
||||
connectToServer: vi.fn()
|
||||
}));
|
||||
vi.mock('../../connection/sdk-client', async (importOriginal) => {
|
||||
const actual =
|
||||
await importOriginal<typeof import('../../connection/sdk-client')>();
|
||||
return {
|
||||
...actual,
|
||||
connectToServer: vi.fn()
|
||||
};
|
||||
});
|
||||
|
||||
describe('ServerInitializeScenario', () => {
|
||||
const serverUrl = 'http://localhost:3000/mcp';
|
||||
|
|
@ -102,4 +107,37 @@ describe('ServerInitializeScenario', () => {
|
|||
}
|
||||
});
|
||||
});
|
||||
|
||||
it('sends an HTTP DELETE with the issued session ID to terminate the raw session', async () => {
|
||||
fetchMock.mockResolvedValue(
|
||||
new Response(null, {
|
||||
headers: {
|
||||
'mcp-session-id': 'session-123_ABC'
|
||||
}
|
||||
})
|
||||
);
|
||||
|
||||
await new ServerInitializeScenario().run(testContext(serverUrl));
|
||||
|
||||
expect(fetchMock).toHaveBeenCalledWith(
|
||||
serverUrl,
|
||||
expect.objectContaining({
|
||||
method: 'DELETE',
|
||||
headers: expect.objectContaining({
|
||||
'mcp-session-id': 'session-123_ABC'
|
||||
})
|
||||
})
|
||||
);
|
||||
});
|
||||
|
||||
it('does not send DELETE when the server did not assign a session ID', async () => {
|
||||
fetchMock.mockResolvedValue(new Response(null));
|
||||
|
||||
await new ServerInitializeScenario().run(testContext(serverUrl));
|
||||
|
||||
const deleteCalls = fetchMock.mock.calls.filter(
|
||||
([, init]) => (init as RequestInit | undefined)?.method === 'DELETE'
|
||||
);
|
||||
expect(deleteCalls).toHaveLength(0);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -8,7 +8,10 @@ import {
|
|||
DRAFT_PROTOCOL_VERSION
|
||||
} from '../../types';
|
||||
import type { RunContext } from '../../connection';
|
||||
import { connectToServer } from '../../connection/sdk-client';
|
||||
import {
|
||||
connectToServer,
|
||||
terminateSessionRaw
|
||||
} from '../../connection/sdk-client';
|
||||
|
||||
const VISIBLE_ASCII_REGEX = /^[\x21-\x7E]+$/;
|
||||
|
||||
|
|
@ -91,20 +94,21 @@ and validates session ID format if one is assigned.`;
|
|||
// Check: Session ID visible ASCII validation
|
||||
// Use a raw fetch to inspect the MCP-Session-Id response header,
|
||||
// since the SDK client transport does not expose it.
|
||||
let rawSessionId: string | null = null;
|
||||
try {
|
||||
const response = await fetch(serverUrl, {
|
||||
method: 'POST',
|
||||
headers: {
|
||||
'Content-Type': 'application/json',
|
||||
Accept: 'application/json, text/event-stream',
|
||||
'mcp-protocol-version': '2025-11-25'
|
||||
'mcp-protocol-version': ctx.specVersion
|
||||
},
|
||||
body: JSON.stringify({
|
||||
jsonrpc: '2.0',
|
||||
id: 1,
|
||||
method: 'initialize',
|
||||
params: {
|
||||
protocolVersion: '2025-11-25',
|
||||
protocolVersion: ctx.specVersion,
|
||||
capabilities: {},
|
||||
clientInfo: {
|
||||
name: 'conformance-session-id-test',
|
||||
|
|
@ -115,6 +119,7 @@ and validates session ID format if one is assigned.`;
|
|||
});
|
||||
|
||||
const sessionId = response.headers.get('mcp-session-id');
|
||||
rawSessionId = sessionId;
|
||||
|
||||
if (!sessionId) {
|
||||
checks.push({
|
||||
|
|
@ -170,6 +175,10 @@ and validates session ID format if one is assigned.`;
|
|||
errorMessage: `Failed to send initialize request for session ID check: ${error instanceof Error ? error.message : String(error)}`,
|
||||
specReferences: SESSION_SPEC_REFERENCES
|
||||
});
|
||||
} finally {
|
||||
if (rawSessionId) {
|
||||
await terminateSessionRaw(serverUrl, rawSessionId, ctx.specVersion);
|
||||
}
|
||||
}
|
||||
|
||||
return checks;
|
||||
|
|
|
|||
|
|
@ -18,6 +18,7 @@ import { buildStandardHeaders, type RunContext } from '../../connection';
|
|||
import { EventSourceParserStream } from 'eventsource-parser/stream';
|
||||
import { Client } from '@modelcontextprotocol/sdk/client/index.js';
|
||||
import { StreamableHTTPClientTransport } from '@modelcontextprotocol/sdk/client/streamableHttp.js';
|
||||
import { terminateSessionBestEffort } from '../../connection/sdk-client';
|
||||
|
||||
export class ServerSSEMultipleStreamsScenario implements ClientScenario {
|
||||
name = 'server-sse-multiple-streams';
|
||||
|
|
@ -293,7 +294,11 @@ export class ServerSSEMultipleStreamsScenario implements ClientScenario {
|
|||
]
|
||||
});
|
||||
} finally {
|
||||
// Clean up
|
||||
// Clean up: terminate the session before closing the client so the
|
||||
// server is left hermetic.
|
||||
if (transport) {
|
||||
await terminateSessionBestEffort(transport, serverUrl);
|
||||
}
|
||||
if (client) {
|
||||
try {
|
||||
await client.close();
|
||||
|
|
|
|||
|
|
@ -17,6 +17,7 @@ import type { RunContext } from '../../connection';
|
|||
import { EventSourceParserStream } from 'eventsource-parser/stream';
|
||||
import { Client } from '@modelcontextprotocol/sdk/client/index.js';
|
||||
import { StreamableHTTPClientTransport } from '@modelcontextprotocol/sdk/client/streamableHttp.js';
|
||||
import { terminateSessionBestEffort } from '../../connection/sdk-client';
|
||||
|
||||
function createLoggingFetch(checks: ConformanceCheck[]) {
|
||||
return async (url: string, options: RequestInit): Promise<Response> => {
|
||||
|
|
@ -598,7 +599,11 @@ export class ServerSSEPollingScenario implements ClientScenario {
|
|||
]
|
||||
});
|
||||
} finally {
|
||||
// Clean up
|
||||
// Clean up: terminate the session before closing the client so the
|
||||
// server is left hermetic.
|
||||
if (transport) {
|
||||
await terminateSessionBestEffort(transport, serverUrl);
|
||||
}
|
||||
if (client) {
|
||||
try {
|
||||
await client.close();
|
||||
|
|
|
|||
Loading…
Reference in a new issue