Skip to content

fix: apply redirect response rate-limit headers to the request that produced them - #1159

Draft
devin-ai-integration[bot] wants to merge 1 commit into
mainfrom
devin/1789429753-limiter-redirect-policy
Draft

devin-ai-integration[bot] wants to merge 1 commit into
mainfrom
devin/1789429753-limiter-redirect-policy

Conversation

@devin-ai-integration

Copy link
Copy Markdown
Contributor

Overview

👉 TL;DR: When an API call is redirected, the rate-limit headers from the redirect target were being applied to the original endpoint's rate-limit policy, so the wrong endpoint could be marked as exhausted.

Specifically, LimiterMixin.send now pairs request with response.history[0] (the response it actually produced) instead of the final response of the redirect chain when calling HttpAPIBudget.update_from_response.

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

Changes

  • LimiterMixin.send: update the budget from the first response in the chain (response.history[0] when redirects were followed, else response). Each redirect hop already passes through send(..., allow_redirects=False) and acquires/updates its own policy, so hop behavior is unchanged.
  • New TestLimiterSessionRedirects in unit_tests/sources/streams/test_call_rate.py (mocks HTTPAdapter.send so LimiterMixin.send is exercised): A→B redirect with separate policies (only B exhausted), A→B redirect with a shared policy (final headers win), and no-redirect (unchanged).

Design notes

Show/Hide Content

requests.Session.send resolves redirects via self.send(hop_request, allow_redirects=False), so for a chain A→B the inner call pairs (B, B-response) correctly; the outer call previously paired (A, B-response). Using response.history[0] gives the outer call A's own 302 response, whose headers are the ones that belong to A's policy. A response hook was considered but this is a smaller change with the same effect.

response.history is read via getattr(..., []) because existing tests in unit_tests/sources/streams/http/test_http.py inject Mock(spec=requests.Response) objects, which lack the instance-level history attribute.

Test plan

  • poetry run pytest unit_tests/sources/streams/test_call_rate.py -q — 41 passed
  • test_redirect_updates_only_target_policy fails with CallRateLimitHit on A when the call_rate.py change is reverted
  • poetry run pytest unit_tests/sources/streams/http -q — 466 passed
  • poetry run ruff check ., poetry run ruff format --check ., poetry run mypy --config-file mypy.ini airbyte_cdk/sources/streams/call_rate.py — clean

Requested via /ai-fix on the linked issue.

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

…roduced them

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/1789429753-limiter-redirect-policy#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/1789429753-limiter-redirect-policy

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 review issues were identified.

Pull request overview

Fixes rate-limit accounting for redirected HTTP requests by associating each response with the request that produced it.

Changes:

  • Corrects redirect response-to-request budget updates.
  • Adds coverage for separate/shared policies and non-redirected requests.
File summaries
File Summary
unit_tests/sources/streams/test_call_rate.py Adds redirect behavior tests.
airbyte_cdk/sources/streams/call_rate.py Corrects rate-limit response pairing.
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 (Fast)

4 391 tests  +3   4 379 ✅ +3   9m 30s ⏱️ -8s
    1 suites ±0      12 💤 ±0 
    1 files   ±0       0 ❌ ±0 

Results for commit 0232038. ± Comparison against base commit 96a7c0a.

@github-actions

Copy link
Copy Markdown

PyTest Results (Full)

4 394 tests  +3   4 382 ✅ +3   13m 22s ⏱️ -39s
    1 suites ±0      12 💤 ±0 
    1 files   ±0       0 ❌ ±0 

Results for commit 0232038. ± 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