[Server] Add an opt-in per-session lock for concurrent requests

[vbcherepanov]

Follow-up to #535 for the part it left open in #275: an opt-in per-session lock, as agreed with @guillaume-sainthillier and @chr-hertel in #275 (comment).

With #535, responses no longer go through the session. The other session keys still do: two requests of one session that both change it (a pending request to the client, the client's answer, client info) are written back whole, and the last save wins. In practice that is tools/list and resources/list sent together on connect, or parallel tool calls from an agent.

What this adds

• SessionLockInterface with acquire(Uuid) / release(Uuid), and SessionLockException.
• FileSessionLock: flock() on one file per session, for the workers of one machine, next to FileSessionStore. Lock files are not unlinked on release (a worker holding the old inode would not see a new file of the same name); FileSessionStore::gc() collects those of expired sessions when both share the directory. Acquiring refreshes the file's mtime so gc keeps the file of a live session.
• SymfonyLockSessionLock: a symfony/lock store (Redis, database, ...) for sessions shared across machines, with the store's TTL so a dead worker does not block a session for good. symfony/lock is a suggest and a dev dependency only.
• Builder::setSessionLock(), and an optional last constructor argument of Protocol.

Off by default. With no lock configured nothing changes.

$server = Server::builder()
    ->setSession(new FileSessionStore(__DIR__.'/sessions'))
    ->setSessionLock(new FileSessionLock(__DIR__.'/sessions'))
    ->build();

Where the lock is held

Protocol takes the lock before a request's session is read and releases it after the save, so a handler runs under it. It is released when a handler suspends to wait for the client, and taken briefly on each turn of the SSE loop (consumeOutgoingMessages(), checkResponse(), handleFiberYield()), so the POST carrying the client's answer gets through. destroySession() takes it too, so a request still running cannot write a session back that the client ended. initialize creates a session nobody else knows yet and takes no lock. Reads that do not write (getPendingRequests()) take none.

A request that cannot get the lock within the timeout (30 s by default) is answered with 503 and a JSON-RPC -32000 error, under its id, instead of running on a session another request is about to overwrite. The polling helpers return nothing in that case and try again next turn; handleFiberYield() throws, since the request it stores must not be lost silently.

Tests

• FileSessionLockTest, SymfonyLockSessionLockTest: a lock held by another instance cannot be acquired until released and times out with the SDK's exception; different sessions do not block each other; acquiring twice and releasing an unheld lock are errors; the lock file's mtime is refreshed; a failing lock store surfaces as SessionLockException.
• ProtocolSessionLockTest: one shared log of the store's reads and writes and the lock's calls, asserting acquire, read, write, release around a request, around each polling helper and around destroy; the lock is released when resolving the session throws; a busy session is refused with 503 before it is read; a busy poll leaves the session untouched; no lock without a session id; nothing but the store is touched when no lock is configured.
• StreamableHttpTransportTest: with FileSessionLock, a second POST that loads the session while the first one runs (the lost-update interleaving of [Server] Fix lost responses on concurrent requests of the same session (Streamable HTTP) #535's test) is answered 503 while the first one gets its 200; two tool calls that suspend on a request to the client both proceed and store ids 1000 and 1001, so the lock is not held across the wait. Both fail when the lock is not taken around doProcessInput().
• FileSessionStoreTest: gc() collects expired lock files, leaves live and foreign .lock files alone.
• BuilderTest: setSessionLock() is wired into the built server.

Unit and integration suites pass, PHPStan (level 8) and php-cs-fixer are clean. Docs: a "Concurrent Requests" section in docs/run/sessions.md and a row in the builder reference.

Not in this PR: an option in symfony/mcp-bundle to wire the lock.

Parts of this change were drafted with an AI coding assistant and reviewed and tested by me.

[Copilot]

🟡 Changes recommended

Lock-file cleanup, lease expiry, session destruction, and swallowed fiber-yield failures can still break serialization.

4 open findings

• Swallowed resume errors leave the protocol in an invalid state · New
• Timed-out destroy can recreate the deleted session · New
• Age-based GC can unlink active lock files · New
• Unrenewed lock lease permits concurrent session access · New

What changed in this PR

Adds opt-in per-session locking to prevent concurrent requests from overwriting shared session state.

Changes:

• Adds file-based and Symfony-backed lock implementations.
• Integrates locking into protocol processing, polling, fiber yields, and session destruction.
• Adds configuration, documentation, and comprehensive tests.

File	Description
CHANGELOG.md	Documents session locking.
composer.json	Adds Symfony Lock support.
docs/​run/​server-builder.md	Documents the builder method.
docs/​run/​sessions.md	Explains lock configuration and behavior.
src/​Exception/​SessionLockException.php	Adds the lock exception.
src/​Server/​Builder.php	Wires locks into Protocol.
src/​Server/​Protocol.php	Applies locking to session operations.
src/​Server/​Session/​FileSessionLock.php	Implements filesystem locking.
src/​Server/​Session/​FileSessionStore.php	Adds lock-file garbage collection.
src/​Server/​Session/​SessionLockInterface.php	Defines the locking contract.
src/​Server/​Session/​SymfonyLockSessionLock.php	Implements distributed locking.
tests/​Unit/​Server/​BuilderTest.php	Tests builder integration.
tests/​Unit/​Server/​ProtocolSessionLockTest.php	Tests protocol lock ordering.
tests/​Unit/​Server/​Session/​Fixture/​RecordingSessionLock.php	Adds a recording lock fixture.
tests/​Unit/​Server/​Session/​Fixture/​RecordingSessionStore.php	Adds a recording store fixture.
tests/​Unit/​Server/​Session/​FileSessionLockTest.php	Tests file locking.
tests/​Unit/​Server/​Session/​FileSessionStoreTest.php	Tests lock-file collection.
tests/​Unit/​Server/​Session/​SymfonyLockSessionLockTest.php	Tests Symfony locking.
tests/​Unit/​Server/​Transport/​StreamableHttpTransportTest.php	Tests concurrent HTTP behavior.

🧠 Review effort: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[chr-hertel]

Thanks @vbcherepanov - nice work on the fiber side, the lock is never held across a suspension 👍 A few things, see comments, plus a rebase (nvm the CHANGELOG but the builder docs conflict).

[chr-hertel]

BaseTransport::handleFiberYield() swallows this - the fiber then gets resumed with null since the awaited id was already consumed. Reproduced with a tool that suspends twice: the second request never reaches the client and the tool sees null (ClientGateway turns that into an internal error, a notification is just lost). Could we resume the fiber with an Error instead and cover it through the transport loop, not only Protocol::handleFiberYield() directly?

[chr-hertel]

findResponseId() also picks the id of a client response - a busy lock on an elicitation answer {"id":1000,"result":…} gets an error back under id 1000, which the client matches to its own request, and the answer is gone. Only answer when the message has a method, please.

[chr-hertel]

not really true - acquire() blocks for the full timeout, so with the default 30s every SSE loop turn stalls ~60s while another request of the session runs. I measured 2s for both calls with a 1s timeout. Should polls use a non-blocking acquire instead?

[chr-hertel]

same here

[chr-hertel]

this locks before resolveSession() checks the session exists - every random Mcp-Session-Id leaves a .lock file behind (never collected if the lock dir isn't the session dir). How about checking exists() first?

[chr-hertel]

agree it's not worse than today, but #568 makes Session::save() skip a session destroyed since it was read - with that the busy case is covered, so I'd rather rely on it than document the resurrection.

[chr-hertel]

GC unlinking lock files is fine if acquire() checks after flock() that it locked the file still at $path (fstat() vs stat() inode, retry if not) - otherwise a holder older than the TTL loses its file and a second worker locks a new one. Reproduced that with a clock 2h ahead.

[chr-hertel]

"null for no expiration" doesn't hold for Redis/PDO - they fall back to their initialTtl (300s). And since nothing refreshes the lease while a handler runs, the docs should say ttl must exceed the longest handler, right?

[vbcherepanov]

Thanks for the review and for reproducing each case. All points are addressed in the last commit, rebased on main.

• Fiber yield failure (Protocol.php:738). BaseTransport::handleFiberYield() now resumes the fiber with an Error when the yield handler throws, and handles whatever the fiber yields next the same way. ClientGateway::notify() throws a ClientException when it is resumed with an Error, so a lost notification is no longer silent. Covered through the SSE loop: StreamableHttpTransportTest::testMessageThatCannotBeStoredFailsInTheHandler runs a tool that sends a request, gets the answer, then sends a second request and a notification while the lock is held elsewhere. The tool sees an error for both, and the call still gets its result.
• Busy lock on a client response (Protocol.php:231). findResponseId() only returns an id for a message with a method. A refused response, or a batch of responses and notifications, gets the 503 with a null id. The same applies to the catch-all in processInput().
• SSE polls (Protocol.php:610, 642). SessionLockInterface has a new tryAcquire(): bool. consumeOutgoingMessages() and checkResponse() use it and return at once when the lock is busy. Protocol::handleFiberYield() still waits, since the message has to be stored.
• Lock files for random ids (Protocol.php:230). The lock is taken only after exists(), and resolveSession() checks again under the lock. destroySession() does the same.
• Destroy while busy (Protocol.php:906). The comment no longer describes the session coming back. It relies on [Server] Stop a deleted session from coming back when an in-flight request saves it #568: Session::save() does not write back a session destroyed since it was read.
• GC vs a long holder (FileSessionLock.php:82). FileSessionStore::gc() unlinks a lock file only while it holds its lock (LOCK_NB), so a holder older than the TTL keeps its file. After flock(), acquire() compares fstat() with stat() of the path and starts over when the file it locked was removed. Tests: testGcKeepsAHeldLockFile, and testWaiterOnARemovedLockFileStartsOver, where another process unlinks the file under its lock while we wait on it.
• ttl (SymfonyLockSessionLock.php:39). $ttl is a plain float now, null is gone. The docblock and sessions.md say the lease is not refreshed and ttl must be longer than the longest handler.

Checked locally: unit 1856, integration 105, phpstan, cs-fixer. Also the unit suite on PHP 8.1 and on PHP 8.2 with Symfony 5.4.

[chr-hertel]

Two more from load testing, see comments - plus a rebase please 🙏

[chr-hertel]

A fatal error inside a handler (OOM, max_execution_time) never reaches the release() in Protocol: neither finally nor Lock::__destruct() runs after an E_ERROR. With Redis the session then stays locked for the full $ttl (300s by default). Every request on it meanwhile waits the 30s timeout, gets a 503 and pins a worker for that time. I could reproduce it with a PdoStore. Could you register a shutdown function once, on the first acquire, that releases whatever is still in $this->locks and swallows lock exceptions? Shutdown functions still run after a fatal. A test that acquires and then calls the registered callback directly would be enough. FileSessionLock doesn't need this, since the flock dies with the request.

[chr-hertel]

A lock store that is down ends up here as "Another request of this session is still being processed." too. SymfonyLockSessionLock wraps every LockExceptionInterface into the same SessionLockException it throws on a timeout, and acquireSessionLock() treats both as busy. With a Redis that refuses connections I get the busy 503 right away, and only a warning in the log, without the actual cause. 503 is fine for both, but could we keep them apart? For example, a timeout is a false return or its own exception, and a store failure throws: answered with an error that says the lock is unavailable, logged as error with the previous exception's message. FileSessionLock has the same problem with a failed fopen()/flock().

[vbcherepanov]

Thanks for the load testing. Both are fixed in the last commit, and the branch is rebased on main.

• Fatal error in a handler (SymfonyLockSessionLock.php:115). The first acquire registers releaseAll() as a shutdown function. It releases every lock the instance still holds, and a release that fails is logged as a warning and left to expire. Tested by acquiring two locks and calling releaseAll() directly, plus a store whose delete() throws. I also checked it with a real PdoStore on SQLite: a worker takes the lock and runs out of memory. Without the shutdown function the next process still sees the lock as held, with it the lock is free. FileSessionLock is unchanged.
• Lock store that is down (Protocol.php:236). A timeout now throws SessionLockTimeoutException, a subclass of SessionLockException, so code that catches the parent still works. Any other SessionLockException means the backend failed. In that case the request gets a 503 at once with The session lock is unavailable., and the log has an error with the cause. A timeout is still a warning. symfony/lock wraps the store exception in its own generic Failed to acquire the "..." lock., so SymfonyLockSessionLock appends the innermost message. With a PDO store that cannot open its database the log line ends with (SQLSTATE[HY000] [14] unable to open database file). A poll that hits a failing backend returns nothing and tries again on its next turn. destroySession() still destroys the session. FileSessionLock already threw a plain SessionLockException for a failed fopen()/flock(), so it is covered by the same split.

Rebase notes:

• checkResponse() keeps the expiry from [Server] Expire timed out client requests in Protocol::checkResponse() #585 inside the lock.
• handleFiberYield() checks the session still exists ([Server] Stop a deleted session from coming back when an in-flight request saves it #568) after it has the lock, not before.
• The yield retry in BaseTransport uses the fiber passed in by [Server] Resume every suspended fiber on stdio #580 instead of $this->sessionFiber.
• Since [Server] Stop concurrent requests from overwriting each other's session changes #572, save() reads the session again before writing, so the lock tests expect one more read.

unit, integration, phpstan and cs pass locally. Locally 6 client snapshot tests in interop fail on my machine's locale (" am" after the time from the everything server). They pass with LC_ALL=C and do not touch this change.