Repository navigation
[Server] Add an opt-in per-session lock for concurrent requests - #584
Conversation
There was a problem hiding this comment.
🟡 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.
There was a problem hiding this comment.
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).
cdea6e2 to
e392b6d
Compare
|
Thanks for the review and for reproducing each case. All points are addressed in the last commit, rebased on main.
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. |
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.
… lock backend from a busy session
e392b6d to
ed412d5
Compare
|
Thanks for the load testing. Both are fixed in the last commit, and the branch is rebased on main.
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 |
…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

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.