-
Notifications
You must be signed in to change notification settings - Fork 2.5k
fix(sessions): fix httpx client closure during event pagination #3756
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
fix(sessions): fix httpx client closure during event pagination #3756
Conversation
Move event iteration inside the api_client context manager in VertexAiSessionService.get_session() to prevent client closure during multi-page event fetching.
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Summary of ChangesHello @AlexisMarasigan, 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 addresses a critical bug where the Highlights
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
|
|
Response from ADK Triaging Agent Hello @AlexisMarasigan, thank you for creating this PR! Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). In addition, could you please fill out the "Testing Plan" section in your PR? This information will help reviewers to review your PR more efficiently. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request addresses a bug in VertexAiSessionService.get_session where the HTTP client was being closed prematurely during event pagination. The fix correctly moves the event iteration logic into the api_client context manager, ensuring the client remains open while iterating through paginated events. The change is correct and effectively resolves the issue. I have added one comment suggesting the addition of a regression test to prevent this issue from recurring, as the current tests do not appear to cover this specific pagination failure scenario.
|
Response from ADK Triaging Agent Hello @AlexisMarasigan, thank you for updating the PR! Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). This information will help reviewers to review your PR more efficiently. Thanks! |
Add test_get_session_pagination_keeps_client_open to verify that get_session() keeps the API client open while iterating through paginated events. The test uses a mock client that tracks context state and raises RuntimeError if iteration occurs outside the async with block, matching real httpx behavior.
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request effectively resolves a critical bug where the httpx client was being closed prematurely during event pagination in VertexAiSessionService.get_session(). The fix, which involves moving the event iteration logic inside the api_client context manager, is correct and well-targeted. The addition of the test_get_session_pagination_keeps_client_open regression test is particularly commendable, as it accurately simulates the failure condition using the MockAsyncClientWithPagination class, ensuring the fix is robust and preventing future regressions. The changes are of high quality and I have no further suggestions.
fix(sessions): move event iteration inside api_client context in get_session
Move event iteration inside the api_client context manager in VertexAiSessionService.get_session() to prevent client closure during multi-page event fetching.
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
1. Link to an existing issue (if applicable):
2. Or, if no issue exists, describe the change:
Problem:
When a session contains more than 100 events (requiring pagination),
VertexAiSessionService.get_session()fails with:The root cause is that the
events_iteratoris consumed outside theasync with self._get_api_client() as api_client:context block. When the iterator needs to fetch page 2, 3, etc., the API client has already been closed because theasync withblock has exited.Solution:
Move the session creation, user validation, and event iteration inside the
async withblock so the API client remains open during the entire pagination process:Testing Plan
Unit Tests:
Added regression test:
test_get_session_pagination_keeps_client_openMockAsyncClientWithPaginationthat tracks whether it's inside theasync withcontextRuntimeErrorif iteration happens outside the context (matching real httpx behavior)Manual End-to-End (E2E) Tests:
get_session()successfully retrieves all events without errorBefore fix:
After fix:
Checklist
Additional context
This bug affects any production deployment where users have extended conversations. Sessions accumulating >100 events (which triggers pagination) become completely unusable as the agent cannot load the session to process new messages.
The fix is minimal and maintains backward compatibility - it only changes the scope of the
async withblock without altering any logic or return values.Affected versions: Tested on google-adk 1.19.0, but the bug exists in earlier versions as well.