feat(taps): Respect rate-limit headers in the default REST backoff#3672
Open
Nazib65 wants to merge 3 commits into
Open
feat(taps): Respect rate-limit headers in the default REST backoff#3672Nazib65 wants to merge 3 commits into
Nazib65 wants to merge 3 commits into
Conversation
`RESTStream.backoff_wait_generator` previously returned a pure `backoff.expo` sequence and never inspected the HTTP response, so a `429` carrying `Retry-After` / `X-RateLimit-Reset` was ignored in favour of guessed exponential waits. It is now a generator driven by backoff's `.send(exception)` protocol. On each retry it reads the response (guarding against errors that carry no response, e.g. connection resets and timeouts) and defers to a new overridable `get_wait_time_from_response` hook, falling back to exponential backoff when no usable header is present. `get_wait_time_from_response` reads `Retry-After` (seconds or HTTP-date, taking precedence) then `X-RateLimit-Reset` (seconds until reset, per the IETF RateLimit header fields draft). Closes meltano#2012
Contributor
Reviewer's GuideUpdates RESTStream's default backoff to respect standard rate-limit headers when present while remaining backward-compatible by falling back to exponential backoff, and adds tests covering header parsing and generator behavior. Sequence diagram for RESTStream backoff_wait_generator using rate-limit headerssequenceDiagram
participant Backoff as backoff_decorator
participant Stream as RESTStream
participant Expo as backoff_expo
participant Response as requests_Response
Backoff->>Stream: backoff_wait_generator()
activate Stream
Stream->>Expo: backoff.expo(factor=2)
Expo-->>Stream: exponential_wait
Note over Stream: Generator primed
loop on each retry
Backoff->>Stream: send(exception)
activate Stream
Stream->>Stream: getattr(exception, response, None)
alt [response is not None]
Stream->>Stream: get_wait_time_from_response(response)
alt [Retry-After or X-RateLimit-Reset]
Stream-->>Backoff: wait_seconds
else [no usable header]
Stream->>Expo: send(None)
Expo-->>Stream: exponential_wait
Stream-->>Backoff: exponential_wait
end
else [response is None]
Stream->>Expo: send(None)
Expo-->>Stream: exponential_wait
Stream-->>Backoff: exponential_wait
end
deactivate Stream
end
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Documentation build overview
3 files changed± genindex.html± classes/singer_sdk.GraphQLStream.html± classes/singer_sdk.RESTStream.html |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3672 +/- ##
==========================================
+ Coverage 94.12% 94.16% +0.03%
==========================================
Files 73 73
Lines 6198 6233 +35
Branches 762 766 +4
==========================================
+ Hits 5834 5869 +35
Misses 270 270
Partials 94 94
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Given how commonly
X-RateLimit-Resetis used as a Unix epoch timestamp in existing APIs, it might be worth makingget_wait_time_from_responsesupport both delta and epoch formats (e.g., by treating large values as epoch seconds or by allowing an overridable parsing helper) rather than only the draft’s delta semantics. - If you expect some users to override
get_wait_time_from_response, consider passing the full exception object (not just the response) into that hook so custom implementations can use error type/context for logging or non-HTTP failures, instead of only having access toresponse.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Given how commonly `X-RateLimit-Reset` is used as a Unix epoch timestamp in existing APIs, it might be worth making `get_wait_time_from_response` support both delta and epoch formats (e.g., by treating large values as epoch seconds or by allowing an overridable parsing helper) rather than only the draft’s delta semantics.
- If you expect some users to override `get_wait_time_from_response`, consider passing the full exception object (not just the response) into that hook so custom implementations can use error type/context for logging or non-HTTP failures, instead of only having access to `response`.
## Individual Comments
### Comment 1
<location path="tests/core/rest/test_failure.py" line_range="232-239" />
<code_context>
+@pytest.mark.parametrize(
+ "headers,expected",
+ [
+ ({"Retry-After": "30"}, 30.0),
+ ({"X-RateLimit-Reset": "45"}, 45.0),
+ # Retry-After takes precedence over X-RateLimit-Reset.
+ ({"Retry-After": "30", "X-RateLimit-Reset": "999"}, 30.0),
+ # Unparsable / missing headers fall back to exponential backoff (None).
+ ({"Retry-After": "not-a-number"}, None),
+ ({"X-RateLimit-Reset": "not-a-number"}, None),
+ ({}, None),
+ ],
+ ids=[
</code_context>
<issue_to_address>
**suggestion (testing):** Add coverage for zero and negative Retry-After / X-RateLimit-Reset values being clamped to 0.0
Since the code clamps values with `max(0.0, float(...))`, this parametrized test should also cover zero and negative inputs for both headers, e.g.:
- `{"Retry-After": "0"}` → `0.0`
- `{"Retry-After": "-10"}` → `0.0`
- `{"X-RateLimit-Reset": "0"}` → `0.0`
- `{"X-RateLimit-Reset": "-5"}` → `0.0`
This will verify the clamping behavior and protect against regressions where negative waits might be reintroduced.
</issue_to_address>
### Comment 2
<location path="tests/core/rest/test_failure.py" line_range="284-293" />
<code_context>
+ return RetriableAPIError("rate limited", response)
+
+
+def test_backoff_wait_generator_respects_headers_and_falls_back(
+ basic_rest_stream: RESTStream,
+):
+ """Header waits are respected; other errors fall back to exponential backoff."""
+ gen = basic_rest_stream.backoff_wait_generator()
+ gen.send(None) # Prime the generator (matches backoff's initialization).
+
+ # 429 with a rate-limit header -> respect the header's wait time.
+ assert gen.send(_retriable({"Retry-After": "30"})) == 30
+
+ # Connection-level error has no response -> exponential fallback, no crash.
+ assert gen.send(requests.exceptions.ConnectionError()) == 2
+
+ # 429 without rate-limit headers -> exponential backoff continues.
+ assert gen.send(_retriable(None)) == 4
</code_context>
<issue_to_address>
**suggestion (testing):** Add a case where backoff_wait_generator sees an unparsable rate-limit header and falls back to exponential
Please also cover the case where rate-limit headers are present but unparsable (e.g. `{"Retry-After": "not-a-number"}`), so that `get_wait_time_from_response` returns `None` and the generator falls back to the exponential sequence. For example:
```python
# 429 with an unparsable rate-limit header -> exponential fallback.
assert gen.send(_retriable({"Retry-After": "not-a-number"})) == 8
```
(or whatever the next value in the sequence is). This will exercise the integration between `get_wait_time_from_response` and the exponential fallback for this edge case.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+232
to
+239
| ({"Retry-After": "30"}, 30.0), | ||
| ({"X-RateLimit-Reset": "45"}, 45.0), | ||
| # Retry-After takes precedence over X-RateLimit-Reset. | ||
| ({"Retry-After": "30", "X-RateLimit-Reset": "999"}, 30.0), | ||
| # Unparsable / missing headers fall back to exponential backoff (None). | ||
| ({"Retry-After": "not-a-number"}, None), | ||
| ({"X-RateLimit-Reset": "not-a-number"}, None), | ||
| ({}, None), |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add coverage for zero and negative Retry-After / X-RateLimit-Reset values being clamped to 0.0
Since the code clamps values with max(0.0, float(...)), this parametrized test should also cover zero and negative inputs for both headers, e.g.:
{"Retry-After": "0"}→0.0{"Retry-After": "-10"}→0.0{"X-RateLimit-Reset": "0"}→0.0{"X-RateLimit-Reset": "-5"}→0.0
This will verify the clamping behavior and protect against regressions where negative waits might be reintroduced.
Comment on lines
+284
to
+293
| def test_backoff_wait_generator_respects_headers_and_falls_back( | ||
| basic_rest_stream: RESTStream, | ||
| ): | ||
| """Header waits are respected; other errors fall back to exponential backoff.""" | ||
| gen = basic_rest_stream.backoff_wait_generator() | ||
| gen.send(None) # Prime the generator (matches backoff's initialization). | ||
|
|
||
| # 429 with a rate-limit header -> respect the header's wait time. | ||
| assert gen.send(_retriable({"Retry-After": "30"})) == 30 | ||
|
|
Contributor
There was a problem hiding this comment.
suggestion (testing): Add a case where backoff_wait_generator sees an unparsable rate-limit header and falls back to exponential
Please also cover the case where rate-limit headers are present but unparsable (e.g. {"Retry-After": "not-a-number"}), so that get_wait_time_from_response returns None and the generator falls back to the exponential sequence. For example:
# 429 with an unparsable rate-limit header -> exponential fallback.
assert gen.send(_retriable({"Retry-After": "not-a-number"})) == 8(or whatever the next value in the sequence is). This will exercise the integration between get_wait_time_from_response and the exponential fallback for this edge case.
Address review suggestions: assert that zero and negative `Retry-After` / `X-RateLimit-Reset` values are clamped to 0.0, and that `backoff_wait_generator` falls back to exponential backoff when a response carries an unparsable rate-limit header.
Exercises the branch in `_parse_retry_after` that treats a timezone-naive HTTP date as UTC, bringing patch coverage of the rate-limit backoff changes to 100%.
Author
|
Thanks @sourcery-ai 🙏 Test suggestions — both added: zero/negative clamping in Design points — I made deliberate choices here, but they're genuinely open and
@edgarrmondragon @ReubenFrankel — any preference on these two? |
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
1 participant
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.
What & why
Closes #2012.
The default
RESTStream.backoff_wait_generatorreturnedbackoff.expo(factor=2)andnever looked at the response, so taps hitting
429 Too Many Requestsignored theserver's own
Retry-After/X-RateLimit-Resetheaders and waited a guessedexponential interval instead.
Per the discussion with @ReubenFrankel and @edgarrmondragon, this implements
header-aware backoff within the existing mechanism (the native-retries refactor is
left for later given the open
RetriableAPIErrorquestions), respecting rate-limitheaders when present and falling back to exponential backoff when not.
Changes
backoff_wait_generatoris now a generator using backoff'sprime-then-
.send(exception)protocol. It reads the response defensively(
getattr(exception, "response", None)) so connection/timeout errors that carry noresponse fall back cleanly instead of raising
AttributeError.get_wait_time_from_response(response)hook readsRetry-After(precedence; seconds or HTTP-date) then
X-RateLimit-Reset._parse_retry_afterhelper for the twoRetry-Aftervalue forms.Decisions worth a look
X-RateLimit-Resetis treated as seconds-until-reset (delta) per the IETFRateLimit header fields draft referenced in the issue, not a Unix epoch timestamp.
Happy to switch to epoch (or support both) if you'd prefer — it's a small change.
Retry-Aftertakes precedence overX-RateLimit-Reset;0-second waits are honoured.backoff_wait_generatorare unaffected.Tests
Added to
tests/core/rest/test_failure.py:get_wait_time_from_responseparsing: Retry-After seconds + HTTP-date, X-RateLimit-Reset,precedence, and unparsable/missing → exponential fallback.
ConnectionError(no response) →exponential fallback without crashing; header-less
429→ exponential continues.Full
tests/core/rest/passes (89);ruff,ruff format,ty, andmypyare clean.🤖 Generated with Claude Code
Summary by Sourcery
Respect HTTP rate-limit headers when computing REST retry backoff and add tests to verify the new behavior.
New Features:
Enhancements:
Tests: