[AkshayShah03]

Link to Issue or Description of Change

1. Link to an existing issue (if applicable):

• Closes: FirestoreSessionService.list_sessions(user_id="") returns every user's sessions #7662
• Related: fix(sessions): list only the empty user's sessions in SqliteSessionService #7171 (same fix for SqliteSessionService)

Problem:
FirestoreSessionService.list_sessions checks if user_id:, so user_id="" falls through to the collection_group query that user_id=None uses, and returns every user's sessions in the app. The other session services (InMemory, Sqlite, Database, Redis, VertexAI) treat "" as a user id.

Solution:

• Return an empty ListSessionsResponse for user_id == "" without querying. The Firestore backend rejects a document path with an empty segment (400 InvalidArgument, checked against the emulator), so querying that user's collection would raise, and create_session(user_id="") can't store a session for that user either. An empty list matches the other session services.
• Check user_id is not None instead of truthiness in the rest of list_sessions, as the other services do.

Testing Plan

Unit Tests:

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

Added test_list_sessions_with_empty_user_id_returns_no_sessions to tests/unittests/integrations/firestore/test_firestore_session_service.py. It fails on main (collection_group is called) and passes with this change.

$ pytest tests/unittests/integrations/firestore
53 passed

$ pytest -n auto tests/unittests/sessions tests/unittests/integrations
2084 passed, 2 skipped, 1 failed

The one failure, integrations/vmaas/test_agent_engine_environment.py::test_reading_directory_raises, also fails on main without this change.

pyink and isort report no changes.

Manual End-to-End (E2E) Tests:

Ran the snippet from #7662 (mocked Firestore client with two other users' sessions):

• on main: [('alice', 's-alice'), ('bob', 's-bob')] collection_group used: True
• with this change: [] collection_group used: False

Also ran against the Firestore emulator (cloud-firestore-emulator v1.19.8), with sessions for alice and bob:

• on main: list_sessions(user_id="") returns the sessions of both users
• with this change: list_sessions(user_id="") returns []; None and "alice" are unchanged
• create_session(user_id="") fails on both with 400 Document name ".../users//sessions/..." lacks a resource id, so no session for "" can exist

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.

[Somay-kousis]

Ran this locally. The fix does what it says for the leak: user_id="" no longer takes the collection_group path that returns every user's sessions.

What I ran (PR head e44692c, merge base d324744)

• New test test_list_sessions_with_empty_user_id_lists_only_that_user: passes on head. With only firestore_session_service.py reverted to the merge base it fails with Expected 'collection_group' to not have been called. Called 1 times.
• tests/unittests/integrations/firestore: 53 passed on head.
• Grepped the file for other truthiness checks on user_id: none left after this change.

Findings

1. Worth considering (suspicion, not verified against a real database): with a real client, user_id="" now builds a query on an empty path segment. src/google/adk/integrations/firestore/firestore_session_service.py:371 calls _get_sessions_ref(app_name, ""), and google-cloud-firestore 2.34.1 accepts document("") locally. The query parent it would send is projects/p/databases/(default)/documents/adk-session/app/users/ (trailing slash, empty document id). I expect the Firestore backend to reject that with INVALID_ARGUMENT rather than return [], so callers would get an exception where the other session services return an empty list. I had no emulator, so I could not confirm the server response. create_session(user_id="") builds the same path, so no session for "" can exist in Firestore anyway. If the server does reject it, returning an empty ListSessionsResponse early when user_id == "" would match the other services and avoid the round trip. Either way this is still better than the current leak.

2. Nit: the second change (line 395, user-state lookup) is not covered by the test. I put line 395 back to if user_id: and the whole firestore suite still passes (53 passed). As far as I can tell the result is the same in that case, because the else branch would fetch user "" through get_all instead, so this line is a consistency change rather than a behaviour fix. Fine to keep, just noting the test only pins line 371.

Not checked

• Behaviour against a real Firestore instance or the emulator (see 1).
• Anything outside list_sessions, and the broader tests/unittests/sessions suite.

[AkshayShah03]

Thanks for running it and for the careful review @Somay-kousis.

On (1), you were right. I checked against the Firestore emulator (cloud-firestore-emulator v1.19.8):

• list_sessions(user_id="") with the first commit raised 400 Document parent name ".../app/users/" has invalid trailing "/".
• create_session(user_id="") fails the same way (400 Document name ".../users//sessions/..." lacks a resource id), so no session for "" can exist.
• On main it returns the other users' sessions, so the leak is real against a live backend too.

af160d6 now returns an empty ListSessionsResponse for user_id == "" without querying. Against the emulator that gives [], and None / "alice" are unchanged.

On (2): with the early return, the empty-string case no longer reaches the user-state lookup, so that line is now only the is not None consistency change, as you said. The test now checks the early return directly, and it fails against main's version of the file.