fix(sessions): migrate v0 event timestamps as local time, not UTC#6396
Open
anxkhn wants to merge 1 commit into
Open
fix(sessions): migrate v0 event timestamps as local time, not UTC#6396anxkhn wants to merge 1 commit into
anxkhn wants to merge 1 commit into
Conversation
The pickle-to-JSON migration read each v0 event's naive `timestamp` column and forced it to UTC via `timestamp.replace(tzinfo=timezone.utc).timestamp()`. But the v0 schema persisted that column as a naive datetime in local time (`StorageEvent.from_event` uses `datetime.fromtimestamp(event.timestamp)`) and read it back as local (`StorageEvent.to_event` uses a naive `.timestamp()`). Reinterpreting the naive value as UTC shifted every migrated event timestamp by the host's UTC offset on any non-UTC host (for example +19800s under Asia/Kolkata), corrupting event timestamps and ordering in the destination database. Interpret the naive value as local time to match how v0 both wrote and read it. `datetime.timestamp()` treats naive datetimes as local and still honors tzinfo when a driver returns a timezone-aware value. Add a regression test that pins a fixed non-UTC zone and asserts the migrated epoch round-trips exactly. Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com>
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 pickle-to-JSON session migration tool
(
src/google/adk/sessions/migration/migrate_from_sqlalchemy_pickle.py)corrupts every migrated event's timestamp on any host whose local timezone is
not UTC.
When rebuilding each
Event,_row_to_eventreads the old row's naivetimestampand forces it to UTC:But the v0 schema persisted that column as a naive datetime in local time,
and read it back the same way:
sessions/schemas/v0.pyStorageEvent.from_event:timestamp=datetime.fromtimestamp(event.timestamp)(naive, local)sessions/schemas/v0.pyStorageEvent.to_event:timestamp=self.timestamp.timestamp()(naive.timestamp()= local)sessions/schemas/shared.pyPreciseTimestamp:impl = DateTime(nottimezone=True), so drivers hand back naive datetimes.Relabeling that naive local wall-clock value as UTC shifts every migrated
timestamp by the host's UTC offset. On a machine in Asia/Kolkata (+5:30) a
stored epoch of
1000000.0migrates to1019800.0(off by 19800 seconds).Because the shift is applied to every event, it silently corrupts event
timestamps and their ordering in the destination database.
Steps to Reproduce:
Run the migration on a host configured to a non-UTC timezone (or simulate it):
Expected Behavior:
The migrated event's timestamp equals the original epoch (
1000000.0),regardless of the host's timezone.
Observed Behavior:
On a non-UTC host the migrated timestamp is shifted by the host's UTC offset
(for example
1019800.0under Asia/Kolkata), corrupting event timestamps andordering.
Solution:
Interpret the naive value as local time to match how v0 both wrote and read it:
datetime.timestamp()treats a naive datetime as local time (matchingStorageEvent.to_event), and still honorstzinfoif a driver ever returns atimezone-aware value (the previous
.replace(tzinfo=timezone.utc)would haveoverwritten a real offset). The destination write remains UTC
(
datetime.fromtimestamp(event_obj.timestamp, timezone.utc)), which is correctonce the epoch itself is right; that line and the
timezoneimport areunchanged.
Environment Details:
main)Regression: Present since the migration tool was introduced; independent of
ADK version.
Testing Plan
Unit Tests:
Added
test_migrate_from_sqlalchemy_pickle_reads_naive_timestamp_as_localintests/unittests/sessions/migration/test_migration.py. It pins a fixed non-UTCzone (
Asia/Kolkata) viamonkeypatch+time.tzset(), feeds a naive-local v0timestamp (built with
datetime.fromtimestamp, exactly as v0 persisted it)through
_row_to_event, and asserts the migrated epoch round-trips exactly. Itfails on the old code (
1019800.0 != 1000000.0) and passes with the fix. Thezone is restored in a
finallyblock.pytestresults:Manual End-to-End (E2E) Tests:
The runnable snippet under "Steps to Reproduce" is the manual repro: it prints
1019800.0before the fix and1000000.0after, on a non-UTC host.Checklist