Skip to content

fix: advance FixedWindowCallRatePolicy windows past all elapsed periods - #1155

Draft
devin-ai-integration[bot] wants to merge 1 commit into
mainfrom
devin/1789429627-fixed-window-rollover
Draft

devin-ai-integration[bot] wants to merge 1 commit into
mainfrom
devin/1789429627-fixed-window-rollover

Conversation

@devin-ai-integration

Copy link
Copy Markdown
Contributor

Summary

👉 TL;DR: Fixed-window rate limiting could stop enforcing its call limit after the limiter sat idle across several windows, and rejected calls made at the exact instant a window reset.

Specifically, FixedWindowCallRatePolicy._update_current_window now rolls forward across every elapsed period at once and treats now == _next_reset_ts as the start of the new window.

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

Related open PRs touching the same class (not merged/conflicting in scope): #985, #989.

Changes

  • _update_current_window: on rollover, compute elapsed_periods = (now - _next_reset_ts) // _offset + 1 and advance _next_reset_ts by that many periods, so alignment is preserved and the pointer lands strictly after now. Previously it advanced by exactly one period per call, so after N missed periods the next N try_acquire calls each reset _calls_num to 0 and the limit was never enforced.
  • Rollover condition changed from now > _next_reset_ts to now >= _next_reset_ts, so a call at the exact reset instant is admitted instead of raising CallRateLimitHit with next reset in 0:00:00.
  • Behavior when fewer than one period has elapsed is unchanged.
  • create_fixed_window_call_rate_policy in the declarative factory is intentionally not touched.

Risks

  • Practically, the missed-period bug is most visible after update() sets _next_reset_ts from API headers to a time in the past; the factory seeds it 10 days ahead. Behavior change is strictly toward enforcing the configured limit.

Test plan

  • New tests in unit_tests/sources/streams/test_call_rate.py::TestFixedWindowCallRatePolicy (freezegun): multiple missed periods enforce the limit; call allowed exactly at reset boundary; alignment preserved after skipping periods; single-period rollover. The first two fail on main and pass on this branch.
  • poetry run pytest unit_tests/sources/streams/test_call_rate.py -q — 42 passed
  • poetry run ruff check ., poetry run ruff format ., poetry run mypy --config-file mypy.ini airbyte_cdk/sources/streams/call_rate.py — clean

Requested via the /ai-fix workflow on the linked issue.

Link to Devin session: https://app.devin.ai/sessions/7d579e79131e44118d81355f44a6b2c6
Open in Devin Desktop: https://app.devin.ai/desktop/session/7d579e79131e44118d81355f44a6b2c6?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/1789429627-fixed-window-rollover#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/1789429627-fixed-window-rollover

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.

@github-actions

Copy link
Copy Markdown

PyTest Results (Fast)

4 392 tests  +4   4 380 ✅ +4   9m 41s ⏱️ +3s
    1 suites ±0      12 💤 ±0 
    1 files   ±0       0 ❌ ±0 

Results for commit 08916a3. ± Comparison against base commit 96a7c0a.

@github-actions

Copy link
Copy Markdown

PyTest Results (Full)

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

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

0 participants