chore(http-client): replace nested backoff decorators with explicit retry loop#982
Open
devin-ai-integration[bot] wants to merge 4 commits intomainfrom
Open
chore(http-client): replace nested backoff decorators with explicit retry loop#982devin-ai-integration[bot] wants to merge 4 commits intomainfrom
devin-ai-integration[bot] wants to merge 4 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 27s ⏱️ -16s Results for commit 9fa8631. ± 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 0s ⏱️ -1s Results for commit 9fa8631. ± 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:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors the CDK HttpClient retry/backoff mechanism by replacing nested backoff.on_exception decorators with an explicit retry loop and a unified retry exception, simplifying control flow in the HTTP retry path.
Changes:
- Rewrote
HttpClient._send_with_retryto use an explicit retry loop driven by a singleRetryRequestException. - Added
RetryRequestExceptionto unify retry signaling (including optional backoff and “retry endlessly” semantics). - Removed
HttpClient-specific backoff handler decorators fromrate_limiting.pyand updated HTTP client unit tests accordingly.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
airbyte_cdk/sources/streams/http/http_client.py |
Replaces decorator-based retries with an explicit loop and raises RetryRequestException for retryable actions. |
airbyte_cdk/sources/streams/http/exceptions.py |
Introduces RetryRequestException as the unified retry signal (with backoff metadata). |
airbyte_cdk/sources/streams/http/rate_limiting.py |
Removes the deleted HttpClient-specific backoff handlers; keeps the shared default_backoff_handler. |
unit_tests/sources/streams/http/test_http_client.py |
Updates assertions and rate-limit retry tests to align with the new unified retry exception and loop behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+284
to
+288
| # Rate-limited requests retry indefinitely unless exit_on_rate_limit was set. | ||
| # All other retryable errors are bounded by max_tries / max_time. | ||
| budget_exhausted = not exc.retry_endlessly and ( | ||
| attempt >= max_tries or elapsed >= max_time | ||
| ) |
Contributor
Author
There was a problem hiding this comment.
Agreed — the PR description was stale after sophiecuiy's follow-up commits (3191bd1, 241f621) which wired retry_endlessly back into the budget check. The code is now correct: rate-limited requests bypass the budget when exit_on_rate_limit=False and no custom backoff strategy matched. Updated the PR description to reflect this.
Also fixed the ruff format issue on this line in the same commit (9fa8631).
Comment on lines
808
to
835
| "response_code, expected_failure_type, error_message, exception_class", | ||
| [ | ||
| (400, FailureType.system_error, "test error message", UserDefinedBackoffException), | ||
| (401, FailureType.config_error, "test error message", UserDefinedBackoffException), | ||
| (403, FailureType.transient_error, "test error message", UserDefinedBackoffException), | ||
| (400, FailureType.system_error, "test error message", DefaultBackoffException), | ||
| (401, FailureType.config_error, "test error message", DefaultBackoffException), | ||
| (403, FailureType.transient_error, "test error message", DefaultBackoffException), | ||
| (400, FailureType.system_error, "test error message", RateLimitBackoffException), | ||
| (401, FailureType.config_error, "test error message", RateLimitBackoffException), | ||
| (403, FailureType.transient_error, "test error message", RateLimitBackoffException), | ||
| (400, FailureType.system_error, "test error message", "user_defined"), | ||
| (401, FailureType.config_error, "test error message", "user_defined"), | ||
| (403, FailureType.transient_error, "test error message", "user_defined"), | ||
| (400, FailureType.system_error, "test error message", "default"), | ||
| (401, FailureType.config_error, "test error message", "default"), | ||
| (403, FailureType.transient_error, "test error message", "default"), | ||
| (400, FailureType.system_error, "test error message", "rate_limited"), | ||
| (401, FailureType.config_error, "test error message", "rate_limited"), | ||
| (403, FailureType.transient_error, "test error message", "rate_limited"), | ||
| ], | ||
| ) | ||
| def test_send_with_retry_raises_airbyte_traced_exception_with_failure_type( | ||
| response_code, expected_failure_type, error_message, exception_class, requests_mock | ||
| ): | ||
| if exception_class == UserDefinedBackoffException: | ||
| if exception_class == "user_defined": | ||
|
|
||
| class CustomBackoffStrategy: | ||
| def backoff_time(self, response_or_exception, attempt_count): | ||
| return 0.1 | ||
|
|
||
| backoff_strategy = CustomBackoffStrategy() | ||
| response_action = ResponseAction.RETRY | ||
| elif exception_class == RateLimitBackoffException: | ||
| elif exception_class == "rate_limited": | ||
| backoff_strategy = None | ||
| response_action = ResponseAction.RATE_LIMITED | ||
| else: |
Contributor
Author
There was a problem hiding this comment.
…lass to retry_scenario Co-Authored-By: bot_apk <apk@cognition.ai>
devin-ai-integration
bot
changed the title
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
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; simplified_handle_error_resolutionfrom three exception branches to oneRetryRequestExceptionraise. Rate-limited requests now retry indefinitely whenexit_on_rate_limit=Falseand no custom backoff strategy matched (viaretry_endlessly), while all other retries remain bounded bymax_tries/max_time.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. Split the oldtest_backoff_strategy_endlessparametrized test intotest_backoff_strategy_rate_limited_with_exit_on_rate_limitandtest_backoff_strategy_rate_limited_retries_endlessly(the latter verifies retries pastmax_triesuntil success).Backward compatibility:
BaseBackoffException,DefaultBackoffException,UserDefinedBackoffException, andRateLimitBackoffExceptionremain inexceptions.pyand importable fromairbyte_cdk/__init__.py.abstract_oauth.py(which independently usesDefaultBackoffException) is untouched.Updates since last revision
backoff_secondsternary expression (split across multiple lines).exception_classtest parameter toretry_scenario(it's now a string selector, not an exception class).Review & Testing Checklist for Human
retry_endlesslybehavioral change: Whenexit_on_rate_limit=Falseand no custom backoff strategy matches, rate-limited requests now truly retry indefinitely (bypassingmax_tries/max_time). The old code passedmax_triestorate_limit_default_backoff_handler, so rate-limited retries were actually bounded. Verify this behavioral change is intentional and acceptable for production connectors.user_defined_backoff_time is Noneguard onretry_endlessly: If a backoff strategy provides abackoff_timefor aRATE_LIMITEDresponse,retry_endlesslyisFalseand retries are bounded. Verify this mutual exclusion matches the desired semantics.time.monotonic()formax_timebudget: The old code used thebackofflibrary's internal elapsed tracking (based ontime.time()). The new code usestime.monotonic(). This is more correct butfreezegun-based test fixtures don't mocktime.monotonic()— themax_timepath relies onmock_sleeppatchingtime.sleeponly. Verifymax_timeenforcement works in real (non-mocked) conditions.DefaultBackoffException/UserDefinedBackoffExceptionfromHttpClient._send()will now seeRetryRequestExceptioninstead. Verify no external callers depend on catching these specific types from_send.Recommended test plan: Run
pytest unit_tests/sources/streams/http/test_http_client.py -v(58 tests) andpytest unit_tests/utils/test_rate_limiting.py -v. Additionally, consider a manual integration test with a connector that hits rate limits to validate real-world retry/endless-retry behavior.Notes
rate_limiting.pywere internal toHttpClientand not exported from__init__.py.2 ** (attempt - 1)yielding[1, 2, 4, 8, …]seconds, matching the oldbackoff.expobase-2 series.Link to Devin session: https://app.devin.ai/sessions/993ea9b604cb4ac2b88a360a614c3894