Skip to content

feat: add REDUCE_PAGE_SIZE response action for dynamic page-size reduction - #1149

Open
Anatolii Yatsuk (tolik0) wants to merge 13 commits into
mainfrom
tolik0/cdk/reduce-page-size
Open

Anatolii Yatsuk (tolik0) wants to merge 13 commits into
mainfrom
tolik0/cdk/reduce-page-size

Conversation

@tolik0

@tolik0 Anatolii Yatsuk (tolik0) commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

What

Adds a REDUCE_PAGE_SIZE response action to the declarative framework. When a response filter resolves to it, the retriever shrinks the page size and re-fetches the same page — same next-page token, same stream slice — instead of failing the sync.

Why

Several declarative connectors already expose a page-size config field whose only purpose is letting a user recover from an API that rejects large pages. Recovery today means: sync fails, user reads the error, lowers the setting, restarts the sync.

Adopters, checked against connector source rather than assumed (each connector's manifest and components.py were read; where a claim below says "not verified", it means not verified against the live API):

  • source-github (airbytehq/airbyte-internal-issues#16519) — hard dependency. 6 GraphQL streams get 502/504 when the query is too expensive. GitHubGraphQLErrorHandler mutates stream.page_size and returns RETRY, but HttpClient replays the same PreparedRequest, so the oversized first is already serialized into the body and the retry never benefits. Only a later page picks up the smaller size, and any non-502/504 response resets it. feat(source-github): migrate the GraphQL streams to the manifest (Step 9) airbyte#85822 adopts this action and pins a prerelease of this branch.
  • source-amazon-seller-partner ListFinancialEvents (manifest.yaml:1274-1362) — plausible drop-in, not verified against the API. CursorPagination on NextToken, MaxResultsPerPage as page_size_option, and a 400/InvalidInput predicate that today resolves to FAIL with an error telling the user to "try reducing it to a smaller value (e.g., 50, 25, 10, or even 1 for very high-volume accounts)". A manual binary search across sync restarts.
  • source-zendesk-support ticket_comments (manifest.yaml:955-1000) — not adoptable as-is. It retries 504 at an unchanged per_page, so the retries cannot succeed, and its spec says "lower values may help prevent timeouts on large datasets". But its page_token_option is a RequestPath: the next page is a URL returned by the API that already carries per_page=100, and the reduced size would be injected next to it (...?per_page=100&start_time=...&per_page=50, checked with HttpRequester._join_url and requests.Request(...).prepare().url). This PR rejects that combination at config time. Supporting it means rewriting the page-size parameter inside the token URL; 27 connectors in the monorepo pair a RequestPath token with a page_size_option, 3 of them (mendeley, sendgrid, zendesk-support) already handle 502/504, so that is a worthwhile follow-up, not part of this PR.
  • source-facebook-pagesneeds restructuring first. It fails on "Please reduce the amount of data you're asking for" and tells the user to lower page_size by hand. The filter lives on the shared requester (manifest.yaml:15-80), the base retriever is NoPagination, and only post and post_insights add a paginator. Since declaring the action without a page_size_reduction block is rejected, the shared handler has to be split per stream before any of them can use it.

No components.py in the monorepo hand-rolls a page-size reduction (searched all 74). The neighbouring shape, splitting a time window on error, exists in source-paypal-transaction and is tracked for source-google-ads in airbytehq/airbyte-internal-issues#17173; it is a separate axis (see Scope).

How

The signal path mirrors RESET_PAGINATION:

HttpResponseFilter action: REDUCE_PAGE_SIZEHttpClient._handle_error_resolution raises PageSizeReductionRequiredException → the exception is not in TRANSIENT_EXCEPTIONS, so it escapes the backoff decorators untouched → SimpleRetriever._read_pages catches it, reduces, and continues without advancing the token. CompositeErrorHandler short-circuits on the new action so it is not swallowed when nested.

retriever:
  type: SimpleRetriever
  page_size_reduction:
    type: PageSizeReduction
    reduction_factor: 2       # default
    minimum_page_size: 1      # default
    max_attempts: 5           # default
    reset_policy: NEVER       # default; or AFTER_SUCCESSFUL_PAGE
    backoff_seconds: 0.5      # default; base wait, multiplied by the attempts made in a row
    retries_at_minimum_page_size: 0   # default; retries of the same page once nothing is left to reduce
  requester:
    error_handler:
      type: DefaultErrorHandler
      response_filters:
        - http_codes: [502, 504]
          action: REDUCE_PAGE_SIZE
          failure_type: transient_error

reset_policy defaults to NEVER because restoring the configured size after every good page re-triggers the error on each subsequent page and roughly doubles request volume.

Thread safety

model_to_component_factory builds one retriever per stream and hands that single object to DeclarativePartitionFactory, which reuses it for every partition; partitions are read concurrently by a thread pool. So the reduction state lives in a PageSizeReducer constructed inside _read_pages, like PaginationTracker — no mutable page-size state on the retriever, paginator, or pagination strategy. The effective size travels as an optional page_size_override keyword passed only when set, so out-of-tree SimpleRetriever, Paginator and PaginationStrategy subclasses that do not accept it keep working. There is a test with two partitions on threads asserting the healthy one keeps its configured size while the failing one runs reduced.

Correctness fix in OffsetIncrement

Its stop condition compared the returned record count against the configured page size. Reduced to 50 with a configured 100, a full 50-record page would hit 50 < 100, end pagination, and silently drop the rest of the partition. The stop condition now honors the override; the offset math already advanced by the actual last_page_size, so that part was fine.

Correctness fix in CursorPagination

The same class of bug existed in CursorPaginationStrategy, which all four named adopters actually use: stop_condition was evaluated against last_page_size with no notion of the size requested, so after a reduction a full page read as short and pagination ended silently. The requested size is now exposed to both stop_condition and cursor_value as page_size.

A stream that enables page_size_reduction also has its stop_condition checked at config time. The check parses the expression with Jinja and inspects the comparisons last_page_size takes part in on the AST — string matching cannot do this job, because \bpage_size\b matches inside config['page_size'] just as well as it matches the reduction-aware variable, and it cannot tell an inequality (which a reduction invalidates) from an emptiness test (which it cannot). The rule is a single question: can the condition be true for a page that is full at the size that was requested?

form verdict
{{ last_page_size == 0 }}, {{ last_page_size < 1 }} accepted — a full page is never empty
{{ last_page_size < page_size }}, {{ page_size > last_page_size }} accepted — the threshold follows the reduction
{{ last_page_size > 1000 }} accepted — a smaller page can only stop satisfying a lower bound
{{ last_page_size < 200 }}, {{ last_page_size < config['page_size'] }} rejected — a full page at a reduced size reads as short
anything the analysis cannot classify warned, not rejected
{{ response['data'] | length < 100 }}, {{ response.get("count", 0) < 1000 }} warned — a page length counted from the response body truncates the same way, and which response field holds one is not knowable here
{{ config['page_size'] > response['data'] | length }}, {{ response.page >= response.total_pages }} warned — both operator directions are mirrored, so a threshold on the left is read the same way
{{ not response.next }}, {{ response.count == 0 }}, {{ 100 < response.count }} accepted — no comparison that bounds a non-literal from above
{{ not (last_page_size >= 100) }}, {{ last_page_size - 100 < 0 }} warned — a comparison that does not decide the condition in its own polarity, or one with arithmetic in front of the operator, is not classified
{{ last_page_size < [page_size, 100] | max }}, {{ last_page_size < page_size + 50 }} warned — a threshold built from page_size can hold the configured size again, so it is not treated as reduction-aware
{% if last_page_size < 100 %}true{% endif %} rejected — a {% if %} that renders truthy text and nothing else follows its test

A condition that never names last_page_size is not waved through, because last_page_size is not the only way to count the records of a page: the response body carries the same number, and {{ response['data'] | length < 100 }} is {{ last_page_size < 100 }} counted one layer out. Which of a connector's own response fields holds a page length cannot be known at config time, so a condition that bounds any non-literal value from above is warned about, and one that makes no such comparison — the common {{ not response.next }} shape, an emptiness test, or a comparison whose bounded side is a literal — is accepted.

Both operator directions are mirrored, the way _MIRRORED_OPERATORS mirrors them on the last_page_size path. An earlier revision read lt/lteq in full but read gt/gteq only when the threshold was a literal, which left {{ config['page_size'] > response['data'] | length }} classified as safe while {{ response['data'] | length < config['page_size'] }} was warned about — the same comparison written backwards. Every one of the four ordering operators bounds one of its operands from above, and only a literal is certain not to be a page length. {{ response.page >= response.total_pages }} warns as a consequence: in a >= b there is an upper bound on b, so reading that shape as "no upper bound" was wrong.

Measured against the monorepo (09124c5aabc, 533 manifests, 1505 stop_conditions parsed as YAML and classified): 1459 accepted, 3 rejected, 43 warned. Of the 14 that use last_page_size, 11 are {{ last_page_size == 0 }} (source-zendesk-support ×6, source-trello ×5) and are accepted; the other 3 are {{ last_page_size < <literal> }} (source-discord ×3) and are rejected, which is the gate doing its job. Of the 43 warnings, 36 are the response-derived page length — source-pardot ×32, source-zendesk-talk ×2, source-zendesk-chat ×2 — which is exactly the shape the warning is for, and 7 are the mirrored gt/gteq reading (source-serpstat ×6 comparing a page number to a config value, source-jira ×1 comparing startAt + maxResults to a total; neither bounds a page length). No connector in the fleet opts into page_size_reduction, so none of the 43 fires on anything shipping today.

source-github's three stop_conditions are all accepted. They live on the adopter branch, tolik0/source-github/graphql-streams at bf71e0e7c23, in airbyte-integrations/connectors/source-github/source_github/manifest.yaml — the connector has none on master, which is why the fleet count above does not include them.

Of the connectors above: facebook-pages stops on response.paging; amazon-seller-partner ListFinancialEvents has no stop_condition at all, so the check returns early; source-github's GraphQL streams use a CustomPaginationStrategy, accepted through the page_size_override signature check; and zendesk-support ticket_comments is {{ last_page_size == 0 }}, which would pass, but that stream is rejected earlier by the RequestPath guard below.

Two caveats stated plainly, since an earlier revision of this description overstated the guarantee:

  • This is not a proof that silent truncation is unreachable. A condition whose shape the analysis does not understand — a Jinja test, a macro, last_page_size piped through a filter before an equality, a negation, arithmetic in front of the operator, a threshold built from page_size rather than being page_size, or a comparison rendered next to other text — produces a warning and the stream is still built. Rejecting a manifest the check merely cannot read would be worse than the bug: the check runs at stream construction, so a false rejection breaks check and discover, not only read. What the check does guarantee is narrower than an earlier revision of this description claimed: the two forms known to truncate through last_page_size — a literal threshold and a config-derived one — cannot be built. The same comparison written against a count read from the response body is warned about, not rejected, because the CDK cannot tell a page length from any other number in a response.
  • Because it runs at stream construction, a rejection surfaces as a failed check, not as a failed sync. spec is unaffected.

One behavior change outside the opt-in

OffsetIncrement with a page_size that interpolates to "" previously raised TypeError on page 2. It now paginates until an empty page. This is the only change visible to a manifest that does not set page_size_reduction.

Rejected at config time, not mid-sync

PageIncrement (the token is a page number, so halving moves record boundaries and skips records — and its page size is also its stop condition), including through a subclass; paginators other than DefaultPaginator; a missing page_size_option; a page_token_option of type RequestPath (the next page is then a URL built by the API that already carries the page size, so the reduced size would be sent alongside the original one and which of the two the API honours is up to the API); query-properties chunking (earlier chunks are already emitted, so a retry would duplicate); file_uploader; LazySimpleRetriever; all six AsyncRetriever sub-requesters and login_requester; and declaring the action without a page_size_reduction block. Each raises with an actionable message naming the stream.

Termination

Every reduction either strictly decreases the page size or, once there is nothing left to reduce, spends one of the retries_at_minimum_page_size — and raises when that budget is spent too. Each one waits backoff_seconds multiplied by the attempts made in a row before re-issuing the page.

There are two budgets, measuring different things, and neither spends the other. max_attempts (default 5) bounds the reductions made in a row without a single page succeeding — the axis that separates a partition which is stuck from one which is merely expensive. retries_at_minimum_page_size (default 0) bounds the waits taken once the page size is already as small as minimum_page_size allows, where reducing is no longer an option but waiting still is. Both restart on every page that succeeds, under both reset policies; reset_policy decides only whether the page size itself is restored.

An earlier revision restarted it under AFTER_SUCCESSFUL_PAGE only, which made it a partition-wide total under the default NEVER. A stream whose per-page cost varies — the GraphQL case this feature exists for, where query cost tracks the nested data volume of whatever lands on a page — then failed at the 6th heavy page of a long partition in which every single reduction had been followed by a successful page, with a transient_error telling the user the source had rejected every page size the connector asked for. That is the same defect as the never-reset MAX_TOTAL_REDUCTIONS = 1000 this PR already removed, with the cliff at the 6th reduction instead of the 1001st page.

The read still terminates. Only a page that succeeded restarts the budget and _read_pages calls that once per page it consumed, so between any two restarts the partition made a page of progress, and the reductions that make no progress are bounded by max_attempts. Under NEVER the page size is also never restored, so it strictly decreases and minimum_page_size bounds the whole partition on its own. test_given_reset_policy_never_when_pages_succeed_then_attempts_are_reset pins the healthy varying-cost stream, ..._then_minimum_page_size_still_ends_the_read pins that it still terminates, and test_given_no_page_succeeds_then_attempts_are_not_reset pins the stuck one.

When a budget is exhausted the failure is transient_error — the source genuinely did keep failing — and the message names the stream and the page size that was actually requested and failed. One case is a config_error instead: a page_size that minimum_page_size blocks from ever being reduced, which no response can fix and which names both numbers to change. That is reported only after the retries at the floor are spent, because the retry budget cannot depend on how the page size arrived at the floor — otherwise the same manifest would give a partition its retries when the reduction walked down to the floor and none when the user's page_size was already there.

minimum_page_size and max_attempts are two bounds on the same reduction and the tighter one wins: a run of failing pages divides the page size by reduction_factor at most max_attempts times, so with the defaults a page size of 1000 bottoms out at 31 records per page and a minimum_page_size: 10 is never reached in that run. The factory now warns when the floor is out of reach of the budget and says how large max_attempts would have to be. It warns rather than raises because pages that succeed in between restart the budget while NEVER keeps the reduced size, so the floor is still reachable over a partition — and it only warns when the floor is above 1, since a floor of 1 is out of reach of the default budget on any page size above 32 and is not a floor worth reaching. (Gating on whether the field was written down instead, via pydantic's __fields_set__, would have warned at minimum_page_size: 1 spelled out longhand, which is ordinary manifest style.)

The wait between reductions is the CDK's own — backoff_seconds (default 0.5) multiplied by the attempts made in a row — and does not consult the error handler's backoff_strategies or a Retry-After header, the same way RESET_PAGINATION does not. The schema says so. backoff_seconds and retries_at_minimum_page_size exist because that bypass also means the HTTP retry budget does not apply: without them a stream that reached the floor got fewer attempts, 0.5s apart, than the same connector had before it adopted the reduction. source-github sets 10 and 3, which comes to seven requests over about two minutes on a persistently failing page — comparable to what max_retries: 5 with the default exponential backoff gives the same 502.

failure_type on the escape path

PageSizeReductionRequiredException is control flow: on the SimpleRetriever path it is always caught and its failure_type is inert. It can still escape from a CustomRetriever, or from a plain Python-CDK HttpStream whose error handler returns the now-public enum value. That escape is kept on purpose — it is the only signal such a stream gets — and it is typed config_error, because nothing re-issues the page there and a job-level retry would fail identically forever. Its message says so.

Adoption, and the three things it caught

airbytehq/airbyte#85822 migrates all six source-github GraphQL streams onto this action. Building it surfaced three defects in earlier drafts of this change, all fixed here rather than worked around in the connector:

  1. The strategy allowlist rejected every CustomPaginationStrategy. The check was by type, which excluded exactly the streams the feature was built for: source-github's nested GraphQL traversal cannot be expressed by a built-in strategy. What actually matters is not whether the CDK recognizes the strategy but whether the strategy can be told the reduced size — and a custom strategy is written by the same person enabling the reduction. A custom strategy is now accepted when its next_page_token takes a page_size_override keyword (or **kwargs), checked by signature at config time so one that would raise TypeError on the first reduction is rejected up front with an actionable message.
  2. The retry contract at the floor was too thin, and its wait was not configurable. A REDUCE_PAGE_SIZE response never reaches the HTTP retry budget, so a stream that reached minimum_page_size — or was configured at it — failed on the first response it could not answer with a smaller page, having waited 0.5s between attempts. An API that answers the same 502 to "your page is too heavy" and to a passing hiccup therefore got fewer attempts after adopting the reduction than before. Hence backoff_seconds and retries_at_minimum_page_size, both defaulting to today's behaviour.
  3. A non-integer page size crashed the reducer. reduce() did current_page_size // reduction_factor on whatever get_page_size returned. Built-in strategies always return an int; a custom one can return anything, and a string page size — which is what a custom component gets when a manifest field is left uninterpolated — failed mid-sync with TypeError: unsupported operand type(s) for //: 'str' and 'float'. Now a config error naming the type.

Two loose ends worth naming

  • The generated model is edited by hand here. bin/generate_component_manifest_files.py does not pass --field-constraints, so a numeric minimum/exclusiveMinimum in the YAML becomes a conint(...)/confloat(...) annotation that mypy rejects — on five fields that predate this PR (DynamicStreamCheckConfig.stream_count, both backoff strategies, AsyncRetriever.failed_retry_wait_time_in_seconds) as well as on PageSizeReduction. Running the generator on this branch produced a 524-line diff that does not type check, so that commit was reverted and PageSizeReduction was written by hand with Field(gt=1.0)/ge=1; the YAML bounds are what manifests are validated against either way. The reason is now recorded as a comment in the generator script. Adding the flag, and a CI check that regenerates and diffs, is its own change — not linked to an issue yet.
  • The .gitignore additions are unrelated to page sizes. Three entries for artifacts pre-existing tests leave behind on macOS (file::memory:?cache=shared, test_response.csv, and the ResponseToFileExtractor uuid4 glob). Harmless and useful, but they are hygiene rather than part of this feature; calling them out here rather than splitting them into their own PR.

Scope

Page size only. Request-window reduction (airbytehq/airbyte-internal-issues#17173) is a separate axis — it re-slices the datetime range and touches cursor state. This action never changes slice boundaries. The two are intended to compose, and this being the second occupant of the ResponseAction → signal-exception → _read_pages seam should make that one a copy of an established pattern rather than a third mechanism.

Relationship to #1056

Supersedes the draft in #1056, which will be closed. Same action name and same exception, different mechanics. #1056 mutated _page_size on the shared strategy instance, so one partition's success wiped another's reduction mid-flight; its reduce loop was unbounded, so a permanently failing endpoint looped forever at page size 1; its OffsetIncrement change routed the reduced size through get_page_size() only, leaving the stop-condition data loss above in place; it let PageIncrement reduce; it did not update the CompositeErrorHandler short-circuit list; and its isinstance(self._paginator, DefaultPaginator) guard silently no-oped under PaginatorTestReadDecorator, so Connector Builder test reads skipped reduction entirely.

Testing

Counts below are collected cases, parametrized tests included.

  • unit_tests/sources/declarative/retrievers/test_page_size_reducer.py40 cases (32 test functions): factor/floor/attempt arithmetic, both reset policies, both budgets restarting on a successful page and not spending each other, the retries at the floor including the stream whose page_size started there, the non-integer page-size guard, the configurable growing backoff and the default time.sleep path, the healthy long stream, the stuck partition, failure_message composition, and the error-message shape (every message names the stream; remediation on config_error only).
  • unit_tests/sources/declarative/parsers/test_stop_condition_safety.py59 cases (6 test functions) over the stop-condition analysis, parametrized on the real monorepo forms in both directions: the shapes where a comparison does not decide the condition on its own (negation, arithmetic, a conditional expression, a {% if %} with an else or a falsy body, a comparison rendered next to text), a threshold merely built from page_size, the response-derived page lengths that are live in the fleet in both operator directions, and the response-reading conditions that stay accepted.
  • test_model_to_component_factory.py57 feature-scoped cases (-k "page_size_reduc or reduce_page_size or stop_condition") covering every config-time rejection, the accepted forms, the warn-don't-reject path, the unreachable-floor warning and its two negative controls, and a custom strategy that accepts the override and one that does not.
  • test_simple_retriever.py — the retry re-fetches the same page, the retry request actually carries the reduced size (asserted on the outgoing request, not on strategy state), reset-policy behavior, floor and max_attempts exhaustion, the action raising when no page_size_reduction is configured, the mid-page-reduction guard, and the two-thread partition-isolation test.
  • Paginator and strategy tests for override forwarding on all six override-carrying methods, including PaginatorTestReadDecorator.
  • test_offset_increment.py and test_cursor_pagination_strategy.py pin the stop-condition fixes and the page_size interpolation variable.
  • test_connector_builder_handler.py — a reduction retry is logged as an auxiliary request and does not count against max_pages_per_slice; test_http_client.py pins the title and description that request carries, so the Builder's side panel does not label it like a successful page fetch.
  • test_concurrent_declarative_source.py — manifest-level HttpMocker read asserting request 1 is first=100, the 502 is followed by request 2 to the same cursor with first=50, and all records arrive.

Locally at 54f86a82: unit_tests/sources/declarative + connector_builder + streams/http 2513 passed, 1 skipped; retrievers + paginators 222, parsers 322, test_concurrent_declarative_source.py 75 passed / 1 skipped, connector_builder 63. Run on its own, streams/http is 450 passed with 1 failure — test_that_response_was_cached, which fails the same way on main because requests_cache's in-memory SQLite URI becomes a real file on macOS, and which passes when the suite runs alongside the others. mypy (461 files), ruff check and ruff format --check are clean.

failure_message

PageSizeReduction.failure_message is an optional sentence the connector appends to the terminal error, shown when the reductions run out — max_attempts reached, or the page size already at minimum_page_size. Without it the message names the stream and the page size that was actually requested and failed, and says only that the API kept rejecting every page size the connector asked for. With it the connector can point at its own remedy, for instance which filter narrows the query down, which is what the connector-specific message this action replaces in source-github did. It is not appended to the misconfiguration errors, which are the connector developer's to fix. Four tests pin the composition; two factory tests pin the field.

The same complaint applies to PaginationTracker's terminal error, raised in the airbytehq/airbyte-internal-issues#17173 triage, and is still open there.

CI

At 01add3fa: Pytest (All) on Python 3.10–3.13, MyPy, Ruff Lint, Ruff Format and the source-intercom / source-pokeapi / source-shopify / source-hardcoded-records connector checks all passed. Check: destination-motherduck fails and is unrelated — it has been red in every round, including on #1162 and #1165 at the same time; its unit tests pass and only the FAST standard tests fail, and this change touches declarative source pagination only. The review fixes in 54f86a82 are running now; the numbers above are local.

🤖 Generated with Claude Code

@github-actions

github-actions Bot commented Sep 9, 2026

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@tolik0/cdk/reduce-page-size#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 tolik0/cdk/reduce-page-size

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.

Comment thread unit_tests/sources/streams/http/test_http_client.py Fixed
@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown

PyTest Results (Fast)

4 674 tests  +286   4 662 ✅ +286   9m 4s ⏱️ -34s
    1 suites ±  0      12 💤 ±  0 
    1 files   ±  0       0 ❌ ±  0 

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

♻️ This comment has been updated with latest results.

@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown

PyTest Results (Full)

4 677 tests  +286   4 665 ✅ +286   14m 29s ⏱️ +28s
    1 suites ±  0      12 💤 ±  0 
    1 files   ±  0       0 ❌ ±  0 

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

♻️ This comment has been updated with latest results.

@tolik0

Anatolii Yatsuk (tolik0) commented Sep 10, 2026

Copy link
Copy Markdown
Contributor Author

/prerelease

Prerelease Job Info

This job triggers the publish workflow with default arguments to create a prerelease.

Prerelease job started... Check job output.

✅ Prerelease workflow triggered successfully.

View the publish workflow run: https://github.com/airbytehq/airbyte-python-cdk/actions/runs/34491835004

@tolik0

Anatolii Yatsuk (tolik0) commented Sep 10, 2026

Copy link
Copy Markdown
Contributor Author

/prerelease

Prerelease Job Info

This job triggers the publish workflow with default arguments to create a prerelease.

Prerelease job started... Check job output.

✅ Prerelease workflow triggered successfully.

View the publish workflow run: https://github.com/airbytehq/airbyte-python-cdk/actions/runs/34504922288

@tolik0

Anatolii Yatsuk (tolik0) commented Sep 10, 2026

Copy link
Copy Markdown
Contributor Author

/prerelease

Prerelease Job Info

This job triggers the publish workflow with default arguments to create a prerelease.

Prerelease job started... Check job output.

✅ Prerelease workflow triggered successfully.

View the publish workflow run: https://github.com/airbytehq/airbyte-python-cdk/actions/runs/34507001618

Anatolii Yatsuk (tolik0) added a commit to airbytehq/airbyte that referenced this pull request Sep 10, 2026
…ifest

Completes Step 9: `reviews`, `issue_reactions` and
`pull_request_comment_reactions` join the Tier 1 streams in `manifest.yaml`.
`GitHubGraphQLStream`, `GitHubGraphQLErrorHandler`, `graphql.py`,
`github_schema.py` and the sgqlc dependency are all removed.

These three streams walk nested GraphQL connections, and a child connection
with more pages cannot be paginated in place: the query has to be re-rooted at
that single parent. `reviews` and `issue_reactions` switch between a parent
listing and a drill-down; `pull_request_comment_reactions` walks four levels
(pullRequests -> reviews -> comments -> reactions) depth-first, so a comment's
remaining reactions are drained before the listing advances.

Two custom pagination strategies replace the four legacy retrievers'
bookkeeping. Both keep all traversal state inside the page token rather than on
the component. That is a fix, not a port: one strategy instance is shared by
every partition of a stream and the partitions are read concurrently, which is
why the legacy `self.reviews_cursors`, `self.issues_cursor` and
`self.cursor_storage` were keyed by repository or, in the four-level case, not
keyed at all. A self-contained token removes the sharing.

Two record extractors handle the fact that records arrive under different paths
depending on which root the query used, and carry fields that only exist on the
parent node (`reviews.pull_request_url`, `issue_reactions.issue_number`,
`pull_request_comment_reactions.comment_id`).

One legacy behavior is deliberately dropped: the four-level stream sent
`first = min(page_size, total_count)` to avoid paying for pages larger than
what remained. `first` has to stay a GraphQL variable for REDUCE_PAGE_SIZE to
shrink it, and a variable cannot vary per token, so the connector may over-ask
on the last page of a connection. GitHub returns fewer records; the cost is a
slightly higher query score.

Adopting the action here turned up two CDK defects, both fixed in
airbytehq/airbyte-python-cdk#1149 rather than worked around: the strategy
allowlist rejected every CustomPaginationStrategy, and a non-integer page size
crashed the reducer with a bare TypeError.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Anatolii Yatsuk (tolik0) added a commit to airbytehq/airbyte that referenced this pull request Sep 10, 2026
…ifest

Completes Step 9: `reviews`, `issue_reactions` and
`pull_request_comment_reactions` join the Tier 1 streams in `manifest.yaml`.
`GitHubGraphQLStream`, `GitHubGraphQLErrorHandler`, `graphql.py`,
`github_schema.py` and the sgqlc dependency are all removed.

These three streams walk nested GraphQL connections, and a child connection
with more pages cannot be paginated in place: the query has to be re-rooted at
that single parent. `reviews` and `issue_reactions` switch between a parent
listing and a drill-down; `pull_request_comment_reactions` walks four levels
(pullRequests -> reviews -> comments -> reactions) depth-first, so a comment's
remaining reactions are drained before the listing advances.

Two custom pagination strategies replace the four legacy retrievers'
bookkeeping. Both keep all traversal state inside the page token rather than on
the component. That is a fix, not a port: one strategy instance is shared by
every partition of a stream and the partitions are read concurrently, which is
why the legacy `self.reviews_cursors`, `self.issues_cursor` and
`self.cursor_storage` were keyed by repository or, in the four-level case, not
keyed at all. A self-contained token removes the sharing.

Two record extractors handle the fact that records arrive under different paths
depending on which root the query used, and carry fields that only exist on the
parent node (`reviews.pull_request_url`, `issue_reactions.issue_number`,
`pull_request_comment_reactions.comment_id`).

One legacy behavior is deliberately dropped: the four-level stream sent
`first = min(page_size, total_count)` to avoid paying for pages larger than
what remained. `first` has to stay a GraphQL variable for REDUCE_PAGE_SIZE to
shrink it, and a variable cannot vary per token, so the connector may over-ask
on the last page of a connection. GitHub returns fewer records; the cost is a
slightly higher query score.

Adopting the action here turned up two CDK defects, both fixed in
airbytehq/airbyte-python-cdk#1149 rather than worked around: the strategy
allowlist rejected every CustomPaginationStrategy, and a non-integer page size
crashed the reducer with a bare TypeError.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ction

An HttpClient retry re-sends the same PreparedRequest, so an error handler that
mutates the stream's page size can never affect the request being retried: the
oversized page size is already serialized into the URL or the body. This adds a
`REDUCE_PAGE_SIZE` response action and a `page_size_reduction` block on
`SimpleRetriever` so a manifest can say "re-issue this page smaller" instead.

On a matching response, `SimpleRetriever._read_pages` shrinks the page size and
re-issues the same page. All reduction state lives in a `PageSizeReducer` created
per `_read_pages` call, mirroring `PaginationTracker`, because one retriever and
one paginator are shared by every partition of a stream and partitions are read
concurrently.

The reduced page size reaches the paginator and the pagination strategy as an
appended `page_size_override: Optional[int] = None` keyword argument, built by
`page_size_override_kwargs` so it is omitted when there is no reduction. A
paginator or strategy defined outside the CDK keeps working unchanged.

Stop conditions see the size that was actually requested:

- `OffsetIncrement` compares `last_page_size` against the requested size.
- `CursorPagination` exposes it to `stop_condition` and `cursor_value` as
  `page_size`, and the factory rejects a `stop_condition` comparing
  `last_page_size` against anything else, because a full page at the reduced size
  would otherwise read as a short page and end the pagination silently.
- `PageIncrement` is rejected, including through a subclass: pages are addressed
  as page number * page size, so a smaller page shifts every following boundary.

Termination is bounded by `max_attempts` reductions. That budget restarts after
every successful page under `reset_policy: AFTER_SUCCESSFUL_PAGE`, which is the
policy for an API that rejects the configured page size on every page - without
the restart such a stream would fail at page `max_attempts + 1` however healthy
the reads are. `PageSizeReducer.MAX_TOTAL_REDUCTIONS` bounds the partition either
way, and each reduction waits a short, growing amount of time before re-issuing.

The action is rejected at config time everywhere it cannot be honored: query
properties, `file_uploader`, `lazy_read_pointer`, all six `AsyncRetriever`
sub-requesters and `login_requester`. A `CustomPaginationStrategy` is accepted
only when its `next_page_token` can receive the override. When the action reaches
a retriever the factory could not inspect, the reduction fails with a config error
naming the stream rather than a bare crash.

`HttpClient` logs a response resolving to `REDUCE_PAGE_SIZE` as an auxiliary
request, so a Connector Builder test read still shows the failed attempt without
counting it against `max_pages_per_slice`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… failing healthy long partitions

Three round-2 review findings, two of them introduced by the round-1 fixes.

The `stop_condition` gate matched the expression as a string, and a regex
cannot do that job in either direction. `\bpage_size\b` matches inside
`config['page_size']` because `'` is a non-word character, so
`{{ last_page_size < config['page_size'] }}` passed the gate and still
silently truncated - the exact bug the gate exists to prevent. And
`{{ last_page_size == 0 }}` was rejected although no reduction can make a
full page empty; that is 11 of the 14 `last_page_size` stop conditions in
the monorepo, including all 6 in source-zendesk-support, which this PR
names as an adopter. Patching the regex was not the fix: two bugs in one
pattern meant the mechanism was wrong.

The condition is now parsed as a Jinja expression and the comparisons
`last_page_size` takes part in are classified on the AST, where a bare
`page_size` Name is distinguishable from a Getitem on `config`. The rule is
whether the condition can be true for a page that is full at the size that
was requested: an emptiness test and a threshold at or below
`minimum_page_size` cannot be, a lower bound can only stop being satisfied
as the page shrinks, and an upper bound is safe only when it follows the
reduction. A shape the analysis cannot classify is warned about rather than
rejected, because this runs at stream construction and a false rejection
takes `check` and `discover` down with `read`. Reverting to the round-1
regex fails 12 of the new factory tests, in both directions.

`MAX_TOTAL_REDUCTIONS = 1000` guaranteed termination but moved the
`AFTER_SUCCESSFUL_PAGE` cliff from page 6 to page 1001 instead of removing
it, and the failure it produced was wrong three ways: `transient_error` on a
job that could never succeed, "the source kept failing" on a partition where
every page succeeded, and a page size that was restored rather than
requested. A stream that gets every page through after one reduction is
healthy and has to complete, so the cap is gone. `max_attempts` now bounds
only the reductions made in a row *without* a page succeeding, which is what
separates a stuck partition from an expensive one. Termination still holds:
only a successful page restarts the budget and `_read_pages` calls that once
per page it consumed, so every restart costs a page of progress. The
schema's `max_attempts` and `reset_policy` descriptions say so, and two
tests pin the healthy long stream and the stuck partition - the first one
fails against the previous revision.

Also:

- `PageSizeReductionRequiredException` goes back to `config_error`. On the
  SimpleRetriever path it is always caught and the type is inert; the only
  way it escapes is a retriever that never re-issues the page, where a
  job-level retry would fail identically forever. The escape path is kept
  deliberately - it is the only signal such a stream gets - and the message
  no longer claims a retry that will not happen.
- Two assertions passed vacuously. `"test" in internal_message` also matched
  the stream name, so it certified nothing about the error handler's
  `error_message` reaching the exception; it now asserts the mapping's text.
  `test_given_no_page_size_then_page_size_interpolates_to_none` held equally
  if `page_size` were never bound at all; it is now parametrized with two
  positive cases that pin the variable.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@tolik0

Anatolii Yatsuk (tolik0) commented Sep 15, 2026

Copy link
Copy Markdown
Contributor Author

/poe build

Running poe build...

Link to job logs.

🤖 Auto-commit successful: 09882ec

🟦 Poe command build completed successfully.

@tolik0

Anatolii Yatsuk (tolik0) commented Sep 15, 2026

Copy link
Copy Markdown
Contributor Author

/autofix

Auto-Fix Job Info

This job attempts to auto-fix any linting or formating issues. If any fixes are made,
those changes will be automatically committed and pushed back to the PR.

Note: This job can only be run by maintainers. On PRs from forks, this command requires
that the PR author has enabled the Allow edits from maintainers option.

PR auto-fix job started... Check job output.

🟦 Job completed successfully (no changes).

…odel

Reverts 09882ec ("Auto-committed changes from Poe command `build`").

Running the codegen pipeline produced a +259/-265 diff against the committed
generated model and turned the MyPy Check red with 8 `valid-type` errors. Every
one is a `conint(...)` or `confloat(...)` used as an annotation, which mypy
rejects; the committed model on `main` contains none of these forms, so `main`
is green and only a regenerated file fails.

Three of the eight come from this PR (`PageSizeReduction.reduction_factor`,
`.minimum_page_size`, `.max_attempts`). The other five are pre-existing and
unrelated to this change: `DynamicStreamCheckConfig.stream_count`,
`ConstantBackoffStrategy` (x2), `ExponentialBackoffStrategy.jitter_range_in_seconds`,
and `AsyncRetriever.failed_retry_wait_time_in_seconds`. They are latent on every
branch and surface the moment anyone regenerates.

The root cause is in bin/generate_component_manifest_files.py: the
datamodel-codegen invocation does not pass `--field-constraints`, so numeric
`minimum` / `exclusiveMinimum` constraints become `conint()` / `confloat()`
instead of `Field(ge=...)`. Fixing that regenerates the whole file and should be
its own PR against main.

Reverting loses no validation. The manifest is checked against
declarative_component_schema.yaml by `jsonschema.validators.validate` in
`_validate_source()`, so the YAML bounds are enforced regardless of what the
generated model expresses - the same mechanism that enforces `minItems` on other
components, which codegen also drops.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@devin-ai-integration devin-ai-integration Bot 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.

Review of 65572c8f34baa808e708307bd84d01acaa4ce6e0 (tolik0/cdk/reduce-page-size)

Everything below was executed locally on that SHA (branch checked out, poetry install --all-extras). Nothing here is from reading alone.

Breaking-change call: NON_BREAKING

Searches run against airbytehq/airbyte at 583b2dee (airbyte-integrations/connectors/, 535 manifest.yaml files):

Search Count Why it matters
class X(PaginationStrategy | Paginator | DefaultPaginator | OffsetIncrement | CursorPaginationStrategy | PageIncrement) in components.py, excl. tests 5 classes in 4 connectors (source-monday ×2, source-hubspot, source-the-guardian-api, source-mixpanel) None of their next_page_token signatures accept page_size_override. They keep working because page_size_override_kwargs() only forwards the kwarg when a reduction is actually in effect, and a reduction requires an explicit page_size_reduction block — so these connectors are never called with the new kwarg.
Manifests referencing a bare page_size inside {{ }} (the newly injected interpolation variable) 0 No existing template silently changes meaning.
Manifests with stop_condition 176 The new factory check only runs when page_size_reduction is set, so none are affected.
REDUCE_PAGE_SIZE / page_size_reduction anywhere 0 New opt-in feature; no adopter yet.
.next_page_token( callers outside unit tests 1 (source-salesforce, its own non-CDK method) Nobody calls the CDK signature positionally with a 5th arg.

Schema additions are additive (new enum value, new optional block, new interpolation variable). PageIncrement.next_page_token gained a kwarg, but the 3 monorepo subclasses of it are only ever called without it.

Verification results (logs under /home/ubuntu/review_logs/ on the review box)

  • poetry run ruff check . → All checks passed; ruff format --check . → 794 files already formatted.
  • poetry run mypy --config-file mypy.ini airbyte_cdk → 1 error: macros.py:14 Library stubs not installed for "pytz" — environment, not this PR.
  • Targeted suites (retrievers, paginators, parsers, streams/http, connector_builder, error_handlers, test_concurrent_declarative_source.py) → 1220 passed, 2 failed; both failures (test_read_with_concurrent_and_synchronous_streams_with_{concurrent,sequential}_state, sqlite3.OperationalError in requests_cache) reproduce identically on a clean origin/main (96a7c0ac) worktree.
  • Full suite → 4514 passed, 3 skipped, 1 failed (test_parse_start_date[with_timezone_offset_converted_to_utc], tz-aware ab_datetime_parse — also fails on main; PR touches nothing under sources/file_based/). Note: pytest unit_tests/ -x -p no:cacheprovider as-is hits INTERNALERROR AttributeError: 'Config' object has no attribute 'cache' inside pytest_memray; had to add -p no:memray. Pre-existing plugin incompatibility, not this PR.
  • Worktree clean afterwards; no UUID CSV left behind this run (the new .gitignore glob does match a UUID name: git check-ignore -v.gitignore:17).

End-to-end probes I ran on top of the PR's own tests (all via ConcurrentDeclarativeSource + HttpMocker)

Probe Result
Reduction on page 2 after page 1 already emitted 100 records (CursorPagination, cursor carried into the retried request) 103 records, no duplicates, 1 state message ✅
Two ListPartitionRouter partitions sharing one retriever; partition a gets 502 at first=100 and succeeds at 50, partition b succeeds at 100 b requested exactly once at first=100 — reducer state is per _read_pages, no leak ✅
stop_condition: "{{ last_page_size < page_size }}" after reduction 100→50, second page of 10 60 records ✅
stop_condition: "{{ not (last_page_size >= 100) }}" — same data Accepted by the factory as SAFE, sync ends after 50 records, 0 error traces ❌ — see P2 inline
minimum_page_size: 50, both 100 and 50 rejected transient_error from the reducer as designed (the outer config_error wrapper is the pre-existing exception_handler message) ✅

Findings

  • P2stop_condition_safety.classify_stop_condition returns SAFE (not UNKNOWN) for {{ not (last_page_size >= 100) }}, {{ not last_page_size >= 100 }} and {{ last_page_size - 100 < 0 }}, all of which truncate exactly like {{ last_page_size < 100 }}. Reproduced end-to-end: silent data loss with no warning. Inline comment on stop_condition_safety.py.
  • P3 — New user-facing AirbyteTracedException.message strings embed remediation and config values (see inline on page_size_reducer.py); this conflicts with the org error-message guideline, though it matches the surrounding CDK style — flagging for a maintainer call.
  • No P0/P1 found. PageSizeReducer, OffsetIncrement/CursorPagination override handling, per-partition state, PageIncrement rejection, AsyncRetriever/file_uploader/login_requester rejections, and the auxiliary-log page counting all behaved as documented in the tests and probes I ran.

Not raised as findings, per the context you gave: the hand-written model vs /poe build drift, the poe assemble diff, destination-motherduck, and the weakness of green connector CI for this component.

Probe file used: a scratch test_probe.py outside the repo (reusing _page_size_reduction_manifest from test_concurrent_declarative_source.py); nothing was committed or pushed.

Written by Devin

Comment thread airbyte_cdk/sources/declarative/parsers/stop_condition_safety.py Outdated
Comment thread airbyte_cdk/sources/declarative/retrievers/page_size_reducer.py Outdated
The stop-condition analysis read each comparison on its own, which is only
sound while that comparison decides the condition in its own polarity.
`{{ not (last_page_size >= 100) }}` and `{{ last_page_size - 100 < 0 }}` both
mean `last_page_size < 100`, yet both were accepted as SAFE, so a full page at
a reduced size read as a short page and the partition was truncated silently -
the exact failure the check exists to prevent.

A comparison is now classified only when it is reached from the root of the
expression through `and`/`or` alone, and the `lt`/`lteq` readings require
`last_page_size` to be compared bare rather than after arithmetic. Anything
else is UNKNOWN, which warns instead of rejecting.

Also brings the reducer's user-facing messages in line with the error-message
guideline: every message names the stream, and the two `transient_error`
messages drop the remediation the user cannot act on. The `config_error`
messages keep theirs.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@devin-ai-integration devin-ai-integration Bot 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.

Re-review of 28f7649

Follow-up to the review on 65572c8f. Diff reviewed: 65572c8f..28f76492 (one commit, stop_condition_safety.py, page_size_reducer.py, 3 test files).

Breaking-change call: NON_BREAKING (unchanged). The fix only touches the opt-in page_size_reduction validation path and error strings; no schema, signature, or manifest-facing behavior changed. Monorepo counts from the first review still apply (0 adopters, 0 bare page_size templates).

Previous findings

  • P2 (false SAFE for negated/arithmetic comparisons) — resolved. All three reported shapes now come back UNKNOWN, and the factory warning fires. Re-ran the end-to-end probe (CursorPagination page_size: 100, 502 at 100, 50 records + cursor at 50): the factory now logs Stream Test uses page_size_reduction with the stop_condition '{{ not (last_page_size >= 100) }}', which could not be checked …. The sync still returns 50 of 60 records with 0 error traces, which is the documented UNKNOWN → warn policy, so I'm treating it as accepted; just flagging that for a manifest author who never reads connector logs this is still silent.
  • P3 (error-message style) — addressed. Both transient_error messages dropped the remediation; the config_error ones keep a one-sentence remediation, which is a reasonable maintainer call.

New findings (both P3)

Inline, anchored. Neither is blocking; both are corner shapes I found by fuzzing classify_stop_condition on this SHA (20 shapes, output below).

What I ran on 28f7649

  • poetry run ruff check . → All checks passed! · ruff format --check . → 794 files already formatted
  • mypy --config-file mypy.ini on the two changed modules → no issues found
  • pytest -p no:cacheprovider -p no:memray on test_stop_condition_safety.py, test_page_size_reducer.py, test_model_to_component_factory.py, test_concurrent_declarative_source.py, test_simple_retriever.py397 passed, 2 failed; the 2 failures are the same requests_cache sqlite tests that fail on clean main (verified in the first review).
  • Classifier fuzz (classify_stop_condition(c, minimum_page_size=1)):
{{ not (last_page_size >= 100) }}                     -> UNKNOWN
{{ not last_page_size >= 100 }}                       -> UNKNOWN
{{ last_page_size - 100 < 0 }}                        -> UNKNOWN
{{ 0 > last_page_size - 100 }}                        -> UNKNOWN
{{ -last_page_size > -100 }}                          -> UNKNOWN
{{ (last_page_size >= 100) == false }}                -> UNKNOWN
{{ (last_page_size >= 100) is false }}                -> UNKNOWN
{{ 'x' if last_page_size < 100 else '' }}             -> UNKNOWN
{{ last_page_size|int < 100 }}                        -> TRUNCATES
{{ last_page_size < page_size }}                      -> SAFE
{{ last_page_size < [page_size, 100] | max }}         -> SAFE      <- P3, see inline
{{ last_page_size < page_size + 50 }}                 -> SAFE
{{ last_page_size < config.page_size }}               -> TRUNCATES
{{ last_page_size < 100 or not response.next }}       -> TRUNCATES
{{ last_page_size >= 100 and not response.next }}     -> SAFE
{{ last_page_size == 0 }}                             -> SAFE
{{ (last_page_size < 100) | string == 'True' }}       -> UNKNOWN
{% if last_page_size < 100 %}true{% endif %}          -> UNKNOWN   <- P3, was TRUNCATES on 65572c8f
{% if not last_page_size >= 100 %}true{% endif %}     -> UNKNOWN
{{ last_page_size < 100 and last_page_size >= 0 }}    -> TRUNCATES

No P0/P1. Worktree clean, nothing committed.


Devin session

Comment thread airbyte_cdk/sources/declarative/parsers/stop_condition_safety.py
Comment thread airbyte_cdk/sources/declarative/parsers/stop_condition_safety.py Outdated
`{{ last_page_size < [page_size, 100] | max }}` was SAFE because the threshold
only had to *mention* `page_size`, while `max(50, 100)` is the configured size
again; `{{ last_page_size < page_size + 50 }}` had the same hole. The `lt`
branch now requires the threshold to be the `page_size` variable itself,
filters aside, so an expression built from it falls to UNKNOWN.

`{% if last_page_size < 100 %}true{% endif %}` had dropped from TRUNCATES to
UNKNOWN when the traversal started tracking whether a comparison decides the
condition. A `{% if %}` renders truthy text exactly when its test holds, so the
test is a decider - but only while the branch it guards is the whole story, so
an `else`, an `elif`, or a body the CDK reads as false keeps it unclassified.
Text rendered next to a comparison is unclassifiable for the same reason: the
condition is then truthy whatever the comparison decided.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@devin-ai-integration devin-ai-integration Bot 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.

Re-review of caafc97

Diff reviewed: 28f76492..caafc973 (one commit, stop_condition_safety.py + its test file).

Breaking-change call: NON_BREAKING (unchanged; validation-only change on the opt-in path).

Previous findings

  • P3 (page_size anywhere on the right → SAFE) — resolved. _is_requested_page_size now requires a bare page_size name (filters allowed). {{ last_page_size < [page_size, 100] | max }} and {{ last_page_size < page_size + 50 }} are UNKNOWN; re-ran the end-to-end probe and the factory warning now fires for the max shape.
  • P3 ({% if %} dropped to UNKNOWN) — resolved. {% if last_page_size < 100 %}true{% endif %} is TRUNCATES again, and the _if_follows_its_test guard correctly refuses to classify an else/elif or a body that renders a false value ({% if … %}false{% endif %}UNKNOWN), which is the right conservative call.

No new findings

I fuzzed 22 shapes on this SHA; every verdict matches the semantics I'd expect, including the edge cases I went looking for: page_size | int / page_size | default(100)SAFE, text next to the expression ({{ … }}x) → UNKNOWN, {% set %} aliasing → UNKNOWN, trailing {# comment #} → still TRUNCATES. One observation, not a finding: FALSE_VALUES is imported from interpolated_boolean, which makes the analysis follow the CDK's actual truthiness — good, and no import cycle (module imports cleanly under pytest and mypy).

What I ran on caafc97

  • poetry run ruff check . → All checks passed! · ruff format --check . → 794 files already formatted
  • mypy --config-file mypy.ini airbyte_cdk/sources/declarative/parsers/stop_condition_safety.py → no issues found
  • pytest -p no:cacheprovider -p no:memray on test_stop_condition_safety.py, test_model_to_component_factory.py, test_page_size_reducer.py286 passed
  • End-to-end probe with {{ last_page_size < [page_size, 100] | max }} (CursorPagination page_size: 100, 502 at 100, 50 records + cursor at 50): factory logs … could not be checked against the reduction because it compares last_page_size against an expression built from page_size …; the sync still returns 50/60 with 0 error traces, per the UNKNOWN → warn policy.
{{ last_page_size < [page_size, 100] | max }}                  -> UNKNOWN
{{ last_page_size < page_size + 50 }}                          -> UNKNOWN
{{ last_page_size < page_size }}                               -> SAFE
{{ last_page_size < page_size | int }}                         -> SAFE
{{ last_page_size < page_size | default(100) }}                -> SAFE
{% if last_page_size < 100 %}true{% endif %}                   -> TRUNCATES
{% if last_page_size < 100 %}true{% else %}false{% endif %}    -> UNKNOWN
{% if last_page_size < 100 %}false{% endif %}                  -> UNKNOWN
{% if last_page_size < 100 %}{{ 'true' }}{% endif %}           -> TRUNCATES
{% if last_page_size >= 100 %}true{% endif %}                  -> SAFE
{{ last_page_size < 100 }}x                                    -> UNKNOWN
{{ last_page_size < 100 }}{# c #}                              -> TRUNCATES
{% set s = last_page_size %}{{ s < 100 }}                      -> UNKNOWN
{{ (last_page_size < 100) or (last_page_size < 200) }}         -> TRUNCATES
{{ not (last_page_size >= 100) }}                              -> UNKNOWN
{{ last_page_size - 100 < 0 }}                                 -> UNKNOWN

No P0/P1/P2/P3 open from my side. Worktree clean, nothing committed.


Devin session

@devin-ai-integration devin-ai-integration Bot 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.

Approving caafc97. All findings from my three reviews (one P2, three P3) are resolved and verified by execution; NON_BREAKING. Details in #1149 (review).


Devin session

…ries at the minimum page size

Two gaps found while adopting `page_size_reduction` in source-github, where GitHub answers 502
both to a GraphQL page that is too expensive and to a transient hiccup:

- The wait between attempts was a hard-coded 0.5s times the attempt count, so a run of five
  reductions spread six requests over ~7s. `backoff_seconds` makes it a knob; the default keeps
  today's behaviour.
- Once the page size reached `minimum_page_size`, the first response asking for a smaller page
  ended the stream. `REDUCE_PAGE_SIZE` bypasses the HTTP retry budget, so at the floor a stream
  got fewer attempts than it had before adopting the reduction. `retries_at_minimum_page_size`
  re-issues the same page unchanged, after the backoff, in a budget of its own, separate from
  `max_attempts` (which only counts reductions) and restarted by every successful page. The
  default of 0 keeps today's behaviour.

The `config_error` raised when the page size can never be reduced is now limited to the case
where `minimum_page_size` is what blocks it, which is the only one the user can act on. A stream
whose configured page size is already 1 gets the transient error instead: one record per page is
as small as a page gets, so the API rejecting it says nothing about the configuration.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@tolik0

Anatolii Yatsuk (tolik0) commented Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

/prerelease

Prerelease Job Info

This job triggers the publish workflow with default arguments to create a prerelease.

Prerelease job started... Check job output.

✅ Prerelease workflow triggered successfully.

View the publish workflow run: https://github.com/airbytehq/airbyte-python-cdk/actions/runs/35216859043

`PageSizeReducer` took `sleep: Callable = time.sleep`, which binds the function object when
the module is imported. A connector test that patches `time.sleep` to keep a run of reductions
from taking its backoff for real therefore had no effect, and a suite covering a reduction down
to the minimum page size paid the whole wait. The override is now stored and `time.sleep` is
looked up per call.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…page size got there

Two findings from the fourth round of review.

R4-F1. `_retry_at_minimum_page_size` tested the `config_error` branch first, and
that branch never consulted `retries_at_minimum_page_size`. So the same manifest
gave a partition its retries when the reduction walked down to the floor and
none at all when the user's `page_size` was already there: the first response
ended the stream, telling the user to raise `page_size` or lower
`minimum_page_size` - a manifest field they cannot reach, for a case the
connector author had already answered by configuring retries. Whether a
transient error was retried therefore depended on how the page size arrived at
the floor rather than on anything about the error.

The retries now come first and the misconfiguration is reported once they are
spent, so it is still reported and still names both numbers to change. Measured
over the eight configurations that reach the floor: `configured=20, min=10,
retries=3` and `configured=10, min=10, retries=3` now take the same three waits
and differ only in the failure type, which is the one thing that should differ -
a floor that blocks every reduction is the connector's to fix, a floor the
reduction reached means the API kept rejecting every size.

R4-F2. The sleep-resolution fix of `01add3fa` was not pinned by any test: every
test here passes its own `sleep`, so none exercised the default path.
`test_given_no_sleep_override_when_reduce_then_wait_through_time_sleep` patches
`page_size_reducer.time.sleep` and asserts the wait lands there. Verified
against the defect: restoring the bound default makes that test fail, and the
file's run takes 7.5s of real sleeping instead of 0.45s.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@tolik0

Anatolii Yatsuk (tolik0) commented Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

/prerelease

Prerelease Job Info

This job triggers the publish workflow with default arguments to create a prerelease.

Prerelease job started... Check job output.

✅ Prerelease workflow triggered successfully.

View the publish workflow run: https://github.com/airbytehq/airbyte-python-cdk/actions/runs/35239229954

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.

🔵 Needs a closer look

It changes core pagination/error-handling interfaces and runtime behavior across declarative and HTTP layers, which warrants final human review despite strong test coverage.

Pull request overview

Adds a new declarative error-resolution path (ResponseAction.REDUCE_PAGE_SIZE) that lets a SimpleRetriever dynamically shrink page size and re-fetch the same page (same token/slice) instead of failing, including config-time validation to prevent silent pagination truncation.

Changes:

  • Introduces PageSizeReduction / PageSizeReducer and integrates them into SimpleRetriever._read_pages() to retry the same page with a reduced page_size_override.
  • Extends HttpClient to raise PageSizeReductionRequiredException on REDUCE_PAGE_SIZE, marks those HTTP logs as auxiliary for Connector Builder, and forwards page_size_override through paginator/strategy plumbing.
  • Adds factory/schema support and safety analysis for CursorPagination.stop_condition, plus extensive unit and integration tests.
File summaries
File Description
unit_tests/sources/streams/http/test_http_client.py Adds coverage for raising/logging behavior when REDUCE_PAGE_SIZE is resolved.
unit_tests/sources/declarative/test_concurrent_declarative_source.py Integration coverage ensuring a rejected page is retried with a smaller page size and correct failure typing on exhaustion.
unit_tests/sources/declarative/retrievers/test_simple_retriever.py Validates retriever retry semantics, reset policies, concurrency isolation, and mid-page safety guard.
unit_tests/sources/declarative/retrievers/test_page_size_reducer.py New focused tests for reduction arithmetic, budgets, backoff, floor behavior, and error messages.
unit_tests/sources/declarative/requesters/paginators/test_stop_condition.py Ensures page_size_override is forwarded through stop-condition decorator.
unit_tests/sources/declarative/requesters/paginators/test_page_increment.py Confirms PageIncrement rejects page-size reduction as a config error.
unit_tests/sources/declarative/requesters/paginators/test_offset_increment.py Tests corrected stop condition behavior under override and empty-string interpolation behavior.
unit_tests/sources/declarative/requesters/paginators/test_default_paginator.py Verifies override injection (params/body), override forwarding, and test-read decorator forwarding.
unit_tests/sources/declarative/requesters/paginators/test_cursor_pagination_strategy.py Pins override-safe token behavior and exposes requested page_size in interpolation context.
unit_tests/sources/declarative/requesters/error_handlers/test_composite_error_handler.py Ensures REDUCE_PAGE_SIZE short-circuits composite error handling.
unit_tests/sources/declarative/parsers/test_stop_condition_safety.py New tests for AST-based stop-condition classification (safe/truncates/unknown).
unit_tests/sources/declarative/parsers/test_model_to_component_factory.py Adds config-time validation coverage and warnings/errors for supported/unsupported combinations.
unit_tests/connector_builder/test_connector_builder_handler.py Ensures reduction retries don’t count as “pages” in Builder test-read limits but remain visible as auxiliary requests.
bin/generate_component_manifest_files.py Documents why generated schema/models still require hand edits for numeric constraints.
airbyte_cdk/sources/streams/http/page_size_reduction_exception.py Adds new traced exceptions for reduction-required and not-supported cases.
airbyte_cdk/sources/streams/http/http_client.py Raises reduction exception and logs reduction-triggering responses as auxiliary requests for Builder UX.
airbyte_cdk/sources/streams/http/error_handlers/response_models.py Adds ResponseAction.REDUCE_PAGE_SIZE.
airbyte_cdk/sources/declarative/retrievers/simple_retriever.py Implements per-read reducer, retry loop behavior, and guards against mid-page reductions.
airbyte_cdk/sources/declarative/retrievers/page_size_reducer.py New reducer implementation with budgets, backoff, floor retries, and reset policies.
airbyte_cdk/sources/declarative/requesters/paginators/strategies/stop_condition.py Forwards page_size_override to delegate strategy.
airbyte_cdk/sources/declarative/requesters/paginators/strategies/pagination_strategy.py Extends strategy interface with page_size_override.
airbyte_cdk/sources/declarative/requesters/paginators/strategies/page_increment.py Explicitly rejects page_size_override at runtime with a config-typed traced exception.
airbyte_cdk/sources/declarative/requesters/paginators/strategies/offset_increment.py Fixes stop condition to compare against the requested size (override-aware).
airbyte_cdk/sources/declarative/requesters/paginators/strategies/cursor_pagination_strategy.py Binds requested page_size into interpolation context to avoid silent truncation after reductions.
airbyte_cdk/sources/declarative/requesters/paginators/paginator.py Adds page_size_override_kwargs() helper and default get_page_size().
airbyte_cdk/sources/declarative/requesters/paginators/no_pagination.py Updates signature to accept page_size_override.
airbyte_cdk/sources/declarative/requesters/paginators/default_paginator.py Adds get_page_size(), injects override into request options, forwards override through strategies/test-read decorator.
airbyte_cdk/sources/declarative/requesters/paginators/init.py Notes page_size_override_kwargs is intentionally not re-exported.
airbyte_cdk/sources/declarative/requesters/error_handlers/composite_error_handler.py Adds reduction action to the short-circuit allowlist.
airbyte_cdk/sources/declarative/parsers/stop_condition_safety.py Adds AST-based analysis to detect stop conditions that could truncate under reductions.
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py Adds config-time validation/rejection/warnings for reduction support and stop-condition safety.
airbyte_cdk/sources/declarative/models/declarative_component_schema.py Updates generated model by hand to include REDUCE_PAGE_SIZE and PageSizeReduction schema.
airbyte_cdk/sources/declarative/declarative_component_schema.yaml Updates declarative schema to add action, page_size interpolation context, and PageSizeReduction definition.
.gitignore Adds ignores for test artifacts (macOS requests-cache sqlite file, CSV fixture, UUID temp files).
Review details
  • Files reviewed: 32/34 changed files
  • Comments generated: 2
  • Review effort level: Lite

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

Comment thread airbyte_cdk/sources/streams/http/http_client.py
Comment thread airbyte_cdk/sources/streams/http/page_size_reduction_exception.py
…nd page_size

`page_size` is bound to the page size that was actually requested, which is what makes
`{{ last_page_size < page_size }}` the one comparison that survives a reduction. But it is
only a number when the CursorPagination strategy declares a `page_size`. Without one it
binds to `None`, and the comparison does not fail: Jinja raises, `JinjaInterpolation._eval`
treats the TypeError as "not a template" and returns the raw template string, and
`InterpolatedBoolean` reads a non-empty string as `True`. The stop condition is then
satisfied on page 1 and the rest of the partition is dropped without anything failing.

This mattered beyond the streams that reduce: the example and the advice to prefer
`page_size` over a hardcoded number sit on `CursorPagination.stop_condition`, a field 710
pagination blocks across 97 connectors already use without declaring a `page_size`.

`CursorPaginationStrategy.__post_init__` now rejects a `stop_condition` or `cursor_value`
that reads `page_size` while the strategy declares none. The reference is read off the
Jinja AST, so the bare variable is told apart from `config['page_size']`, which is a
lookup on the config and is bound either way. Rejecting at construction rather than
failing per page keeps a sync from emitting a truncated partition first.

The schema now says the variable needs a declared `page_size`, and points at
`{{ last_page_size == 0 }}` for the case where there is none.

No manifest in the monorepo is affected: 0 of 535 declare a CursorPagination without a
`page_size` whose `stop_condition` or `cursor_value` reads the variable.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Anatolii Yatsuk (tolik0) added a commit to airbytehq/airbyte that referenced this pull request Sep 18, 2026
…15.dev35369167792

The previous pin predates the fix for the unbound `page_size` interpolation
variable in airbytehq/airbyte-python-cdk#1149, so CI was exercising older code
than the PR now carries.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Anatolii Yatsuk (tolik0) added a commit to airbytehq/airbyte that referenced this pull request Sep 18, 2026
….3.post15.dev35369167792

The previous pin predates the fix for the unbound `page_size` interpolation
variable in airbytehq/airbyte-python-cdk#1149, so CI was exercising older code
than the PR now carries.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Anatolii Yatsuk (tolik0) added a commit to airbytehq/airbyte that referenced this pull request Sep 18, 2026
…369167792

The previous pin predates the fix for the unbound `page_size` interpolation
variable in airbytehq/airbyte-python-cdk#1149, so CI was exercising older code
than the PR now carries.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants