Skip to content

fix: make APIBudget honor maximum_attempts_to_acquire exactly - #1157

Draft
devin-ai-integration[bot] wants to merge 1 commit into
mainfrom
devin/1789429689-call-rate-max-attempts
Draft

devin-ai-integration[bot] wants to merge 1 commit into
mainfrom
devin/1789429689-call-rate-max-attempts

Conversation

@devin-ai-integration

Copy link
Copy Markdown
Contributor

Overview

👉 TL;DR: The rate limiter's retry loop made one fewer attempt than configured, so with a single allowed attempt it never checked the rate limit at all and let calls through even when the budget was exhausted. It also slept once more than necessary before giving up.

Specifically, APIBudget._do_acquire now iterates range(1, maximum_attempts_to_acquire + 1), breaks before sleeping on the final failed attempt, and APIBudget.__init__ rejects maximum_attempts_to_acquire < 1 with a ValueError.

Resolves https://github.com/airbytehq/airbyte-internal-issues/issues/17247:

Part of epic https://github.com/airbytehq/airbyte-internal-issues/issues/17243.

Changes

  • APIBudget._do_acquire: loop bound range(1, n) -> range(1, n + 1) so exactly n try_acquire calls are made; break after the last failed attempt so block=True no longer sleeps for time_to_wait before raising.
  • APIBudget.__init__: raise ValueError when maximum_attempts_to_acquire < 1.
  • New TestAPIBudgetMaximumAttempts tests: exhausted policy + n=1 raises CallRateLimitHit (blocking and non-blocking); try_acquire is called exactly n times with n-1 sleeps for n in {1, 2, 5}; values < 1 rejected.

Reproduction

Before the fix, with an exhausted FixedWindowCallRatePolicy(call_limit=1):

APIBudget([policy], maximum_attempts_to_acquire=1).acquire_call(req, block=False)  # returned silently
APIBudget([policy], maximum_attempts_to_acquire=2).acquire_call(req, block=False)  # raised CallRateLimitHit

Risks

  • Behavior change for the default (100000) is one extra attempt (100000 vs 99999); the only known external caller passing this argument is source-slack, which uses the default.
  • Not a breaking change; no public interface changed.

Test plan

  • poetry run pytest unit_tests/sources/streams/test_call_rate.py -q (45 passed; the 3 new n>=2 count tests fail on main without the fix)
  • poetry run ruff check ., poetry run ruff format --check ., poetry run mypy --config-file mypy.ini airbyte_cdk/sources/streams/call_rate.py

Requested via /ai-fix on the linked issue (triage session: https://app.devin.ai/sessions/33bc7ef3d07c40578437ec2c41099b62).

Link to Devin session: https://app.devin.ai/sessions/55032d81326149dcba6a546fb345a1d8
Open in Devin Desktop: https://app.devin.ai/desktop/session/55032d81326149dcba6a546fb345a1d8?variant=devin

Co-Authored-By: bot_apk <apk@cognition.ai>
@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment, CI, and merge conflict monitoring

@github-actions

Copy link
Copy Markdown

👋 Greetings, Airbyte Team Member!

Here are some helpful tips and reminders for your convenience.

💡 Show Tips and Tricks

Testing This CDK Version

You 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/1789429689-call-rate-max-attempts#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/1789429689-call-rate-max-attempts

PR Slash Commands

Airbyte Maintainers can execute the following slash commands on your PR:

  • /autofix - Fixes most formatting and linting issues
  • /poetry-lock - Updates poetry.lock file
  • /test - Runs connector tests with the updated CDK
  • /prerelease - Triggers a prerelease publish with default arguments
  • /poe build - Regenerate git-committed build artifacts, such as the pydantic models which are generated from the manifest JSON schema in YAML.
  • /poe <command> - Runs any poe command in the CDK environment
📚 Show Repo Guidance

Helpful Resources

📝 Edit this welcome message.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

No unresolved issues block approval.

Pull request overview

Updates APIBudget to honor configured retry counts, avoid unnecessary final sleeps, and reject invalid attempt values.

Changes:

  • Corrects retry-loop counting and final-attempt behavior.
  • Validates maximum_attempts_to_acquire >= 1.
  • Adds focused tests for retries, exhaustion, and validation.
File summaries
File Description
unit_tests/sources/streams/test_call_rate.py Adds coverage for maximum-attempt behavior.
airbyte_cdk/sources/streams/call_rate.py Updates retry logic and input validation.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@github-actions

Copy link
Copy Markdown

PyTest Results (Full)

4 398 tests  +7   4 386 ✅ +7   8m 29s ⏱️ - 5m 32s
    1 suites ±0      12 💤 ±0 
    1 files   ±0       0 ❌ ±0 

Results for commit 6c3c480. ± Comparison against base commit 96a7c0a.

@github-actions

Copy link
Copy Markdown

PyTest Results (Fast)

4 395 tests  +7   4 383 ✅ +7   9m 35s ⏱️ -3s
    1 suites ±0      12 💤 ±0 
    1 files   ±0       0 ❌ ±0 

Results for commit 6c3c480. ± Comparison against base commit 96a7c0a.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants