From 2546348410db8dc97c2a67cdea885a70b7fad696 Mon Sep 17 00:00:00 2001 From: Tom Owers Date: Sun, 2 Aug 2026 14:02:47 +0100 Subject: [PATCH 1/2] fix(shopify): classify exhausted rate-limit retries as retryable noise `ShopifySource` didn't override `get_retryable_errors()`, so once `_make_paginated_shopify_request`'s internal tenacity retries (5 attempts, backoff honoring Shopify's throttle refill time) were exhausted, the resulting `ShopifyRetryableError` was reported to error tracking as a full exception instead of a warning. Temporal already retries the whole activity in this case, so the failure is self-recovering. Add `get_retryable_errors()` covering the messages `ShopifyRetryableError` can carry from the same retry loop (rate limit exceeded, upstream 5xx/malformed payload, and a dropped connection mid-response), matching the pattern already used by Stripe, Hubspot, and Google Analytics. Refs `products/warehouse_sources/backend/temporal/data_imports/sources/shopify/source.py`. Generated-By: PostHog Code Task-Id: 5e876cfa-28d3-4be9-88d4-1b261a6103a4 --- .../data_imports/sources/shopify/source.py | 12 ++++++++++++ .../tests/test_non_retryable_errors.py | 19 +++++++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/products/warehouse_sources/backend/temporal/data_imports/sources/shopify/source.py b/products/warehouse_sources/backend/temporal/data_imports/sources/shopify/source.py index 936796ce45cd..ac2389b5ab0c 100644 --- a/products/warehouse_sources/backend/temporal/data_imports/sources/shopify/source.py +++ b/products/warehouse_sources/backend/temporal/data_imports/sources/shopify/source.py @@ -87,6 +87,18 @@ def get_non_retryable_errors(self) -> dict[str, str | None]: SHOPIFY_GRAPHQL_UNAUTHORIZED_ERROR_MATCH: SHOPIFY_GRAPHQL_UNAUTHORIZED_ERROR_MESSAGE, } + def get_retryable_errors(self) -> set[str]: + # `_make_paginated_shopify_request`'s `execute` already retries these in-process via + # tenacity (5 attempts, exponential backoff honoring Shopify's throttle refill time) + # before re-raising `ShopifyRetryableError`. Surviving all 5 attempts means the rate + # limit or upstream blip is still live, but Temporal retries the whole activity and it's + # self-recovering, so keep it out of error tracking as noise. + return { + "Shopify: rate limit exceeded", + "Shopify: internal error", + "Shopify: connection broken while reading response", + } + @property def get_source_config(self) -> SourceConfig: return SourceConfig( diff --git a/products/warehouse_sources/backend/temporal/data_imports/sources/shopify/tests/test_non_retryable_errors.py b/products/warehouse_sources/backend/temporal/data_imports/sources/shopify/tests/test_non_retryable_errors.py index 5110d575d9f9..6380df7c9e2a 100644 --- a/products/warehouse_sources/backend/temporal/data_imports/sources/shopify/tests/test_non_retryable_errors.py +++ b/products/warehouse_sources/backend/temporal/data_imports/sources/shopify/tests/test_non_retryable_errors.py @@ -83,3 +83,22 @@ def test_transient_http_errors_stay_retryable(status_code, reason): assert not any(pattern in error_message for pattern in patterns), ( f"transient error '{error_message}' should remain retryable" ) + + +@pytest.mark.parametrize( + "error_message", + [ + "Shopify: rate limit exceeded...", + "Shopify: internal error from request 500 Internal Server Error", + "Shopify: internal errors in payload {'errors': ['throttled']}", + "Shopify: connection broken while reading response: Connection broken: IncompleteRead(0 bytes read)", + ], +) +def test_exhausted_internal_retries_are_classified_as_retryable(error_message): + # These messages only reach `_handle_import_error` after `_make_paginated_shopify_request`'s + # own tenacity retries (5 attempts) are exhausted, so they should be logged as a warning + # and left for Temporal's activity retry rather than reported to error tracking as noise. + patterns = ShopifySource().get_retryable_errors() + assert any(pattern in error_message for pattern in patterns), ( + f"exhausted-retry error '{error_message}' should be classified as retryable" + ) From 31c5d3a1b5b407c93fb695e480031dc5cbe6573b Mon Sep 17 00:00:00 2001 From: Tom Owers Date: Sun, 2 Aug 2026 14:09:09 +0100 Subject: [PATCH 2/2] fix(shopify): address review feedback on retryable-error classification Clarify the get_retryable_errors comment to note ConnectionError/Timeout are also retried alongside ShopifyRetryableError, and use a realistic JSON-shaped serialized_errors string in the internal-error test case instead of a Python dict repr. Generated-By: PostHog Code Task-Id: 5e876cfa-28d3-4be9-88d4-1b261a6103a4 --- .../temporal/data_imports/sources/shopify/source.py | 11 ++++++----- .../shopify/tests/test_non_retryable_errors.py | 2 +- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/products/warehouse_sources/backend/temporal/data_imports/sources/shopify/source.py b/products/warehouse_sources/backend/temporal/data_imports/sources/shopify/source.py index ac2389b5ab0c..3b7a25e9a2d8 100644 --- a/products/warehouse_sources/backend/temporal/data_imports/sources/shopify/source.py +++ b/products/warehouse_sources/backend/temporal/data_imports/sources/shopify/source.py @@ -88,11 +88,12 @@ def get_non_retryable_errors(self) -> dict[str, str | None]: } def get_retryable_errors(self) -> set[str]: - # `_make_paginated_shopify_request`'s `execute` already retries these in-process via - # tenacity (5 attempts, exponential backoff honoring Shopify's throttle refill time) - # before re-raising `ShopifyRetryableError`. Surviving all 5 attempts means the rate - # limit or upstream blip is still live, but Temporal retries the whole activity and it's - # self-recovering, so keep it out of error tracking as noise. + # These are the messages `ShopifyRetryableError` carries once `_make_paginated_shopify_ + # request`'s `execute` exhausts its own tenacity retries (5 attempts, exponential backoff + # honoring Shopify's throttle refill time — retried alongside transient `ConnectionError`/ + # `Timeout`) and re-raises. Surviving all 5 attempts means the rate limit or upstream blip + # is still live, but Temporal retries the whole activity and it's self-recovering, so keep + # it out of error tracking as noise. return { "Shopify: rate limit exceeded", "Shopify: internal error", diff --git a/products/warehouse_sources/backend/temporal/data_imports/sources/shopify/tests/test_non_retryable_errors.py b/products/warehouse_sources/backend/temporal/data_imports/sources/shopify/tests/test_non_retryable_errors.py index 6380df7c9e2a..df2015f361b9 100644 --- a/products/warehouse_sources/backend/temporal/data_imports/sources/shopify/tests/test_non_retryable_errors.py +++ b/products/warehouse_sources/backend/temporal/data_imports/sources/shopify/tests/test_non_retryable_errors.py @@ -90,7 +90,7 @@ def test_transient_http_errors_stay_retryable(status_code, reason): [ "Shopify: rate limit exceeded...", "Shopify: internal error from request 500 Internal Server Error", - "Shopify: internal errors in payload {'errors': ['throttled']}", + 'Shopify: internal errors in payload [{"message": "internal error", "extensions": {"code": "internal_server_error"}}]', "Shopify: connection broken while reading response: Connection broken: IncompleteRead(0 bytes read)", ], )