modelcontextprotocol / modelcontextprotocol/php-sdk
[Server] Concurrent requests corrupt the session file in FileSessionStore, crashing Session::readData() with JsonException
Nobody has claimed this yet.
- Dominant language
- PHP
- Stars
- 1.6k
- Forks
- 173
- Avg merge
- 2d 49m
- Merged PRs (30d)
- 23
Description
Describe the bug
Under PHP-FPM, two concurrent requests carrying the same Mcp-Session-Id can leave the session file empty (0 bytes). The next read of that session throws an uncaught JsonException out of Session::readData(), and the server answers -32603 for a tool call that had already completed successfully — the tool ran, its response was queued in the session, and then the response could not be retrieved.
Two independent defects combine here; each is a necessary condition.
-
FileSessionStore::write()uses a fixed temporary filename.$tmp = $path.'.tmp';is identical for every concurrent writer of the same session, andfile_put_contents()opens the stream inwmode — so truncation happens beforeLOCK_EXis acquired. One process can thereforerename()a temp file that another process has just truncated to 0 bytes, publishing an empty session file. The// Atomic movecomment applies torename()alone; the operation as a whole is not atomic because the temp name is shared. Thecopy()fallback has the same problem (it truncates the destination and then streams into it), andread()takes noLOCK_SH. -
Session::readData()does not guard against an empty string. Onlyfalseis handled, so''reachesjson_decode(..., JSON_THROW_ON_ERROR):
$rawData = $this->store->read($this->id);
if (false === $rawData) { return $this->data = []; } // only false is handled
$decoded = json_decode($rawData, true, flags: \JSON_THROW_ON_ERROR); // throws on ''
if (!\is_array($decoded)) { return $this->data = []; } // unreachable for ''
FileSessionStore::read() returns '' for a zero-byte file — it returns false only when the file is missing, expired, or unreadable. This half is store-agnostic: any store that can return an empty string triggers it.
To Reproduce
Steps to reproduce the behavior:
- Run a server on PHP-FPM with
StreamableHttpTransportandFileSessionStore(handshake era). POSTaninitializerequest without a session id; takeMcp-Session-Idfrom the response headers.POSTnotifications/initializedwith that header.- Deterministic variant — truncate the session file by hand:
: > <session-dir>/<session-id>(or write invalid JSON:printf '{"a":' > <session-dir>/<session-id>). POSTtools/listwith the sameMcp-Session-Id. The request fails with-32603and theJsonExceptionbelow.- Concurrency variant (shows the actual cause rather than the symptom) — instead of step 4, fire ~40 parallel
tools/listPOSTs sharing oneMcp-Session-Id, e.g.seq 40 | xargs -P 8 -I{} curl -s -o /dev/null -w '%{http_code}\n' -X POST ..., and repeat a few rounds. Occasional requests fail with-32603.
Expected behavior
- Concurrent writes to the same session never publish a partial or empty session file.
- An empty or undecodable payload from the store degrades to an empty session instead of a fatal error. The existing
!\is_array($decoded)branch already shows this is the intended behaviour — it is simply unreachable whileJSON_THROW_ON_ERRORfires first.
Logs
JsonException: Syntax error in src/Server/Session/Session.php:168
#0 Session.php(168): json_decode('', true, 512, 4194304)
#1 Session.php(53): Mcp\Server\Session\Session->readData()
#2 Protocol.php(517): Mcp\Server\Session\Session->get(Array, Array)
#3 BaseTransport.php(81): Mcp\Server\Protocol->consumeOutgoingMessages(Symfony\Component\Uid\UuidV4)
#4 StreamableHttpTransport.php(218): Mcp\Server\Transport\BaseTransport->getOutgoingMessages(...)
#5 StreamableHttpTransport.php(202): ...->createJsonResponse()
#6 StreamableHttpTransport.php(437): ...->handlePostRequest('{"jsonrpc":"2.0...')
#7 Server.php(67): Mcp\Server\Transport\StreamableHttpTransport->listen()
Additional context
Why the exception escapes every catch block. The crash happens in Protocol::consumeOutgoingMessages(), which builds a fresh Session instance:
// Protocol.php:516
$session = $this->sessionManager->createWithId($sessionId);
$queue = $session->get(self::SESSION_OUTGOING_QUEUE, []); // second, independent disk read
A single POST therefore reads the session file twice, through two objects that share no memory — and the second read happens after the request's own save() at Protocol.php:223. That second read is reached from BaseTransport::getOutgoingMessages() while the response is being built, which is outside Protocol::processInput()'s try/catch (that one wraps only doProcessInput()); Server::run() has just try/finally. The exception propagates out of Server::run() into the application's own error handler.
Silent lost writes. After the losing process's rename() fails (its temp file is gone) and copy() fails too, write() returns false — but Session::save()'s return value is discarded at Protocol.php:223, :519, :551 and :644, so the failed write is invisible.
Where a fix would belong. Both halves look small and independent — the shared temp filename in FileSessionStore::write(), and the missing '' case in Session::readData().
One more observation, separate from the bug. Since #479, gc() skips anything whose filename is not a valid UUID, so orphaned *.tmp files are never cleaned up. Harmless at one temp file per session, but worth knowing if the temp name ever becomes unique.
Related but distinct issues. #275 (lost update on concurrent read-modify-write) and #467 (concurrent POSTs returning a JSON array). A per-session lock as proposed in #275 would incidentally prevent this crash, but would not close item 2.
Environment. mcp/sdk v0.8.1, PHP 8.5, PHP-FPM, StreamableHttpTransport with withoutModernEra(), FileSessionStore on local disk (Linux), TTL 14400 s. The same code is present on main, so this is not fixed by upgrading.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with src/Server/Session/Session.php and the FileSessionStore::write() and read() paths, then trace the second session read from Protocol.php through BaseTransport::getOutgoingMessages(). Reproduce the empty-file case and the parallel tools/list requests described in the issue. Done means concurrent writes do not publish empty or partial files, and empty store data is treated as an empty session without an uncaught JsonException.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- php
- Domain
- backend
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 68/100