Skip to content

Fix: align the listSessions() state contract across all session services (stacked on #376) - #477

Open
AmaadMartin wants to merge 3 commits into
fix/list-sessions-optional-user-idfrom
fix/list-sessions-state-contract-parity
Open

Fix: align the listSessions() state contract across all session services (stacked on #376)#477
AmaadMartin wants to merge 3 commits into
fix/list-sessions-optional-user-idfrom
fix/list-sessions-state-contract-parity

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Aug 1, 2026

Copy link
Copy Markdown
Owner

Please ensure you have read the contribution guide before creating a pull request.

Link to Issue or Description of Change

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

  2. Or, if no issue exists, describe the change:

Problem: ListSessionsResponse.sessions[].state means three different things depending on which BaseSessionService you instantiate, so a caller cannot swap one implementation for another without silently changing what listSessions() returns:

Implementation state returned by listSessions()
InMemorySessionService always {} — the .map() hardcoded state: {}
DatabaseSessionService mergedmergeStates(appState, uState, ss.state) per session
VertexAiSessionService the remote sessionState ({} when absent)

This is the exact swap the dev server performs based on the session URI (core/src/sessions/registry.ts), so GET /apps/:appName/users/:userId/sessions returned state-bearing sessions on a sqlite:// backend and stateless stubs on the in-memory one.

It also diverges from adk-python, where the in-memory service behaves like the adk-js database service: src/google/adk/sessions/in_memory_session_service.py _list_sessions_impl() clears events and then calls self._merge_state(app_name, uid, copied_session) for every returned session — keyed on each session's own owner uid, not on the requested user. database_session_service.py and vertex_ai_session_service.py do the same, and tests/unittests/sessions/test_session_service.py asserts merged state across the in-memory / database / sqlite parameterisation. Merged-state-on-list is an intentional, tested contract in Python.

The JSDoc on ListSessionsResponse claimed "The events and states are not set within each Session object." The "events" half is true everywhere; the "states" half was false for two of the three JS implementations and for every Python one. The doc, not the database service, was the thing that was wrong.

Solution: adopt one contract and make all three implementations plus the doc agree on it:

listSessions() returns each session with events always empty and state populated with the same merged view getSession() would return for that session — the session-scoped state plus app:-prefixed application state plus the user:-prefixed state of that session's own user, for services that keep separate app/user state stores.

Per the cross-language rule that parity wins for anything observable across the boundary, sessions[].state is observable API surface, so adk-python decides it. That makes the adk-js in-memory service the outlier:

  • InMemorySessionService.listSessions() — now returns merged state. One line, reusing the existing mergeStates() helper that createSession() and getSession() in the same file already call. This is the only production behaviour change.
  • DatabaseSessionService.listSessions() — unchanged, already merged.
  • VertexAiSessionService.listSessions() — unchanged. Agent Engine has no separate app/user state stores in either language, so the remote sessionState is the merged view.
  • events stays [] in all three; nothing new is loaded.

mergeStates() cloneDeeps the session state it is handed, so the returned state is caller-owned — mutating it cannot reach the store. No extra cloneDeep was added, which keeps the isolation depth identical to the sibling getSession()/createSession() paths.

Where this deviates from the plan, and why. The design called for mergeStates(this.appState[appName], this.userState[appName]?.[userId], session.state), using the request's userId. This PR keys user state off session.userId instead. The two are identical when a userId is supplied (sessions are bucketed by user), but this PR is stacked on #376, which makes userId optional; with the request's userId the all-users listing would merge no user state at all, while the database service on the same base builds a per-owner userStateMap keyed on ss.userId. session.userId is also literally what adk-python does (self._merge_state(app_name, uid, copied_session) inside the user_id is None loop). A new test, "merges each session owner's user state when userId is omitted", pins it and mirrors the database service's test of the same name.

Behaviour change: InMemorySessionService.listSessions() starts returning populated state where it previously returned {}. Non-breaking at the type level — no signature or type changed, so nothing fails to compile, and no export list moved. Risk is low: no adk-js caller reads .state off a listed session (dev/src/server/adk_api_server.ts:425 serialises the response straight to JSON, dev/src/server/adk_api_client.ts:106-135 only unwraps .sessions), so this adds fields to a JSON payload rather than removing any, and brings the in-memory dev-server response in line with what the same endpoint already returned for the database backend.

Collision check (required before implementation): gh pr list --repo AmaadMartin/adk-js --state open --limit 300 returned 300 open PRs. Cross-referencing every fork branch against core/src/sessions/in_memory_session_service.ts and core/src/sessions/base_session_service.ts found exactly two open PRs touching either file:

One cleanup on the stacked base. A complexity review of the full diff-against-main flagged the where binding #376 added at database_session_service.ts:265-269 — a parenthesised satisfies FilterQuery<StorageSession> & FilterQuery<StorageUserState> plus a two-line comment — as ceremony. Verified: npx tsc --noEmit reports the same 276 pre-existing errors with and without it, and a deliberately typo'd key (userIdd) is caught by neither form, because MikroORM's FilterQuery is permissive enough that the assertion adds no checking the em.count/em.find call sites do not already perform. Collapsed to one line (const where = userId === undefined ? {appName} : {appName, userId};); all 30 database tests still pass. Net -4 lines. Flagging it because it is the one file in this PR's delta that the state contract did not require.

Out of scope (unchanged here, deliberately): InMemorySessionService.createSession() still does not route app:/user:-prefixed initial state into the app/user stores the way the database service does — an independent defect, which is why the tests below establish app/user state through appendEvent(). The identical stale docstring on adk-python's ListSessionsResponse lives in a different repo. Widening ListSessionsRequest.userId is #376's job, not this PR's.

Testing Plan

Please describe the tests that you ran to verify your changes. This is required for all PRs that are not small documentation or typo fixes.

Unit Tests:
[x] I have added or updated unit tests for my change.
[x] All unit tests pass locally.

Eleven new it() cases, all purely additive — this PR's own two commits edit, rename, skip or delete no existing test (no adk-js test asserted empty state from listSessions, so nothing needed rewriting).

One disclosure about the stacked base, since a reviewer diffing against main sees it inside this PR: stacked commit 9ba55cac (#376) did edit an existing test. It dropped the trailing listAll block from should filter sessions by userId in listSessions. That block re-issued the identical request as the listU1 block two lines above it ({appName: 'app1', userId: 'u1'}) and asserted only sessions.length === 1 — a verbatim duplicate with a strictly weaker assertion, and mis-parameterised besides: the name listAll says it meant to exercise all-users listing, but it passed userId: 'u1'. #376 replaced it with lists sessions for all users in an app when userId is omitted, which does what the old name claimed. No coverage was lost. That edit belongs to #376's review, not this one.

core/test/sessions/in_memory_session_service_test.ts (nine, inside the existing describe('listSessions')):

  1. returns merged app, user and session state for each listed session — exact toEqual on {sessionKey, app:appKey, user:userKey}.
  2. returns the same state from listSessions as getSession — the swap-safety invariant.
  3. app state is visible on every listed session of the app.
  4. does not leak another user's user: state into listed sessionsu2 must not see u1's user: key.
  5. merges each session owner's user state when userId is omitted — per-owner merge on the all-users path (the Fix: make ListSessionsRequest.userId optional (adk-python parity) #376 interaction).
  6. listed sessions carry state but never events — guards against over-correcting into loading events.
  7. mutating returned state does not affect stored session state — caller ownership.
  8. merged state is returned on the paginated path — the limit !== undefined branch, not just the unpaginated one.
  9. returns session state when no app or user state exists — the undefined internal maps handled by mergeStates' parameter defaults.

core/test/sessions/database_session_service_test.ts: listSessions returns merged app, user and session state.
core/test/sessions/vertex_ai_session_service_test.ts: listSessions state matches getSession state for the same session.

The last two pin behaviour those services already had, so the shared contract cannot regress on one service without the others noticing.

Proof every new test can fail. Each was run against mutated source; all mutations were reverted afterwards.

Mutation Result
in_memory_session_service.ts: state: mergeStates(...)state: {} (the pre-fix code) 8 of 9 new in-memory tests fail, e.g. AssertionError: expected {} to deeply equal { sessionKey: 'sv', …(2) }, expected undefined to be 'av', expected undefined to be 'A'
in_memory_session_service.ts: state: mergeStates(...)state: session.state (hand out the live reference) the 9th, mutating returned state does not affect stored session state, fails: AssertionError: expected { sessionKey: 'mutated', …(1) } to deeply equal { sessionKey: 'sv' }
database_session_service.ts: const merged = mergeStates(appState, uState, ss.state)const merged = {} listSessions returns merged app, user and session state fails: AssertionError: expected {} to deeply equal { 'app:appKey': 'av', …(2) }
vertex_ai_session_service.ts: state: sessionObj.sessionStatestate: {} listSessions state matches getSession state for the same session fails: AssertionError: expected {} to deeply equal { sessionKey: 'sv', 'app:k': 'av' }

Two mutations were needed on the in-memory service precisely because coverage alone would not have distinguished them: state: {} leaves the isolation test green, and state: session.state leaves the merge tests green.

Coverage. core/src/sessions/in_memory_session_service.ts measures 97.18% statements / 95.38% branches / 100% functions. The new lines are fully covered; the only uncovered lines (248-250, 253-255) are pre-existing appendEvent() warning guards this PR does not touch.

Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.

Start the dev API server against the in-memory backend, create a session, append an event carrying app:, user: and plain state deltas, then GET /apps/<app>/users/<user>/sessions and compare against the same probe run with a sqlite:// session service. Before this change the in-memory response carried "state": {}; after it, both backends return the same merged state over real HTTP:

in-memory 200 {"app:appKey":"av","user:userKey":"uv","sessionKey":"sv"}
in-memory events: []
sqlite    200 {"sessionKey":"sv","app:appKey":"av","user:userKey":"uv"}
sqlite    events: []
backends agree: true

(Key order differs only because the in-memory session's own state already holds the literal prefixed keys written by updateSessionState, while the database service routes them into its app/user tables; the merged contents are identical.)

Local validation on the pushed commit. This PR is stacked on #376, so its base is not main and the pull_request: branches: [main] workflows do not trigger — CI is absent, not green. Everything was therefore run locally on the exact pushed commit:

  • npx vitest run --project unit:core core/test/sessions/in_memory_session_service_test.ts core/test/sessions/database_session_service_test.ts core/test/sessions/vertex_ai_session_service_test.ts → 3 files, 130 tests passed
  • npx vitest run --project unit:dev dev/test/server/adk_api_server_test.ts51 passed; dev/test/server/adk_api_client_test.ts20 passed (the only production consumers of listSessions)
  • npm run build → clean
  • npm run lint → clean
  • npm run format:checkAll matched files use Prettier code style!
  • npm run docs:check (typedoc --treatWarningsAsErrors) → clean, so the reworded JSDoc resolves
  • npx tsc --noEmit276 errors before and after this change, i.e. no new type errors. (The tree has a pre-existing type-check backlog; several open PRs address it.)

Checklist

[x] I have read the CONTRIBUTING.md document.
[x] I have performed a self-review of my own code.
[x] I have commented my code, particularly in hard-to-understand areas.
[x] I have added tests that prove my fix is effective or that my feature works.
[x] New and existing unit tests pass locally with my changes.

Amaad Martin added 3 commits August 1, 2026 12:44
…e lists

InMemorySessionService.listSessions() hardcoded state: {} on every returned
session, while DatabaseSessionService merged app/user/session state and
VertexAiSessionService returned the remote session state. Swapping one service
for another therefore silently changed what listSessions() returned, and the
in-memory service was the outlier against adk-python, whose in-memory
_list_sessions_impl calls _merge_state per session.

Merge the stored app state and the state of each session's own owner into the
listed session, reusing the existing mergeStates helper (which cloneDeeps the
session state, so callers cannot mutate the store). Keying user state off
session.userId rather than the request userId keeps the all-users listing
correct, matching how DatabaseSessionService builds its per-owner user state
map.

Also correct the ListSessionsResponse JSDoc, which claimed neither events nor
state are populated. Events are indeed always empty, but state is populated by
every implementation in both languages.
… services

Nine new cases in the in-memory suite cover the merged app/user/session view,
equality with getSession, app state on every session of the app, user-state
isolation between users, per-owner user state when userId is omitted,
events staying empty, caller ownership of the returned state object, the
paginated path, and the no-app-no-user-state case.

One case each on the database and Vertex suites pins the behaviour those
services already had, so the shared contract cannot regress on one service
without the others noticing.
… clause

The parenthesised satisfies FilterQuery<StorageSession> &
FilterQuery<StorageUserState> asserted a type the em.count and em.find call
sites already check, and it caught nothing they do not: tsc reports the same
error count with and without it, and a typo'd key is rejected by neither form
because MikroORM's FilterQuery is permissive. The comment above it restated
what the shared binding and the comment at the user-state query already say.

Collapse to a single line. No behaviour change.
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.

1 participant