fix(source-granola): retry throttled and server-error responses with backoff - #84278
Conversation
…backoff Co-Authored-By: bot_apk <apk@cognition.ai>
Co-Authored-By: bot_apk <apk@cognition.ai>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
👋 Greetings, Airbyte Team Member!Here are some helpful tips and reminders for your convenience. 💡 Show Tips and TricksPR Slash CommandsAirbyte Maintainers (that's you!) can execute the following slash commands on your PR:
📚 Show Repo GuidanceHelpful Resources
|
Co-Authored-By: bot_apk <apk@cognition.ai>
|
Note Autopilot progressive rollouts are not enabled for the following modified connector(s):
This is a courtesy heads-up only — it does not block merge or fail any check. |
|
|
Deploy preview for airbyte-docs ready!
Deployed with vercel-action |
There was a problem hiding this comment.
remove tests from this PR, covered by cdk tests
log hydra feedback: tests should test custom behavior implemented in the code that is being modified, no need to test features that are tested by default in the cdk
There was a problem hiding this comment.
👍 On it. Understood — the retry/backoff behavior here is CDK-owned (DefaultErrorHandler, WaitTimeFromHeader, ExponentialBackoffStrategy) and already covered by CDK tests, so a connector-level test that mostly asserts CDK sleep math isn't worth the maintenance. Removing the whole unit_tests/ project from this PR so the diff is just the manifest handler, the version bump, and the changelog row.
Noting the general feedback for the fix workflow: connector-level tests should cover custom behavior in the code being changed, not CDK features that ship with their own tests.
There was a problem hiding this comment.
☑️ Resolved in e22cc90. The unit_tests/ project is gone; the diff is now just manifest.yaml (the error handler), metadata.yaml (0.2.11 → 0.2.12), and the granola.md changelog row. The PR's Test Coverage section explains why there's no connector-level test.
Co-Authored-By: bot_apk <apk@cognition.ai>
What
Resolves https://github.com/airbytehq/airbyte-internal-issues/issues/16928 (certification criterion P-3):
#/definitions/base_requesterdeclared onlytype,url_baseand aBearerAuthenticator— there was noerror_handleranywhere in the manifest (grep forerror_handler|backoff|max_retrieson master returns nothing). Retryable responses therefore fell back to the CDK's generic retry behavior with no declared backoff strategy, nomax_retriesceiling, and nothing able to read aRetry-Afterheader. Granola publishes a burst limit of 25 requests per 5s and 5 req/s sustained (https://docs.granola.ai/help-center/sharing/integrations/granola-api), so throttling pressure is real.Retry-Afteris not confirmed by vendor docs. Granola's OpenAPI spec (https://docs.granola.ai/api-reference/openapi.json) documents only 200/400/401/404/413 responses — no 429 — and contains zero occurrences ofRetry-After. No live throttled response was observed, so the header name here is the HTTP-standard one, not one verified against the API. Ordering makes that safe either way (see below), but a follow-up that observes a real 429 should confirm the header name.How
One
DefaultErrorHandleronbase_requester(bothnotesanddetailed_notes$refit, so one handler covers both):Header-first ordering is safe when the header is absent:
WaitTimeFromHeader.backoff_time()returnsNoneandDefaultErrorHandler.backoff_time()returns the first non-Noneresult, so a missing header degrades to exponential backoff rather than to a zero wait.No
response_filters— the CDK'sDEFAULT_ERROR_MAPPINGalready retries 429 and 5xx. Sibling sub-issues https://github.com/airbytehq/airbyte-internal-issues/issues/16929 (E-1/E-3 response filters) and https://github.com/airbytehq/airbyte-internal-issues/issues/16925 (413 handling) extend this same handler; they should not add a second one.Worst-case cumulative backoff (for reconciliation with
maxSecondsBetweenMessages, currently unset — tracked in https://github.com/airbytehq/airbyte-internal-issues/issues/16926):max_waiting_time_in_seconds: 60per attempt → 5 × 60 = 300sSo 300s is the ceiling. Whatever value 16926 sets for
maxSecondsBetweenMessagesshould comfortably exceed 300s; values set by other connectors in this repo range from 2 to 6 hours, so no conflict is expected.api_budget(25 calls / PT5S) andconcurrency_level.default_concurrency: 6are untouched (P-1/P-2 already pass). The changelog's incorrect claim that 0.2.0-rc.4 reverted concurrency to 5 is left alone — it's tracked in 16926.Declarative-First Evaluation
Fully declarative: built-in
DefaultErrorHandler+WaitTimeFromHeader+ExponentialBackoffStrategy. Nocomponents.pyexists and none was added.Test Coverage
No connector-level test. An earlier revision of this PR added a
unit_tests/project asserting the retry/backoff behavior throughrequests_mock; per reviewer feedback (#84278 (comment)) it was removed, since the behavior it exercised belongs entirely to CDK components (DefaultErrorHandler,WaitTimeFromHeader,ExponentialBackoffStrategy) that carry their own CDK test coverage, and connector tests should cover custom connector behavior instead. This change adds no custom behavior.For the record, before removal those tests did discriminate: all three cases (429 with
Retry-After: 7, 429 without the header, 500) failed against master's manifest — each observed a generic 1s sleep instead of the configured backoff — and passed with the handler in place.Not run:
poe test-integration-tests(needs GSM secrets).Review guide
airbyte-integrations/connectors/source-granola/manifest.yamlmetadata.yaml(0.2.11 → 0.2.12) anddocs/integrations/sources/granola.mdUser Impact
Rate-limited and transient server-error responses are retried with backoff instead of failing fast, so syncs are less likely to fail on throttling. No schema, spec, state, or data-scope change — not breaking.
Can this PR be safely reverted and rolled back?
Link to Devin session: https://app.devin.ai/sessions/b16d8d6a9de8433fa68440b15f279a13
Important
⚡ Auto-merge enabled.
This PR is set to merge automatically when all requirements are met.