Repository navigation
Conversation
chr-hertel
added
Client
There was a problem hiding this comment.
🟡 Changes recommended
Constructor-supplied session headers remain active after expiry and prevent a fresh reconnection.
1 open finding
What changed in this PR
Invalidates expired HTTP sessions promptly so clients can reconnect instead of timing out while appearing connected.
Changes:
- Detects session-bound HTTP 404 responses and invalidates client state.
- Adds regressions for response cleanup, cancellation, and reconnection.
| File | Description |
|---|---|
src/Client/Transport/HttpTransport.php |
Handles expired-session 404 responses. |
tests/Unit/Client/Transport/HttpTransportTest.php |
Tests expiry and cancellation behavior. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // not a timeout that leaves the expired session reusable. | ||
| if (404 === $response->getStatusCode() && $request->hasHeader('Mcp-Session-Id')) { | ||
| $response->getBody()->close(); | ||
| $this->sessionId = null; |
There was a problem hiding this comment.
In theory that's true, but it's not logical.
If someone pass for some reason with $headers in constructor session id, I guess they have some intent to reuse specifically that session, by silently dropping it we can kinda hide error.
In this case user receives clear error that session is closed, so whoever passed that session id with headers need to supply new session id and not reuse old one.
IMHO, this should stay as is, I don't see a reason to parse $headers and clear it there.
Fixes #559.
Problem
A request carrying
Mcp-Session-Idcan receive HTTP 404 when its session expires. Empty or plain-text responses currently leave the request waiting until timeout, while the client still reports itself connected.Session expiry during a legacy
notifications/cancelledPOST has the same problem.Protocol::notifyCancellation()logs notification failures and preserves the original interruption, so throwing a connection exception alone does not invalidate the client.Changes
On HTTP 404 for a request carrying
Mcp-Session-Id,HttpTransport::send()now:ConnectionExceptionwith code 404.An ordinary request fails promptly. During cancellation, the original interruption still reaches the caller, but
Client::isConnected()remains false even when the notification failure is logged. Callers can evict the client and reconnect without the expired session ID. This does not replay the failed tool call.Requests without a session header and other HTTP status codes are unchanged. This is narrower than #425 and addresses the session-expiry behavior required by the 2025-11-25 session management rules.
Validation
RequestCancelledException.make cipassed: 1,971 tests, 5,353 assertions, 8 existing skips; formatting and PHPStan level 8 passed.make conformance-tests: server 80/80; client 12/54 matches the expected-failure baseline.P.S. I'm really thinking now that I might just need to upgrade my MCP client version, but it was working just fine all this time!