Skip to content

Fix: let the database enforce the events -> sessions cascade in DatabaseSessionService - #740

Open
AmaadMartin wants to merge 3 commits into
mainfrom
fix/database-session-events-cascade-foreign-key
Open

Fix: let the database enforce the events -> sessions cascade in DatabaseSessionService#740
AmaadMartin wants to merge 3 commits into
mainfrom
fix/database-session-events-cascade-foreign-key

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Aug 6, 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: The events table has no foreign key onto sessions, so an event row can outlive its session. deleteSession issues two separate DELETE statements, and a failure between them leaves orphaned event rows. Those rows are unreachable through the service, so they accumulate silently and skew any direct query over events. adk-python's v1 schema prevents this with a composite foreign key.

Solution: The events table now declares the composite foreign key (app_name, user_id, session_id) onto sessions (app_name, user_id, id) with ON DELETE CASCADE. StorageSession's primary key moves to (app_name, user_id, id), because MikroORM binds a composite foreign key in the target's declaration order; that order is what keeps the events column order and primary key unchanged. ensureDatabaseCreated deletes orphaned rows and retries once, because the constraint cannot be added while they exist. deleteSession keeps its explicit event delete, because existing SQLite databases never acquire the constraint.

Generated DDL (SQLite, fresh database):

create table `events` (`id` ..., `app_name` ..., `user_id` ..., `session_id` ..., ...,
  constraint `events_app_name_user_id_session_id_foreign`
    foreign key(`app_name`, `user_id`, `session_id`)
    references `sessions`(`app_name`, `user_id`, `id`)
    on delete cascade on update cascade,
  primary key (`id`, `app_name`, `user_id`, `session_id`));

Parity note: the primary-key order is adopted from adk-python's schemas/v1.py. Parity wins here because the order is observable in the physical schema. Local convention wins for the mirrors: appName, userId and sessionId stay declared as persist: false properties, so every existing query filter keeps working.

Operational impact:

  • First init() after upgrade on PostgreSQL or MySQL runs DDL against live tables: sessions' primary key is dropped and recreated, and the foreign key is added to events. sessions holds one row per session, so this is bounded, but it takes an exclusive lock. Operators of large deployments may prefer to apply the DDL out of band.
  • Orphaned event rows are deleted during that upgrade.
  • Existing SQLite databases keep working and do not gain the constraint. MikroORM's schema comparator does not emit ADD CONSTRAINT for SQLite.
  • A caller who passes an options object can set MikroORM's schemaGenerator: {createForeignKeyConstraints: false} to opt out of foreign-key DDL.
  • The schema version pin is unchanged. validateDatabaseSchemaVersion accepts one value and has no upgrade path, so a bump would break every existing deployment. Row contents do not change, so no bump is warranted.

Existing test changed, deliberately: "keeps events composite key columns within the MySQL index limit" read length off the StorageEvent properties appName, userId and sessionId. Those three are now persist: false mirrors. They carry no length and no longer drive the DDL, so the old assertion cannot hold and could not be kept alongside a new one. The test now reads the four key-column lengths from StorageEvent.id and the session relation, which are the properties that emit those columns. It keeps its name and both assertions, 191 per column and the 3072-byte budget, and it moves from operations_test.ts to schema_test.ts, the file that mirrors the source it tests.

Collision check: gh pr list --repo AmaadMartin/adk-js --state all --limit 1000 returns no open PR that touches core/src/sessions/db/schema.ts. #641 and #492 change deleteSession and appendEvent; this PR does not touch deleteSession and makes a one-expression change in appendEvent. #291 adds a doc comment to parseDbUri in operations.ts, a different region from ensureDatabaseCreated.

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/db/schema_test.ts (7 tests) drives a real in-memory SQLite database through MikroORM with no mocks: the generated DDL declares the cascade, the events primary key keeps its column order, a raw DELETE FROM sessions removes the events, an event naming a missing session is rejected, pragma foreign_keys returns 1, and the read-only mirrors still serve find and nativeDelete. It also holds the key-column length test moved out of operations_test.ts.

core/test/sessions/db/operations_test.ts adds five cases: the orphan repair runs and the schema update is retried, the caught failure is logged with its cause, a second failure propagates unchanged, deleteOrphanedEvents removes only the row whose session is gone, and it reports how many rows it deleted.

No CI check ran on this pull request, so I validated locally on the pushed commit:

npx vitest run --project unit:core core/test/sessions/                        # 7 files, 149 passed
npx vitest run --project integration tests/integration/lazy_load_db_drivers/  # 5 passed
npm run build                                                                 # clean
npm run lint                                                                  # clean, exit 0
npx prettier --check 'core/{src,test}/sessions/**/*.ts'                       # clean
npx tsc --noEmit                                                              # no new errors vs main

tests/integration/build_setup/build_setup_test.ts does not run in my sandbox. Its fixtures run npm install, which fails with E403 against the registry mirror I am behind. The failure is in the fixture install step and is independent of this change.

Coverage on the touched sources, measured with --coverage over these test files: core/src/sessions/db/operations.ts is 100% of statements, branches, functions and lines. core/src/sessions/db/schema.ts is 99.23% of lines; the one uncovered line is the pre-existing non-string arm of CamelCaseToSnakeCaseJsonType.convertToJSValue, which this change does not touch.

Proof that the tests can fail. Each mutation was applied to the source, the tests were run, and the source was restored.

  1. Removed deleteRule: 'cascade' from the session relation. "declares an events -> sessions foreign key that cascades on delete" failed with expected ... to contain 'foreign key(...)... on delete cascade'. "deletes the events when the session row alone is deleted" failed with DELETE FROM sessions ... - SQLITE_CONSTRAINT: FOREIGN KEY constraint failed.
  2. Restored StorageSession's primary key to (id, app_name, user_id) and set joinColumns to match. "keeps the events primary key columns in their original order" failed with expected ... to contain 'primary key (\id`, `app_name`, `user_id`, `session_id`)'; the generated table was primary key (`id`, `session_id`, `app_name`, `user_id`)`.
  3. Replaced the session relation with the three scalar @PrimaryKey columns. Four of the six new tests failed, including "rejects an event that names a session which does not exist".
  4. Removed the retry from ensureDatabaseCreated. "deletes orphaned events and retries when the schema update fails" failed with promise rejected ... instead of resolving, and "propagates the error when the schema update fails again" failed with expected ... 'second failure' but got 'first failure'.
  5. Widened DELETE_ORPHANED_EVENTS_SQL to DELETE FROM events WHERE 1 = 1. "removes only the events whose session is gone" failed with expected [] to deeply equal [ 'live-event' ].
  6. Dropped the 'run' method argument from the purge execute() call, which is what makes affectedRows real. "logs how many rows it deleted" failed: the log read Deleted undefined event rows ....
  7. Dropped the caught error from the retry log. "logs the schema update failure that triggered the retry" failed with Received: "Schema update failed; retrying after deleting orphaned events." against the expected message ending in Error: lock timeout.

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

The SQLite path is covered end to end by schema_test.ts against a real database. To check the PostgreSQL upgrade path against a live instance:

  1. On main, create a session with several events, then run DELETE FROM sessions WHERE ... so the event rows are orphaned. Confirm SELECT count(*) FROM events is non-zero.
  2. Switch to this branch and construct a DatabaseSessionService against the same database. Confirm init() resolves and the orphaned rows are gone.
  3. Inspect the tables: sessions' primary key is (app_name, user_id, id) and events carries the foreign key with ON DELETE CASCADE.
  4. Create a new session with events, delete the session row directly, and confirm the events disappear with no application statement.
  5. Restart against the same database and confirm init() emits no further DDL.

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 6, 2026 15:15
An event row could outlive its session, because nothing but application
code linked the two tables. The events table now declares a composite
foreign key onto sessions with ON DELETE CASCADE.

StorageSession's primary key moves to (app_name, user_id, id) to match
adk-python. MikroORM binds a composite foreign key in the target's
declaration order, so that order is what keeps the events column order
and primary key unchanged.

The MySQL index-limit test now derives the key column lengths from the
properties that emit them, because the session relation owns three of
the four columns.
Adding the events -> sessions foreign key fails on a PostgreSQL or MySQL
database that still holds event rows whose session is gone, so the
service would not start. ensureDatabaseCreated now deletes those rows
and retries the schema update once. The purge does not run when the
schema update succeeds, which is the common case.
The bare catch discarded every updateSchema failure before running a
destructive DELETE, so a lock timeout or a permissions error triggered
the orphan purge with no trace of the cause. The retry now logs the
error it caught, and deleteOrphanedEvents logs the affected row count at
warn, because that step deletes user rows during startup.

The events key-column length test moves to schema_test.ts, which is
where it belongs; operations_test.ts no longer carries a second suite
named 'storage schema'.
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