fix: enforce total timeout in APIBudget._do_acquire instead of per-attempt - #1156
Draft
devin-ai-integration[bot] wants to merge 1 commit into
Draft
devin-ai-integration[bot] wants to merge 1 commit into
devin-ai-integration[bot] wants to merge 1 commit into
Conversation
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/1789429618-api-budget-total-timeout#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/1789429618-api-budget-total-timeoutPR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
|
This branch has not been deployed
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.
Overview
👉 TL;DR: When the rate limiter is asked to wait at most N seconds for a call credit, it could previously wait up to N seconds per retry attempt, so the total wait exceeded the promised limit. It now tracks a single deadline and stops once that total time has elapsed.
Specifically,
APIBudget._do_acquire()now computesdeadline = time.monotonic() + timeoutonce and caps each sleep to the remaining time, raising the lastCallRateLimitHitwhen the deadline is exhausted.Resolves https://github.com/airbytehq/airbyte-internal-issues/issues/17246:
Changes
airbyte_cdk/sources/streams/call_rate.py:_do_acquiretracks a monotonic deadline whenblock=Trueandtimeout is not None; each sleep ismin(exc.time_to_wait, remaining); onceremaining <= 0the currentCallRateLimitHitis re-raised. Behavior whentimeout is Noneorblock=Falseis unchanged, and the non-negative sleep guard is preserved.unit_tests/sources/streams/test_call_rate.py: newTestAPIBudgetAcquireTimeoutclass using a fake monotonic clock / patchedtime.sleepand a stub policy covering: total timeout enforced,timeout=0raises without sleeping, success before deadline,timeout=Noneunchanged (success and exhaustion),block=Falseraises immediately.Review Spotlight
Reviewers with limited time, please review first:
call_rate.py_do_acquiredeadline logicReproduction
With a policy that always raises
CallRateLimitHit(time_to_wait=60s),maximum_attempts_to_acquire=4, andacquire_call(timeout=1), the old code calledtime.sleep(1.0)three times (3s total).test_timeout_is_total_budgetfails onmainwithassert [1.0, 1.0, 1.0] == [1.0]and passes with this change.Risks
timeout. The only in-repo production caller (HttpClientviaacquire_call) passes no timeout, so this is a latent-bug fix with low blast radius.try_acquireis attempted before raising, so a caller withtimeout=Tgets at most ~T seconds of sleep plus one extra acquire attempt.Follow-ups
range(1, self._maximum_attempts_to_acquire)off-by-one in the attempt count is intentionally left unchanged; it is tracked separately in https://github.com/airbytehq/airbyte-internal-issues/issues/17247.Test plan
poetry run pytest unit_tests/sources/streams/test_call_rate.py -q— 44 passedpoetry run ruff check ./poetry run ruff format --check .on the touched files — cleanpoetry run mypy --config-file mypy.ini airbyte_cdk/sources/streams/call_rate.py— cleanRequested via
/ai-fixon the linked issue (triage: https://github.com/airbytehq/airbyte-internal-issues/issues/17246#issuecomment-5672425513).Link to Devin session: https://app.devin.ai/sessions/b7b8d969b340427fb9efecfb95ece46f
Open in Devin Desktop: https://app.devin.ai/desktop/session/b7b8d969b340427fb9efecfb95ece46f?variant=devin