#fix:1733 mutiple /run call fails on DBsession using SQL fix #2973
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @akshatshaw, 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 resolves an issue where the DatabaseSessionService would incorrectly raise a ValueError when processing multiple concurrent messages for the same session ID. By commenting out a specific timestamp validation check, the service now behaves consistently with the InMemorySessionService, preventing unnecessary failures and improving the robustness of session handling in SQL-backed environments.
Highlights
- Error Handling: The
ValueErrorthat was previously raised whensession.last_update_timewas older thanstorage_session.update_timestamp_tzin theDatabaseSessionServicehas been commented out. This change addresses an issue where multiple/runcalls would fail due to this error, aligning its behavior with theInMemorySessionService.
Using Gemini Code Assist
The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in pull request comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
|
Response from ADK Triaging Agent Hello @akshatshaw, thank you for creating this PR! To help reviewers evaluate your contribution more effectively, could you please add a In addition, since this is a bug fix, could you please provide logs or a screenshot demonstrating that the fix works as expected? You can find more details in our contribution guidelines. Thanks! |
adk-bot
added
bot triaged
There was a problem hiding this comment.
Code Review
This pull request aims to fix an issue with concurrent /run calls failing for database-backed sessions by commenting out an optimistic locking check. While this change prevents the ValueError from being raised, it removes a crucial safeguard against race conditions, potentially leading to silent data loss or inconsistent session states when multiple requests modify the same session simultaneously. My review suggests that if this change is intentional, the commented-out code should be removed completely for clarity, and the implications of removing optimistic locking should be carefully considered and documented. An alternative, like pessimistic locking, might be more appropriate if session state integrity is paramount.
| # if storage_session.update_timestamp_tz > session.last_update_time: | ||
| # raise ValueError( | ||
| # "The last_update_time provided in the session object" | ||
| # f" {datetime.fromtimestamp(session.last_update_time):'%Y-%m-%d %H:%M:%S'} is" | ||
| # " earlier than the update_time in the storage_session" | ||
| # f" {datetime.fromtimestamp(storage_session.update_timestamp_tz):'%Y-%m-%d %H:%M:%S'}." | ||
| # " Please check if it is a stale session." | ||
| # ) |
There was a problem hiding this comment.
Commenting out this optimistic locking check resolves the immediate issue with concurrent requests failing, but it introduces a risk of race conditions and silent data loss. When two requests operate on the same session concurrently, the last one to write will overwrite any state changes made by the other. This could lead to inconsistent session state.
If this check is being removed intentionally, the commented-out code should be deleted rather than left in the source. Leaving dead code can be confusing for future maintenance. It would also be beneficial to add a comment explaining the rationale for removing the check and the accepted concurrency model.
If data consistency is critical, consider alternative concurrency control mechanisms, such as pessimistic locking (e.g., using with_for_update() on the query that fetches storage_session).
raise ValueError for the case when multiple messages are sent to the same session-id when using DatabaseSessionService/ SQL DB
this Error is not raised in the InMemorySessionService as discussed in the issue #1733 , the raise error is commented out here.
#1733