Skip to content

fix(sessions): raise SessionNotFoundError when appending to an unknown Redis session - #7501

Open
Hugo-DDT wants to merge 1 commit into
google:mainfrom
Hugo-DDT:fix/redis-append-event-unknown-session
Open

Hugo-DDT wants to merge 1 commit into
google:mainfrom
Hugo-DDT:fix/redis-append-event-unknown-session

Conversation

@Hugo-DDT

Copy link
Copy Markdown

Please ensure you have read the contribution guide before creating a pull request.

Link to Issue or Description of Change

2. No issue exists; description of the change:

Problem:
RedisSessionService.append_event writes the session key unconditionally, so appending an event to a session that was never created (or was deleted/expired) silently creates it instead of raising SessionNotFoundError like InMemorySessionService, SqliteSessionService and DatabaseSessionService do. App- and user-scoped state deltas from such an event were also written before the session write. The shared session-contract suite already records this as a known divergence: tests/unittests/sessions/_conformance.py marks test_append_event_to_unknown_session_raises_session_not_found as xfail(strict=True) for the redis backend, noting that whoever fixes the backend must delete the entry.

Repro on main:

pytest tests/unittests/sessions/test_session_service.py -k unknown_session_raises --runxfail
FAILED ...::test_append_event_to_unknown_session_raises_session_not_found[redis] - Failed: DID NOT RAISE SessionNotFoundError

Solution:
Check that the session key exists before any write (skipped for partial events, which return early as in the other services) and raise SessionNotFoundError otherwise. Checking first means a rejected append leaves no app/user state behind. The existence check uses EXISTS rather than GET, so the existing concurrent-change retry test still exercises the transaction path. The divergence entry is removed, so the contract test now runs strictly for Redis.

Testing Plan

Unit Tests:

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

  • All unit tests pass locally.

  • New test_append_event_to_unknown_session_writes_nothing (Redis): raises, and nothing is stored.

  • The shared contract test now passes for redis (xfail entry removed).

  • pytest tests/unittests/sessions tests/unittests/integrations/redis: 615 passed (Python 3.11 and 3.13).

  • FakeRedisAsync gains an exists() mirroring redis-py's.

Manual End-to-End (E2E) Tests:

N/A: storage-layer change covered by the shared session contract tests; no UI or runner behavior change for existing sessions.

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

Prepared with the help of an AI coding assistant; I reviewed, ran and can explain every change. Formatting checked with the pinned pre-commit tools (ruff 0.15.17, isort 8.0.1, pyink 25.12.0).

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.

2 participants