remove SessionManager and update TelegramService to handle session management directly#19
Conversation
…to handle session management directly
Summary of ChangesHello @MarvelNwachukwu, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the Telegram bot's session management by eliminating a custom Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This PR refactors session management by removing the custom SessionManager and using ADK features directly in TelegramService. This is a great simplification. The implementation is solid, but I found a potential race condition in the /new command that could cause data loss when saving sessions. My review includes a fix for this.
src/services/TelegramService.ts
Outdated
| const freshSession = await sessionService.getSession( | ||
| currentSession.appName, | ||
| currentSession.userId, | ||
| currentSession.id, | ||
| ); | ||
| if (freshSession) { | ||
| await memoryService.addSessionToMemory(freshSession); | ||
| } |
There was a problem hiding this comment.
Fetching a 'fresh' session from sessionService is redundant and risks a race condition. The runner holds the most up-to-date session in currentSession. Re-fetching from the persistence layer could retrieve a stale session if recent events aren't persisted yet, leading to data loss when saving to memory. It's safer to use currentSession directly.
await memoryService.addSessionToMemory(currentSession);… directly saving current session to memory
|
/gemini review |
There was a problem hiding this comment.
Code Review
The pull request successfully refactors the session management by removing the custom SessionManager and leveraging the ADK's built-in session lifecycle. This simplifies the codebase and aligns it better with the ADK framework. However, the current implementation introduces a shared session state across all users. While this is a major issue for privacy and correctness in multi-user or multi-group scenarios, it might be acceptable if the application is designed for single-user or paired-user contexts, as per repository guidelines (Rule 2).
| currentSession.appName, | ||
| currentSession.userId, | ||
| ); | ||
| runner.setSession(newSession); |
There was a problem hiding this comment.
Calling runner.setSession(newSession) on a shared runner instance will globally change the active session for all Telegram users. If one user triggers the /new command, it will effectively reset the conversation for every other user currently interacting with the bot. This is a significant issue if the application is intended for multi-user scenarios, where session state must be isolated per chatId. However, if this application is designed for single-user or paired-user contexts (e.g., a personal bot), this behavior might be acceptable as per repository guidelines.
References
- In applications designed for single-user or paired-user contexts (e.g., personal bots), using a hardcoded session user ID is acceptable, as the risks associated with multi-user data crossover are not present.
| currentSession.appName, | ||
| currentSession.userId, | ||
| ); | ||
| runner.setSession(newSession); |
There was a problem hiding this comment.
Similar to the /new command, using runner.setSession here affects the global state of the shared runner. This will cause a session reset for all users simultaneously if any single user runs the /reset command. This is a significant issue if the application is intended for multi-user scenarios, where session state must be isolated per chatId. However, if this application is designed for single-user or paired-user contexts (e.g., a personal bot), this behavior might be acceptable as per repository guidelines.
References
- In applications designed for single-user or paired-user contexts (e.g., personal bots), using a hardcoded session user ID is acceptable, as the risks associated with multi-user data crossover are not present.
| // Get response from agent | ||
| // Get response from agent (ADK tracks events in session automatically) | ||
| try { | ||
| const response = await runner.ask(messageText); |
There was a problem hiding this comment.
The removal of SessionManager has eliminated the isolation of conversations between different Telegram chats. Since runner.ask() uses the session currently set on the runner, and there is only one runner instance for the entire service, all users and groups will now share the same conversation history. This is a regression from the previous implementation which used chatId to isolate sessions. This is a significant issue for privacy and correctness if the application is intended for multi-user or multi-group scenarios. To fix this while using ADK's session management, you should ensure that the runner uses a specific sessionId (e.g., the chatId) for each request, likely by using runner.runAsync(messageText, { sessionId: chatId }) and extracting the response, rather than the simplified ask() method which relies on a single global session context. However, if this application is designed for single-user or paired-user contexts (e.g., a personal bot), this shared state might be acceptable as per repository guidelines.
References
- In applications designed for single-user or paired-user contexts (e.g., personal bots), using a hardcoded session user ID is acceptable, as the risks associated with multi-user data crossover are not present.
No description provided.