fix(executor): reuse HTTP client and implement global retry#536
fix(executor): reuse HTTP client and implement global retry#536dumko2001 wants to merge 8 commits intogoogleworkspace:mainfrom
Conversation
🦋 Changeset detectedLatest commit: d4168b4 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Summary of ChangesHello, 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 significantly enhances the engine's performance and resilience by optimizing HTTP client usage and implementing robust error handling. It introduces connection pooling for paginated requests to reduce latency and integrates a global retry mechanism for rate limits and transient server errors, ensuring more reliable API interactions. 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. Footnotes
|
![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 introduces global retry logic for HTTP requests, extending the send_with_retry function in client.rs to handle 429 (Too Many Requests) and transient 5xx status codes (500, 502, 503). It also optimizes performance by reusing the HTTP client across paginated requests in executor.rs and refactors the build_http_request function to be synchronous. A critical review comment highlights a potential issue where the build_http_request call within the send_with_retry closure uses .expect(), which could lead to application panics if request building fails, suggesting a more robust error handling approach.
googleworkspace-bot
added
the
area: core
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces valuable improvements by reusing the HTTP client for better performance and adding global retry logic for increased resilience. The changes in executor.rs and the helper modules are well-implemented.
I've confirmed the critical issue in the new retry logic in src/client.rs where the final retry attempt is unreachable, which undermines the goal of increased resilience. My review includes a specific suggestion to fix this.
|
/gemini review |
|
Updated to handle fallible request builders in |
There was a problem hiding this comment.
Code Review
This pull request introduces two significant improvements: reusing the HTTP client for performance and adding a global retry mechanism for resilience. The changes are well-structured and achieve the stated goals. I've identified a critical issue with a blocking I/O call in an async context and a high-severity issue where the new retry logic doesn't handle transient network errors. My review comments provide detailed explanations and suggestions for fixing these issues, including an acknowledgment of potential TOCTOU race conditions in file path validation.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request enhances the HTTP request retry mechanism by extending the send_with_retry function in src/client.rs to handle transient 5xx errors (500, 502, 503) alongside existing 429 rate limit retries. It also optimizes HTTP client usage in src/executor.rs by reusing a single reqwest::Client instance across paginated requests instead of creating a new one for each iteration. The build_http_request function was refactored to be synchronous, returning a RequestBuilder directly, and its usage, along with other helper functions, was updated to integrate with the enhanced retry logic.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces two significant improvements to the executor: reusing the HTTP client across paginated requests for better performance, and implementing a global retry mechanism for increased resilience against transient network and server errors. The implementation is well-structured, moving I/O operations out of loops and centralizing retry logic. However, I've identified a logic issue in the new retry mechanism in src/client.rs where network errors are not handled as robustly as server errors, which could impact the intended resilience improvements.
src/client.rs
Outdated
|
|
||
| let resp = match req_result { | ||
| Ok(r) => r, | ||
| Err(e) if attempt < MAX_RETRIES - 1 && (e.is_timeout() || e.is_connect()) => { |
There was a problem hiding this comment.
The retry logic for network errors is inconsistent with server errors. A network error on the last loop iteration (attempt == MAX_RETRIES - 1) will fail immediately, while a server error in the same situation will lead to one final retry attempt after the loop. This reduces the resilience to transient network issues. To make the behavior consistent, the attempt < MAX_RETRIES - 1 check should be removed.
| Err(e) if attempt < MAX_RETRIES - 1 && (e.is_timeout() || e.is_connect()) => { | |
| Err(e) if e.is_timeout() || e.is_connect() => { |
|
Refactored |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces significant improvements to the engine's performance and reliability. Key changes include reusing the reqwest::Client across paginated requests to leverage connection pooling, and implementing a robust global retry mechanism in send_with_retry that handles transient network errors and specific 5xx/429 server responses. The logic for file uploads has also been refactored to pre-resolve file metadata, making build_http_request synchronous and avoiding I/O within the request-building closure. The changes are well-implemented and substantially enhance the application's resilience and efficiency. I have reviewed the code and found no issues.
2 participants
Description
This PR implements critical engine improvements for performance and reliability:
reqwest::Clientcreation outside loops to enable TCP/TLS pooling across paginated requests (saving ~300ms/page).send_with_retryto handle429 TOO_MANY_REQUESTS, transient server errors (500,502,503), and transient network errors (timeouts and connection resets).build_http_requestto pre-resolve file metadata asynchronously, avoiding blocking I/O in the async execution path..expect()calls in retry closures with properResultpropagation.Checklist:
AGENTS.mdguidelines (no generatedgoogle-*crates).cargo fmt --allto format the code perfectly.cargo clippy -- -D warningsand resolved all warnings.pnpx changeset) to document my changes.