refactor(http-client): replace nested backoff decorators with explicit retry loop#982
Draft
devin-ai-integration[bot] wants to merge 3 commits intomainfrom
Draft
refactor(http-client): replace nested backoff decorators with explicit retry loop#982devin-ai-integration[bot] wants to merge 3 commits intomainfrom
devin-ai-integration[bot] wants to merge 3 commits intomainfrom
Conversation
…t retry loop - Add RetryRequestException to exceptions.py extending BaseBackoffException - Rewrite _send_with_retry as explicit while-loop with _compute_backoff helper - Simplify _handle_error_resolution to raise single RetryRequestException - Delete 3 internal backoff handlers from rate_limiting.py (keep default_backoff_handler) - Remove unused imports from http_client.py - Update test_http_client.py to use RetryRequestException - Preserve backward compatibility: all old exception classes remain importable Co-Authored-By: bot_apk <apk@cognition.ai>
Contributor
Author
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
👋 Greetings, Airbyte Team Member!Here are some helpful tips and reminders for your convenience. 💡 Show Tips and TricksTesting This CDK VersionYou can test this version of the CDK using the following: # Run the CLI from this branch:
uvx 'git+https://github.com/airbytehq/airbyte-python-cdk.git@devin/1775518713-refactor-http-client-retry#egg=airbyte-python-cdk[dev]' --help
# Update a connector to use the CDK from this branch ref:
cd airbyte-integrations/connectors/source-example
poe use-cdk-branch devin/1775518713-refactor-http-client-retryPR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
|
PyTest Results (Fast)4 012 tests ±0 4 001 ✅ ±0 7m 29s ⏱️ -14s Results for commit 241f621. ± Comparison against base commit 4aaafcf. This pull request removes 15 and adds 15 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
PyTest Results (Full)4 015 tests ±0 4 003 ✅ ±0 11m 3s ⏱️ +4s Results for commit 241f621. ± Comparison against base commit 4aaafcf. This pull request removes 15 and adds 15 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
- Fix exponential backoff sequence: 2^(attempt-1) to match old backoff.expo (1, 2, 4, 8s) instead of 2^attempt (2, 4, 8, 16s) - Wire retry_endlessly into the retry loop so rate-limited requests without exit_on_rate_limit genuinely retry past the budget - Guard retry_endlessly with user_defined_backoff_time is None to preserve the old mutually-exclusive branching (custom backoff always bounded, endless only for rate limits without a strategy) - Add _MAX_BACKOFF_SECONDS (300s) cap on exponential backoff - Split test_backoff_strategy_endless into two tests that verify bounded vs endless behavior independently Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Drop _compute_backoff helper and inline the logic directly in the retry loop. Remove the 300s backoff cap to preserve the original unbounded exponential behavior. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
👋 Greetings, Airbyte Team Member!Here are some helpful tips and reminders for your convenience. 💡 Show Tips and TricksTesting This CDK VersionYou can test this version of the CDK using the following: # Run the CLI from this branch:
uvx 'git+https://github.com/airbytehq/airbyte-python-cdk.git@devin/1775518713-refactor-http-client-retry#egg=airbyte-python-cdk[dev]' --help
# Update a connector to use the CDK from this branch ref:
cd airbyte-integrations/connectors/source-example
poe use-cdk-branch devin/1775518713-refactor-http-client-retryPR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replaces the three-layer
backoff.on_exceptiondecorator chain inHttpClient._send_with_retrywith a single explicitwhile Trueretry loop. This eliminates three control-flow-only exception types (UserDefinedBackoffException,RateLimitBackoffException,DefaultBackoffException) from the retry path, replacing them with a singleRetryRequestException.What changed:
exceptions.py: AddedRetryRequestException(BaseBackoffException)withbackoff_timeandretry_endlesslyfields.http_client.py: Rewrote_send_with_retryas an explicit loop; added_compute_backoffhelper; simplified_handle_error_resolutionfrom three exception branches to oneRetryRequestExceptionraise.rate_limiting.py: Deletedhttp_client_default_backoff_handler,user_defined_backoff_handler, andrate_limit_default_backoff_handler. Keptdefault_backoff_handler(exported fromairbyte_cdk/__init__.py).test_http_client.py: Updated all exception assertions to useRetryRequestException.Backward compatibility:
BaseBackoffException,DefaultBackoffException,UserDefinedBackoffException, andRateLimitBackoffExceptionremain inexceptions.pyand importable fromairbyte_cdk/__init__.py.abstract_oauth.py(which independently usesDefaultBackoffException) is untouched.Review & Testing Checklist for Human
_compute_backoffreturns2 ** attemptwhereattemptstarts at 1, yielding[2, 4, 8, …]seconds. The oldbackoff.expowith base 2 produced[1, 2, 4, …]. Verify this ~1s first-retry increase is acceptable for production connectors.retry_endlesslyfield is set but not used in the loop: The budget is always enforced (max_tries/max_time). This matches the old behavior (the oldrate_limit_default_backoff_handleralso receivedmax_tries), but the field on the exception is effectively dead code. Decide if it should be removed or kept for future use.DefaultBackoffException/UserDefinedBackoffExceptionfromHttpClient._send()will no longer catch the retry signal. Verify no external callers depend on this.time.monotonic()formax_timebudget: The old code usedbackofflibrary's internal elapsed tracking (based ontime.time()). The new code usestime.monotonic(). Confirm this is appropriate and thatfreezegun-based test fixtures don't mask issues with themax_timepath.Recommended test plan: Run the full HTTP client test suite (
pytest unit_tests/sources/streams/http/test_http_client.py -v) and rate limiting tests (pytest unit_tests/utils/test_rate_limiting.py -v). All 62 tests pass locally. Additionally, consider a manual integration test with a connector that hits rate limits to validate real-world retry behavior.Notes
rate_limiting.pywere internal toHttpClientand not exported from__init__.py.Link to Devin session: https://app.devin.ai/sessions/993ea9b604cb4ac2b88a360a614c3894