refactor: remove SessionStore, migrate to StorageProvider - #180
Conversation
fastmcp 3.4.4 passes follow_redirects=True to httpx_client_factory, but our factory also passed it explicitly, causing 'httpx.AsyncClient() got multiple values for keyword argument' on MCP server initialization. Use kwargs.setdefault() so caller-provided value takes precedence.
Remove the deprecated SessionStore Protocol and MemorySessionStore class. All session persistence now uses StorageProvider/SessionPersistence. Changes: - Delete src/agentpool/sessions/store.py (SessionStore, MemorySessionStore) - Remove SessionStore re-export from sessions/__init__.py - Remove get_session_store() from StorageConfig and SQLStorageConfig - Fix MemoryStorageProvider.delete_session() to clean up checkpoints - Add no-op MemoryStorageProvider.update_sdk_session_id() override - Migrate 17 test files from MemorySessionStore to MemoryStorageProvider - Fix async with store: pattern in test_create_child_session.py (MemoryStorageProvider.cleanup() clears data on __aexit__) Closes #170
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request removes the deprecated SessionStore and MemorySessionStore implementations, transitioning the codebase and test suites to use MemoryStorageProvider and the SessionPersistence protocol. Additionally, it refactors mcp_server.py to prevent duplicate follow_redirects arguments when initializing httpx.AsyncClient. The review feedback points out minor code quality issues, including PEP 8 spacing violations in test_session_hierarchy.py and duplicate # type: ignore comments in test_resume_concurrency.py.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
|
||
| async def test_create_with_parent_id( | ||
| self, mock_pool: MagicMock, memory_store: MemorySessionStore | ||
| self, mock_pool: MagicMock, memory_store: MemoryStorageProvider |
There was a problem hiding this comment.
There are multiple consecutive spaces before mock_pool: MagicMock. Please format this line to adhere to PEP 8 spacing guidelines.
| self, mock_pool: MagicMock, memory_store: MemoryStorageProvider | |
| self, mock_pool: MagicMock, memory_store: MemoryStorageProvider |
References
- Adhere to PEP 8 spacing guidelines by avoiding multiple consecutive spaces. (link)
There was a problem hiding this comment.
Fixed. ruff format already corrected the consecutive spaces in the latest push.
|
|
||
| async def test_create_with_invalid_parent( | ||
| self, mock_pool: MagicMock, memory_store: MemorySessionStore | ||
| self, mock_pool: MagicMock, memory_store: MemoryStorageProvider |
There was a problem hiding this comment.
There are multiple consecutive spaces before mock_pool: MagicMock. Please format this line to adhere to PEP 8 spacing guidelines.
| self, mock_pool: MagicMock, memory_store: MemoryStorageProvider | |
| self, mock_pool: MagicMock, memory_store: MemoryStorageProvider |
References
- Adhere to PEP 8 spacing guidelines by avoiding multiple consecutive spaces. (link)
| await original_save(data) | ||
|
|
||
| store.save = tracking_save # type: ignore[method-assign] | ||
| store.save_session = tracking_save # type: ignore[method-assign] # type: ignore[method-assign] |
There was a problem hiding this comment.
There is a duplicate # type: ignore[method-assign] comment on this line. Please remove the duplicate to keep the code clean.
| store.save_session = tracking_save # type: ignore[method-assign] # type: ignore[method-assign] | |
| store.save_session = tracking_save # type: ignore[method-assign] |
There was a problem hiding this comment.
Fixed. Duplicate # type: ignore[method-assign] removed in latest push.
| await original_save(data) | ||
|
|
||
| store.save = tracking_save # type: ignore[method-assign] | ||
| store.save_session = tracking_save # type: ignore[method-assign] # type: ignore[method-assign] |
There was a problem hiding this comment.
There is a duplicate # type: ignore[method-assign] comment on this line. Please remove the duplicate to keep the code clean.
| store.save_session = tracking_save # type: ignore[method-assign] # type: ignore[method-assign] | |
| store.save_session = tracking_save # type: ignore[method-assign] |
- Fix import sorting in test_acp_session_manager_child_session.py - Run ruff format on all test files (fixes consecutive spaces) - Remove deleted agentpool.sessions.store from import linter config - Remove duplicate # type: ignore[method-assign] in test_resume_concurrency.py
1 participant
Summary
Remove the deprecated
SessionStoreProtocol andMemorySessionStoreclass, migrating all session persistence toStorageProvider/SessionPersistence.Closes #170
Motivation
AgentPool had two coexisting session storage hierarchies:
SessionStore(deprecated,sessions/store.py) — 4-method Protocol with onlyMemorySessionStoreas implementationSessionPersistence(current,agentpool_storage/protocols.py) — 6-method Protocol backed byStorageProviderAll production code already used
SessionPersistence.SessionStorewas dead code — onlyMemorySessionStoresurvived as a config fallback and test fixture. This dual hierarchy caused confusion and maintenance burden.Changes
Production code
src/agentpool/sessions/store.py(SessionStore Protocol + MemorySessionStore class)SessionStorere-export fromsessions/__init__.py(keepSessionPersistence)get_session_store()fromStorageConfigandSQLStorageConfigMemoryStorageProvider.delete_session()to clean up checkpoints (was a bug —MemorySessionStoredid this butMemoryStorageProviderdidn't)MemoryStorageProvider.update_sdk_session_id()override (base class raisesNotImplementedError)Test migration (17 files)
Migrated all test files from
MemorySessionStoretoMemoryStorageProvider:from agentpool.sessions.store→from agentpool_storage.memory_provider.provider.save()→.save_session(),.load()→.load_session(),.delete()→.delete_session(),.list_sessions()→.list_session_ids()save_checkpoint3rd param nowstr(JSON-encoded),load_checkpointreturnstuple[str, str]list_sessions(parent_id=)→list_session_ids()+load_session()+ Python-side filter bySessionData.parent_idasync with store:pattern intest_create_child_session.py(MemoryStorageProvider.cleanup() clears data on exit)Verification
ruff check src/— all checks passedpytest— 192 passed, 1 skipped, 0 failures (across all 17 migrated test files)MemorySessionStoreoragentpool.sessions.storeinsrc/ortests/Breaking changes
SessionStoreProtocol andMemorySessionStoreclass removed (were already deprecated)StorageConfig.get_session_store()method removed (dead code)MemoryStorageProviderconstructor takesMemoryStorageConfig | None, not a path string