modelcontextprotocol / modelcontextprotocol/php-sdk
[Server] Concurrent requests corrupt the session file in FileSessionStore, crashing Session::readData() with JsonException
Dieses Issue hat noch niemand übernommen.
- Vorherrschende Sprache
- PHP
- Sterne
- 1.6k
- Forks
- 173
- Ø Merge
- 2 T. 49 Min.
- Gemergte PRs (30 T.)
- 23
Beschreibung
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.
Beitragsleitfaden
Erste Schritte
- Lies das ganze Issue und danach den Beitragsleitfaden des Projekts.
- Schreib ins Issue, dass du es übernimmst — das erspart doppelte Arbeit.
- Forke das Repository und arbeite in einem Branch.
- Öffne einen Pull Request, der die Issue-Nummer nennt.
Rechercherichtung
Beginne mit src/Server/Session/Session.php und den Pfaden FileSessionStore::write() und read(), und verfolge dann den zweiten Session-Lesevorgang von Protocol.php durch BaseTransport::getOutgoingMessages(). Reproduziere den Fall einer leeren Datei und die im Issue beschriebenen parallelen tools/list-Anfragen. Als abgeschlossen gilt die Änderung, wenn nebenläufige Schreibvorgänge keine leeren oder unvollständigen Dateien veröffentlichen und leere Store-Daten als leere Session behandelt werden, ohne eine nicht abgefangene JsonException auszulösen.
Vom Indexierungsmodell aus dem Issue-Text verfasst.
Bewertung
- Tech-Stack
- php
- Bereich
- backend
- Issue-Typ
- Bug
- Schwierigkeit
- 4/5
- Geschätzter Aufwand
- 3-5 Tage
- Aktivitätsstatus
- Aktiv
- Klarheit
- Klar beschrieben
- Anfängerfreundlichkeit
- 68/100