Fix timezone-aware datetime roundtrip on SQLite - #1462
Open
edwinyyyu wants to merge 2 commits into
Open
Conversation
SQLite's DateTime(timezone=True) does not store timezone information: it discards tzinfo and persists the datetime's wall-clock fields verbatim, reading them back naive. Stores that wrote a timezone-aware datetime without first normalizing to UTC therefore corrupted the value on read by the original UTC offset. PostgreSQL is unaffected because timestamptz stores a true instant, so this fix is a no-op there. Normalize to UTC (ensure_tz_aware(...).astimezone(UTC)) before persisting in every affected store: - SQLAlchemySegmentStore.timestamp (the original offset is already stored separately and reapplied on read; only the write was missing the UTC normalization) - ClusterStateStorageSqlAlchemy last_ts and pending created_at - SqlAlchemyEpisodeStore created_at, plus its start_time/end_time filter bounds so range comparisons stay consistent with the stored UTC instant Add SQLite regression tests covering non-UTC timezones for each store. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
edwinyyyu
force-pushed
the
sqlalchemy_timezone_roundtrip
branch
from
June 19, 2026 21:51
b98ebeb to
68222ce
Compare
Contributor
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. If you are still working on this, please push a commit or leave a comment. Reviewers: please respond, or add the |
edwinyyyu
added
the
keep-open
Contributor
Author
|
Pushed a follow-up commit: the comparison side needed the same normalization as the write side. Storing normalizes to UTC because SQLite's
Test asserts that one instant spelled in three different zones selects the same rows, parametrized over both backends. |
The write path normalizes a timestamp to UTC before storing it, because
SQLite's DateTime(timezone=True) does not persist tzinfo. The comparison
path did not do the same: an aware value bound as-is is rendered with its
own offset and compared against the stored text lexically, so the wall
clock decides. `timestamp <= date('2024-01-01T08:00:30+08:00')` excluded a
row stored at 2024-01-01T00:00:30Z, which is thirty seconds EARLIER than
the bound.
Normalize aware datetimes to UTC in the column comparison path, matching
what the write path and the typed-JSON comparison path already do.
PostgreSQL compares TIMESTAMPTZ by instant and is unaffected, so this
makes the two backends agree rather than changing Postgres.
Test asserts the same instant spelled in three zones selects the same
rows, and runs against both backends.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
edwinyyyu
force-pushed
the
sqlalchemy_timezone_roundtrip
branch
from
August 10, 2026 20:33
cab92b9 to
2d1529d
Compare
edwinyyyu
added a commit
to edwinyyyu/MemMachine
that referenced
this pull request
Aug 11, 2026
Expansion needs the segments AROUND an address, but the only method that could reach them spliced the seed into its result. That forced a choice with no good answer once a filter was in play: return the seed even though it failed the filter, or drop the whole neighbourhood because its centre did. Both are wrong, and the confusion belongs to the caller's rendering, not to the store. get_neighbor_segments and get_neighbor_events return the surroundings only. The caller already holds the seed — it named it — so it can render it, mark it, or omit it as its own display concern. Neither method can ever return a seed that failed the filter, because neither returns the seed at all. Both reuse the existing lateral/loop split rather than adding a query path, so the plan is the one get_segment_contexts already had. Measured on both backends they are not meaningfully slower. Two correctness fixes ride along, both found while testing the above: Column-encoded datetime comparisons normalize to UTC before binding. SQLite has no datetime type and compares the stored ISO string, so a tz-aware bound and a stored naive-UTC value compared as text — the same bug as upstream MemMachine#1462. is_gapped compared only segment.index, but chunks of one event share an index and differ by offset, so every within-event gap read as contiguous. It now compares (index, offset) as the adjacency it means. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.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
1 participant
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.
Problem
SQLAlchemySegmentStore(and a couple of other SQLAlchemy-backed stores) can corrupt timezone-aware timestamps when running on SQLite.SQLite's
DateTime(timezone=True)does not store timezone information. SQLAlchemy's SQLite dialect discardstzinfoand persists the datetime's wall-clock fields verbatim, reading them back naive. So when a store wrote a timezone-aware datetime without first normalizing it to UTC, the stored wall-clock was the local time of the original zone. On read the value was treated as UTC and then shifted to the recorded offset — corrupting the instant by the original UTC offset.Concretely, a segment timestamp of
2024-01-01 13:30:45-08:00came back as2024-01-01 05:30:45-08:00.PostgreSQL is unaffected —
timestamptzstores a true instant — so this change is a no-op there.Proof
Fix
Normalize to UTC (
ensure_tz_aware(...).astimezone(UTC)) before persisting wherever an app-supplied timezone-aware datetime is written to aDateTime(timezone=True)column:SQLAlchemySegmentStore.timestamp— the primary report. The original offset is already stored separately (timestamp_timezone_offset) and reapplied on read; only the write was missing UTC normalization.ClusterStateStorageSqlAlchemy—last_tsand pendingcreated_at.SqlAlchemyEpisodeStore—created_at, plus thestart_time/end_timefilter bounds so range comparisons stay consistent with the stored UTC instant on SQLite.Server-generated columns (
server_default=func.now()) are always UTC and roundtrip fine, so they are left unchanged. No schema changes; no migration (existing SQLite rows are not rewritten — the read path already assumes UTC, which new writes now honor).Tests
Added SQLite regression tests covering non-UTC timezones (
-08:00and+05:30) for the segment store, cluster store, and episode store. Each was confirmed to fail before the fix and pass after. Existing tests for the affected modules continue to pass (952 passed locally, non-integration).🤖 Generated with Claude Code