Skip to content

fix(sessions): stop DatabaseSessionService.update_time onupdate from masking explicit writes - #7277

Open
chelsealong wants to merge 1 commit into
google:mainfrom
chelsealong:fix-database-session-stale-update-time
Open

chelsealong wants to merge 1 commit into
google:mainfrom
chelsealong:fix-database-session-stale-update-time

Conversation

@chelsealong

Copy link
Copy Markdown
Contributor

Link to Issue or Description of Change

1. Link to an existing issue (if applicable):

Problem:

DatabaseSessionService.append_event raises StaleSessionError when there is
only one writer, whenever an event shares the same microsecond timestamp as
the previous event and carries a state change.

append_event always sets StorageSession.update_time explicitly from the
event's timestamp (database_session_service.py), and the column is also
declared with onupdate=func.now() (schemas/v1.py). When the new timestamp
equals the value already stored, SQLAlchemy sees no change to update_time
and drops it from the UPDATE statement's explicit values. The UPDATE still
runs because another column (state) changed, so onupdate fires and the
database's own clock is written instead — whole seconds on SQLite, the
transaction start time on Postgres. The in-memory revision marker, read
before commit, no longer matches what storage actually holds, so the next
append_event on that session is incorrectly rejected as stale.

Equal timestamps are common with coarser clocks (e.g. Python 3.12 on Windows,
15.6ms GetSystemTimeAsFileTime ticks), so a workflow emitting several events
per step hits this regularly.

Solution:

Drop onupdate=func.now() from StorageSession.update_time
(src/google/adk/sessions/schemas/v1.py). create_session already sets a
default for inserts, and append_event is the only code path that updates
this column on an existing row — and it always sets it explicitly. No other
column relies on this onupdate (verified StorageAppState/StorageUserState
are untouched, since they still rely on their own onupdate and are never set
explicitly).

Testing Plan

Unit Tests:

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

Added test_append_event_same_timestamp_single_writer_not_stale to
tests/unittests/sessions/test_session_service.py, reproducing the issue's
repro script against the DATABASE (SQLite) backend: two events with an
identical timestamp, the second carrying a state delta, followed by a third
event from the same session object.

$ git checkout HEAD~1 -- src/google/adk/sessions/schemas/v1.py
$ pytest tests/unittests/sessions/test_session_service.py -k test_append_event_same_timestamp_single_writer_not_stale
FAILED ... StaleSessionError: The session has been modified in storage since it was loaded.

$ git checkout HEAD -- src/google/adk/sessions/schemas/v1.py
$ pytest tests/unittests/sessions/test_session_service.py -k test_append_event_same_timestamp_single_writer_not_stale
1 passed

$ pytest tests/unittests/sessions -q
502 passed, 2 xfailed

$ pytest tests/unittests -n auto -q
16177 passed, 82 skipped, 26 xfailed, 2 xpassed

pre-commit run --files src/google/adk/sessions/schemas/v1.py tests/unittests/sessions/test_session_service.py passes (isort, pyink, ruff,
addlicense, compliance checks, codespell). The one hook failure it reports
(check-new-py-prefix on src/google/adk/memory/_sqlite_memory_service.py)
is pre-existing on main/unmodified HEAD and unrelated to this change.

Checklist

  • I have read the CONTRIBUTING.md document.
  • I have performed a self-review of my own code.
  • I have added tests that prove my fix is effective.
  • New and existing unit tests pass locally with my changes.

AI assistance disclosure

This change (analysis, fix, and test) was developed with the assistance of
Claude Code (Anthropic).

…masking explicit writes

append_event always sets StorageSession.update_time explicitly from the
event's timestamp. When that timestamp equals the value already stored,
SQLAlchemy sees no change to the column and drops it from the UPDATE, but
the UPDATE still runs for other changed columns (e.g. state), so the
onupdate=func.now() default fires and overwrites it with the database's
own clock instead. The in-memory revision marker, read before commit, no
longer matches what storage holds, and the next append_event from the
same, only, writer is rejected with a false StaleSessionError.

Drop onupdate=func.now() from StorageSession.update_time: create_session
already sets a default for inserts, and append_event is the only code
path that updates the column, always explicitly.

Fixes google#7276
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.

DatabaseSessionService raises false StaleSessionError when two events share a timestamp

2 participants