Skip to content

fix: send HTTP DELETE to terminate server sessions after each scenario - #316

Merged
pcarleton merged 3 commits into
modelcontextprotocol:mainfrom
pugsatoshi:fix/79-session-cleanup
Jul 31, 2026
Merged

fix: send HTTP DELETE to terminate server sessions after each scenario#316
pcarleton merged 3 commits into
modelcontextprotocol:mainfrom
pugsatoshi:fix/79-session-cleanup

Conversation

@pugsatoshi

Copy link
Copy Markdown
Contributor

This pull request makes server-targeting scenarios issue an HTTP DELETE to terminate the MCP session before closing the
client, so that each scenario leaves the server under test in a hermetic state. The fix lands in the shared
connectToServer helper plus a new terminateSessionRaw for scenarios that bypass the SDK transport. A follow-up PR will
add a dedicated session-lifecycle scenario that exercises the second half of the issue (servers MUST return 404 for
requests on a terminated session ID).

Motivation and Context

Fixes #79.

The Streamable HTTP transport spec says clients SHOULD send an HTTP DELETE with the Mcp-Session-Id header to explicitly terminate a session when they no longer need it.
The conformance harness never did this — every scenario opened a session via initialize, ran its checks, and let the server hold onto the dangling session.
For real servers that gate resources by active session (in-memory state, connection slots, etc.), this is observable as drift between conformance runs.

How Has This Been Tested?

  • npm run check — typecheck + lint pass.
  • npm run test — 173/173 tests pass. lifecycle.test.ts gains two new cases asserting that DELETE is sent with the issued
  • Manual smoke against examples/servers/typescript/everything-server.ts:

$ PORT=3010 npx tsx examples/servers/typescript/everything-server.ts
$ npx tsx src/index.ts server --url http://localhost:3010/mcp --scenario tools-call-simple-text
Running client scenario 'tools-call-simple-text' against server: http://localhost:3010/mcp
Checks:
2026-05-21T10:23:18.394Z [tools-call-simple-text] SUCCESS Tool returns simple text content

Test Results:
Passed: 1/1, 0 failed, 0 warnings

The everything-server now logs Received session termination request for session <id> for every scenario, which was previously absent.

Breaking Changes

None

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

@pkg-pr-new

pkg-pr-new Bot commented May 25, 2026

Copy link
Copy Markdown

Open in StackBlitz

npx https://pkg.pr.new/@modelcontextprotocol/conformance@316

commit: 363088f

@pugsatoshi
pugsatoshi force-pushed the fix/79-session-cleanup branch from 072f6e7 to a0d3b86 Compare May 28, 2026 03:01
@pugsatoshi

Copy link
Copy Markdown
Contributor Author

http-standard-headers.ts is excluded because it was made stateless in #319

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 modelcontextprotocol#79

Signed-off-by: Satoshi Ito <satoshi.ito.tf@hitachi.com>
@pugsatoshi
pugsatoshi force-pushed the fix/79-session-cleanup branch from a0d3b86 to 48e51e3 Compare June 4, 2026 09:14
@pugsatoshi

Copy link
Copy Markdown
Contributor Author

Rebased onto latest main (25fd443) to resolve conflicts introduced by #318 (version-aware Connection abstraction), which moved connectToServer from src/scenarios/server/client-helper.ts to src/connection/sdk-client.ts.

Changes from the previous revision:

  • Import path: ./client-helper../../connection/sdk-client in lifecycle.ts / lifecycle.test.ts.
  • MCPClientConnection.transport is no longer exposed on the public interface; the SDK transport is captured by the close() closure since no caller needs direct access. Smaller API surface, same behavior.
  • lifecycle.test.ts uses vi.mock(..., importOriginal) partial mocking so terminateSessionRaw remains real and is observable via the global fetch stub used by the new DELETE assertions.

All 270 tests pass locally; build + lint clean.

Resolves the conflict in src/connection/sdk-client.ts from the
connectToServer/close() rewrites on main: keeps main's rewritten
connectToServer (transport instrumentation, ConnectOptions signature)
with this branch's session-termination payloads (terminateSession in
close(), terminateSessionRaw helper) applied on top.
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 modelcontextprotocol#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 modelcontextprotocol#415/modelcontextprotocol#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.
@pcarleton

Copy link
Copy Markdown
Member

Thanks @pugsatoshi, this is a correct read of the spec's session-termination SHOULD and worth landing. To save a timezone round trip I pushed two commits onto your branch rather than requesting changes:

  1. Merge with current main. The connectToServer/close() rewrites from Validate all wire messages against the per-version spec JSON schema #399/Fix wire-validation attribution bugs, extend schema erratum to 2025-06-18 #421 conflicted with the docstring hunk; your two payloads (terminateSession in close(), the terminateSessionRaw helper) applied cleanly on top.
  2. Review follow-ups: the teardown DELETE now sends the negotiated protocol version instead of a hardcoded 2025-11-25 (a strict server 400s a version it didn't negotiate, the helper swallows the error, and the session survives, which is the exact failure Clean up streamable session after test #79 describes). It is also bounded with AbortSignal.timeout(5s) so a wedged server can't stall the suite, and there's a regression test asserting close() sends the DELETE.

Verified locally: full test suite green, and the typescript, python, go, rust, and csharp everything-servers all pass the touched scenarios with the new teardown.

Follow-ups we'll take separately (not blockers here): a runner-level per-scenario timeout, and bounding the pre-existing raw initialize fetch in lifecycle.ts.

@pcarleton pcarleton left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed, fixes applied on-branch, SDK matrix green (ts/py/go/rust/csharp).

@pcarleton
pcarleton merged commit c5af567 into modelcontextprotocol:main Jul 31, 2026
4 checks passed
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.

Clean up streamable session after test

2 participants