Repository navigation
[Server] Add an opt-in per-session lock for concurrent requests - #584
Open
vbcherepanov wants to merge 2 commits into
Open
vbcherepanov wants to merge 2 commits into
vbcherepanov wants to merge 2 commits into
Conversation
vbcherepanov
requested review from
CodeWithKyrian,
Nyholm,
chr-hertel and
soyuka
as code owners
October 10, 2026 08:46
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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/listandresources/listsent together on connect, or parallel tool calls from an agent.What this adds
SessionLockInterfacewithacquire(Uuid)/release(Uuid), andSessionLockException.FileSessionLock:flock()on one file per session, for the workers of one machine, next toFileSessionStore. 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 asuggestand a dev dependency only.Builder::setSessionLock(), and an optional last constructor argument ofProtocol.Off by default. With no lock configured nothing changes.
Where the lock is held
Protocoltakes 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.initializecreates 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
503and a JSON-RPC-32000error, 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 asSessionLockException.ProtocolSessionLockTest: one shared log of the store's reads and writes and the lock's calls, assertingacquire, read, write, releasearound a request, around each polling helper and around destroy; the lock is released when resolving the session throws; a busy session is refused with503before 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: withFileSessionLock, 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 answered503while the first one gets its200; two tool calls that suspend on a request to the client both proceed and store ids1000and1001, so the lock is not held across the wait. Both fail when the lock is not taken arounddoProcessInput().FileSessionStoreTest:gc()collects expired lock files, leaves live and foreign.lockfiles 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.mdand 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.