Fix: return the winner's session when getOrCreateSession loses a create race - #821
Open
AmaadMartin wants to merge 4 commits into
Open
Fix: return the winner's session when getOrCreateSession loses a create race#821AmaadMartin wants to merge 4 commits into
AmaadMartin wants to merge 4 commits into
Conversation
added 4 commits
August 8, 2026 12:55
Two callers that ask for the same session id can both read undefined and both call createSession. The loser rejects, with a service error or a driver constraint error. Re-read the key once when createSession fails, and return the session a concurrent caller created. Rethrow the original error when no session exists.
init() opens the MikroORM connection and nothing could release it, so every test reached the private field through a cast. Close the connection through a public method instead.
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
Closes: #issue_number
Related: #issue_number
Problem:
BaseSessionService.getOrCreateSessionreads the session, then creates it. Two callers that ask for the same session id both readundefinedand both callcreateSession, so the loser rejects.AgentTool.runAsyncand the dev server routePOST /api/reasoning_engineboth hit this, because they pass a fixed session id.DatabaseSessionServicefails today;InMemorySessionServiceinstead overwrites the winner's session object.Solution:
getOrCreateSessionnow catches acreateSessionfailure, reads the key once more, and returns the session a concurrent caller created. It rethrows the original error when no session exists, so a real outage still surfaces. The re-read runs only on the error path, so the success path issues the same calls as before. The method does not match on the error message or class:DatabaseSessionServicealone reports a duplicate id as either its ownErroror a sqlite constraint violation, and the remote services define their own shapes.Two notes for the reviewer:
request.state. That is the behaviour the method already has when the session exists before the call.adk-pythonhas the same shape inRunner._get_or_create_session. It is not a mechanical port (different class, and Python can catch its ownAlreadyExistsError), so it is queued as a separate task.Collision check, per the pipeline rule:
gh pr list --repo AmaadMartin/adk-js --state open --limit 300returns 300 open PRs. No PR changesgetOrCreateSession. Three PRs overlap on files: #818 (addsflush()), #616 (adds a pagination helper) and #711 (addsgetUserState) each edit a different region ofcore/src/sessions/base_session_service.tsand each addcore/test/sessions/base_session_service_test.ts. This PR branches frommainrather than stacking, because the three siblings overlap equally and stacking on one would not remove the conflict with the other two.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.
New file
core/test/sessions/base_session_service_test.ts, 6 tests. No existing test was changed or deleted.Coverage of the rewritten method is 100% of statements and branches. Measured with
--coverage.include=core/src/sessions/base_session_service.ts, then read from the v8 JSON report: zero uncovered statements and zero uncovered branches between the method's first and last line. The file total is lower only because that run does not exercise the other helpers in the file.DatabaseSessionService.close()is executed by the new teardown, with no uncovered statement or branch.Mutation check. I reverted the
try/catchback toreturn this.createSession(request);and re-ran the new file. 3 of the 6 tests failed, with these messages:The sqlite failure is the driver-level error, not the service's own message. That is the reason the fix re-reads the key instead of matching the error.
The other 3 tests pass before and after, by design. They pin the two early-return branches (
no sessionId,session exists) and theInMemorySessionServiceconcurrent case.InMemorySessionService.createSessionoverwrites a duplicate id today rather than rejecting, so that third test is a forward guard: it becomes a race regression test when #717 lands.DatabaseSessionService.close()is new.init()opens the MikroORM connection and nothing could release it, so all four existing teardowns incore/test/sessions/database_session_service_test.tsreach the privateormfield throughas unknown as {orm: MikroORM}. The new test callsawait service.close()instead. This is a second file undersrc/, which the spec did not ask for; the complexity review asked for it, and it removes the only cast in the diff. The four pre-existing cast sites are left alone, as a separate cleanup. The diff now contains no suppression,any, or coverage pragma at all.Manual End-to-End (E2E) Tests:
Start the dev server against a database session service, then send two simultaneous
POST /api/reasoning_enginerequests with the same body and nosessionId. Both default to'default-session', so they race.Before the change one request returns 500 with
Session with id default-session already exists.. After the change both return 200.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.
CI note
The first two runs failed on the Windows leg, and the first also failed on macOS. The failing tests differed between runs:
UnsafeLocalCodeExecutor > should execute shell code and return stdout(5s timeout), andAgentLoader discovery and loading integration(40s timeouts). None of them touch session services, andcore/test/sessions/base_session_service_test.tspassed on every leg of every run. A rerun turned all three platforms green.