- Notifications
You must be signed in to change notification settings - Fork96
fix: preserve exception cause#879
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 ourterms of service andprivacy 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: preserve exception cause#879
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 thisfailed 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@fernandocorreia-galileo, 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 retry mechanism would inadvertently clear the "cause" attribute of exceptions, breaking explicit exception chaining. By ensuring that the original cause is preserved, the change significantly improves the debuggability and traceability of errors that occur within retryable operations, providing clearer context for why an operation ultimately failed. Highlights
🧠New Feature in Public Preview: You can now enableMemory to helpGemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style.Clickhere to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on ourdocumentation 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 uphere. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with theGemini Code Assist IDE Extension. Footnotes
|
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 correctly fixes an issue where the exception cause (__cause__) was not being preserved in the retry logic, leading to broken exception chains. The change ingoogle/api_core/retry/retry_base.py ensures that the original cause is carried over when an exception is re-raised. The newly added asynchronous tests intests/asyncio/retry/test_exception_chain_preservation.py effectively validate this fix for async scenarios.
My review includes a suggestion to simplify the implementation for better readability and a recommendation to add synchronous tests to ensure full coverage of the fix, as the change impacts both sync and async retry mechanisms.
Uh oh!
There was an error while loading.Please reload this page.
| @@ -0,0 +1,155 @@ | |||
| # Copyright 2025 Google LLC | |||
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.
These tests are great for verifying the fix for async retries. Since the underlying change inretry_base.py also affects synchronous retries, please consider adding a parallel set of tests for the synchronous case to ensure complete coverage.
f2f0500 to9434a4bCompare
Uh oh!
There was an error while loading.Please reload this page.
Fixes#880
Problem:
When the retry mechanism re-raises non-retryable exceptions, it explicitly clears the exception chain by using raise final_exc from None. This breaks explicit exception chaining created with raise ... from ..., making the original cause (cause) inaccessible for debugging.
Fix:
Modified
_default_exception_factory()inretry_base.pyto preserve the__cause__attribute when re-raising non-retryable exceptions. Instead of returning(exc_list[-1], None), we now return(exc_list[-1], getattr(exc_list[-1], '__cause__', None)).This ensures that exception chains are maintained through the retry mechanism, preserving debugging information and meeting developer expectations for exception handling.
Testing:
test_exception_chain_preservation.pyThank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #<issue_number_goes_here> 🦕