diff --git a/posthog/models/integration.py b/posthog/models/integration.py index c0f14c11202c..5db464902f21 100644 --- a/posthog/models/integration.py +++ b/posthog/models/integration.py @@ -40,6 +40,7 @@ from rest_framework.request import Request from slack_sdk import WebClient from slack_sdk.errors import SlackApiError +from tenacity import retry, retry_if_exception_type, stop_after_attempt, wait_fixed from posthog.cache_utils import cache_for from posthog.egress.github.transport import github_request @@ -1931,6 +1932,21 @@ def google_ads_hierarchy_level(account: dict) -> int: return int(account.get("level") or 0) +@retry( + stop=stop_after_attempt(3), + wait=wait_fixed(2), + retry=retry_if_exception_type((requests.exceptions.Timeout, requests.exceptions.ConnectionError)), + reraise=True, +) +def _google_ads_request(method: str, url: str, **kwargs) -> requests.Response: + """`requests.request` with retries for a transient timeout/connection blip. + + `list_google_ads_accessible_accounts` walks the account hierarchy with a chain of sequential + requests, so a single transient blip on any one of them would otherwise fail the whole walk. + """ + return requests.request(method, url, **kwargs) + + class GoogleAdsIntegration: integration: Integration @@ -1990,7 +2006,7 @@ def list_google_ads_conversion_actions(self, customer_id, parent_id=None) -> lis # Google Ads manager accounts can have access to other accounts (including other manager accounts). # Filter out duplicates where a user has direct access and access through a manager account, while prioritizing direct access. def list_google_ads_accessible_accounts(self) -> list[dict[str, Any]]: - response = requests.request( + response = _google_ads_request( "GET", "https://googleads.googleapis.com/v24/customers:listAccessibleCustomers", headers={ @@ -2030,7 +2046,7 @@ def list_google_ads_accessible_accounts(self) -> list[dict[str, Any]]: def dfs(account_id, accounts=None, parent_id=None) -> list[dict]: if accounts is None: accounts = [] - response = requests.request( + response = _google_ads_request( "POST", f"https://googleads.googleapis.com/v24/customers/{account_id}/googleAds:searchStream", json={ diff --git a/posthog/models/test/test_integration_model.py b/posthog/models/test/test_integration_model.py index 7d5b80000687..b5f944a71405 100644 --- a/posthog/models/test/test_integration_model.py +++ b/posthog/models/test/test_integration_model.py @@ -3335,6 +3335,35 @@ def test_accessible_accounts_empty_when_login_has_no_accessible_customers(self, assert accounts == [] + @override_settings(GOOGLE_ADS_DEVELOPER_TOKEN="dev_token") + @patch("tenacity.nap.time.sleep") + @patch("posthog.models.integration.requests.request") + def test_accessible_accounts_rides_out_one_transient_read_timeout(self, mock_request, mock_sleep): + # The walk is a chain of sequential requests; a single timed-out request used to fail the whole + # walk. It must instead be retried once and succeed. + accessible = MagicMock(status_code=200) + accessible.json.return_value = {"resourceNames": ["customers/6501924158"]} + stream = MagicMock(status_code=200) + stream.json.return_value = [{"results": [self._customer_client("1234567890", "Client One", level="1")]}] + mock_request.side_effect = [accessible, requests.exceptions.ReadTimeout("read timed out"), stream] + + accounts = GoogleAdsIntegration(self._integration()).list_google_ads_accessible_accounts() + + assert [account["id"] for account in accounts] == ["1234567890"] + + @override_settings(GOOGLE_ADS_DEVELOPER_TOKEN="dev_token") + @patch("tenacity.nap.time.sleep") + @patch("posthog.models.integration.requests.request") + def test_accessible_accounts_raises_after_repeated_read_timeouts(self, mock_request, mock_sleep): + # Retries must be bounded: a persistently unreachable endpoint should still fail rather than + # retry forever or get silently swallowed. + mock_request.side_effect = requests.exceptions.ReadTimeout("read timed out") + + with pytest.raises(requests.exceptions.ReadTimeout): + GoogleAdsIntegration(self._integration()).list_google_ads_accessible_accounts() + + assert mock_request.call_count == 3 + class TestPinterestAdsIntegrationDisplayName(BaseTest): @parameterized.expand( diff --git a/products/warehouse_sources/backend/temporal/data_imports/sources/google_ads/source.py b/products/warehouse_sources/backend/temporal/data_imports/sources/google_ads/source.py index aae65f98c076..2dbb8615c90e 100644 --- a/products/warehouse_sources/backend/temporal/data_imports/sources/google_ads/source.py +++ b/products/warehouse_sources/backend/temporal/data_imports/sources/google_ads/source.py @@ -3,6 +3,7 @@ from django.core.cache import cache +import requests from rest_framework.exceptions import ValidationError from posthog.schema import ( @@ -318,6 +319,13 @@ def get_oauth_accounts( "Google rejected the credentials for this integration. Please reconnect your Google Ads " "integration and make sure the connected account can access your Google Ads accounts." ) from e + except (requests.exceptions.Timeout, requests.exceptions.ConnectionError) as e: + # The walk retries a transient blip internally; this means every attempt on some request + # timed out or failed to connect. Actionable and retryable from the user's side, so surface + # a clean message instead of the raw connection error. + raise IntegrationAccountListingError( + "Google Ads did not respond in time while listing your accounts. Please try again." + ) from e names_by_id = {account["id"]: account["name"] for account in accounts} integration_accounts = [ diff --git a/products/warehouse_sources/backend/temporal/data_imports/sources/google_ads/tests/test_google_ads_source.py b/products/warehouse_sources/backend/temporal/data_imports/sources/google_ads/tests/test_google_ads_source.py index 279f5d3ba10f..d3cf04ee9df0 100644 --- a/products/warehouse_sources/backend/temporal/data_imports/sources/google_ads/tests/test_google_ads_source.py +++ b/products/warehouse_sources/backend/temporal/data_imports/sources/google_ads/tests/test_google_ads_source.py @@ -13,6 +13,7 @@ import grpc import pyarrow as pa +import requests from google.ads.googleads.errors import GoogleAdsException from google.ads.googleads.v23.enums import types as ga_enums from google.ads.googleads.v23.errors.types.errors import ErrorCode, GoogleAdsError, GoogleAdsFailure @@ -24,6 +25,9 @@ from posthog.models.integration import Integration +from products.warehouse_sources.backend.temporal.data_imports.sources.common.integration_accounts import ( + IntegrationAccountListingError, +) from products.warehouse_sources.backend.temporal.data_imports.sources.generated_configs.googleads import ( GoogleAdsIsMccAccountConfig, GoogleAdsSourceConfig, @@ -1204,6 +1208,34 @@ def test_distinct_search_terms_reuse_one_hierarchy_walk(self): assert [account.value for account in second] == ["987-654-3210"] +class TestGetOAuthAccountsNetworkErrorHandling: + @pytest.mark.parametrize( + "network_error", + [ + requests.exceptions.ReadTimeout("read timed out"), + requests.exceptions.ConnectionError("connection reset"), + ], + ) + def test_transient_network_error_becomes_actionable(self, network_error): + # list_google_ads_accessible_accounts retries a transient blip internally, so this exception means + # every attempt failed. Previously nothing here caught it, so it propagated as an unhandled 500 + # instead of the same actionable error the credential-rejection path already raises. + cache.clear() + source = GoogleAdsSource() + integration = mock.Mock(errors=None) + + with ( + mock.patch.object(GoogleAdsSource, "get_oauth_integration", return_value=integration), + mock.patch(f"{_SOURCE_MODULE}.OauthIntegration") as mock_oauth, + mock.patch(f"{_SOURCE_MODULE}.GoogleAdsIntegration") as mock_google_ads, + ): + mock_oauth.return_value.access_token_expired.return_value = False + mock_google_ads.return_value.list_google_ads_accessible_accounts.side_effect = network_error + + with pytest.raises(IntegrationAccountListingError): + source.get_oauth_accounts(1, 2) + + class TestGoogleAdsQueryConstruction: _MODULE = "products.warehouse_sources.backend.temporal.data_imports.sources.google_ads.google_ads"