Feat: port FirestoreSessionService from adk-python to @google/adk-integrations - #466
Open
AmaadMartin wants to merge 7 commits into
Open
Feat: port FirestoreSessionService from adk-python to @google/adk-integrations#466AmaadMartin wants to merge 7 commits into
AmaadMartin wants to merge 7 commits into
Conversation
added 7 commits
August 1, 2026 09:18
…cripts vitest.config.ts has defined a unit:integrations project since it was added, but the root test/test:unit/test:coverage scripts never passed --project unit:integrations, so integrations/test never ran even though integrations/src is in the coverage include list. Wiring the project in surfaces integrations/test/version_test.ts, which had rotted to expect 1.3.0 while integrations/src/version.ts exports 1.5.0. The expectation is corrected, not skipped or deleted. Also re-export randomUUID from @google/adk so consumer workspaces can generate session IDs from the same source core uses, rather than a deep subpath import that core/package.json does not expose.
Added with npm install --workspace=integrations so npm picks the caret range and regenerates the lockfile. It goes in integrations, not core, so the Firestore SDK stays off every adk-js user's install path.
Adds a document-store session backend so users already running Firestore do not have to fall back to SQL. Sessions live under <root>/<appName>/users/<userId>/sessions/<sessionId>, with app- and user-scoped state in the sibling app_states and user_states collections and merged back under the app:/user: prefixes on read. appendEvent runs in a Firestore transaction, serialized per session by a reference-counted in-process lock, and bumps a revision counter. deleteSession writes a DELETING marker first so a concurrent append fails rather than resurrecting a half-deleted session. Collection names, the document hierarchy and the DELETING marker match firestore_session_service.py byte for byte. Timestamps diverge deliberately: client-side Timestamp values in epoch milliseconds rather than SERVER_TIMESTAMP sentinels in seconds, matching adk-js's Event.timestamp and Session.lastUpdateTime units.
…y fake Adds an in-memory Firestore that needs no emulator, network or credentials, so the suite runs on every OS in the CI matrix. Its transaction buffers writes until the callback resolves, and its reads suspend on a shared per-tick barrier so concurrent transactions really do interleave -- a plain setTimeout would not, because Node drains the microtask queue between timer callbacks and each transaction would run to completion before the next resumed. The fake refuses query shapes it does not implement rather than returning a wrong answer, and has its own tests for merge semantics and transaction rollback.
…port The numRecentEvents === 0 short-circuit was justified as working around the Node SDK rejecting limitToLast(0). It does not: validateInteger only rejects non-integers, so limitToLast(0) is accepted. The short-circuit is still load-bearing for a different reason -- 0 is falsy, so without it the limit is skipped and every event comes back -- and returning no events is what VertexAiSessionService and the adk-python database, sqlite, in-memory and Vertex AI backends do. The 500-write batch cap is a Firestore server-side limit, not something the client enforces, so the comment no longer implies otherwise.
…rocess lock
appendEvent derived the persisted session state from the caller's
in-memory session.state rather than from the snapshot the transaction
read. That put the written value outside the transaction's read set, so
Firestore could not detect a conflicting concurrent write, and it let a
stale caller clobber keys another writer had committed.
Deriving it from data.state fixes both. Every value the transaction
writes now comes from the session document it read, so Firestore aborts
and re-runs the callback on conflict -- across processes, which the
in-process lock never covered. SessionLockMap and its plumbing are
deleted; the lock did not even close the race it existed for, because
super.appendEvent merges the delta into session.state after the lock is
released. It also matches DatabaseSessionService and
InMemorySessionService, which both persist storage state plus the delta.
The shared app/user state documents are no longer read inside the
transaction: set(..., {merge: true}) merges field by field on the
server, so the reads were dead work, and keeping those documents out of
the read set stops unrelated sessions of one app aborting each other.
The fake now tracks document versions, refuses a commit whose read set
moved, and retries like the real client -- without that, the
concurrent-append test passed against a fake weaker than Firestore.
splitStateDelta moves to core beside its inverse mergeStates, replacing
the two hand-rolled copies in DatabaseSessionService, and randomUUID
comes from node:crypto rather than widening the @google/adk public API
for one Node-only call site.
The Firestore backend is the only consumer and it is Node-only, so it takes randomUUID from node:crypto rather than permanently widening the @google/adk public API for one call site.
This was referenced Aug 1, 2026
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
N/A — no existing issue.
Problem: adk-js ships three session backends —
InMemorySessionService,DatabaseSessionService(SQL via MikroORM) andVertexAiSessionService— and no document-store backend. adk-python has one,google/adk/integrations/firestore/firestore_session_service.py. Users already running Firestore have to fall back to SQL or write their own backend.git grep -il firestore -- 'core/src/**' 'integrations/src/**'returned nothing before this change, so this is a from-scratch port.Solution:
FirestoreSessionServiceinintegrations/src/firestore/, exported from@google/adk-integrations.createSession/getSession/listSessions/deleteSession/appendEvent, using the adk-js single-request-object signatures.app:/user:prefixes on read.appendEventruns in a Firestore transaction and bumps arevisioncounter, serialized per session by a reference-counted in-process lock (SessionLockMap).deleteSessionwrites astatus: "DELETING"marker first, so a concurrent append fails rather than resurrecting a half-deleted session.It lives in
integrations, notcore, becauseintegrations/package.jsonexists to hold integrations that pull third-party SDKs; putting@google-cloud/firestoreincorewould put a Firestore SDK on every adk-js user's install path.Collision check
gh pr list --repo AmaadMartin/adk-js --state all --limit 300— no open or closed PR touches Firestore or adds a session backend, so nobody is building this feature.One PR overlaps on a prerequisite: #236 ("run the orphaned unit:integrations vitest project from the root test scripts") changes the same three
package.jsontest scripts and the sameintegrations/test/version_test.tsexpectation. I did not stack on it. Its branch is 25 commits behindmainand its base predatesRelease: v1.5.0 (#503), so stacking would have rebased this work back across a release commit and made the dependency install resolve against a pre-1.5.0 lockfile — and a stacked base also means CI never runs (validation.yamltriggers onpull_request: branches: [main]). Whichever lands first, the other resolves trivially; #236 additionally de-rots the version test againstpackage.json, which is the better long-term fix.One repo-level fix this PR needs
--project unit:integrationsadded to the roottest,test:unitandtest:coveragescripts.vitest.config.tshas defined that project since it was added, but no root script ran it, while the coverage config already includesintegrations/src/**/*.ts. Adding ~640 lines ofintegrations/srcto a coverage run that never executes its tests would have pushed global coverage down. Measured on this tree:--project unit:core --project unit:dev(the old script list)+ --project unit:integrations(this PR)Thresholds are
statements 86 / branches 87 / functions 88 / lines 86. Wiring the project in raises coverage rather than lowering it, because the new code is 100% covered.This changes one line of an existing test assertion, and that is deliberate.
integrations/test/version_test.ts:12expected'1.3.0'whileintegrations/src/version.tsexports'1.5.0'. Normally I would add a case rather than edit one, but this assertion had gone stale unrun:vitest.config.ts:64defined theunit:integrationsproject and no root script listed it, so the file never executed. There is no regression signal to preserve — the assertion has never passed at1.5.0, and the same commit is what starts running it. It is corrected in place, not skipped,.only'd or deleted, and it still pins exactly what it always pinned: that the exportedversionmatches the package's released version.One repo-level fix, not two
An earlier revision of this PR also re-exported
randomUUIDfrom@google/adkso the new backend could reach it. That is gone: it permanently widened the@google/adkpublic API for a single internal call site.@google-cloud/firestoreis Node-only and this module is deliberately excluded fromindex_web.ts, so it now takesrandomUUIDfromnode:crypto— stdlib, already used elsewhere incore(a2a/auth.ts) anddev, and without theMath.random()fallback branch thatenv_aware_utils.randomUUIDcarries for browsers.core/src/common.tsis no longer touched at all.Divergences from
firestore_session_service.py(all deliberate)Parity wins for anything observable outside the process; local convention wins for in-process concerns.
adk-session,sessions,events,app_states,user_states), theuserspath segment, the document hierarchy, the session/event document field names (including the stored snake_caseevent_data), and theDELETINGstatus string. Pinned by an explicit test asserting each literal, and by tests asserting the full literal document pathadk-session/test-app/users/test-user/sessions/<id>.Event.timestampandSession.lastUpdateTimeare epoch milliseconds (core/src/events/event.tsusesDate.now()); Python's are seconds. This port uses milliseconds throughout, viaTimestamp.fromMillis/toMillis.Timestampinstead ofSERVER_TIMESTAMP. A server sentinel does not resolve until a follow-up read, sosession.lastUpdateTimewould be unknowable at write time — Python papers over that by using a local clock for the returned session anyway. Client-side values also matchDatabaseSessionService, which usesnew Date().collection_group/ optional-userIdpath.ListSessionsRequest.userIdis required in adk-js, so Python's cross-user branch is unreachable through the typed interface, and it would need a composite index in production.listSessionsqueries the user'ssessionssubcollection directly. Python's extrawhere('appName', '==', appName)filter is dropped: it is redundant once the query is path-scoped under<root>/<appName>/users/<userId>/sessions.in_memory_session_service.tsexactly (sameasc/deschandling with theid.localeComparetie-break, samelimit === 0/page/offsetarithmetic). Server-side pagination would still need a separate count query fortotalItems, and the other two backends already paginate in memory.temp:keys are dropped from the in-memory session too. Python's_apply_temp_stateleaves them onsession.state; adk-js'sBaseSessionService.appendEventdeliberately strips them for every backend. This port callssuper.appendEvent(...)and follows the base class rather than special-casing itself.numRecentEvents: 0returns no events. adk-python's Firestore backend uses a truthy check (if config.num_recent_events:), so0there returns every event. That is the outlier:database_session_service.py,sqlite_session_service.py,in_memory_session_service.pyandvertex_ai_session_service.pyall special-case== 0, as does adk-jsvertex_ai_session_service.ts. This port follows the majority convention.google.adk.errorsequivalent, so failures use the wording the existing adk-js backends use (Session with id ${id} already exists.,Session ${id} not found for appendEvent) rather thanAlreadyExistsError/SessionNotFoundError. Swapping to typed errors when that module lands is a follow-up.Concurrency: fixed since the first revision of this PR
The first revision derived the persisted session state from the caller's in-memory
session.state(as adk-python's Firestore backend does) and guarded appends with a reference-counted in-process mutex. A complexity review caught that this was wrong on three counts, and it has been reworked:super.appendEventmerges the delta intosession.stateafter that, so a queued append could still read a pre-merge view;appendEventnow derives the persisted state fromdata.state, the snapshot the transaction itself read. Every written value is therefore in the read set, andrunTransactionaborts and re-runs the callback on conflict (DEFAULT_MAX_TRANSACTION_ATTEMPTS = 5, verified in@google-cloud/firestore/build/src/index.js:124). That serializes concurrent appends across processes — strictly stronger than the mutex, which only ever covered one.SessionLockMap,sessionLockKey, thesessionLocksfield andsession_lock_test.tsare all deleted; the test file goes with the module it tested.This also converges with the rest of adk-js:
DatabaseSessionServiceandInMemorySessionServiceboth persist storage state plus the delta, never the caller's in-memory view. It is a deliberate divergence fromfirestore_session_service.py:541-548, which usessession.state; parity loses here because the Python behaviour is the defect.The shared app- and user-state documents are no longer read inside the append transaction.
set(ref, delta, {merge: true})merges field by field on the server, so{...currentApp, ...appDelta}produced an identical document — the reads were dead work. Removing them also keeps those documents out of the read set, so two sessions of the same app no longer abort each other; a test pins that (transactionRetryCount === 0).revisionis still written and is read inside the transaction in order to be incremented. It is on-disk schema shared with adk-python.Scope note
The diff is ~2.6k lines, but 1.67k of that is tests and the in-memory fake; the shipped source is 527 lines in one new file plus small edits to
base_session_service.ts,database_session_service.ts,index.tsand the rootpackage.json. I did not split it into a stack: after the rework there is no seam left that leaves a coherent, working PR, and putting the substantive half on a non-mainbase means CI never runs there.Two helpers moved, one left in place
splitStateDeltanow lives incore/src/sessions/base_session_service.ts, directly besidemergeStates, whose exact inverse it is. It replaces the two hand-rolled copies indatabase_session_service.ts(createSessionandappendEvent), and its tests moved with it to the newcore/test/sessions/base_session_service_test.ts, mirroring the source path.core/test/sessions/is green (137 tests).paginateSessionsI deliberately left in the Firestore module, against the review's suggestion. Converging it would mean rewriting the pagination of three shipped backends (in_memory,database,vertex_ai) plus their test suites inside a feature PR — real regression risk to working code, for a readability win, and explicitly out of scope in the approved spec.splitStateDeltawas a safe move because the copies are byte-equivalent logic; the four pagination implementations are not (vertex_ai_session_service.ts:355-381differs in shape). It is queued as its own refactor.One suppression, disclosed
integrations/test/firestore/fake_firestore.tscontains a singleas unknown as Firestore, in thecreateFakeFirestorefactory, commented with its reason. There is noany,@ts-expect-error,@ts-ignoreoreslint-disableanywhere in the diff.The review proposed removing it by declaring a structural slice (
SessionFirestore) and typing the option as that instead ofFirestore. I tried it and rejected it, as the review permitted if it cost more than it saved. The service does not touch three members but a whole reference graph —collection/doc/listDocuments/orderBy/where/limitToLast/geton collections and queries,collection/get/deleteon document refs,get/getAll/set/updateon transactions,delete/commiton batches, plus snapshots — so the slice is seven interfaces (~45 lines with JSDoc at this repo's print width) of hand-maintained shadow of the SDK's type surface, shipped in production code, that must be kept in sync by hand. It also degrades the public API:client?: Firestoreis precise and discoverable;client?: SessionFirestoreis neither. Trading one commented cast in a test double for that is a net loss.Follow-up queued (not done here)
Converging the four hand-rolled
listSessionspagination implementations (in_memory_session_service.ts:136-229,database_session_service.ts:280-323,vertex_ai_session_service.ts:355-381and this one) onto a sharedcorehelper, for the reasons above.in_memory_session_service.ts'sappendEventalso still has an inline app/user split thatsplitStateDeltacould replace; its loop has a different shape (it writes into nested maps rather than building delta objects), so I left it rather than convert it untested in a feature PR.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.
65 tests across three files, all offline — no emulator, no network, no credentials, on any OS in the CI matrix. (
splitStateDelta's four tests moved tocore/test/sessions/base_session_service_test.tswith the function;session_lock_test.tswas deleted along with the module it tested.)integrations/test/firestore/fake_firestore.tsis an in-memory Firestore holding every document in one flat map keyed by full path. Three properties make it worth trusting, and it has its own tests:DELETINGrollback tests mean something;setTimeout. Node drains the microtask queue between timer callbacks, so with a per-callsetTimeouteach transaction would run to completion before the next resumed and no conflict would ever arise.It also refuses query shapes it does not implement instead of quietly returning a wrong answer. It is deliberately stricter than the real SDK on
limitToLast(0), which the real client accepts.Coverage of the new source is 100% on all four axes:
Coverage is not the proof — every test was re-run against broken code after the rework. Eleven mutations, each reverted afterwards:
session.stateagain (the bug this revision fixes)expected { b: 2 } to deeply equal { a: 1, b: 2 }expected 1 to be +0expected 1 to be 5status === 'DELETING'guardpromise resolved "{ author: 'user', …(6) }" instead of rejectingnumRecentEvents === 0guard infetchEventsexpected [ { author: 'user', …(5) } ] to deeply equal []revision: 1expected 1 to be 2,expected 1 to be 5expected [Function] to throw error including 'Session ghost not found for appendEve…' but got 'Cannot read properties of undefined (…'afterTimestampcursor by +1msexpected [] to deeply equal [ 'boundary' ]event.partialearly returnexpected [ [ …(2) ], [ …(2) ] ] to deeply equal [ [ …(2) ] ]MAX_DELETES_PER_BATCHto 1000expected 1 to be 2userspath segment touserexpected 'user' to be 'users'Mutations 1–3 are new in this revision and are the ones that matter most: 1 and 2 pin the correctness fix, and 3 proves the concurrency test now depends on real Firestore semantics rather than on a fake that could not lose an update. Two earlier rounds are worth recording because both found real weaknesses in my own tests:
trimTempDeltaStatehad already stripped the key before the filter ran — the filter was never exercised;Other gates, on the exact pushed commit:
npm run ts:checkis not green onmain(281 pre-existing errors incore/test,dev/testandtests/, and it is not wired intovalidation.yaml); this PR adds zero errors — verified by diffing the error list before and after.npm run test:coveragecannot complete locally: 16e2efiles need a Gemini API key, 4 install-heavyintegrationfixtures are the known-flaky ones, anddev/test/cli/cli_create_test.tsasserts against the machine'sgcloud config get project. All 21 failures reproduce without this change and none are inintegrationsorcore/test/sessions. CI runs the full gate and is the authority here — it was green on all three OSes for the previous revision.Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.
I was not able to run this against a real Firestore backend, and I am not going to claim otherwise. The sandbox has no GCP credentials, and
gcloud emulators firestore startfails there — thecloud-firestore-emulatorcomponent is not installed and the CLI component manager is disabled. No document paths in this PR body are transcribed from a live console; the paths asserted in the tests come from the fake and from the reference Python implementation.What I could verify against the real SDK, offline and with no fake involved, is that the reference paths and query shapes this service builds are ones the real client accepts. Running the real
Firestoreclass (no network — reference construction is lazy):That is byte-identical to the hierarchy documented at
firestore_session_service.py:79-99. TheorderBy('timestamp').where('timestamp','>=',…).limitToLast(n)chain also builds against the realQuery. It is also how I found that the real SDK'slimitToLastvalidates integrality only and acceptslimitToLast(0); thenumRecentEvents === 0short-circuit had been commented as working around a rejection that does not happen. The guard now sits at the top offetchEvents, where the falsy-zero hazard actually is, with an accurate comment.To verify end to end against a real backend:
gcloud components install cloud-firestore-emulator gcloud emulators firestore start --host-port=127.0.0.1:8080 export FIRESTORE_EMULATOR_HOST=127.0.0.1:8080 npm run buildthen, from the repo root, drive
createSession→appendEvent→getSession→listSessions→deleteSessionwithnew FirestoreSessionService({client: new Firestore({projectId: '<your-project>'})})and confirm in the emulator UI that the documents appear at the four paths above, that
revisionincrements once per append, that noapp:,user:ortemp:key appears in the session document'sstate, and that the session and every event document are gone after the delete.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.