fix(call-rate): honor RateLimit-Remaining when a reset header is present - #1133
fix(call-rate): honor RateLimit-Remaining when a reset header is present#1133devin-ai-integration[bot] wants to merge 1 commit into
Conversation
Co-Authored-By: bot_apk <apk@cognition.ai>
🤖 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/1787707125-movingwindow-honor-ratelimit-headers#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/1787707125-movingwindow-honor-ratelimit-headersPR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
|
There was a problem hiding this comment.
Pull request overview
This PR fixes MovingWindowCallRatePolicy.update() so it properly synchronizes the local moving-window bucket with API-provided rate-limit feedback (notably when both “remaining” and “reset” headers are present), and adds unit tests covering the corrected behavior and HttpAPIBudget.update_from_response() integration.
Changes:
- Update moving-window rate-limit state from
available_callsregardless ofcall_reset_ts, instead of silently ignoring the “both headers present” case. - Add
_calls_left()helper to compute remaining allowance across multiple configured rates (most constraining rate wins). - Add new unit tests covering update semantics (both headers present, no-op cases, most-constraining rate behavior, and the zero-available-calls bucket fill case) plus an end-to-end
HttpAPIBudgetheader-driven update test.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
airbyte_cdk/sources/streams/call_rate.py |
Fixes moving-window update logic and introduces _calls_left() to reconcile bucket state with API-reported remaining calls. |
unit_tests/sources/streams/test_call_rate.py |
Adds unit tests validating the corrected moving-window update behavior and HttpAPIBudget.update_from_response() integration. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| for rate in self._bucket.rates: | ||
| lower_bound_idx = binary_search(items, now - rate.interval) | ||
| calls_used = len(items) - lower_bound_idx if lower_bound_idx >= 0 else 0 | ||
| calls_left.append(rate.limit - calls_used) |
There was a problem hiding this comment.
🚫 Not fixing — I don't think this one holds, though it's a reasonable thing to check.
InMemoryBucket.put() expands weight into individual list entries rather than storing a single weighted item:
# pyrate_limiter/buckets/in_memory_bucket.py
def put(self, item: RateItem) -> bool:
for rate in self.rates:
lower_bound_idx = binary_search(self.items, item.timestamp - rate.interval)
if lower_bound_idx >= 0:
count_existing_items = len(self.items) - lower_bound_idx # <- counts entries
space_available = rate.limit - count_existing_items
...
self.items.extend(item.weight * [item]) # <- N entries for weight NSo a weight=5 dummy call (or a weight=5 try_acquire) becomes 5 entries in items, and len(items) is already the weighted count. _calls_left() deliberately mirrors put()'s own accounting — same binary_search on the same list, same len(items) - lower_bound_idx — so the two cannot disagree about how much room is left. Summing item.weight instead would double-count by a factor of the weight.
This policy always uses InMemoryBucket (self._bucket = InMemoryBucket(pyrate_rates) in MovingWindowCallRatePolicy.__init__), so there's no alternative backend where the expansion wouldn't hold. If pyrate-limiter ever switched to storing weighted items compactly, put() itself would break the same way and both would need updating together.
Happy to be overruled if a reviewer sees a bucket path I've missed.
Summary
MovingWindowCallRatePolicy.update()silently dropped the rate-limit state reported by the API whenever the response carried both a remaining-calls header and a reset header — the exact shape Klaviyo (and many other APIs) return on every non-429 response. The both-present branch was a commented-outTODO, so every connector declaring anapi_budgetwith moving-window policies was purely feed-forward: it throttled to manifest constants and only learned about upstream state from a429after the fact.The single-header path was also effectively inert.
items_to_addwas assigned a comparison, not a count:so
available_calls == 0added a single dummy call instead of filling the bucket.After this change,
update()reacts toavailable_callsregardless ofcall_reset_ts, and fills the bucket until what it still allows equals what the API reports:_calls_left()is the new helper: for each configured rate it counts the items inside that rate's own interval (viapyrate_limiter.utils.binary_search, the same primitiveInMemoryBucket.putuses) and returns the most constraining remainder. That matters for the burst+steady rate pairs connectors actually declare — comparing againstrates[0].limitalone (the old code) would let a tighter long-window rate go unenforced, andbucket.count()counts the whole max-interval window rather than the rate's own.Deliberately not changed:
call_reset_tsstays unused, and is documented as such. A moving window has no reset point, so the window length remains the configured one and the only actionable signal is the number of calls left. This also sidesteps the fact thatHttpAPIBudget.get_reset_ts_from_response()parses the reset header as an absolute epoch timestamp while several APIs (Klaviyo included) document it as seconds remaining — a real latent bug forFixedWindowCallRatePolicyusers, but out of scope here and best fixed with an explicit semantics option.api_budget(e.g.source-klaviyo) pick this up with no manifest change once the CDK version ships insource-declarative-manifest.Declarative-First Evaluation
The originating issue is on
source-klaviyo, a manifest-only connector, so a custom Python component was evaluated and rejected.source-klaviyoalready declares the right thing in its manifest — anHTTPAPIBudgetwith per-endpointMovingWindowCallRatePolicyburst/steady rates, using the defaultratelimit-remaining/ratelimit-resetheader names, which match Klaviyo's headers (lookup is case-insensitive). None of the declarative building blocks (RecordFilter,AddFields/RemoveFields,DatetimeBasedCursor,DefaultPaginator,SubstreamPartitionRouter, requester error handlers, transformations,$refoverrides) can affect inter-request pacing — that is entirely theapi_budgetpolicy's job. The gap was therefore not in the manifest or in any connector-side component, but in the shared CDK policy the manifest already points at, so the fix belongs here. Net result: no connector custom component, and no manifest change either.Behavior compatibility
available_calls is None→ early return, unchanged.test_update_respects_the_most_constraining_rate.Not a breaking change under the connector breaking-change checklist: no schema, spec, state, or stream changes — only rate-limiting timing. No connector version bump here either; this is CDK-only, and
source-klaviyopicks it up when itssource-declarative-manifestbase image (pinned at7.24.0) is bumped.Reproduction
No live Klaviyo account was available (no private key), so this was not reproduced against the real API — it is verified statically and by unit tests. The gap is directly visible in the pre-change source:
update()guarded onavailable_calls is not None and call_reset_ts is None, whileHttpAPIBudget.update_from_response()passes both values whenever both headers are present, so Klaviyo's responses took the ignored path every time.test_update_available_calls_with_reset_tsreproduces that at the policy level (all 10 calls go through before the change), andTestHttpAPIBudget::test_update_from_responsereproduces it end-to-end through a response object carrying Klaviyo-shaped headers.Test Coverage
unit_tests/sources/streams/test_call_rate.py— six new tests:test_update_available_calls_with_reset_ts— the both-headers-present combination now throttles.test_update_only_lowers_allowance—available_calls=50against a limit of 10 is a no-op.test_update_is_noop_without_available_calls— headerless APIs unaffected.test_update_respects_the_most_constraining_rate— burst (3/s) + steady (60/m); also asserts the wait stays within the configured window.test_update_available_calls_zero_fills_bucket— covers theweight=Truebug above.TestHttpAPIBudget::test_update_from_response— end-to-end throughHttpAPIBudget.update_from_responsewithRateLimit-Remaining/-Reset/-Limit, proving a declaredapi_budgetlowers its allowance from headers alone.poetry run pytest unit_tests/sources/streams/test_call_rate.py→ 44 passed.ruff check/ruff formatclean;mypy airbyte_cdk/sources/streams/call_rate.pyclean.Resolves https://github.com/airbytehq/airbyte-internal-issues/issues/17029:
Link to Devin session: https://app.devin.ai/sessions/878b6be04bb648618e2fe0b6834a9151