Skip to content

Fix: route app:/user: initial state to the scope stores in InMemorySessionService (stacked on #609) - #623

Open
AmaadMartin wants to merge 2 commits into
feat/shared-state-delta-helperfrom
fix/in-memory-session-create-state-scoping
Open

Fix: route app:/user: initial state to the scope stores in InMemorySessionService (stacked on #609)#623
AmaadMartin wants to merge 2 commits into
feat/shared-state-delta-helperfrom
fix/in-memory-session-create-state-scoping

Conversation

@AmaadMartin

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: InMemorySessionService.createSession does not honour the app: /
user: state-scope prefixes. It runs the supplied initial state through
trimTempState, which only strips temp: keys, and stores everything else
verbatim in the per-session state map. The two scope stores (appState,
userState) are only ever written by appendEvent. Two defects follow from
that single root cause:

  1. A scoped key set at creation time is invisible to sibling sessions.
    createSession({state: {'app:config': 'dark-mode'}}) traps the value in one
    session.
  2. A scoped key set at creation time cannot update an existing scope value.
    mergeStates clones the session state and then overwrites the app: /
    user: entries from the authoritative scope stores, so if app:config
    already exists the caller's new value is returned as the old one and dropped
    — a silent write loss.

This makes state scoping depend on which backend is plugged in.
DatabaseSessionService.createSession already splits by prefix, and so does
adk-python (src/google/adk/sessions/in_memory_session_service.py,
_create_session_impl_session_util.extract_state_delta). The in-memory
service is the default for InMemoryRunner, local adk web, and nearly every
sample and test, so it is the one backend where the documented contract does not
hold.

Solution: Split the incoming state with the existing extractStateDelta
helper and write the app / user buckets into the scope stores, keeping only
the session bucket on the session itself.

The ordering is load-bearing: the scope-store write must happen before the
mergeStates call at the end of the method, or defect 2 survives — mergeStates
would re-apply the stale value over the fresh one. There is a comment at the
call site saying so, and a test that fails if the write is moved (see the
mutation log below).

The resulting scope-store write is now byte-identical to the one already in
appendEvent, so rather than copy ten lines it moves into a private
applyScopedDeltas() used by both. That is the whole of the non-test diff:
52 lines changed in one file.

This is an intentional behavioural change, not an internal refactor. Callers
who pass app:- or user:-prefixed initial state to InMemorySessionService
today get session-private storage; after this change that state is shared across
the app / user exactly as DatabaseSessionService and adk-python already
share it. Callers who want session-private state have always had the unprefixed
form. Public API surface is unchanged — nothing added, renamed or removed.

Collision check (required — and it found something)

gh pr list --repo AmaadMartin/adk-js --state open --limit 1000 \
  --json number,title,headRefName          # 522 open PRs screened
gh pr diff <n> --repo AmaadMartin/adk-js --name-only

Screened every open PR whose title or branch mentioned session / state / scope /
prefix / memory, then diffed the ones touching core/src/sessions/.

Cross-language parity

Parity wins here, since the destination of a state key is observable across the
backend boundary. The mapping now matches DatabaseSessionService and
adk-python exactly:

Key form Destination
app:k appState[appName]['k'] — shared by all sessions of the app
user:k userState[appName][userId]['k'] — shared by all sessions of that user
temp:k dropped
k the per-session state map (unchanged)

Local TypeScript convention wins for the things that do not cross the boundary:
applyScopedDeltas is a private method with no _ prefix and has no
counterpart in the Python source.

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.

Seven new cases were appended to the existing
describe('createSession') block in
core/test/sessions/in_memory_session_service_test.ts. No existing test was
modified, weakened, skipped or deleted; the four pre-existing createSession
cases still pass unchanged.

  1. shares app: state from initial state with other sessions of the same app — defect 1, the headline regression.
  2. shares user: state from initial state with other sessions of the same user
  3. does not share user: state from initial state with a different user — pins that user scope stays per-user.
  4. keeps unprefixed initial state scoped to the session that created it — pins that plain keys did not start leaking.
  5. drops temp: keys from initial state instead of promoting them — pins that temp: was not misrouted into app scope by a mis-ordered branch chain.
  6. lets app: initial state overwrite an existing app value — defect 2, the silent write loss.
  7. merges initial app: state with app state set by an event — proves the two write paths cooperate rather than clobber.

Proof each test can fail (both mutations run against the final tree, then
reverted):

Mutation A — revert createSession to const filteredState = state ? trimTempState(state) : undefined (i.e. the bug this PR fixes). 5 failed | 36 passed:

× shares app: state from initial state with other sessions of the same app
  → expected {} to have property "app:config" with value 'dark-mode'
× shares user: state from initial state with other sessions of the same user
  → expected {} to have property "user:pref" with value 'A'
× drops temp: keys from initial state instead of promoting them
  → expected {} to deeply equal { 'app:kept': 1 }
× lets app: initial state overwrite an existing app value
  → expected { 'app:config': 'dark-mode' } to have property "app:config" with value 'light-mode'
× merges initial app: state with app state set by an event
  → expected { 'app:k2': 'v2' } to deeply equal { 'app:k1': 'v1', 'app:k2': 'v2' }

Cases 3 and 4 correctly keep passing under mutation A — they pin behaviour that
is already right today and must not regress.

Mutation B — keep the split, but move applyScopedDeltas(...) to after the
mergeStates(...) call
, which is the subtle way to "fix" this and still ship
defect 2. 2 failed | 39 passed:

× lets app: initial state overwrite an existing app value
  → expected { 'app:config': 'dark-mode' } to have property "app:config" with value 'light-mode'
× drops temp: keys from initial state instead of promoting them
  → expected {} to deeply equal { 'app:kept': 1 }

Coverage. The new code (applyScopedDeltas and the rewritten createSession
prologue, lines 53–92) is at 100% statement and 100% branch coverage,
measured with --coverage.include='core/src/sessions/in_memory_session_service.ts'
and the uncovered-line set read out of the v8 JSON report. The lines that report
uncovered in that file — 176, 178, 292–294, 297–299 and branches on 173, 175,
177, 223, 229, 291, 296 — are all pre-existing listSessions pagination and
appendEvent guard paths that this change does not touch.
state_utils.ts remains at 100%/100%.

No integration test was added, deliberately: there is no integration suite for
the in-memory session service (tests/integration/sessions/ holds only
vertex_ai_session_service_test.ts), and the behaviour is fully observable
through the public InMemorySessionService API at the unit level.

Manual End-to-End (E2E) Tests:

CI does not run on this PR because its base is feat/shared-state-delta-helper,
not main (the workflow triggers on pull_request: branches: [main]), so it was
validated locally on the exact pushed commit instead:

npm ci
npm run build                                                          # ok
npx vitest run --project unit:core core/test/sessions/                 # 7 files, 156 tests passed
npm run lint                                                           # clean
npm run format:check                                                   # "All matched files use Prettier code style!"
npm run docs:check                                                     # typedoc --treatWarningsAsErrors, clean
npx tsc --noEmit -p core/tsconfig.json                                 # clean

core/test/sessions/database_session_service_test.ts (26 tests) and
core/test/sessions/state_utils_test.ts (13 tests) are included in that run and
stay green, which is what pins that the shared helper's behaviour is unchanged
for the other backend.

To reproduce the fix by hand:

import {InMemorySessionService} from '@google/adk';

const service = new InMemorySessionService();
await service.createSession({
  appName: 'app',
  userId: 'u1',
  state: {'app:config': 'dark-mode'},
});
const s2 = await service.createSession({appName: 'app', userId: 'u2'});
console.log(s2.state['app:config']); // 'dark-mode' (was undefined before)

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 2 commits August 4, 2026 00:12
InMemorySessionService.createSession pushed every non-temp: key of the
supplied initial state into the per-session state map, so an `app:` or
`user:` prefixed key never reached this.appState / this.userState. Two
defects followed: a scoped key set at creation time was invisible to
sibling sessions, and mergeStates then overwrote it from the scope
stores, silently discarding a value that was meant to update an existing
one.

Split the initial state with extractStateDelta and write the app/user
buckets to the scope stores before mergeStates re-applies the prefixes,
matching DatabaseSessionService.createSession and adk-python's
InMemorySessionService._create_session_impl.

The scope-store write is now identical in createSession and appendEvent,
so it moves into a private applyScopedDeltas().
Seven cases in the existing createSession block: app:/user: sharing and
non-sharing, unprefixed keys staying session-local, temp: keys dropped
rather than promoted, a later app: value overwriting an earlier one, and
initial app: state coexisting with app state set by an event.
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