Skip to content

Commit c2940a8

Browse files
committed
fix(sessions): raise AlreadyExistsError on concurrent create_session races
The has_user_provided_id existence check in DatabaseSessionService.create_session() is not atomic with the insert that follows it: two concurrent callers can both pass the check and then race the same INSERT on (app_name, user_id, session_id). The loser saw a raw, unhandled IntegrityError instead of a clean error. Wrap the insert flush in try/except IntegrityError and raise AlreadyExistsError, mirroring the SAVEPOINT pattern _get_or_create_state already uses for app_state/user_state races. Verified against both sqlite+aiosqlite and postgres+asyncpg with a concurrent create_session() reproduction: 5/5 trials on each backend now raise a clean AlreadyExistsError instead of a raw IntegrityError/UniqueViolationError. This does not address the separate orphaned user_state row issue also reported in #6823 -- _rollback_on_exception_session rolls back the whole transaction on any exception (including the AlreadyExistsError raised here), so this particular race shouldn't be able to leave a row behind on its own. That deeper issue needs more data to root-cause. Related: #6823
1 parent 775c1bd commit c2940a8

2 files changed

Lines changed: 72 additions & 1 deletion

File tree

src/google/adk/sessions/database_session_service.py

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -639,7 +639,18 @@ async def create_session(
639639
storage_app_state.state, storage_user_state.state, session_state
640640
)
641641
# Call to_session before commit to avoid post-commit lazy-load.
642-
await sql_session.flush()
642+
try:
643+
await sql_session.flush()
644+
except IntegrityError:
645+
# A concurrent caller won the race on this (app_name, user_id,
646+
# session_id) primary key: the has_user_provided_id check above is
647+
# not atomic with this insert, so two callers can both pass it and
648+
# then race the same insert. Same failure mode _get_or_create_state
649+
# guards against for app_state/user_state; surface the same clean
650+
# error here instead of letting the raw IntegrityError propagate.
651+
raise AlreadyExistsError(
652+
f"Session with id {session_id} already exists."
653+
)
643654
session = storage_session.to_session(
644655
state=merged_state, is_sqlite=is_sqlite, is_postgresql=is_postgresql
645656
)

tests/unittests/sessions/test_session_service.py

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1190,6 +1190,66 @@ async def test_create_session_with_existing_id_raises_error(session_service):
11901190
)
11911191

11921192

1193+
@pytest.mark.asyncio
1194+
async def test_create_session_concurrent_same_id_raises_already_exists_error(
1195+
tmp_path,
1196+
):
1197+
"""Two concurrent create_session() calls for the same caller-provided id.
1198+
1199+
The has_user_provided_id existence check in create_session() is not atomic
1200+
with the insert that follows it, so both callers can pass the check and
1201+
then race the same INSERT. The loser must see a clean AlreadyExistsError
1202+
(mirroring the up-front check above and the _get_or_create_state
1203+
savepoint pattern for app_state/user_state), not a raw IntegrityError.
1204+
1205+
Uses a file-backed sqlite db (not ':memory:') so the two concurrent
1206+
sessions get real, independent connections from the pool instead of
1207+
sharing the single StaticPool connection ':memory:' relies on to survive
1208+
across connections -- sharing one physical connection between the two
1209+
concurrent sessions here made the loser's rollback able to interleave
1210+
with the winner's commit on the same connection.
1211+
"""
1212+
db_path = tmp_path / 'race.db'
1213+
session_service = DatabaseSessionService(f'sqlite+aiosqlite:///{db_path}')
1214+
1215+
async with session_service:
1216+
app_name = 'my_app'
1217+
user_id = 'user'
1218+
1219+
# Pre-warm app_state/user_state with an unrelated session first, so the
1220+
# race below is purely on the StorageSession primary key and not
1221+
# confounded by the (separate) app_state/user_state creation race.
1222+
await session_service.create_session(
1223+
app_name=app_name, user_id=user_id, session_id='warmup-session'
1224+
)
1225+
1226+
for i in range(5):
1227+
session_id = f'race-session-{i}'
1228+
results = await asyncio.gather(
1229+
session_service.create_session(
1230+
app_name=app_name, user_id=user_id, session_id=session_id
1231+
),
1232+
session_service.create_session(
1233+
app_name=app_name, user_id=user_id, session_id=session_id
1234+
),
1235+
return_exceptions=True,
1236+
)
1237+
errors = [result for result in results if isinstance(result, Exception)]
1238+
successes = [
1239+
result for result in results if not isinstance(result, Exception)
1240+
]
1241+
assert len(successes) == 1
1242+
assert len(errors) == 1
1243+
assert isinstance(errors[0], AlreadyExistsError)
1244+
assert session_id in str(errors[0])
1245+
1246+
final_session = await session_service.get_session(
1247+
app_name=app_name, user_id=user_id, session_id=session_id
1248+
)
1249+
assert final_session is not None
1250+
assert final_session.id == successes[0].id
1251+
1252+
11931253
@pytest.mark.asyncio
11941254
async def test_append_event_bytes(session_service):
11951255
app_name = 'my_app'

0 commit comments

Comments
 (0)