Skip to content

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

Merged
chr-hertel merged 4 commits into
modelcontextprotocol:mainfrom
vbcherepanov:feat/session-lock
Oct 14, 2026
Merged

chr-hertel merged 4 commits into
modelcontextprotocol:mainfrom
vbcherepanov:feat/session-lock

Conversation

@vbcherepanov

Copy link
Copy Markdown
Contributor

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 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

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

4 open findings
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.

Comment thread src/Server/Protocol.php Outdated
Comment thread src/Server/Protocol.php Outdated
Comment thread src/Server/Session/FileSessionStore.php Outdated
Comment thread src/Server/Session/SymfonyLockSessionLock.php
@chr-hertel chr-hertel added the Server Issues & PRs related to the Server component label Oct 12, 2026
@chr-hertel chr-hertel added this to the 0.9.0 milestone Oct 12, 2026

@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 @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).

Comment thread src/Server/Protocol.php
Comment thread src/Server/Protocol.php Outdated
Comment thread src/Server/Protocol.php Outdated
Comment thread src/Server/Protocol.php Outdated
Comment thread src/Server/Protocol.php Outdated
Comment thread src/Server/Protocol.php Outdated
Comment thread src/Server/Session/FileSessionLock.php Outdated
Comment thread src/Server/Session/SymfonyLockSessionLock.php Outdated
@vbcherepanov

Copy link
Copy Markdown
Contributor Author

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 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.

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

Comment thread src/Server/Session/SymfonyLockSessionLock.php
Comment thread src/Server/Protocol.php Outdated
symfony/lock 5.4 calls PersistingStoreInterface::exists() from Lock::__destruct and the interface has no return type there, so a bare mock returned null and isAcquired() raised a TypeError. The mock now reports the lock as held between save() and delete().
Resume a fiber with an error when what it yields cannot be stored, answer only requests when the lock is busy, poll without waiting, lock only known sessions, let gc() unlink a lock file only under its lock, and require a finite ttl for the symfony/lock lease.
@vbcherepanov

Copy link
Copy Markdown
Contributor Author

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:

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.

@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 @vbcherepanov!

@chr-hertel
chr-hertel merged commit 9e16c80 into modelcontextprotocol:main Oct 14, 2026
28 checks passed
chr-hertel added a commit to chr-hertel/php-sdk that referenced this pull request Oct 14, 2026
chr-hertel added a commit that referenced this pull request Oct 14, 2026
…s answers (#581)

* [Server] Fix parallel elicitations on one session getting each other's answers

* [Server] Address review: cap client request ids at PHP_INT_MAX on 32-bit PHP

* [Server] Read the random client request ids in the tests of #580 and #585

* [Server] Address review: send requests to the client with UUID ids

* [Server] Address review: pass client request ids as Uuid instances

* [Server] Address review: answer the second stream's request by its UUID in the test

* [Server] Address review: don't send a client request the session could not store on a fiber yield

* [Server] Read the client request ids in the tests of #584
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