Repository navigation
Conversation
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
2. No issue exists; description of the change:
Problem:
RedisSessionService.append_eventwrites the session key unconditionally, so appending an event to a session that was never created (or was deleted/expired) silently creates it instead of raisingSessionNotFoundErrorlikeInMemorySessionService,SqliteSessionServiceandDatabaseSessionServicedo. 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.pymarkstest_append_event_to_unknown_session_raises_session_not_foundasxfail(strict=True)for theredisbackend, noting that whoever fixes the backend must delete the entry.Repro on
main:Solution:
Check that the session key exists before any write (skipped for partial events, which return early as in the other services) and raise
SessionNotFoundErrorotherwise. Checking first means a rejected append leaves no app/user state behind. The existence check usesEXISTSrather thanGET, 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).FakeRedisAsyncgains anexists()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
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).