Skip to content

fix(sessions): stop concurrent writes losing app: and user: state - #7942

Open
AtulJoshi1206 wants to merge 1 commit into
google:mainfrom
AtulJoshi1206:fix/sessions-scoped-state-lost-updates
Open

AtulJoshi1206 wants to merge 1 commit into
google:mainfrom
AtulJoshi1206:fix/sessions-scoped-state-lost-updates

Conversation

@AtulJoshi1206

Copy link
Copy Markdown
Contributor

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, and user: state by every session of a user. DatabaseSessionService keeps each scope as one JSON row. It writes by reading the row, merging the delta 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. Its in-process lock is per session. So on SQLite, two sessions writing at once each overwrite the other's keys.
  • create_session merges its initial app:/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_session has the same unguarded GET, merge and SET.

Impact:

This hits anyone running DatabaseSessionService on SQLite (sqlite+aiosqlite://...) with more than one request in flight: several users writing app: keys, or one user with two tabs writing user: 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:

  • One user runs four run_async turns at once in four sessions, each passing its own user: and app: key in state_delta. Afterwards get_user_state returns 1 of the 4 keys.
  • Appending from 10 sessions of one user plus 10 other users at once keeps 1/10 user: keys and 1/20 app: keys.
  • Concurrent create_session calls keep 1/9.

Expected Behavior:

Each committed delta merges with dict.update semantics on top of the latest state. That's what the other services already do:

  • InMemorySessionService;
  • SqliteSessionService, with a single-statement SQL merge;
  • Firestore, with transactions;
  • Vertex, server-side.

After those turns, all four keys are there.

Solution:

  • DatabaseSessionService now also takes in-process locks on the app and user rows an append or a create actually writes.
    • It takes them after the session lock, with app before user, so there are no cycles.
    • Events without scoped deltas take no extra lock.
  • create_session reads those rows FOR UPDATE where row locks exist, so PostgreSQL and MySQL are covered across processes too.
    • A row is locked only once it's known to exist. On MySQL a locking read of a missing key takes a gap lock, and two such inserts deadlock.
  • RedisSessionService.create_session now merges each shared key in a watched transaction, the same primitive append_event already uses for the session key.

There are no schema or API changes. Two related cases are already handled elsewhere:

Testing Plan

Unit Tests:

  • I have added or updated unit tests for my change.
  • All unit tests pass locally.

New tests (21 cases):

  • A Runner test with concurrent turns on a SQLite-backed DatabaseSessionService.
  • Two contract tests in the conformance suite, running on every registered backend.
  • A parametrized test that create_session requests row locks only for the rows it writes.
  • A Redis test that injects a write between create_session's read and write.

On current main the 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.

pyink and isort are 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_session locks.

Checklist

  • I have read the CONTRIBUTING.md document.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing unit tests pass locally with my changes.
  • I have manually tested my changes end-to-end.
  • Any dependent changes have been merged and published in downstream modules. (none)

Additional context

The nearest earlier discussions are different problems: #6236 (a merge_state feature, 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

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.
@joinwell52-AI

Copy link
Copy Markdown

Thanks for making the shared app: / user: write ordering explicit here. Your distinction between per-session ordering and shared rows prompted a separate check in our file-backed per-session metadata store. In a deterministic read/read/write/write probe, both saves resolved and the JSON stayed valid, but the second stale snapshot removed the first writer's recovery-state field; reloading before the second save retained both fields. This is a module-level comparison, not a reproduction of the ADK bug or a live runtime race. The bounded method and outputs are in our English evidence record. One question your PR raised for us: for a stale per-session object passed to append_event, is rejection guaranteed at the storage boundary, or does the contract rely on the higher-level per-session writer ordering? That boundary will shape our next cancellation-versus-settlement test.

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.

3 participants