diff --git a/.sampo/changesets/flags-no-retry-connection-refused.md b/.sampo/changesets/flags-no-retry-connection-refused.md new file mode 100644 index 000000000..e5a465d32 --- /dev/null +++ b/.sampo/changesets/flags-no-retry-connection-refused.md @@ -0,0 +1,5 @@ +--- +pypi/posthog: patch +--- + +Stop retrying `/flags` requests when the connection is refused, so the flag error surfaces right away instead of after the retry backoff. diff --git a/posthog/request.py b/posthog/request.py index 03316fc1f..52854f922 100644 --- a/posthog/request.py +++ b/posthog/request.py @@ -15,6 +15,7 @@ import requests from requests.adapters import HTTPAdapter from urllib3.connection import HTTPConnection +from urllib3.exceptions import NewConnectionError from urllib3.util.retry import Retry from posthog._logging import _configure_posthog_logging @@ -322,6 +323,29 @@ def _process_response( raise APIError(res.status_code, res.text, retry_after=retry_after) +def _is_connection_refused(exc: BaseException) -> bool: + """Return True when a requests transport error wraps a refused connection.""" + pending: List[Any] = [exc] + seen = set() + while pending: + current = pending.pop() + if current is None or id(current) in seen: + continue + seen.add(id(current)) + if isinstance(current, ConnectionRefusedError): + return True + if isinstance(current, BaseException): + pending.extend(current.args) + pending.append(current.__cause__) + # __context__ records whatever exception was being handled, which may + # have nothing to do with this request. urllib3 1.x only links the + # socket error to NewConnectionError that way, so follow it just there. + if isinstance(current, NewConnectionError): + pending.append(current.__context__) + pending.append(getattr(current, "reason", None)) + return False + + def _feature_flags_retry_delay(failed_attempt: int) -> float: return _FEATURE_FLAGS_RETRY_BACKOFF_SECONDS * (2**failed_attempt) @@ -354,8 +378,12 @@ def flags( return _process_response( res, success_message="Feature flags evaluated successfully" ) - except (requests.exceptions.ConnectionError, requests.exceptions.Timeout): - if failed_attempt >= retries: + except ( + requests.exceptions.ConnectionError, + requests.exceptions.Timeout, + ) as exc: + # A refused connection is not transient, so it is not retried. + if failed_attempt >= retries or _is_connection_refused(exc): raise except APIError as exc: if ( diff --git a/posthog/test/test_request.py b/posthog/test/test_request.py index e23b0a446..d9d44df79 100644 --- a/posthog/test/test_request.py +++ b/posthog/test/test_request.py @@ -1,5 +1,6 @@ import gzip import json +import socket import unittest import zlib from datetime import date, datetime, timedelta @@ -702,6 +703,60 @@ def test_flags_retries_transport_errors_once_by_default( self.assertEqual(mock_session.post.call_count, 2) mock_sleep.assert_called_once_with(0.3) + @mock.patch("posthog.request.time.sleep") + def test_flags_does_not_retry_refused_connection(self, mock_sleep): + # Bind and close a socket so the port refuses connections. + with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as sock: + sock.bind(("127.0.0.1", 0)) + port = sock.getsockname()[1] + + with mock.patch("posthog.request.post", wraps=request_module.post) as mock_post: + with self.assertRaises(requests.exceptions.ConnectionError): + flags( + "test-key", + f"http://127.0.0.1:{port}", + max_retries=2, + distinct_id="user123", + ) + + self.assertEqual(mock_post.call_count, 1) + mock_sleep.assert_not_called() + + @mock.patch("posthog.request.time.sleep") + @mock.patch("posthog.request._get_flags_session") + def test_flags_retries_when_handling_an_unrelated_refused_connection( + self, mock_get_flags_session, mock_sleep + ): + mock_response = requests.Response() + mock_response.status_code = 200 + mock_response._content = json.dumps( + { + "featureFlags": {"test-flag": True}, + "featureFlagPayloads": {}, + "errorsWhileComputingFlags": False, + } + ).encode("utf-8") + + mock_session = mock.MagicMock() + mock_session.post.side_effect = [ + requests.exceptions.Timeout("timed out"), + mock_response, + ] + mock_get_flags_session.return_value = mock_session + + # The application evaluates flags while handling its own refused + # connection, so the timeout inherits that error as its __context__. + try: + raise ConnectionRefusedError("database is down") + except ConnectionRefusedError: + response = flags( + "test-key", "https://test.posthog.com", distinct_id="user123" + ) + + self.assertEqual(response["featureFlags"], {"test-flag": True}) + self.assertEqual(mock_session.post.call_count, 2) + mock_sleep.assert_called_once_with(0.3) + @mock.patch("posthog.request.time.sleep") @mock.patch("posthog.request._get_flags_session") def test_flags_retry_count_zero_disables_retries(