Skip to content

fix: report the events the SQLite migration actually migrated - #7299

Closed
feiiiiii5 wants to merge 2 commits into
google:mainfrom
feiiiiii5:fix/migration-event-count
Closed

feiiiiii5 wants to merge 2 commits into
google:mainfrom
feiiiiii5:fix/migration-event-count

Conversation

@feiiiiii5

Copy link
Copy Markdown
Contributor

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):

Testing Plan

Problem:

migrate_from_sqlalchemy_sqlite skips any event row whose to_event() raises, warns per row, and then logs Migrated {len(events)} events. — the source row count. The transaction commits and the run ends with Migration completed successfully., so a run that dropped events claims the full count, and the skipped rows are only visible one warning at a time. migrate_from_sqlalchemy_pickle in the same package already reports the number of successful inserts (num_rows).

Solution:

Count the successful inserts and report that, then name the skipped ids in one warning so the operator knows the destination is incomplete while the source database still holds those events.

# before
WARNING  Failed to migrate event event2: unreadable event payload
INFO     Migrated 2 events.            # 1 actually migrated
INFO     Migration completed successfully.

# after
WARNING  Failed to migrate event event2: unreadable event payload
INFO     Migrated 1 events.
WARNING  Skipped 1 event(s) that could not be migrated: event2. They are
         still in the source database; re-run the migration once the cause
         is fixed.
INFO     Migration completed successfully.

Unit Tests:

  • I have added or updated unit tests for my change.
  • All unit tests pass locally.

tests/unittests/sessions/migration/test_migration.py::test_sqlite_migration_reports_only_migrated_events builds a v0 database with two events, makes one to_event() raise, runs migrate(), and asserts the destination holds event1 only, the log says Migrated 1 events., does not say Migrated 2 events., and names event2.

  • On main (044a1ec) that test fails: the log says Migrated 2 events. with one row in the destination.
  • On this branch: .venv/bin/python -m pytest tests/unittests/sessions/migration/ -q → 61 passed.
  • pre-commit run --files <changed files> → fix end of files, trim trailing whitespace, ruff, isort, pyink, addlicense, codespell, ADK compliance checks and the doc-link check all pass.
  • .venv/bin/python -m mypy src/google/adk/sessions/migration/migrate_from_sqlalchemy_sqlite.py → no issues.

Manual End-to-End (E2E) Tests:

Not run — this is an offline migration script, and the regression test drives the real migrate() against a real v0 SQLite database plus a real destination database, so the only mocked part is the one to_event() call that raises.

Checklist

  • I have read the CONTRIBUTING.md document.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing unit tests pass locally with my changes.
  • I have manually tested my changes end-to-end.
  • Any dependent changes have been merged and published in downstream modules.

Additional context

Scope is deliberately one file plus one test: the count, and the warning that names what was dropped. I did not change whether the migration commits when rows are skipped — the pickle path commits too, so that stays a separate decision for maintainers. Happy to drop the second warning if you would rather have only the corrected count.

A row whose to_event() raises is logged and skipped, but the summary
printed len(events) - the source row count - so a run that dropped rows
reported the full count and then "Migration completed successfully". The
pickle migration in the same package already counts successful inserts
(migrate_from_sqlalchemy_pickle.py num_rows). Count the inserts here too and
name the skipped ids, so the operator sees which events are still only in
the source database.
Re-running the migration into the same destination fails with
"UNIQUE constraint failed: sessions..." because the session rows are
already there, so telling the operator to just re-run sends them into
an error. The test now covers that advice end to end: a second run into
a fresh destination recovers the skipped row, and re-running into the
used one exits.
@feiiiiii5

Copy link
Copy Markdown
Contributor Author

You are right, and I confirmed it end to end before changing the wording — a second migrate() into the same dest.db dies on UNIQUE constraint failed: sessions.app_name, sessions.user_id, sessions.id at migrate_from_sqlalchemy_sqlite.py:107, because the session rows are already committed there. That advice would have sent the operator into an error.

Done in c220ea08: the warning now says to re-run into a new, empty destination, and says why. The test covers the whole loop rather than just the string —

  • first run: Migrated 1 events., event2 named, destination holds event1 only;
  • with the cause fixed, a run into a new destination recovers both rows and logs no Skipped line;
  • re-running into the used destination exits.

I also ran your key-comparison workaround shape and it is the right instinct. The destination is not the only place to check, though: the sessions and app_states/user_states inserts run before events and have no per-row try, so a bad row there aborts the whole run rather than being skipped. The events count was the only place that could report success over a partial migration, which is why I kept the scope there.

61 passed for tests/unittests/sessions/migration/, mypy clean on the changed file, and pyink/isort/ruff at the versions pinned in .pre-commit-config.yaml are clean on both files. I ran the pre-commit hooks via their pinned tools rather than pre-commit run itself, since the hook environment is not installed in this worktree.

One open question for the sessions owners, same as in #7300: I did not change whether the migration commits when rows are skipped. The pickle path commits too, so leaving it is consistent for now, but a run that reports success over a knowingly incomplete destination is a decision worth making explicitly.

copybara-service Bot pushed a commit that referenced this pull request Sep 28, 2026
@adk-bot

adk-bot commented Sep 28, 2026

Copy link
Copy Markdown
Collaborator

Thank you @feiiiiii5 for your contribution! 🎉

Your changes have been successfully imported and merged via Copybara in commit 31e5358.

Closing this PR as the changes are now in the main branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merged [Status] This PR is merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SQLite migration reports the source event count after skipping rows

3 participants