fix(sessions): stop the v0 sqlite migration shifting event timestamps - #7337
Draft
olaflaitinen wants to merge 1 commit into
Draft
olaflaitinen wants to merge 1 commit into
olaflaitinen wants to merge 1 commit into
Conversation
migrate_from_sqlalchemy_sqlite read the naive v0 events.timestamp column through StorageEvent.to_event(), which treats it as UTC, while migrate_from_sqlalchemy_pickle read it as local time. v0 writers before 2.7.0 stored naive local time, so on a non-UTC host the sqlite migration shifted those events by the host's UTC offset. Read the column as naive local time in both migrations through one shared helper. Fixes google#7300
olaflaitinen
force-pushed
the
fix/v0-migration-timestamp-local
branch
from
September 29, 2026 10:46
72e0621 to
d8ce246
Compare
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
1. Link to an existing issue (if applicable):
2. Or, if no issue exists, describe the change:
Problem:
The two v0 session migrations read the naive
events.timestampcolumn differently:migrate_from_sqlalchemy_picklereads it as local time, whilemigrate_from_sqlalchemy_sqlitegoes throughStorageEvent.to_event(), which reads it as UTC. The same v0 database therefore migrates to timestamps that differ by the host's UTC offset depending on which path is used.Solution:
This PR implements option 1 from #7300 and is opened as a draft because the sessions owners have not yet confirmed that option.
This change makes both paths read the value as naive local time through one shared helper,
v0_event_timestamp_to_epochinsessions/migration/_schema_check_utils.py. An aware value keeps its own tzinfo.Before 2.7.0 (1b8ed3d9), and on every 1.x release,
StorageEvent.from_eventwrote this column as a naive local datetime. Naive UTC values can only exist in a v0 database that was written by 2.7.0 or later, so naive local is the reading that is correct for every row an older v0 database can hold. The discussion is in #7300.In the #7300 discussion I said I would avoid patching the value after
to_event(). The sqlite path still builds the event withto_event()and then replaces its timestamp (model_copy) with the value the shared helper computes fromstorage_event.timestamp. I did this to avoid duplicating theto_event()code in the migration module; the conversion logic itself lives only in the helper. If reviewers prefer a different structure, for example building the event withoutto_event(), I am happy to change it.Testing Plan
Unit Tests:
New tests in
tests/unittests/sessions/migration/test_migration.py:test_v0_migrations_agree_on_naive_local_event_timestamps(America/New_York and Asia/Kolkata): runs the same v0 fixture through both migrations withTZset andtime.tzset()called inside the test, and checks the pickle output'sevent_data, the sqlite output'stimestampcolumn andevent_data, and the eventsSqliteSessionServiceloads back.test_v0_migrations_lose_fall_back_hour_to_first_occurrence: pins the ambiguous 01:30 on 2025-11-02 in America/New_York to the first occurrence.Local run on Linux (WSL, Ubuntu 24.04, Python 3.11, tzdata 2025b), single process, on this branch rebased onto e4c0d94. I ran only
tests/unittests/sessions/migration/test_migration.pylocally:The whole
test_migration.pywith the sources from main (e4c0d94) and the new test file: the three new tests fail and every other test passes:pyink, isort and codespell are clean on the changed files.
pre-commit run --fileson the four changed files: every hook that had files to check passed.addlicenseis not installed locally, so that hook skipped itself; the four files already carry the license header.mypy on the three changed source files (
_schema_check_utils.py,migrate_from_sqlalchemy_pickle.py,migrate_from_sqlalchemy_sqlite.py):Success: no issues found in 3 source files.Manual End-to-End (E2E) Tests:
I built a small v0 SQLite database by hand with one event written the way a pre-2.7.0 writer stored it (naive local time from
datetime.fromtimestamp), then ranpython -m google.adk.sessions.migration.migrate_from_sqlalchemy_sqlite --source_db_path v0.db --dest_db_path migrated.dbwithTZ=Asia/Kolkata, first with the sources from main (e4c0d94) and then with this branch.Steps to reproduce (run from a checkout with the package installed, once with main and once with this branch):
Output:
The columns are the sqlite
timestampcolumn, thetimestampinsideevent_data, and the offset from the real instant (1750000000.0).This was a hand-made v0 database on Linux (WSL, Ubuntu 24.04), not a real production database.
The new unit tests cover the same flow for both migrations: they build a v0 SQLite database, run both
migrate()entry points and read the result back throughSqliteSessionService.Checklist
Additional context
Behavior change for existing users
The pickle path (
adk migrate session) does not change: it already read naive values as local time and now does so through the shared helper.The sqlite migration module (
python -m google.adk.sessions.migration.migrate_from_sqlalchemy_sqlite) changes on hosts whose local time is not UTC: rows written before 2.7.0 are now read correctly, but rows written into a v0 database by 2.7.0 or later now come out shifted by the host's UTC offset. On a UTC host nothing changes.Known limitations
DatabaseSessionServicekeeps reading legacy v0 databases without migration throughStorageEvent.to_event(), which treats naive values as UTC. This PR does not touchv0.py, so pre-2.7.0 rows read through that runtime path still come back shifted. This PR makes the two migration paths agree with each other and with pre-2.7.0 rows; it does not make every v0 reader correct. Changing the runtime read path is a separate decision, and I can take it on in a separate PR if the sessions owners want it.test_v0_migrations_lose_fall_back_hour_to_first_occurrencepins this behavior.time.tzsetexists, so they are skipped on Windows. If tzdata is missing and the requested zone resolves to UTC, they are skipped with an explicit message instead of failing with a confusing mismatch.Verified by others in #7300:
tzset), the sessions suite 501 passed, and the IST reproduction goes from +19800s on main to both paths agreeing (comment). After rebasing onto e4c0d94, including fix: report the events the SQLite migration actually migrated #7299: migration tests 61 passed with 3 skipped and the sessions suite green (comment).test_migration.py44 passed with the timezone tests running, the three new tests fail on the base, and the sessions suite shows no new failures compared with the base in the same environment (which was missing some optional test dependencies) (comment).Not run locally: tox and the full unit test suite, because my machine does not have enough memory for them. The unit test matrix in CI runs the same suite on Python 3.10 to 3.14.
No adk-docs change is needed: the session migration page does not describe how timestamps are interpreted.