Fix: delete a session and its events in a single database transaction

[AmaadMartin]

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):
   Closes: #issue_number
   Related: #issue_number
2. Or, if no issue exists, describe the change:

Problem: DatabaseSessionService.deleteSession() issued two independent nativeDelete calls on a forked EntityManager:

await em.nativeDelete(StorageSession, {appName, userId, id: sessionId});
await em.nativeDelete(StorageEvent, {appName, userId, sessionId});

No transaction is open on that fork, so each statement runs in its own implicit auto-commit transaction and the session row is durably deleted before the event delete is even attempted. If the second statement fails (constraint/trigger, lock timeout, connection drop, permission error) or the process dies in between, the event rows are permanently orphaned.

Those orphans are unreachable and never reaped:

• getSession() returns undefined as soon as the sessions lookup misses, so it never reads them.
• listSessions() selects from StorageSession only.
• Nothing else in the repo deletes from StorageEvent (writes in appendEvent(), reads in getSession()/appendEvent()).
• StorageSession and StorageEvent in core/src/sessions/db/schema.ts are unrelated entities — there is no foreign key, so there is no ON DELETE CASCADE to clean up after the failure. (adk-python declares that FK — src/google/adk/sessions/schemas/v1.py — which is why it cannot exhibit this bug. Adding the constraint to adk-js is deliberately out of scope here and queued separately.)

Two concrete consequences: an unbounded storage leak (the full event_data history of every failed delete, forever), and resurrection of deleted data — createSession() accepts a caller-supplied sessionId and only checks the sessions table for a collision, so re-creating the same (appName, userId, sessionId) makes the previous session's leaked events readable again on the next getSession().

Solution: wrap both deletes in a single em.transactional() block, following the pattern appendEvent() in the same file already uses. Both statements now commit together or roll back together.

return em.transactional(async (txEm) => {
  await txEm.nativeDelete(StorageSession, {appName, userId, id: sessionId});
  await txEm.nativeDelete(StorageEvent, {appName, userId, sessionId});
});

Why this shape:

• Both deletes are issued on txEm, not on the outer fork — a statement issued on the outer em runs outside the transaction and reintroduces the bug in a harder-to-see form (see mutation 2 below, which deadlocks).
• No explicit txEm.commit() in the callback: em.transactional() commits on return and rolls back + rethrows on rejection (@mikro-orm/core, declared as ^6.6.10 in core/package.json, resolved to 6.6.14 in the lockfile).
• The statement order (sessions then events) is unchanged.
• The driver error is not caught, wrapped or retried; it propagates unchanged, so the existing callers (core/src/runner/runner.ts:207, dev/src/server/adk_api_server.ts:522) need no change.

Not a breaking change. The public signature, exports and success-path behaviour are identical; deleting a non-existent session is still a no-op that resolves. Only the failure path changes, by leaving no partial state behind. One consequence worth stating: both DELETEs now hold their locks until the single commit, marginally widening the lock window on concurrent backends. That is the intended cost of atomicity and matches what appendEvent() already does.

Collision check. All 539 open PRs on the fork were listed; three touch core/src/sessions/database_session_service.ts — #492 (wraps appendEvent in one transaction), #386 (removes private-ORM casts from the test, adds a public close()), #607 (temp: state visibility). gh pr diff on each shows none of them touches deleteSession, and gh search prs for "deleteSession transaction" / "atomically delete session events" returns nothing. No overlap, so this branches from main rather than stacking. This change stays strictly inside deleteSession plus two added test blocks so it rebases cleanly over all three.

Deviation from the plan, disclosed. The commit-path test asserts through the public API (re-create the same key, assert the event list is empty) instead of counting rows via (service as unknown as {orm: MikroORM}). It pins the same invariant — and states the user-visible "resurrection" consequence directly — without adding a private-member cast, which the JS guidelines forbid and which #386 is actively removing from this file.

One unavoidable private access, named once. The rollback test has to install a sqlite abort trigger on the live in-memory connection the service holds, and DatabaseSessionService exposes no accessor for its ORM handle on this base. Rather than add a fifth inline cast, the test file now names it once in a module-level ormOf() helper and routes all call sites through it:

function ormOf(service: DatabaseSessionService): MikroORM | undefined {
  return (service as unknown as {orm?: MikroORM}).orm;
}

The return type mirrors the optional private orm?: MikroORM field, so afterEach keeps its existing guard and the initialized sites assert with !. The production file is unchanged for this — widening private orm to satisfy a test would be strictly worse than the cast. Net effect: this branch takes the file from four inline unknown-casts (one of them double-casting redundantly with a trailing as MikroORM) down to one, in a named and documented helper.

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.

Two tests added to the existing describe('DatabaseSessionService', ...) block in core/test/sessions/database_session_service_test.ts. The pre-existing 'should delete a session' test is untouched, and no existing test was modified or removed.

1. 'should delete a session and its events in one transaction' (commit path) — creates a session, appends an event, deletes, asserts getSession() is undefined, then re-creates the same (appName, userId, sessionId) and asserts events is [].
2. 'should not delete the session when deleting its events fails' (rollback path — the regression test) — creates a session, appends an event, installs a sqlite BEFORE DELETE ON events trigger that does RAISE(ABORT, 'event delete blocked'), asserts deleteSession() rejects with that message, then asserts the session row count is 1, the event row count is 1, and getSession() still returns a session with one event. ABORT (not ROLLBACK) is used deliberately: it undoes only the offending statement and leaves the enclosing transaction open for MikroORM to roll back, which is exactly the failure shape this fix addresses.

Each test runs against a fresh :memory: database from the file's beforeEach, so the trigger cannot leak between tests and no DROP TRIGGER cleanup is needed.

npx vitest run --project unit:core core/test/sessions/database_session_service_test.ts
  → Test Files 1 passed (1) | Tests 28 passed (28)
npm run lint          → clean
npm run format:check  → All matched files use Prettier code style!
npm run build         → success

npm run ts:check reports 273 pre-existing tsc --noEmit errors in the test tree on the base commit; this branch adds none (identical count with the change stashed and applied).

Proof the tests can fail (mutation testing). Coverage was not treated as proof — each new test was run against mutated source:

#	Mutation	Result
1	Revert deleteSession to the two bare em.nativeDelete calls (the unfixed code)	Test 2 FAILS: AssertionError: expected +0 to be 1 on the session-count assertion — the session row was committed away while the blocked event row survived. rejects.toThrow still passes, which is why the count assertion is the regression signal. Test 1 passes, as expected for a companion assertion.
2	Issue the session delete on the outer em instead of txEm	All three delete tests FAIL (sqlite lock contention, 15s timeout) — confirms both statements must be on the transactional fork.
3	Drop the StorageEvent delete from the transaction	Test 1 FAILS: expected [ { timestamp: … } ] to deeply equal [] (the orphaned event resurfaces on the re-created session); test 2 FAILS: promise resolved "undefined" instead of rejecting.

Coverage. New lines 357–375 of core/src/sessions/database_session_service.ts have zero uncovered statements and zero uncovered branches (measured with @vitest/coverage-v8, parsed from coverage-final.json restricted to the new range). Whole-file coverage from this one test file is 85.67% lines / 81.63% branches; the shortfall is entirely pre-existing gaps in appendEvent, init and error paths outside this change, which other test files exercise.

Manual End-to-End (E2E) Tests:
No CLI, server or model interaction is involved, so no e2e fixture was added — the behaviour is exercised end-to-end against a real sqlite engine through the public DatabaseSessionService API by the two unit tests above. To sanity-check by hand:

npm ci && npm run build
npx vitest run --project unit:core core/test/sessions/database_session_service_test.ts

Confirm the two new tests pass and that the pre-existing 'should delete a session' test still passes unchanged, which demonstrates the success path is untouched.

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.