feat: add TypesenseSessionService#140
Conversation
|
@gemini-cli /review |
|
🤖 Hi @DeanChensj, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for the detailed contribution! The implementation of TypesenseSessionService is clean, well-tested, and follows the project's patterns.
I've left a few comments regarding:
- Memory management: The state locks dictionaries grow indefinitely.
- Scalability:
list_sessionsmight struggle with a large number of users. - Distributed concurrency: Local locks won't work in multi-instance deployments (as noted in your docstring).
Overall, this is a great addition to the community plugins. Please consider addressing the memory leak and scalability points if this is intended for high-traffic use cases.
| collection_prefix: str = 'adk', | ||
| connection_timeout_seconds: int = 5, | ||
| ): | ||
| try: |
There was a problem hiding this comment.
These dictionaries grow indefinitely as new apps or users are encountered, which could lead to a memory leak in long-running services with many unique users. Consider using an LRU cache or a similar mechanism to limit the number of active locks.
There was a problem hiding this comment.
A naive LRU here would risk evicting a lock that's currently held, breaking serialization for that key. A correct implementation would require reference counting or evict-only-when-idle semantics. I've opted to leave the dicts unbounded and documented the limitation in the class docstring. I'm happy to revisit this if a bounded structure is preferred.
| return None | ||
|
|
||
| async def _search_all( | ||
| self, collection: str, filter_by: str, sort_by: Optional[str] = None |
There was a problem hiding this comment.
As noted in the docstring, these local locks won't synchronize state updates across multiple instances/processes. While Typesense doesn't have native transactions, using a distributed lock (e.g., via Redis) would be a more robust solution for multi-process deployments. For this community contribution, the current approach is acceptable but the limitation is worth highlighting.
| 'filter_by': filter_by, | ||
| 'sort_by': 'timestamp:desc', | ||
| 'per_page': min(remaining, 250), | ||
| 'page': page, |
There was a problem hiding this comment.
Fetching all user states for an app when user_id is None could be extremely slow and memory-intensive if the app has a large number of users. Since list_sessions already fetches session documents, perhaps user states could be fetched on-demand or only for the users returned in the session list (e.g., using a filter with multiple IDs if the number of sessions is small).
There was a problem hiding this comment.
Fixed. list_sessions now collects the unique user_ids from the returned session documents and fetches user state only for those rather than retrieving all user states for the app.
| * App and user state updates are serialized per-key within a single process. | ||
| Multi-process deployments sharing the same Typesense instance may still | ||
| lose concurrent state updates because Typesense has no native transactions. | ||
| """ |
There was a problem hiding this comment.
Nice job handling the optional typesense dependency with lazy imports and clear error messages. This keeps the core package lightweight for users who don't need this specific service.
|
@DeanChensj addressed the comments in my recent commits.
Lock dictionary growth I held off on a fix because a naive LRU would risk evicting held locks and breaking serialization. A ref-counted evict-when-idle approach would work correctly. Docstring now documents the limitation. Happy to add the ref-counted version in this PR if you'd prefer otherwise can be a follow-up. Distributed locking agree this is a real limitation for multi-process deployments, but a proper fix needs a distributed lock backend (Redis etc.) which feels out of scope here. Noted in the class docstring for now. Open to revisiting in a follow-up. |
2 participants
This change adds
TypesenseSessionService, implementing all five BaseSessionService methods —create_session,get_session,list_sessions,delete_session, andappend_event— backed by Typesense collections. Four collections are managed automatically on first use({prefix}_sessions, {prefix}_events, {prefix}_app_states, {prefix}_user_states), following the same collection-per-scope pattern used by DatabaseSessionService for app, user, and session state. The typesense package is added as an optional dependency with a lazy import in init.py to avoid breaking users who don't have typesense installed.App and user state updates are serialized per-key via asyncio locks within a single process. Note that app_name, user_id, and session_id must not contain || (used as an internal document-ID separator).