Repository navigation
fix(sessions): stop concurrent writes losing app: and user: state - #7942
AtulJoshi1206 wants to merge 1 commit into
Conversation
Every session of an app shares one app-state row, and every session of a user shares one user-state row. DatabaseSessionService writes them by reading the row, merging the delta into the JSON in Python and writing the whole document back. append_event orders those writers only with SELECT ... FOR UPDATE, which SQLAlchemy renders on PostgreSQL and MySQL alone, and its in-process lock is per session. On SQLite, the documented local setup, sessions writing at once overwrite each other's keys: four concurrent turns from one user's tabs keep one of four user: keys. create_session merged its initial app: and user: state into rows it read without any lock, so it lost concurrent writes on every dialect, PostgreSQL included. Take in-process locks on the app and user rows an append or a create writes (after the session lock, app before user) and have create_session read those rows FOR UPDATE where row locks exist. A row is locked only once it is known to exist, because on MySQL a locking read of a missing key takes a gap lock, and two such inserts deadlock. RedisSessionService.create_session did the same unguarded read, merge and write of the shared keys. It now merges each key in a watched transaction, as append_event already does for the session key.
|
Thanks for making the shared |
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
No existing issue; described below following the bug-report structure.
Describe the Bug:
app:state is shared by every session of an app, anduser:state by every session of a user.DatabaseSessionServicekeeps each scope as one JSON row. It writes by reading the row, merging the delta in Python, and writing the whole document back.append_eventorders those writers only withSELECT ... FOR UPDATE, which SQLAlchemy renders on PostgreSQL and MySQL. Its in-process lock is per session. So on SQLite, two sessions writing at once each overwrite the other's keys.create_sessionmerges its initialapp:/user:state into rows it reads without any lock, so it can drop a concurrent writer's keys on every dialect, PostgreSQL included.RedisSessionService.create_sessionhas the same unguarded GET, merge and SET.Impact:
This hits anyone running
DatabaseSessionServiceon SQLite (sqlite+aiosqlite://...) with more than one request in flight: several users writingapp:keys, or one user with two tabs writinguser:keys. Nothing errors; the keys just disappear. On PostgreSQL and MySQL,create_session(state=...)with scoped keys can wipe keys another session just committed.Observed Behavior:
run_asyncturns at once in four sessions, each passing its ownuser:andapp:key instate_delta. Afterwardsget_user_statereturns 1 of the 4 keys.user:keys and 1/20app:keys.create_sessioncalls keep 1/9.Expected Behavior:
Each committed delta merges with
dict.updatesemantics on top of the latest state. That's what the other services already do:InMemorySessionService;SqliteSessionService, with a single-statement SQL merge;After those turns, all four keys are there.
Solution:
DatabaseSessionServicenow also takes in-process locks on the app and user rows an append or a create actually writes.create_sessionreads those rowsFOR UPDATEwhere row locks exist, so PostgreSQL and MySQL are covered across processes too.RedisSessionService.create_sessionnow merges each shared key in a watched transaction, the same primitiveappend_eventalready uses for the session key.There are no schema or API changes. Two related cases are already handled elsewhere:
Testing Plan
Unit Tests:
New tests (21 cases):
Runnertest with concurrent turns on a SQLite-backedDatabaseSessionService.create_sessionrequests row locks only for the rows it writes.create_session's read and write.On current
mainthe 8 cases covering the bug fail; with the change all 21 pass. 30 repeated runs gave the same result every time, both ways.Suite results with the change:
tests/unittests/sessions: 751 passed, 1 xfailed (main: 732 passed, 1 xfailed).tests/unittests/runners: 94 passed, 1 skipped, 3 xfailed.tests/unittests/test_runners.py: 237 passed.tests/unittests/integrations/redis: 37 passed.tests/unittests/integrations/mongodb: 128 passed.tests/unittests/integrations/firestore: 80 passed.tests/unittests/apps: 147 passed.pyinkandisortare clean.Manual End-to-End (E2E) Tests:
Not run against a live PostgreSQL or Redis server. The PostgreSQL row-lock path is covered by the unit test that checks which rows
create_sessionlocks.Checklist
Additional context
The nearest earlier discussions are different problems: #6236 (a
merge_statefeature, closed pending design), #4751/#4752 (session-level optimistic concurrency) and #4954 (the insert race). This keeps #4655's rule that only scopes with deltas get locked.🤖 Generated with Claude Code