Skip to content

fix(sessions): stop the v0 sqlite migration shifting event timestamps - #7337

Draft
olaflaitinen wants to merge 1 commit into
google:mainfrom
olaflaitinen:fix/v0-migration-timestamp-local
Draft

olaflaitinen wants to merge 1 commit into
google:mainfrom
olaflaitinen:fix/v0-migration-timestamp-local

Conversation

@olaflaitinen

@olaflaitinen olaflaitinen commented Sep 29, 2026 •

Copy link
Copy Markdown

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.timestamp column differently: migrate_from_sqlalchemy_pickle reads it as local time, while migrate_from_sqlalchemy_sqlite goes through StorageEvent.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_epoch in sessions/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_event wrote 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 with to_event() and then replaces its timestamp (model_copy) with the value the shared helper computes from storage_event.timestamp. I did this to avoid duplicating the to_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 without to_event(), I am happy to change it.

Testing Plan

Unit Tests:

  • I have added or updated unit tests for my change.
  • All unit tests pass locally. (Only the migration test file was run locally; see below.)

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 with TZ set and time.tzset() called inside the test, and checks the pickle output's event_data, the sqlite output's timestamp column and event_data, and the events SqliteSessionService loads 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.py locally:

45 passed
test_migrate_from_sqlalchemy_pickle_reads_naive_timestamp_as_local PASSED
test_v0_migrations_agree_on_naive_local_event_timestamps[America/New_York] PASSED
test_v0_migrations_agree_on_naive_local_event_timestamps[Asia/Kolkata] PASSED
test_v0_migrations_lose_fall_back_hour_to_first_occurrence PASSED

The whole test_migration.py with the sources from main (e4c0d94) and the new test file: the three new tests fail and every other test passes:

FAILED test_v0_migrations_agree_on_naive_local_event_timestamps[America/New_York]
FAILED test_v0_migrations_agree_on_naive_local_event_timestamps[Asia/Kolkata]
FAILED test_v0_migrations_lose_fall_back_hour_to_first_occurrence
3 failed, 42 passed

pyink, isort and codespell are clean on the changed files.

pre-commit run --files on the four changed files: every hook that had files to check passed. addlicense is 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 ran python -m google.adk.sessions.migration.migrate_from_sqlalchemy_sqlite --source_db_path v0.db --dest_db_path migrated.db with TZ=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):

export TZ=Asia/Kolkata
rm -f v0.db migrated.db

# 1. A v0 database with one event stored as naive local time,
#    the way StorageEvent.from_event wrote it before 2.7.0.
python - <<'PY'
from datetime import datetime, timezone
from google.adk.events.event_actions import EventActions
from google.adk.sessions.schemas import v0
from sqlalchemy import create_engine
from sqlalchemy.orm import sessionmaker

engine = create_engine("sqlite:///v0.db")
v0.Base.metadata.create_all(engine)
session = sessionmaker(bind=engine)()
now = datetime.now(timezone.utc)
session.add(v0.StorageSession(app_name="app", user_id="user", id="s1", state={}, create_time=now, update_time=now))
session.add(v0.StorageEvent(id="e1", app_name="app", user_id="user", session_id="s1", invocation_id="i1", author="user", actions=EventActions(), timestamp=datetime.fromtimestamp(1750000000.0)))
session.commit()
PY

# 2. Migrate it with the sqlite migration module.
python -m google.adk.sessions.migration.migrate_from_sqlalchemy_sqlite --source_db_path v0.db --dest_db_path migrated.db

# 3. Compare the migrated timestamps with the real instant, 1750000000.0.
python -c "import json, sqlite3; ts, data = sqlite3.connect('migrated.db').execute('SELECT timestamp, event_data FROM events').fetchone(); print(ts, json.loads(data)['timestamp'], ts - 1750000000.0)"

Output:

# main (e4c0d946)
1750019800.0 1750019800.0 19800.0
# this branch
1750000000.0 1750000000.0 0.0

The columns are the sqlite timestamp column, the timestamp inside event_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 through SqliteSessionService.

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

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

  1. DatabaseSessionService keeps reading legacy v0 databases without migration through StorageEvent.to_event(), which treats naive values as UTC. This PR does not touch v0.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.
  2. Naive local values written during the repeated hour when DST ends are ambiguous and are read as the first occurrence (fold=0), so rows from the second pass come back 3600 seconds early. No helper can recover this, because the information is not stored. The skipped hour when DST starts cannot occur in real v0 data. test_v0_migrations_lose_fall_back_hour_to_first_occurrence pins this behavior.
  3. The new timezone tests only run where time.tzset exists, 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:

  • Windows, by @surajksharma07: migration tests 60 passed with 3 skipped (no 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).
  • macOS, by @feiiiiii5: test_migration.py 44 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.

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
olaflaitinen force-pushed the fix/v0-migration-timestamp-local branch from 72e0621 to d8ce246 Compare September 29, 2026 10:46
@olaflaitinen olaflaitinen changed the title fix(sessions): read v0 event timestamps as naive local time in both migrations fix(sessions): stop the v0 sqlite migration shifting event timestamps Sep 29, 2026
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.

The two migrate session paths disagree on v0 event timestamps

2 participants