Retry on HTTP 400 / "failedPrecondition"#2724
Conversation
|
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, 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 enhances the client's robustness by improving its error handling and retry mechanisms. It centralizes the logic for parsing error reasons from HTTP response bodies and extends the automatic retry functionality to include specific HTTP 400 errors, alongside existing HTTP 403 rate limit retries. This change aims to make the client more resilient to transient API issues without requiring manual intervention. Highlights
Changelog
Activity
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
|
![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 refactors the logic for decoding error reasons into a new helper function, _decode_reason_from_content, to improve code clarity and enable a retry mechanism for HTTP 400 errors with a failedPrecondition reason. However, the implementation introduces several security and robustness issues. The response parsing logic is fragile, potentially leading to unhandled exceptions (Denial of Service) with unexpected response structures. Furthermore, logging unsanitized data from the response body risks log injection and sensitive data exposure (PII/secrets in logs). I also have a couple of suggestions for the new helper function to improve its robustness and compatibility.
| reason = reason["reason"] | ||
| else: | ||
| reason = data[0]["error"]["errors"]["reason"] | ||
| except (UnicodeDecodeError, ValueError, KeyError): |
There was a problem hiding this comment.
The logic in the else block on line 110 (reason = data[0]["error"]["errors"]["reason"]) can raise a TypeError if data[0]["error"]["errors"] is a list, which is a common format for error details. This TypeError is not currently caught, which could lead to an unhandled exception. Please add TypeError to the list of caught exceptions to make this more robust.
| except (UnicodeDecodeError, ValueError, KeyError): | |
| except (UnicodeDecodeError, ValueError, KeyError, TypeError): |
| if "reason" in reason: | ||
| reason = reason["reason"] | ||
| else: | ||
| reason = data[0]["error"]["errors"]["reason"] |
There was a problem hiding this comment.
The _decode_reason_from_content function lacks robust error handling when parsing the response content. Specifically, it only catches UnicodeDecodeError, ValueError, and KeyError. However, the logic on line 110 (reason = data[0]["error"]["errors"]["reason"]) can raise IndexError (if data is an empty list) or TypeError (if data is not a list or if the nested structure is not as expected, e.g., data[0]["error"]["errors"] is a list instead of a dict). Since these exceptions are not caught, a malformed response from the server will cause the entire request process to crash, leading to a denial of service for the client application.
| return False | ||
| reason = _decode_reason_from_content(content) | ||
|
|
||
| LOGGER.warning('Encountered 403 Forbidden with reason "%s"', reason) |
There was a problem hiding this comment.
The _should_retry_response function logs the reason extracted from the server response without any sanitization. Since the reason is derived from the response body, which can be influenced by an attacker (e.g., via error messages that echo back user input), an attacker could include newline characters in the reason field to forge log entries or manipulate the log output.
| else: | ||
| reason = data[0]["error"]["errors"]["reason"] | ||
| except (UnicodeDecodeError, ValueError, KeyError): | ||
| LOGGER.warning("Invalid JSON content from response: %s", content) |
There was a problem hiding this comment.
In _decode_reason_from_content, the raw response content is logged when JSON parsing fails or when expected keys are missing. This response body could potentially contain sensitive information, such as PII or API keys, if the server includes them in error messages. Logging the entire response body increases the risk of sensitive data exposure in log files.
googleapiclient/http.py
Outdated
| _LEGACY_BATCH_URI = "https://www.googleapis.com/batch" | ||
|
|
||
|
|
||
| def _decode_reason_from_content(content) -> str | None: |
There was a problem hiding this comment.
The | operator for type hints was introduced in Python 3.10. To maintain compatibility with older Python versions (e.g., 3.9 and earlier) that this library may support, it is recommended to use Optional[str] from the typing module. Using a string forward reference "Optional[str]" avoids needing the import at module load time, but you will still need to add from typing import Optional (likely under a TYPE_CHECKING guard) for type checkers to work correctly.
| def _decode_reason_from_content(content) -> str | None: | |
| def _decode_reason_from_content(content) -> "Optional[str]": |
Extracts out a function to decode the contents.
|
Except for the usage of |
2 participants
This PR extracts out the function to decode the error codes. For HTTP 403, certain error codes are already retried. This adds a path for HTTP 400 where failedPrecondition is also retried.
Fixes #2723