Skip to content

[Server] Answer a DELETE of an unknown or ended session with 404 - #638

Merged
chr-hertel merged 2 commits into
modelcontextprotocol:mainfrom
nicolas-grekas:delete-unknown-session-404
Oct 28, 2026
Merged

chr-hertel merged 2 commits into
modelcontextprotocol:mainfrom
nicolas-grekas:delete-unknown-session-404

Conversation

@nicolas-grekas

@nicolas-grekas nicolas-grekas commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

A DELETE of an unknown or already ended session is answered 200, but once a session is terminated, the server "MUST respond to requests containing that session ID with HTTP 404 Not Found".

Protocol::destroySession() now returns whether the session existed, and StreamableHttpTransport answers 404 when the session-end listener returns false. A store that can't be reached doesn't count as an unknown session.

InMemorySessionStore::exists() now applies the TTL like read() does, so that an expired session counts as ended too (spotted by the review below).

This touches the same lines as #627: once both are in, a DELETE from another principal should return false too, so that it stays answered like an unknown session.

The spec requires a 404 for any request that carries the id of a terminated session.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The default in-memory store treats expired sessions as existing, causing DELETE to return HTTP 200 instead of 404; the existence check needs expiry handling and regression coverage.

1 open issue
What changed in this PR

This PR propagates session-existence results from the server protocol to Streamable HTTP, allowing DELETE requests for unknown or ended sessions to return 404 instead of 200.

Changes:

  • Return a boolean from session destruction and pass it through transport callbacks.
  • Map an explicit false result to HTTP 404 while preserving 200 for listeners that return nothing.
  • Add repeated-DELETE regression coverage, assert destruction results, and document the contract changes.
File Description
tests/​Unit/​Server/​Transport/​StreamableHttpTransportTest.php Tests that deleting an ended session returns 404.
tests/​Unit/​Server/​ProtocolSessionLockTest.php Asserts destruction results alongside lock behavior.
src/​Server/​Transport/​TransportInterface.php Documents the boolean session-end callback contract.
src/​Server/​Transport/​StreamableHttpTransport.php Returns 404 when session termination reports no session.
src/​Server/​Transport/​ManagesTransportCallbacks.php Updates the callback return-type annotation.
src/​Server/​Transport/​BaseTransport.php Propagates explicit false results from session-end listeners.
src/​Server/​Protocol.php Reports session existence without treating store failures as absence.
CHANGELOG.md Documents the DELETE behavior and return-contract changes.

Review effort: Balanced

Comment thread src/Server/Protocol.php
public function destroySession(Uuid $sessionId): bool
{
try {
$exists = $this->sessionManager->exists($sessionId);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 0c1fb09: exists() applies the same TTL check as read(), and a new test moves the clock past the TTL before sending the DELETE.

@chr-hertel chr-hertel 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.

Thanks @nicolas-grekas!

@chr-hertel
chr-hertel merged commit c26cf5c into modelcontextprotocol:main Oct 28, 2026
28 checks passed
@chr-hertel chr-hertel added the Server Issues & PRs related to the Server component label Nov 2, 2026
chr-hertel added a commit to BrocksiNet/php-sdk that referenced this pull request Nov 4, 2026
…et/php-sdk into pr-618-end-a-listen-stream-that-can

* 'fix/listen-empty-filter' of https://github.com/BrocksiNet/php-sdk: (25 commits)
  [Server] Pass an explicit null to a nullable tool parameter with a default (modelcontextprotocol#642)
  [Capability] Check tool names against the spec's 128-character limit and character set (modelcontextprotocol#639)
  [Tests] Stop php -S servers together with their forked workers (modelcontextprotocol#658)
  chore(deps): bump astral-sh/setup-uv from 10.3.2 to 10.3.3 (modelcontextprotocol#659)
  [Server] Accept plain http on `*.localhost` and `127.0.0.0/8` hosts in OAuth URLs (modelcontextprotocol#620)
  [Capability] Map PHPDoc pseudo-types to the JSON Schema type of their base type (modelcontextprotocol#643)
  Raise minimum versions of `phpdocumentor/reflection-docblock` and `psr/container` (modelcontextprotocol#657)
  [Server] Use compact JSON in result formatters (modelcontextprotocol#647)
  [Server] Filter list completions in one pass (modelcontextprotocol#649)
  [Server] Fix result lists being sent as JSON objects (modelcontextprotocol#656)
  Avoid repeated array merges in prompt formatting (modelcontextprotocol#650)
  [Docs] Add AGENTS.md (modelcontextprotocol#634)
  Avoid repeated array merges in resource formatting (modelcontextprotocol#651)
  [Server] Fix `"0"` prefix offering all completions (modelcontextprotocol#655)
  Filter enum completions in one pass (modelcontextprotocol#652)
  Paginate registry references before mapping (modelcontextprotocol#653)
  Extract JWT scopes in one pass (modelcontextprotocol#654)
  [Server] Fix `Session::save()` wiping a session that the store failed to read (modelcontextprotocol#637)
  [Server] Answer a DELETE of an unknown or ended session with 404 (modelcontextprotocol#638)
  [Server] Add `Psr6SessionStore` and `Psr6NotificationBus` (modelcontextprotocol#635)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Server Issues & PRs related to the Server component

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants