Skip to content

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

Open
vbcherepanov wants to merge 2 commits into
modelcontextprotocol:mainfrom
vbcherepanov:feat/session-lock
Open

vbcherepanov wants to merge 2 commits into
modelcontextprotocol:mainfrom
vbcherepanov:feat/session-lock

Conversation

@vbcherepanov

Copy link
Copy Markdown

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.

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

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant