fix(shopify): classify exhausted rate-limit retries as retryable noise - #76421
Conversation
`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
|
😎 Merged successfully - details. |
|
Hey @Gilbert09! 👋 It looks like your git author email on this PR isn't your
You can fix it for this repo with: git config user.email "you@posthog.com"Or set it globally with |
There was a problem hiding this comment.
Pull request overview
This PR reduces error-tracking noise for the Shopify warehouse source by teaching the import pipeline to treat “exhausted internal retry budget” Shopify failures (rate limits, transient upstream issues) as retryable, so they’re logged as warnings and left to Temporal’s activity retry instead of being reported as tracked exceptions.
Changes:
- Add
ShopifySource.get_retryable_errors()to classify known ShopifyRetryableError message prefixes as retryable noise. - Add a parameterized pytest case to lock in the retryable classification for the relevant exhausted-retry messages.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| products/warehouse_sources/backend/temporal/data_imports/sources/shopify/source.py | Adds Shopify-specific retryable error message patterns to keep exhausted internal retries out of error tracking. |
| products/warehouse_sources/backend/temporal/data_imports/sources/shopify/tests/test_non_retryable_errors.py | Adds a regression test ensuring those exhausted-retry messages are classified as retryable. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
There was a problem hiding this comment.
Small, additive error-classification fix matching an existing pattern used by many sibling sources; author is on the owning team, tests added, and the two Copilot review comments were addressed and resolved.
- Author wrote 0% of the modified lines and has 6 merged PRs in these paths (familiarity MODERATE).
- 👍 on the PR from hex-security-app[bot].
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 13L, 1F substantive, 32L/2F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1b-small (32L, 2F, single-area, fix) |
| stamphog 2.0.0b4 | .stamphog/policy.yml @ f381e58 · reviewed head 31c5d3a |
|
/trunk merge |
Problem
Error tracking surfaced a
ShopifyRetryableError: Shopify: rate limit exceeded...fromproducts/warehouse_sources/backend/temporal/data_imports/sources/shopify/shopify.py(issue).The stack trace shows this comes from
_make_paginated_shopify_request's tenacity-wrappedexecute, which already retries rate limits and transient upstream errors in-process (5 attempts, exponential backoff that honors Shopify's throttle refill time). Once that budget is exhausted, the exception escapes to_handle_import_error, which is where sources get a chance to classify their own already-retried, self-recovering failures viaget_retryable_errors()— matched errors are logged as a warning and left for Temporal's own activity retry, instead of being reported to error tracking.ShopifySourcenever implementedget_retryable_errors(), unlike Stripe, Hubspot, and Google Analytics, which all classify their internally-retried rate-limit/transient errors this way. So every time Shopify's rate limit stayed hot for the full local retry budget, the benign, self-recovering failure got reported as a tracked exception instead of a warning.This isn't a user/upstream problem (nothing for the customer to fix) and it isn't a code bug in the retry/backoff logic itself — it's a missing noise-reduction classification that its sibling sources already have.
Changes
Added
ShopifySource.get_retryable_errors(), matching the messagesShopifyRetryableErrorcan carry out of the same retry loop:"Shopify: rate limit exceeded"— the reported error"Shopify: internal error"— covers both the 5xx-from-HTTP-status and 5xx-from-GraphQL-payload variants"Shopify: connection broken while reading response"— a dropped connection mid-responseHow did you test this code?
Added
test_exhausted_internal_retries_are_classified_as_retryabletotest_non_retryable_errors.py, parameterized over the messages above — this locks in that once Shopify's own retries are exhausted, the failure is classified as retryable noise rather than reported to error tracking. Ran the full test file (15 passed),ruff check/ruff format, andmypyon the shopify source module — all clean.Automatic notifications
Docs update
N/A — internal retry/error-classification behavior, no user-facing or documented change.
🤖 Agent context
Autonomy: Fully autonomous
ShopifyRetryableError: rate limit exceeded) end to end: confirmed the stack trace originates in the Shopify source, read the retry mechanism inshopify.py, and compared against how other sources (Stripe, Hubspot, Google Analytics) classify the same category of exhausted-internal-retry error viaget_retryable_errors()./writing-testsbefore adding the regression test.