Repository navigation
feat(declarative): add WaitTimeFromResponse backoff strategy - #1233
devin-ai-integration[bot] wants to merge 7 commits into
Conversation
…d add max_waiting_time_in_seconds Co-Authored-By: Daryna Ishchenko <darina.ishchenko17@gmail.com>
Co-Authored-By: Daryna Ishchenko <darina.ishchenko17@gmail.com>
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
👋 Greetings, Airbyte Team Member!Here are some helpful tips and reminders for your convenience. 💡 Show Tips and TricksTesting This CDK VersionYou can test this version of the CDK using the following: # Run the CLI from this branch:
uvx 'git+https://github.com/airbytehq/airbyte-python-cdk.git@devin/1792070042-constant-backoff-response-interpolation#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/1792070042-constant-backoff-response-interpolationPR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
|
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Co-Authored-By: Daryna Ishchenko <darina.ishchenko17@gmail.com>
Co-Authored-By: Daryna Ishchenko <darina.ishchenko17@gmail.com>
devin-ai-integration
Bot
changed the title
response in ConstantBackoffStrategy and add max_waiting_time_in_secondsWaitTimeFromResponse backoff strategy
… non-numeric WaitTimeFromResponse result A wait computed as `retry_at - now` goes negative once that time has passed. Returned as-is, `HttpClient` kept it as a user-defined backoff and `time.sleep(wait + 1)` failed the stream with "sleep length must be non-negative". A non-numeric result raised a bare `ValueError` from `float()`, and NaN slipped past the cap into `sleep`. - Return `None` for a negative or non-finite wait, as `WaitUntilTimeFromHeader` does for a past timestamp. A zero wait is still returned as 0, like `WaitTimeFromHeader`. - Return `None` for a non-numeric result with a warning naming the strategy and the result's type (never its value), as the header strategies do. - Say so in the `wait_time_in_seconds` description (schema and generated model, which still matches `datamodel-codegen==0.26.3` output byte for byte). - Tests for negative, NaN, infinite and non-numeric results, and for the floor-less `timestamp(response.retry_at) - now_utc().timestamp()` expression with a past and a future time. Copyright year on the two new files. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… not fit the response body
The strategy runs on every retryable response, not only the rate limit its expression was
written for. The documented Gmail example called `.get` on `response` and on `response.error`,
so a 5xx with a JSON list body or `{"error": "..."}` raised `UndefinedError` out of
`backoff_time` and failed the stream instead of falling back to the next strategy.
- Catch `UndefinedError`, `TypeError` and `ValueError` from the evaluation, log a warning with
the error type (never the body), and return `None`.
- Make the example shape-safe (`response is mapping and response.get('error') is mapping and
...`) in the schema, the generated model and the test constant. The model still matches
`datamodel-codegen==0.26.3` output byte for byte.
- Tests: the example on a list body, a string `error`, an `error` without `message`, a null
`message` and a scalar body; and an expression that raises on the body.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Anatolii Yatsuk (tolik0)
left a comment
Anatolii Yatsuk (tolik0)
left a comment
There was a problem hiding this comment.
Review of the new WaitTimeFromResponse strategy. The change is additive and opt-in, and I confirmed the hand-edited model matches datamodel-codegen 0.26.3 output exactly. One P1 (an odd response body can turn the strategy off for the rest of the sync) and four P2s are inline below.
| failure_type=FailureType.transient_error, | ||
| ) | ||
|
|
||
| return wait_time |
There was a problem hiding this comment.
P2: nothing bounds the wait when no max_waiting_time_in_seconds is set
Any finite positive value reaches time.sleep as it is. max_time does not shorten a sleep that has already started, because the sleep runs in the on_backoff handler (rate_limiting.py:110-119). Response bodies often carry absolute epoch or millisecond timestamps rather than relative seconds, so a one-character manifest mistake goes a long way:
{{ response.reset }}with{"reset": 1760500000}sleeps for about 55 years, until the heartbeat kills the job.- A value of about
9.2e9or more makestime.sleepraiseOverflowError: timestamp out of range for platform time_t, an untraced failure.
WaitTimeFromHeader has the same pattern, but headers rarely carry epochs. At minimum, please say in the field description that the result must be relative seconds and that a cap is strongly recommended. A hard ceiling here (return None with a warning above, say, a day) would be safer still.
There was a problem hiding this comment.
Addressed the minimum in 19c24ff. The wait_time_in_seconds description now says the result must be in seconds relative to now, shows how to convert an absolute timestamp (timestamp(...) - now_utc().timestamp()), and says that nothing bounds the wait unless max_waiting_time_in_seconds is set, so setting it is strongly recommended. The generated model has the same text.
I did not add a hard ceiling. It would change behaviour, and WaitTimeFromHeader has none either. Leaving this thread open in case you think the CDK should enforce one here.
…ed response body Addresses review feedback on the strategy's error handling and docs. - Evaluate with `JinjaInterpolation` directly. `InterpolatedString` cached the template as plain text when a render `TypeError` returned it unchanged, so one odd body disabled the strategy for the rest of the sync. A result equal to the raw template now defers. - Fail at construction when the expression uses a variable other than `config`, `response`, `headers` or `parameters`, instead of silently deferring on every response. - Catch `ArithmeticError` (division by zero, float overflow) during evaluation and conversion. - Fall back to an empty `response` on any unreadable body (`ValueError`, `RequestException`, `RecursionError`), not only `JSONDecodeError`. - Descriptions: relative seconds, JSON body only, zero/unevaluable results defer, cap strongly recommended. Fix the code comment on zero waits. - Tests: instance reuse after a failed render, arithmetic errors, unreadable body, undeclared variable, `$parameters`, factory-built `backoff_time`, and `HttpClient` end to end (sleep and cap-exceeded trace). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Re-reviewed at 19c24ff. The plain-string cache fix and all four P2s check out: a failed render no longer disables the strategy, typos fail at startup, arithmetic and unreadable-body errors defer, and the new HttpClient tests cover the sleep and the cap. I am fine leaving the wait without a hard ceiling, for parity with WaitTimeFromHeader, now that the description recommends a cap.
This branch was successfully deployed
2 participants
Summary
Adds a
WaitTimeFromResponsebackoff strategy so a manifest can compute the wait from the response body without a custom component. This covers APIs that only send the wait time in the body, such as Gmail:{"error": {"message": "... Retry after 2026-10-15T13:15:00Z"}}, with noRetry-Afterheader.WaitTimeFromResponseBackoffStrategy:wait_time_in_seconds: an interpolated expression withconfig,response(the parsed JSON body,{}when the body isn't JSON or the input is an exception) andheaders.None, soDefaultErrorHandlermoves on to the next strategy. So does an expression that cannot be evaluated on the response body (a list, or a string where an object was expected): the strategy runs on every retryable response, not only the one the expression was written for. A warning names the strategy and the error type, never the body.max_waiting_time_in_seconds, with the same helper and semantics asWaitTimeFromHeader: if the wait is >= the cap, it raisestransient_error. The cap is validated at construction.DefaultErrorHandler.backoff_strategiesunion andModelToComponentFactory.Example (Gmail):
The generated model in
declarative_component_schema.pywas edited by hand, because runningpoe assemblelocally pulled in unrelated generator drift.Tests:
unit_tests/sources/declarative/requesters/error_handlersplus a factory test (518 passed together withtest_model_to_component_factory.py). They cover the cap, empty and missing results, a non-JSON body, negative, NaN, infinite and non-numeric results, the floor-lessretry_at - nowexpression with a past and a future time, and the Gmail example on five other body shapes.Link to Devin session: https://app.devin.ai/sessions/4b1e021516004e0ab53c25b7293450a2
Open in Devin Desktop: https://app.devin.ai/desktop/session/4b1e021516004e0ab53c25b7293450a2?variant=devin